From 04444062643a4ed34eae6e0a5d651853bcd72232 Mon Sep 17 00:00:00 2001 From: Daniel Bily Date: Thu, 22 Sep 2022 17:31:19 +0200 Subject: [PATCH 1/7] bump utils and types pkgs --- go.mod | 4 ++-- go.sum | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index 67a2389d..6a318390 100644 --- a/go.mod +++ b/go.mod @@ -6,9 +6,9 @@ require ( github.com/BurntSushi/toml v0.3.1 github.com/DATA-DOG/go-sqlmock v1.4.1 github.com/RedHatInsights/insights-content-service v0.0.0-20201009081018-083923779f00 - github.com/RedHatInsights/insights-operator-utils v1.23.8 + github.com/RedHatInsights/insights-operator-utils v1.24.2 github.com/RedHatInsights/insights-results-aggregator-data v1.3.6 - github.com/RedHatInsights/insights-results-types v1.3.18 + github.com/RedHatInsights/insights-results-types v1.3.20 github.com/Shopify/sarama v1.27.1 github.com/deckarep/golang-set v1.7.1 github.com/gchaincl/sqlhooks v1.3.0 diff --git a/go.sum b/go.sum index 20b1c4a8..e256a6f4 100644 --- a/go.sum +++ b/go.sum @@ -43,8 +43,8 @@ github.com/RedHatInsights/insights-operator-utils v1.12.0/go.mod h1:mN5jURLpSG+j github.com/RedHatInsights/insights-operator-utils v1.21.0/go.mod h1:B2hizFGwXCc8MT34QqVJ1A8ANTyGQZQWXQw/gSCEsaU= github.com/RedHatInsights/insights-operator-utils v1.21.2/go.mod h1:3Pfsgsi7GCu2wgIqQlt1llpyQyyxsDWEGdgnPvadM40= github.com/RedHatInsights/insights-operator-utils v1.21.8/go.mod h1:qa1a8NdarIzcZkr5mu9fBw4OarOfg1qZFZC1vNGbyas= -github.com/RedHatInsights/insights-operator-utils v1.23.8 h1:7JOQ+PMSjCsn95WYvIVQAJ49Rr53bsJQG2qW1vW1g2g= -github.com/RedHatInsights/insights-operator-utils v1.23.8/go.mod h1:xsmvqwngwHqFvHPK+XARoZH8aR5Qcr13AGh4SJa3f40= +github.com/RedHatInsights/insights-operator-utils v1.24.2 h1:+xUPgBGB/KGYqSUvsuVQ0PYMRBP5r5wh9RYp/ymaCNA= +github.com/RedHatInsights/insights-operator-utils v1.24.2/go.mod h1:7qR/8rlMdiqoXAkZyQ5JhVrVNCa6SBwNUt4KMq/17j4= github.com/RedHatInsights/insights-results-aggregator v0.0.0-20200604090056-3534f6dd9c1c/go.mod h1:7Pc15NYXErx7BMJ4rF1Hacm+29G6atzjhwBpXNFMt+0= github.com/RedHatInsights/insights-results-aggregator-data v0.0.0-20200825113234-e84e924194bc/go.mod h1:DcDgoCCmBuUSKQOGrTi0BfFLdSjAp/KxIwyqKUd46sM= github.com/RedHatInsights/insights-results-aggregator-data v0.0.0-20201014142608-de97c4b07d5c/go.mod h1:x8IvreR2g24veCKVMXDPOR6a0D86QK9UCBfi5Xm5Gnc= @@ -58,9 +58,8 @@ github.com/RedHatInsights/insights-results-aggregator-data v1.3.6 h1:RcZsn25t+km github.com/RedHatInsights/insights-results-aggregator-data v1.3.6/go.mod h1:tOwmlIB5irSv1mTLgONuLrsbTp4vokl4ClJFClrrXX0= github.com/RedHatInsights/insights-results-types v1.2.0/go.mod h1:6VVdMTGU/BAS2cW0KrHAUiDyocpyKqpPpEyp6AJ1tk8= github.com/RedHatInsights/insights-results-types v1.3.7/go.mod h1:6VVdMTGU/BAS2cW0KrHAUiDyocpyKqpPpEyp6AJ1tk8= -github.com/RedHatInsights/insights-results-types v1.3.12/go.mod h1:6VVdMTGU/BAS2cW0KrHAUiDyocpyKqpPpEyp6AJ1tk8= -github.com/RedHatInsights/insights-results-types v1.3.18 h1:V8wngjEc1j8r99P7e0EsCbh2FcsLXQkvDoWN3LJdfIY= -github.com/RedHatInsights/insights-results-types v1.3.18/go.mod h1:6VVdMTGU/BAS2cW0KrHAUiDyocpyKqpPpEyp6AJ1tk8= +github.com/RedHatInsights/insights-results-types v1.3.20 h1:02zlYmyZWkUCryHYgShrfYYO0ZJK3oYV5DaxeP5IZ2g= +github.com/RedHatInsights/insights-results-types v1.3.20/go.mod h1:6VVdMTGU/BAS2cW0KrHAUiDyocpyKqpPpEyp6AJ1tk8= github.com/RedHatInsights/kafka-zerolog v0.0.0-20210304172207-928f026dc7ec/go.mod h1:HJul5oCsCRNiRlh/ayJDGdW3PzGlid/5aaQwJBn7was= github.com/RedHatInsights/kafka-zerolog v1.0.0 h1:4zPrLcwnfFl07qv9/ximlm1E/rWs93TkYnHrgNiU73A= github.com/RedHatInsights/kafka-zerolog v1.0.0/go.mod h1:HJul5oCsCRNiRlh/ayJDGdW3PzGlid/5aaQwJBn7was= From bb67515240ef792439c9b85fa5a065b63756cde0 Mon Sep 17 00:00:00 2001 From: Daniel Bily Date: Thu, 22 Sep 2022 17:31:47 +0200 Subject: [PATCH 2/7] remove unused Migration 29 --- migration/actual_migrations_test.go | 127 ------------------ migration/mig_0029_nullify_user_id_columns.go | 76 ----------- 2 files changed, 203 deletions(-) delete mode 100644 migration/mig_0029_nullify_user_id_columns.go diff --git a/migration/actual_migrations_test.go b/migration/actual_migrations_test.go index c3d655f8..c3d94608 100644 --- a/migration/actual_migrations_test.go +++ b/migration/actual_migrations_test.go @@ -1083,130 +1083,3 @@ func TestMigration28(t *testing.T) { // not possible to insert more than 1 row per org assert.Error(t, err) } - -// to be enabled after smart-proxy is merged -/* -func TestMigration29(t *testing.T) { - const selectUserIDQuery = "SELECT user_id FROM %v" - var tablesAffected = []string{ - "advisor_ratings", - "cluster_rule_toggle", - "cluster_rule_user_feedback", - "cluster_user_rule_disable_feedback", - "rule_disable", - } - - db, dbDriver, closer := prepareDBAndInfo(t) - defer closer() - - if dbDriver == types.DBDriverSQLite3 { - // sqlite is no longer supported - return - } - - err := migration.SetDBVersion(db, dbDriver, 28) - helpers.FailOnError(t, err) - - // insert into report table because of DB constraints - _, err = db.Exec(` - INSERT INTO report (org_id, cluster, report, reported_at, last_checked_at) - VALUES ($1, $2, $3, $4, $5) - `, - testdata.OrgID, - testdata.ClusterName, - testdata.ClusterReport3Rules, - testdata.LastCheckedAt, - testdata.LastCheckedAt, - ) - helpers.FailOnError(t, err) - - // insert into advisor_ratings - _, err = db.Exec( - `INSERT INTO advisor_ratings - (org_id, user_id, rule_fqdn, error_key, rule_id) - VALUES - ($1, $2, $3, $4, $5)`, - testdata.OrgID, - testdata.UserID, - testdata.Rule1CompositeID, - testdata.ErrorKey1, - testdata.Rule1ID, - ) - helpers.FailOnError(t, err) - - // insert into cluster_rule_toggle - _, err = db.Exec( - `INSERT INTO cluster_rule_toggle (cluster_id, rule_id, error_key, user_id, disabled, updated_at, org_id) - VALUES ($1, $2, $3, $4, $5, $6, $7)`, - testdata.ClusterName, - testdata.Rule1ID, - testdata.ErrorKey1, - testdata.UserID, - 1, - testdata.LastCheckedAt, - testdata.OrgID, - ) - helpers.FailOnError(t, err) - - // insert into cluster_user_rule_disable_feedback - _, err = db.Exec( - `INSERT INTO cluster_user_rule_disable_feedback - (cluster_id, rule_id, error_key, user_id, message, added_at, updated_at, org_id) - VALUES - ($1, $2, $3, $4, $5, $6, $7, $8)`, - testdata.ClusterName, - testdata.Rule1ID, - testdata.ErrorKey1, - testdata.UserID, - "", - testdata.LastCheckedAt, - testdata.LastCheckedAt, - testdata.OrgID, - ) - helpers.FailOnError(t, err) - - // insert into cluster_rule_user_feedback - _, err = db.Exec( - `INSERT INTO cluster_rule_user_feedback - (cluster_id, rule_id, error_key, user_id, message, added_at, updated_at, user_vote, org_id) - VALUES - ($1, $2, $3, $4, $5, $6, $7, $8, $9)`, - testdata.ClusterName, - testdata.Rule1ID, - testdata.ErrorKey1, - testdata.UserID, - "", - testdata.LastCheckedAt, - testdata.LastCheckedAt, - 1, - testdata.OrgID, - ) - helpers.FailOnError(t, err) - - // insert into rule_disable - _, err = db.Exec( - `INSERT INTO rule_disable - (org_id, user_id, rule_id, error_key, created_at) - VALUES - ($1, $2, $3, $4, $5)`, - testdata.OrgID, - testdata.UserID, - testdata.Rule1ID, - testdata.ErrorKey1, - testdata.LastCheckedAt, - ) - helpers.FailOnError(t, err) - - err = migration.SetDBVersion(db, dbDriver, 29) - helpers.FailOnError(t, err) - - for _, table := range tablesAffected { - var userID string - userIDQuery := fmt.Sprintf(selectUserIDQuery, table) - err = db.QueryRow(userIDQuery).Scan(&userID) - helpers.FailOnError(t, err) - // expect user_ids to be "0" - assert.Equal(t, userID, "0") - } -} -*/ diff --git a/migration/mig_0029_nullify_user_id_columns.go b/migration/mig_0029_nullify_user_id_columns.go deleted file mode 100644 index a08dbad5..00000000 --- a/migration/mig_0029_nullify_user_id_columns.go +++ /dev/null @@ -1,76 +0,0 @@ -// Copyright 2022 Red Hat, Inc -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// -// This migration nullifies user_id columns in every single table where such -// a column exists. Previously we used to mistakenly populate this colummn with account_number -// which used to serve the same purpose as org_id (as in one value to represent the whole organization), -// instead of the actual ID of a single user, like we thought we did. In some tables the user_id is not -// really used for functionality, because such tables are used for org-wide functionality, but we want to -// keep it for informational/debugging purposes. -// This migration requires corresponding changes in smart-proxy/aggregator to start populating the user_id -// columns with the proper values. - -package migration - -import ( - "database/sql" - "fmt" - - "github.com/RedHatInsights/insights-results-aggregator/types" - "github.com/rs/zerolog/log" -) - -const ( - nullifyUserIDQuery = "UPDATE %v SET user_id = '0'" -) - -var tablesToNullify = []string{ - "advisor_ratings", - "cluster_rule_toggle", - "cluster_rule_user_feedback", - "cluster_user_rule_disable_feedback", - "rule_disable", -} - -var mig0029NullifyUserIDColumns = Migration{ - StepUp: func(tx *sql.Tx, driver types.DBDriver) error { - - if driver == types.DBDriverPostgres { - for _, table := range tablesToNullify { - nullifyQuery := fmt.Sprintf(nullifyUserIDQuery, table) - - // exec query to nullify user_id in all rows - res, err := tx.Exec(nullifyQuery) - if err != nil { - return err - } - - // check number of affected rows - nOfRows, err := res.RowsAffected() - if err != nil { - log.Error().Err(err).Msg("unable to retrieve number of affected rows") - return err - } - - log.Info().Msgf("updated %d rows from table %v", nOfRows, table) - } - } - - return nil - }, - StepDown: func(tx *sql.Tx, driver types.DBDriver) error { - log.Info().Msg("Mig0028 is a one-way ticket. Nothing to be done for StepDown.") - return nil - }, -} From 878c7dc86e4b1116a4b815247996f0a0f918cbee Mon Sep 17 00:00:00 2001 From: Daniel Bily Date: Thu, 22 Sep 2022 17:33:00 +0200 Subject: [PATCH 3/7] refactor auth mechanism to only expect one token format --- server/auth.go | 39 ++++++++------------------------------- server/auth_test.go | 15 +++++++++++++-- 2 files changed, 21 insertions(+), 33 deletions(-) diff --git a/server/auth.go b/server/auth.go index bcd2d40c..4a276497 100644 --- a/server/auth.go +++ b/server/auth.go @@ -36,9 +36,6 @@ const ( malformedTokenMessage = "Malformed authentication token" ) -// Internal contains information about organization ID -type Internal = types.Internal - // Identity contains internal user info type Identity = types.Identity @@ -78,8 +75,6 @@ func (server *HTTPServer) Authentication(next http.Handler, noAuthURLs []string) } tk := &types.Token{} - tkV2 := &types.TokenV2{} - // if we took JWT token, it has different structure than x-rh-identity if server.Config.AuthType == "jwt" { jwtPayload := &types.JWTPayload{} @@ -93,26 +88,13 @@ func (server *HTTPServer) Authentication(next http.Handler, noAuthURLs []string) // Map JWT token to inner token tk.Identity = types.Identity{ AccountNumber: jwtPayload.AccountNumber, - Internal: types.Internal{ - OrgID: jwtPayload.OrgID, - }, + OrgID: jwtPayload.OrgID, User: types.User{ UserID: jwtPayload.UserID, }, } } else { // auth type is xrh (x-rh-identity header) - - // unmarshal new token structure (org_id on top level) - err = json.Unmarshal(decoded, tkV2) - if err != nil { - // malformed token, returns with HTTP code 403 as usual - log.Error().Err(err).Msg(malformedTokenMessage) - handleServerError(w, &UnauthorizedError{ErrString: malformedTokenMessage}) - return - } - - // unmarshal old token structure (org_id nested) too err = json.Unmarshal(decoded, tk) if err != nil { // malformed token, returns with HTTP code 403 as usual @@ -122,25 +104,20 @@ func (server *HTTPServer) Authentication(next http.Handler, noAuthURLs []string) } } - if tkV2.IdentityV2.OrgID != 0 { - log.Info().Msg("org_id found on top level in token structure (new format)") - // fill in old types.Token because many places in smart-proxy and aggregator rely on it. - tk.Identity.Internal.OrgID = tkV2.IdentityV2.OrgID - } else { - log.Error().Msg("org_id not found on top level in token structure (old format)") - } - - if tk.Identity.AccountNumber == "" || tk.Identity.Internal.OrgID == 0 { - msg := fmt.Sprintf("error retrieving requester data from token. org_id [%v], account_number [%v], user data [%+v]", - tk.Identity.Internal.OrgID, + if tk.Identity.OrgID == 0 { + msg := fmt.Sprintf("error retrieving requester org_id from token. account_number [%v], user data [%+v]", tk.Identity.AccountNumber, - tkV2.IdentityV2.User, + tk.Identity.User, ) log.Error().Msg(msg) handleServerError(w, &UnauthorizedError{ErrString: msg}) return } + if tk.Identity.User.UserID == "" { + tk.Identity.User.UserID = "0" + } + // Everything went well, proceed with the request and set the // caller to the user retrieved from the parsed token ctx := context.WithValue(r.Context(), types.ContextKeyUser, tk.Identity) diff --git a/server/auth_test.go b/server/auth_test.go index 3afb1e3d..350b86eb 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -21,8 +21,11 @@ import ( "net/http" "testing" + "github.com/RedHatInsights/insights-results-aggregator-data/testdata" "github.com/RedHatInsights/insights-results-aggregator/server" "github.com/RedHatInsights/insights-results-aggregator/tests/helpers" + ctypes "github.com/RedHatInsights/insights-results-types" + types "github.com/RedHatInsights/insights-results-types" ) var configAuth = server.Configuration{ @@ -89,13 +92,21 @@ func TestInvalidJsonAuthToken(t *testing.T) { // TestBadOrganizationID checks if organization ID is checked properly func TestBadOrganizationID(t *testing.T) { providedOrgID := 12345 - orgIDInXRH := 1 + orgIDInXRH := 2 body := fmt.Sprintf(`{"status":"you have no permissions to get or change info about the organization with ID %v; you can access info about organization with ID %v"}`, providedOrgID, orgIDInXRH) helpers.AssertAPIRequest(t, nil, &configAuth, &helpers.APIRequest{ Method: http.MethodGet, Endpoint: server.ClustersForOrganizationEndpoint, EndpointArgs: []interface{}{providedOrgID}, - XRHIdentity: "eyJpZGVudGl0eSI6IHsiYWNjb3VudF9udW1iZXIiOiAiMDAwMDAwMSIsICJpbnRlcm5hbCI6IHsib3JnX2lkIjogIjEifX19Cg==", + XRHIdentity: helpers.MakeXRHTokenString(t, &types.Token{ + Identity: ctypes.Identity{ + AccountNumber: testdata.UserID, + OrgID: ctypes.OrgID(orgIDInXRH), + User: ctypes.User{ + UserID: testdata.UserID, + }, + }, + }), }, &helpers.APIResponse{ StatusCode: http.StatusForbidden, Body: body, From ab32d10548cd4b866593cbbec0975b742c8cbdcb Mon Sep 17 00:00:00 2001 From: Daniel Bily Date: Thu, 22 Sep 2022 17:34:26 +0200 Subject: [PATCH 4/7] clean up rest of aggregator code, remove some long dead/unused code --- aggregator_test.go | 42 ------------- server/rules.go | 4 -- server/server.go | 4 -- server/server_test.go | 31 +--------- server/vote_endpoints_benchmarks_test.go | 4 +- storage/storage.go | 12 ++-- storage/storage_rules_test.go | 75 ------------------------ tests/rest/rule_vote.go | 16 ++--- types/types.go | 2 - 9 files changed, 17 insertions(+), 173 deletions(-) diff --git a/aggregator_test.go b/aggregator_test.go index 23596dae..b0cf76fc 100644 --- a/aggregator_test.go +++ b/aggregator_test.go @@ -94,48 +94,6 @@ func TestStartService(t *testing.T) { *main.AutoMigratePtr = false } -// TODO: fix with new groups consumer -// func TestStartServiceWithMockBroker(t *testing.T) { -// const topicName = "topic" -// *main.AutoMigratePtr = true -// -// helpers.RunTestWithTimeout(t, func(t *testing.T) { -// mockBroker := sarama.NewMockBroker(t, 0) -// defer mockBroker.Close() -// -// mockBroker.SetHandlerByMap(helpers.GetHandlersMapForMockConsumer(t, mockBroker, topicName)) -// -// setEnvSettings(t, map[string]string{ -// "INSIGHTS_RESULTS_AGGREGATOR__BROKER__ADDRESS": mockBroker.Addr(), -// "INSIGHTS_RESULTS_AGGREGATOR__BROKER__TOPIC": topicName, -// "INSIGHTS_RESULTS_AGGREGATOR__BROKER__ENABLED": "true", -// -// "INSIGHTS_RESULTS_AGGREGATOR__SERVER__ADDRESS": ":8080", -// "INSIGHTS_RESULTS_AGGREGATOR__SERVER__API_PREFIX": "/api/v1/", -// "INSIGHTS_RESULTS_AGGREGATOR__SERVER__API_SPEC_FILE": "openapi.json", -// "INSIGHTS_RESULTS_AGGREGATOR__SERVER__DEBUG": "true", -// -// "INSIGHTS_RESULTS_AGGREGATOR__STORAGE__DB_DRIVER": "sqlite3", -// "INSIGHTS_RESULTS_AGGREGATOR__STORAGE__SQLITE_DATASOURCE": ":memory:", -// -// "INSIGHTS_RESULTS_AGGREGATOR__CONTENT__PATH": "./tests/content/ok/", -// }) -// -// go func() { -// exitCode := main.StartService() -// if exitCode != main.ExitStatusOK { -// panic(fmt.Errorf("StartService exited with a code %v", exitCode)) -// } -// }() -// -// main.WaitForServiceToStart() -// errCode := main.StopService() -// assert.Equal(t, main.ExitStatusOK, errCode) -// }, testsTimeout) -// -// *main.AutoMigratePtr = false -//} - func TestStartService_DBError(t *testing.T) { helpers.RunTestWithTimeout(t, func(t testing.TB) { buf := new(bytes.Buffer) diff --git a/server/rules.go b/server/rules.go index f0a1be93..c0945d92 100644 --- a/server/rules.go +++ b/server/rules.go @@ -223,7 +223,6 @@ func (server HTTPServer) getRuleToggleMapForCluster( if err != nil { return toggleMap, err } - log.Info().Msgf("disabled rules for orgID [%+v]", disabledRules) for _, rule := range disabledRules { if rule.ClusterID != clusterName { @@ -239,8 +238,6 @@ func (server HTTPServer) getRuleToggleMapForCluster( toggleMap[compositeRuleID] = true } - log.Info().Msgf("disabled rules for cluster_id %v [%+v]", clusterName, toggleMap) - return toggleMap, nil } @@ -284,7 +281,6 @@ func (server HTTPServer) getFeedbackAndTogglesOnRules( } if disabled, found := togglesRules[compositeRuleID]; found { - log.Info().Msgf("rule %v disabled on cluster %v", compositeRuleID, clusterName) rules[i].Disabled = disabled } else { rules[i].Disabled = false diff --git a/server/server.go b/server/server.go index 94e161a1..2d8606ec 100644 --- a/server/server.go +++ b/server/server.go @@ -144,7 +144,6 @@ func (server *HTTPServer) readReportForCluster(writer http.ResponseWriter, reque // everything has been handled already return } - log.Info().Msgf("readReportForCluster start for cluster %v", clusterName) userID, successful := readUserID(writer, request) if !successful { @@ -169,9 +168,6 @@ func (server *HTTPServer) readReportForCluster(writer http.ResponseWriter, reque hitRulesCount := getHitRulesCount(reports) reports, err = server.getFeedbackAndTogglesOnRules(clusterName, userID, orgID, reports) - log.Info().Str("cluster_id", string(clusterName)).Int(orgIDStr, int(orgID)).Msgf( - "hit rules count %v, rules on report [%+v]", hitRulesCount, reports, - ) if err != nil { log.Error().Err(err).Msg("An error has occurred when getting feedback or toggles") diff --git a/server/server_test.go b/server/server_test.go index 8e2c9503..39e52b1e 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -404,33 +404,6 @@ func TestHTTPServer_UserFeedback_ClusterDoesNotExistError(t *testing.T) { } } -// TODO: make working with the new arch -// func TestHTTPServer_UserFeedback_RuleDoesNotExistError(t *testing.T) { -// mockStorage, closer := helpers.MustGetMockStorage(t, true) -// defer closer() -// -// err := mockStorage.WriteReportForCluster( -// testdata.OrgID, testdata.ClusterName, testdata.Report3Rules, testdata.LastCheckedAt, testdata.KafkaOffset, -// ) -// helpers.FailOnError(t, err) -// -// for _, endpoint := range []string{ -// server.LikeRuleEndpoint, server.DislikeRuleEndpoint, server.ResetVoteOnRuleEndpoint, -// } { -// helpers.AssertAPIRequest(t, mockStorage, nil, &helpers.APIRequest{ -// Method: http.MethodPut, -// Endpoint: endpoint, -// EndpointArgs: []interface{}{testdata.ClusterName, testdata.Rule1ID, testdata.UserID}, -// }, &helpers.APIResponse{ -// StatusCode: http.StatusNotFound, -// Body: fmt.Sprintf( -// `{"status": "Item with ID %v was not found in the storage"}`, -// testdata.Rule1ID, -// ), -// }) -// } -//} - func TestRuleFeedbackErrorBadClusterName(t *testing.T) { buf := new(bytes.Buffer) log.Logger = zerolog.New(buf) @@ -821,9 +794,7 @@ func TestHTTPServer_SaveDisableFeedback_Error_CheckUserClusterPermissions(t *tes XRHIdentity: helpers.MakeXRHTokenString(t, &types.Token{ Identity: ctypes.Identity{ AccountNumber: testdata.UserID, - Internal: ctypes.Internal{ - OrgID: testdata.Org2ID, - }, + OrgID: testdata.Org2ID, User: ctypes.User{ UserID: testdata.UserID, }, diff --git a/server/vote_endpoints_benchmarks_test.go b/server/vote_endpoints_benchmarks_test.go index d05e7a28..2cf3babc 100644 --- a/server/vote_endpoints_benchmarks_test.go +++ b/server/vote_endpoints_benchmarks_test.go @@ -119,9 +119,7 @@ func benchmarkHTTPServerVoteEndpointsWithStorage(b *testing.B, mockStorage stora // authorize user identity := types.Identity{ AccountNumber: userID, - Internal: types.Internal{ - OrgID: orgID, - }, + OrgID: orgID, User: types.User{ UserID: userID, }, diff --git a/storage/storage.go b/storage/storage.go index 625ed211..88e89aeb 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -683,8 +683,6 @@ func (storage DBStorage) ReadReportsForClusters(clusterNames []types.ClusterName func (storage DBStorage) ReadReportForCluster( orgID types.OrgID, clusterName types.ClusterName, ) ([]types.RuleOnReport, types.Timestamp, types.Timestamp, types.Timestamp, error) { - log.Info().Msgf("ReadReportForCluster start for cluster %v", clusterName) - var lastChecked time.Time var reportedAt time.Time var gatheredAtInDB sql.NullTime // to avoid problems @@ -695,7 +693,6 @@ func (storage DBStorage) ReadReportForCluster( "SELECT last_checked_at, reported_at, gathered_at FROM report WHERE org_id = $1 AND cluster = $2;", orgID, clusterName, ).Scan(&lastChecked, &reportedAt, &gatheredAtInDB) - log.Info().Msgf("ReadReportForCluster query from report table raw DB error %v for cluster %v", err, clusterName) // convert timestamps to string var lastCheckedStr = types.Timestamp(lastChecked.UTC().Format(time.RFC3339)) @@ -710,18 +707,21 @@ func (storage DBStorage) ReadReportForCluster( err = types.ConvertDBError(err, []interface{}{orgID, clusterName}) if err != nil { - log.Error().Err(err).Msgf("ReadReportForCluster query from report table converted DB error %v for cluster %v", err, clusterName) + log.Error().Err(err).Str(clusterKey, string(clusterName)).Msg( + "ReadReportForCluster query from report table error", + ) return report, lastCheckedStr, reportedAtStr, gatheredAtStr, err } rows, err := storage.connection.Query( "SELECT template_data, rule_fqdn, error_key, created_at FROM rule_hit WHERE org_id = $1 AND cluster_id = $2;", orgID, clusterName, ) - log.Info().Msgf("ReadReportForCluster query from rule_hit table raw DB error %v for cluster %v", err, clusterName) err = types.ConvertDBError(err, []interface{}{orgID, clusterName}) if err != nil { - log.Info().Msgf("ReadReportForCluster query from rule_hit table converted DB error %v for cluster %v", err, clusterName) + log.Error().Err(err).Str(clusterKey, string(clusterName)).Msg( + "ReadReportForCluster query from rule_hit table error", + ) return report, lastCheckedStr, reportedAtStr, gatheredAtStr, err } diff --git a/storage/storage_rules_test.go b/storage/storage_rules_test.go index 206efc85..404e99ae 100644 --- a/storage/storage_rules_test.go +++ b/storage/storage_rules_test.go @@ -193,7 +193,6 @@ func TestDBStorageListRulesReasonsEmptyDB(t *testing.T) { // TestDBStorageListOfRulesReasonsOneRule checks that one rule is returned // for non empty DB. -// TODO: enable when user_id is properly handled (stored) into database! func TestDBStorageListOfRulesReasonsOneRule(t *testing.T) { mockStorage, closer := ira_helpers.MustGetMockStorage(t, true) defer closer() @@ -339,57 +338,6 @@ func TestDBStorageListOfDisabledRulesNoRule(t *testing.T) { assert.Len(t, disabledRules, 0) } -// TODO: make it work with the new arch -// func TestDBStorageToggleRulesAndList(t *testing.T) { -// mockStorage, closer := ira_helpers.MustGetMockStorage(t, true) -// defer closer() -// -// mustWriteReport3Rules(t, mockStorage) -// -// helpers.FailOnError(t, mockStorage.ToggleRuleForCluster( -// testdata.ClusterName, testdata.Rule1ID, testdata.UserID, storage.RuleToggleDisable, -// )) -// -// helpers.FailOnError(t, mockStorage.ToggleRuleForCluster( -// testdata.ClusterName, testdata.Rule2ID, testdata.UserID, storage.RuleToggleDisable, -// )) -// -// toggledRules, err := mockStorage.ListDisabledRulesForCluster(testdata.ClusterName, testdata.UserID) -// helpers.FailOnError(t, err) -// -// assert.Len(t, toggledRules, 2) -//} - -// TODO: make it work with the new arch -// func TestDBStorageDeleteDisabledRule(t *testing.T) { -// mockStorage, closer := ira_helpers.MustGetMockStorage(t, true) -// defer closer() -// -// mustWriteReport3Rules(t, mockStorage) -// -// helpers.FailOnError(t, mockStorage.ToggleRuleForCluster( -// testdata.ClusterName, testdata.Rule1ID, testdata.UserID, storage.RuleToggleDisable, -// )) -// -// helpers.FailOnError(t, mockStorage.ToggleRuleForCluster( -// testdata.ClusterName, testdata.Rule2ID, testdata.UserID, storage.RuleToggleDisable, -// )) -// -// toggledRules, err := mockStorage.ListDisabledRulesForCluster(testdata.ClusterName, testdata.UserID) -// helpers.FailOnError(t, err) -// -// assert.Len(t, toggledRules, 2) -// -// helpers.FailOnError(t, mockStorage.DeleteFromRuleClusterToggle( -// testdata.ClusterName, testdata.Rule2ID, testdata.UserID, -// )) -// -// toggledRules, err = mockStorage.ListDisabledRulesForCluster(testdata.ClusterName, testdata.UserID) -// helpers.FailOnError(t, err) -// -// assert.Len(t, toggledRules, 1) -//} - func TestDBStorageVoteOnRule(t *testing.T) { for _, vote := range []types.UserVote{ types.UserVoteDislike, types.UserVoteLike, types.UserVoteNone, @@ -435,29 +383,6 @@ func TestDBStorageVoteOnRule_NoCluster(t *testing.T) { } } -// TODO: fix according to the new architecture -// func TestDBStorageVoteOnRule_NoRule(t *testing.T) { -// for _, vote := range []types.UserVote{ -// types.UserVoteDislike, types.UserVoteLike, types.UserVoteNone, -// } { -// func(vote types.UserVote) { -// mockStorage, closer := ira_helpers.MustGetMockStorage(t, true) -// defer closer() -// -// err := mockStorage.WriteReportForCluster( -// testdata.OrgID, testdata.ClusterName, report3Rules, testdata.LastCheckedAt, testdata.KafkaOffset, -// ) -// helpers.FailOnError(t, err) -// -// err = mockStorage.VoteOnRule( -// testdata.ClusterName, testdata.Rule1ID, testdata.UserID, vote, -// ) -// assert.Error(t, err) -// assert.Regexp(t, "operation violates foreign key", err.Error()) -// }(vote) -// } -//} - func TestDBStorageChangeVote(t *testing.T) { mockStorage, closer := ira_helpers.MustGetMockStorage(t, true) defer closer() diff --git a/tests/rest/rule_vote.go b/tests/rest/rule_vote.go index 80ed386a..e74b4b5a 100644 --- a/tests/rest/rule_vote.go +++ b/tests/rest/rule_vote.go @@ -35,6 +35,8 @@ const ( anyErrorKey = "0" // we don't care unexpectedErrorStatusMessage = "Expected error status, but got '%s' instead" + + orgID = "1" ) var knownClustersForOrganization1 []string = []string{ @@ -170,15 +172,15 @@ func reproducerForIssue385() { const message = "Reproducer for issue #385 (https://github.com/RedHatInsights/insights-results-aggregator/issues/385)" // like - url := constructURLLikeRule(improperClusterID, anyRule, anyErrorKey, fmt.Sprintf("%v", testdata.OrgID), string(testdata.UserID)) + url := constructURLLikeRule(improperClusterID, anyRule, anyErrorKey, orgID, string(testdata.UserID)) checkInvalidUUIDFormatPut(url, message) // dislike - url = constructURLDislikeRule(improperClusterID, anyRule, anyErrorKey, fmt.Sprintf("%v", testdata.OrgID), string(testdata.UserID)) + url = constructURLDislikeRule(improperClusterID, anyRule, anyErrorKey, orgID, string(testdata.UserID)) checkInvalidUUIDFormatPut(url, message) // reset vote - url = constructURLResetVoteForRule(improperClusterID, anyRule, anyErrorKey, fmt.Sprintf("%v", testdata.OrgID), string(testdata.UserID)) + url = constructURLResetVoteForRule(improperClusterID, anyRule, anyErrorKey, orgID, string(testdata.UserID)) checkInvalidUUIDFormatPut(url, message) } @@ -187,7 +189,7 @@ func testRuleVoteAPIendpoint(clusters, rules, errorKeys []string, message string for _, cluster := range clusters { for _, rule := range rules { for _, errorKey := range errorKeys { - url := urlConstructor(cluster, rule, errorKey, fmt.Sprintf("%v", testdata.OrgID), string(testdata.UserID)) + url := urlConstructor(cluster, rule, errorKey, orgID, string(testdata.UserID)) checker(url, message) } @@ -299,17 +301,17 @@ type RuleVoteResponse struct { } func likeRule(cluster, rule, errorKey string) { - url := constructURLLikeRule(cluster, rule, errorKey, fmt.Sprintf("%v", testdata.OrgID), string(testdata.UserID)) + url := constructURLLikeRule(cluster, rule, errorKey, orgID, string(testdata.UserID)) checkOkStatusUserVote(url, "Let's vote") } func dislikeRule(cluster, rule, errorKey string) { - url := constructURLDislikeRule(cluster, rule, errorKey, fmt.Sprintf("%v", testdata.OrgID), string(testdata.UserID)) + url := constructURLDislikeRule(cluster, rule, errorKey, orgID, string(testdata.UserID)) checkOkStatusUserVote(url, "Let's dislike") } func resetVoteForRule(cluster, rule, errorKey string) { - url := constructURLResetVoteForRule(cluster, rule, errorKey, fmt.Sprintf("%v", testdata.OrgID), string(testdata.UserID)) + url := constructURLResetVoteForRule(cluster, rule, errorKey, orgID, string(testdata.UserID)) checkOkStatusUserVote(url, "Let's reset voting") } diff --git a/types/types.go b/types/types.go index 9b9db6a2..c06f546c 100644 --- a/types/types.go +++ b/types/types.go @@ -85,8 +85,6 @@ type ( KafkaOffset = types.KafkaOffset // DBDriver type for db driver enum DBDriver = types.DBDriver - // Internal contains information about organization ID - Internal = types.Internal // Identity contains internal user info Identity = types.Identity // Token is x-rh-identity struct From a5ad8e61c81d64399e0fa3add874821781d732dd Mon Sep 17 00:00:00 2001 From: Daniel Bily Date: Thu, 22 Sep 2022 17:35:51 +0200 Subject: [PATCH 5/7] decrease gocyclo limit after refactoring --- gocyclo.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gocyclo.sh b/gocyclo.sh index 6732c365..a5e28219 100755 --- a/gocyclo.sh +++ b/gocyclo.sh @@ -20,4 +20,4 @@ then go get github.com/fzipp/gocyclo/cmd/gocyclo fi -gocyclo -over 12 -avg . +gocyclo -over 10 -avg . From 753d89697626b101b7711c0395cf49205f664548 Mon Sep 17 00:00:00 2001 From: Daniel Bily Date: Thu, 22 Sep 2022 17:36:01 +0200 Subject: [PATCH 6/7] increase code coverage limit slighlty --- check_coverage.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/check_coverage.sh b/check_coverage.sh index 95b5840c..35500721 100755 --- a/check_coverage.sh +++ b/check_coverage.sh @@ -14,7 +14,7 @@ # limitations under the License. -THRESHOLD=${COV_THRESHOLD:=77} +THRESHOLD=${COV_THRESHOLD:=78} RED_BG=$(tput setab 1) GREEN_BG=$(tput setab 2) From 90d5c889fef9d17738a209156a6e5cf7bb4296fc Mon Sep 17 00:00:00 2001 From: Daniel Bily Date: Fri, 23 Sep 2022 11:05:45 +0200 Subject: [PATCH 7/7] fix rest api tests to expect new token --- tests/rest/common.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/rest/common.go b/tests/rest/common.go index 79e98148..af4c765e 100644 --- a/tests/rest/common.go +++ b/tests/rest/common.go @@ -76,9 +76,7 @@ func setAuthHeaderForOrganization(f *frisby.Frisby, orgID int) { plainHeader := fmt.Sprintf(` { "identity": { - "internal": { - "org_id": "%d" - }, + "org_id": "%d", "account_number":"%v", "user": { "user_id":"%v"