From d1ce9965447461f4899fd9a48b9f6a33fdafda19 Mon Sep 17 00:00:00 2001 From: Daniel Bily Date: Sun, 6 Feb 2022 19:30:07 +0100 Subject: [PATCH 1/4] add endpoint to get list of user disabled rules for given cluster list --- server/endpoints.go | 12 ++- server/rules.go | 36 +++++++++ server/server_test.go | 18 +++++ storage/noop_storage.go | 9 +++ storage/noop_storage_test.go | 1 + storage/rule_list.go | 65 ++++++++++++++- storage/storage.go | 4 + storage/storage_rules_test.go | 148 ++++++++++++++++++++++++++++++++++ 8 files changed, 289 insertions(+), 4 deletions(-) diff --git a/server/endpoints.go b/server/endpoints.go index 8998d343..a9d0ffef 100644 --- a/server/endpoints.go +++ b/server/endpoints.go @@ -63,6 +63,8 @@ const ( DisableRuleFeedbackEndpoint = "clusters/{cluster}/rules/{rule_id}/error_key/{error_key}/users/{user_id}/disable_feedback" // ListOfDisabledRules returns a list of rules disabled from current account ListOfDisabledRules = "rules/users/{user_id}/disabled" + // ListOfDisabledRulesForClusters returns a list of rules disabled from current account for given list of clusters in POST body + ListOfDisabledRulesForClusters = "rules/users/{user_id}/disabled_for_clusters" // ListOfDisabledRulesFeedback returns a list of reasons why rule has been disabled ListOfDisabledRulesFeedback = "rules/users/{user_id}/disabled/feedback" // ListOfDisabledClusters returns a list of clusters which the user disabled for a rule with latest justification @@ -131,12 +133,10 @@ func (server *HTTPServer) addEndpointsToRouter(router *mux.Router) { router.HandleFunc(apiPrefix+DislikeRuleEndpoint, server.dislikeRule).Methods(http.MethodPut, http.MethodOptions) router.HandleFunc(apiPrefix+ResetVoteOnRuleEndpoint, server.resetVoteOnRule).Methods(http.MethodPut, http.MethodOptions) router.HandleFunc(apiPrefix+ClustersForOrganizationEndpoint, server.listOfClustersForOrganization).Methods(http.MethodGet) - router.HandleFunc(apiPrefix+DisableRuleForClusterEndpoint, server.disableRuleForCluster).Methods(http.MethodPut, http.MethodOptions) - router.HandleFunc(apiPrefix+EnableRuleForClusterEndpoint, server.enableRuleForCluster).Methods(http.MethodPut, http.MethodOptions) - router.HandleFunc(apiPrefix+DisableRuleFeedbackEndpoint, server.saveDisableFeedback).Methods(http.MethodPost) router.HandleFunc(apiPrefix+ReportForListOfClustersEndpoint, server.reportForListOfClusters).Methods(http.MethodGet) router.HandleFunc(apiPrefix+ReportForListOfClustersPayloadEndpoint, server.reportForListOfClustersPayload).Methods(http.MethodPost) router.HandleFunc(apiPrefix+ListOfDisabledRules, server.listOfDisabledRules).Methods(http.MethodGet) + router.HandleFunc(apiPrefix+ListOfDisabledRulesForClusters, server.listOfDisabledRulesForClusters).Methods(http.MethodPost, http.MethodOptions) router.HandleFunc(apiPrefix+ListOfDisabledRulesFeedback, server.listOfReasons).Methods(http.MethodGet) router.HandleFunc(apiPrefix+ListOfDisabledClusters, server.listOfDisabledClusters).Methods(http.MethodGet) router.HandleFunc(apiPrefix+Rating, server.setRuleRating).Methods(http.MethodPost) @@ -163,6 +163,12 @@ func (server *HTTPServer) addEndpointsToRouter(router *mux.Router) { // addRuleEnableDisableEndpointsToRouter method registers handlers for endpoints that // allow for rules to be enabled, disabled, updated, and queried system-wide func (server *HTTPServer) addRuleEnableDisableEndpointsToRouter(router *mux.Router, apiPrefix string) { + // single cluster disable functionality + router.HandleFunc(apiPrefix+DisableRuleForClusterEndpoint, server.disableRuleForCluster).Methods(http.MethodPut, http.MethodOptions) + router.HandleFunc(apiPrefix+EnableRuleForClusterEndpoint, server.enableRuleForCluster).Methods(http.MethodPut, http.MethodOptions) + router.HandleFunc(apiPrefix+DisableRuleFeedbackEndpoint, server.saveDisableFeedback).Methods(http.MethodPost) + + // system-wide (acknowledge) disable functionality router.HandleFunc(apiPrefix+EnableRuleSystemWide, server.enableRuleSystemWide).Methods(http.MethodPut, http.MethodOptions) router.HandleFunc(apiPrefix+DisableRuleSystemWide, server.disableRuleSystemWide).Methods(http.MethodPut, http.MethodOptions) router.HandleFunc(apiPrefix+UpdateRuleSystemWide, server.updateRuleSystemWide).Methods(http.MethodPost, http.MethodOptions) diff --git a/server/rules.go b/server/rules.go index 1ba8a2ac..2aafbf1c 100644 --- a/server/rules.go +++ b/server/rules.go @@ -15,6 +15,7 @@ package server import ( + "encoding/json" "net/http" "time" @@ -129,6 +130,41 @@ func (server HTTPServer) listOfReasons(writer http.ResponseWriter, request *http } } +// listOfDisabledRulesForClusters returns list of rules disabled from an account for given clusters +func (server HTTPServer) listOfDisabledRulesForClusters(writer http.ResponseWriter, request *http.Request) { + // Extract user_id from URL + userID, ok := readUserID(writer, request) + if !ok { + // everything has been handled + return + } + log.Info().Str(userIDstr, string(userID)).Msg("listOfDisabledRulesForClusters") + + var listOfClusters []string + err := json.NewDecoder(request.Body).Decode(&listOfClusters) + if err != nil { + handleServerError(writer, err) + return + } + log.Info().Str(userIDstr, string(userID)).Msgf("listOfDisabledRulesForClusters number of clusters: %d", len(listOfClusters)) + + // try to read list of disabled rules by an account/user from database for given list of clusters + disabledRules, err := server.Storage.ListOfDisabledRulesForClusters(listOfClusters, userID) + if err != nil { + log.Error().Err(err).Msg("Unable to read list of disabled rules") + handleServerError(writer, err) + return + } + log.Info().Str(userIDstr, string(userID)).Int("#disabled rules", len(disabledRules)).Msg("listOfDisabledRulesForClusters") + + // try to send JSON payload to the client in a HTTP response + err = responses.SendOK(writer, + responses.BuildOkResponseWithData("rules", disabledRules)) + if err != nil { + log.Error().Err(err).Msg(responseDataError) + } +} + // listOfDisabledClusters returns list of clusters disabled for a rule and user func (server HTTPServer) listOfDisabledClusters(writer http.ResponseWriter, request *http.Request) { log.Info().Msg("Lisf of disabled clusters") diff --git a/server/server_test.go b/server/server_test.go index eba161a3..5c33e07f 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -834,6 +834,24 @@ func TestHTTPServer_ListOfReasons(t *testing.T) { }) } +func TestHTTPServer_ListDisabledRulesForClusters(t *testing.T) { + mockStorage, closer := helpers.MustGetMockStorage(t, true) + defer closer() + + clusterList := []types.ClusterName{testdata.ClusterName} + reqBody, _ := json.Marshal(clusterList) + + helpers.AssertAPIRequest(t, mockStorage, nil, &helpers.APIRequest{ + Method: http.MethodPost, + Endpoint: server.ListOfDisabledRulesForClusters, + EndpointArgs: []interface{}{testdata.UserID}, + Body: reqBody, + }, &helpers.APIResponse{ + StatusCode: http.StatusOK, + Body: `{"rules":[],"status":"ok"}`, + }) +} + func TestHTTPServer_EnableRuleSystemWide(t *testing.T) { mockStorage, closer := helpers.MustGetMockStorage(t, true) defer closer() diff --git a/storage/noop_storage.go b/storage/noop_storage.go index 5fba9442..d030dbfd 100644 --- a/storage/noop_storage.go +++ b/storage/noop_storage.go @@ -262,6 +262,15 @@ func (*NoopStorage) ListOfDisabledClusters( return nil, nil } +// ListOfDisabledRulesForClusters function returns list of disabled rules for given clusters from a +// specified account (noop). +func (*NoopStorage) ListOfDisabledRulesForClusters( + clusterList []string, + userID types.UserID, +) ([]ctypes.DisabledRule, error) { + return nil, nil +} + // RateOnRule function stores the vote (rating) given by an user to a rule+error key func (*NoopStorage) RateOnRule( types.UserID, diff --git a/storage/noop_storage_test.go b/storage/noop_storage_test.go index 0020f6f0..32e3aab1 100644 --- a/storage/noop_storage_test.go +++ b/storage/noop_storage_test.go @@ -72,6 +72,7 @@ func TestNoopStorage_Methods_Cont(t *testing.T) { _, _ = noopStorage.DoesClusterExist("") _, _ = noopStorage.ListOfDisabledRules("") _, _ = noopStorage.ListOfReasons("") + _, _ = noopStorage.ListOfDisabledRulesForClusters([]string{}, types.UserID("99")) _ = noopStorage.WriteRecommendationsForCluster(0, "", "", "") _ = noopStorage.RateOnRule(types.UserID("99"), types.OrgID(1), "", "", types.UserVote(1)) diff --git a/storage/rule_list.go b/storage/rule_list.go index 9664698c..12de7d50 100644 --- a/storage/rule_list.go +++ b/storage/rule_list.go @@ -16,6 +16,7 @@ package storage import ( "database/sql" + "fmt" "github.com/rs/zerolog/log" @@ -123,7 +124,69 @@ func (storage DBStorage) ListOfDisabledRules(userID types.UserID) ([]ctypes.Disa &disabledRule.Disabled) if err != nil { - log.Error().Err(err).Msg("ReadListOfDisabledRules") + log.Error().Err(err).Msg("ReadListOfDisabledRules database error") + // return partially filled slice + error + return disabledRules, err + } + + // append disabled rule read from database to a slice + disabledRules = append(disabledRules, disabledRule) + } + + // everything seems ok -> return slice with all the data + return disabledRules, nil +} + +// ListOfDisabledRulesForClusters function returns list of all rules disabled from a +// specified account for given list of clusters. +func (storage DBStorage) ListOfDisabledRulesForClusters( + clusterList []string, + userID types.UserID, +) ([]ctypes.DisabledRule, error) { + disabledRules := make([]ctypes.DisabledRule, 0) + + if len(clusterList) < 1 { + return disabledRules, nil + } + + // #nosec G201 + whereClause := fmt.Sprintf(`WHERE user_id = $1 AND disabled = $2 AND cluster_id IN (%v)`, inClauseFromSlice(clusterList)) + + // disable "G202 (CWE-89): SQL string concatenation" + // #nosec G202 + query := ` + SELECT + cluster_id, + rule_id, + error_key, + disabled_at, + updated_at, + disabled + FROM + cluster_rule_toggle + ` + whereClause + + // run the query against database + rows, err := storage.connection.Query(query, userID, RuleToggleDisable) + + // return empty list in case of any error + if err != nil { + return disabledRules, err + } + defer closeRows(rows) + + for rows.Next() { + var disabledRule ctypes.DisabledRule + + err = rows.Scan(&disabledRule.ClusterID, + &disabledRule.RuleID, + &disabledRule.ErrorKey, + &disabledRule.DisabledAt, + &disabledRule.UpdatedAt, + &disabledRule.Disabled) + + if err != nil { + log.Error().Err(err).Msg("ReadListOfDisabledRulesForClusters database error") // return partially filled slice + error return disabledRules, err } diff --git a/storage/storage.go b/storage/storage.go index 88416e95..cd07c715 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -158,6 +158,10 @@ type Storage interface { DoesClusterExist(clusterID types.ClusterName) (bool, error) ListOfDisabledRules(userID types.UserID) ([]ctypes.DisabledRule, error) ListOfReasons(userID types.UserID) ([]DisabledRuleReason, error) + ListOfDisabledRulesForClusters( + clusterList []string, + userID types.UserID, + ) ([]ctypes.DisabledRule, error) ListOfDisabledClusters( userID types.UserID, ruleID types.RuleID, diff --git a/storage/storage_rules_test.go b/storage/storage_rules_test.go index 416bc823..e183a9d8 100644 --- a/storage/storage_rules_test.go +++ b/storage/storage_rules_test.go @@ -1100,3 +1100,151 @@ func TestDBStorageListOfDisabledClustersFeedbackUpdate(t *testing.T) { assert.Equal(t, clusters[0], disabledCluster.ClusterID) assert.Equal(t, newFeedback, disabledCluster.Justification) } + +// TestDBStorageListOfDisabledRulesForClustersDBError checks for DB error. +func TestDBStorageListOfDisabledRulesForClustersDBError(t *testing.T) { + mockStorage, closer := ira_helpers.MustGetMockStorage(t, true) + closer() + + clusters := make([]string, 3) + for i := range clusters { + clusters[i] = string(testdata.GetRandomClusterID()) + } + + // try to read list of disabled rules + _, err := mockStorage.ListOfDisabledRulesForClusters(clusters, testdata.UserID) + assert.EqualError(t, err, "sql: database is closed") +} + +// TestDBStorageListOfDisabledRulesForClustersEmptyDB checks that no rules are returned +// for empty DB. +func TestDBStorageListOfDisabledRulesForClustersEmptyDB(t *testing.T) { + mockStorage, closer := ira_helpers.MustGetMockStorage(t, true) + defer closer() + + clusters := make([]string, 3) + for i := range clusters { + clusters[i] = string(testdata.GetRandomClusterID()) + } + + // try to read list of disabled rules + disabledRules, err := mockStorage.ListOfDisabledRulesForClusters(clusters, testdata.UserID) + helpers.FailOnError(t, err) + + // we expect no rules to be returned + assert.Len(t, disabledRules, 0) +} + +// TestDBStorageListOfDisabledRulesForClustersOneRule checks that one rule is returned +// for non empty DB and selected cluster +func TestDBStorageListOfDisabledRulesForClustersOneRule(t *testing.T) { + mockStorage, closer := ira_helpers.MustGetMockStorage(t, true) + defer closer() + + clusters := make([]string, 3) + for i := range clusters { + clusterID := testdata.GetRandomClusterID() + clusters[i] = string(clusterID) + mustWriteReport3RulesForCluster(t, mockStorage, clusterID) + } + + // write some rules into database + mustWriteReport3Rules(t, mockStorage) + + // disable one rule + helpers.FailOnError(t, mockStorage.ToggleRuleForCluster( + ctypes.ClusterName(clusters[0]), testdata.Rule1ID, testdata.ErrorKey1, + testdata.UserID, storage.RuleToggleDisable, + )) + + // try to read list of disabled rules + disabledRules, err := mockStorage.ListOfDisabledRulesForClusters(clusters, testdata.UserID) + helpers.FailOnError(t, err) + + // we expect 1 rule to be returned + assert.Len(t, disabledRules, 1) + + // check the content of returned data + disabledRule := disabledRules[0] + assert.Equal(t, ctypes.ClusterName(clusters[0]), disabledRule.ClusterID) + assert.Equal(t, testdata.Rule1ID, disabledRule.RuleID) + assert.Equal(t, testdata.ErrorKey1, string(disabledRule.ErrorKey)) + assert.Equal(t, int(storage.RuleToggleDisable), int(disabledRule.Disabled)) +} + +// TestDBStorageListOfDisabledRulesForClustersTwoRules checks that two rules are returned +// for non empty DB. +func TestDBStorageListOfDisabledRulesForClustersTwoRules(t *testing.T) { + mockStorage, closer := ira_helpers.MustGetMockStorage(t, true) + defer closer() + + clusters := make([]string, 3) + for i := range clusters { + clusterID := testdata.GetRandomClusterID() + clusters[i] = string(clusterID) + mustWriteReport3RulesForCluster(t, mockStorage, clusterID) + } + + // write some rules into database + mustWriteReport3Rules(t, mockStorage) + + // disable same rule, different clusters + helpers.FailOnError(t, mockStorage.ToggleRuleForCluster( + ctypes.ClusterName(clusters[0]), testdata.Rule1ID, testdata.ErrorKey1, + testdata.UserID, storage.RuleToggleDisable, + )) + helpers.FailOnError(t, mockStorage.ToggleRuleForCluster( + ctypes.ClusterName(clusters[1]), testdata.Rule1ID, testdata.ErrorKey1, + testdata.UserID, storage.RuleToggleDisable, + )) + + // try to read list of disabled rules + disabledRules, err := mockStorage.ListOfDisabledRulesForClusters(clusters, testdata.UserID) + helpers.FailOnError(t, err) + + // we expect 2 rules to be returned + assert.Len(t, disabledRules, 2) +} + +// TestDBStorageListOfDisabledRulesForClustersNoRule checks that no rule is returned +// for cluster that wasnt requested +func TestDBStorageListOfDisabledRulesForClustersNoRule(t *testing.T) { + mockStorage, closer := ira_helpers.MustGetMockStorage(t, true) + defer closer() + + clusters := make([]string, 3) + for i := range clusters { + clusterID := testdata.GetRandomClusterID() + clusters[i] = string(clusterID) + mustWriteReport3RulesForCluster(t, mockStorage, clusterID) + } + + // write some rules into database + mustWriteReport3Rules(t, mockStorage) + + // disable one rule for one cluster + helpers.FailOnError(t, mockStorage.ToggleRuleForCluster( + ctypes.ClusterName(clusters[0]), testdata.Rule1ID, testdata.ErrorKey1, + testdata.UserID, storage.RuleToggleDisable, + )) + + // try to read list of disabled rules, we're requesting a list for a different list of clusters + disabledRules, err := mockStorage.ListOfDisabledRulesForClusters([]string{clusters[1], clusters[2]}, testdata.UserID) + helpers.FailOnError(t, err) + + // we expect no rules to be returned + assert.Len(t, disabledRules, 0) + + // disable one rule for one of the clusters we want + helpers.FailOnError(t, mockStorage.ToggleRuleForCluster( + ctypes.ClusterName(clusters[1]), testdata.Rule1ID, testdata.ErrorKey1, + testdata.UserID, storage.RuleToggleDisable, + )) + + // try to read list of disabled rules, this time there should be one rule among them + disabledRules, err = mockStorage.ListOfDisabledRulesForClusters([]string{clusters[1], clusters[2]}, testdata.UserID) + helpers.FailOnError(t, err) + + // we expect 1 rule + assert.Len(t, disabledRules, 1) +} From 86a55b5e6c0ca6ce2ccc8d414a0e000645e8b28e Mon Sep 17 00:00:00 2001 From: Daniel Bily Date: Sun, 6 Feb 2022 19:30:47 +0100 Subject: [PATCH 2/4] update OpenAPI spec with list of user disabled rules for given clusters --- openapi.json | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/openapi.json b/openapi.json index 326d5e9e..6cc75124 100644 --- a/openapi.json +++ b/openapi.json @@ -1616,6 +1616,46 @@ ] } }, + "/rules/users/{userId}/disabled_for_clusters": { + "post": { + "summary": "Returns a list of rules disabled from current account for given list of clusters", + "operationId": "ListOfDisabledRulesForClusters", + "description": "Returns a list of rules disabled from current account for given list of clusters", + "parameters": [ + { + "name": "userId", + "in": "path", + "required": true, + "description": "Numeric ID of the user. An example: `42`", + "schema": { + "type": "string" + } + } + ], + "requestBody": { + "description": "List of cluster IDs. Each ID must conform to UUID format. An example: `34c3ecc5-624a-49a5-bab8-4fdc5e51a266.", + "required": true, + "content": { + "text/plain": { + "schema": { + } + } + } + }, + "responses": { + "200": { + "description": "List of disabled rules", + "content": { + "application/json": { + } + } + } + }, + "tags": [ + "prod" + ] + } + }, "/rules/users/{userId}/disabled/feedback": { "get": { "summary": "Returns a list of reasons why rule or rules has been disabled", From c0b2eafb6b2a8a29d0d5266b80685ae8f206e7e4 Mon Sep 17 00:00:00 2001 From: Daniel Bily Date: Thu, 10 Feb 2022 11:31:59 +0100 Subject: [PATCH 3/4] bump types package --- go.mod | 2 +- go.sum | 10 ++-------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 9ddcb665..14dd21b3 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ require ( github.com/RedHatInsights/insights-content-service v0.0.0-20201009081018-083923779f00 github.com/RedHatInsights/insights-operator-utils v1.23.3 github.com/RedHatInsights/insights-results-aggregator-data v1.3.3 - github.com/RedHatInsights/insights-results-types v1.3.5 + github.com/RedHatInsights/insights-results-types v1.3.6 github.com/Shopify/sarama v1.27.1 github.com/deckarep/golang-set v1.7.1 github.com/dgrijalva/jwt-go v3.2.0+incompatible diff --git a/go.sum b/go.sum index 6d6ada47..b88cd737 100644 --- a/go.sum +++ b/go.sum @@ -56,8 +56,8 @@ github.com/RedHatInsights/insights-results-aggregator-data v1.3.2/go.mod h1:E1Ua github.com/RedHatInsights/insights-results-aggregator-data v1.3.3 h1:K6j+Sr+S0iUqFEfevl+MI0dEXn5AxFeJn9FTU5razWA= github.com/RedHatInsights/insights-results-aggregator-data v1.3.3/go.mod h1:udHNC7lBxYnu9AqMahABqvuclCzWUWSkbacQbUaehfI= github.com/RedHatInsights/insights-results-types v1.2.0/go.mod h1:6VVdMTGU/BAS2cW0KrHAUiDyocpyKqpPpEyp6AJ1tk8= -github.com/RedHatInsights/insights-results-types v1.3.5 h1:Q1awNYeJqIFgst7zQgKfQwbdMieyJTG/eDIau+yIUpo= -github.com/RedHatInsights/insights-results-types v1.3.5/go.mod h1:6VVdMTGU/BAS2cW0KrHAUiDyocpyKqpPpEyp6AJ1tk8= +github.com/RedHatInsights/insights-results-types v1.3.6 h1:mC5vZn4hK4f6L/Y4MSv0EsN/Z1avAh5qWB2Z2m11PWk= +github.com/RedHatInsights/insights-results-types v1.3.6/go.mod h1:6VVdMTGU/BAS2cW0KrHAUiDyocpyKqpPpEyp6AJ1tk8= github.com/RedHatInsights/kafka-zerolog v0.0.0-20210304172207-928f026dc7ec h1:/msFfckx6EIj0rZncrMUfNixFvsLbOiRIe4J0AurhDo= github.com/RedHatInsights/kafka-zerolog v0.0.0-20210304172207-928f026dc7ec/go.mod h1:HJul5oCsCRNiRlh/ayJDGdW3PzGlid/5aaQwJBn7was= github.com/Shopify/goreferrer v0.0.0-20181106222321-ec9c9a553398/go.mod h1:a1uqRtAwp2Xwc6WNPJEufxJ7fx3npB4UV/JOLmbu5I0= @@ -221,7 +221,6 @@ github.com/golang/groupcache v0.0.0-20190702054246-869f871628b6/go.mod h1:cIg4er github.com/golang/mock v1.1.1/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A= github.com/golang/mock v1.2.0/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A= github.com/golang/mock v1.3.1/go.mod h1:sBzyDLLjw3U8JLTeZvSv8jJB+tU5PVekmnlKIyFUx0Y= -github.com/golang/mock v1.4.3 h1:GV+pQPG/EUUbkh47niozDcADz6go/dUwhVzdUQHIVRw= github.com/golang/mock v1.4.3/go.mod h1:UOMv5ysSaYNkG+OFQykRIcU/QvvxJf3p21QfJ2Bt3cw= github.com/golang/mock v1.6.0 h1:ErTB+efbowRARo13NNdxyJji2egdxLGQhRaY+DUumQc= github.com/golang/mock v1.6.0/go.mod h1:p6yTPP+5HYm5mzsMV8JkE6ZKdX+/wYM6Hr+LicevLPs= @@ -260,7 +259,6 @@ github.com/google/pprof v0.0.0-20190515194954-54271f7e092f/go.mod h1:zfwlbNMJ+OI github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI= github.com/google/uuid v1.0.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= -github.com/google/uuid v1.1.2 h1:EVhdT+1Kseyi1/pUmXKaFxYsDNy9RQYkMWRH68J/W7Y= github.com/google/uuid v1.1.2/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I= github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= @@ -602,7 +600,6 @@ github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXf github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA= -github.com/stretchr/testify v1.6.1 h1:hDPOHmpOpP40lSULcqw7IrRb/u7w6RpDC9399XyoNd0= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= @@ -727,7 +724,6 @@ golang.org/x/net v0.0.0-20200602114024-627f9648deb9/go.mod h1:qpuaurCH72eLCgpAm/ golang.org/x/net v0.0.0-20200625001655-4c5254603344/go.mod h1:/O7V0waA8r7cgGh81Ro3o1hOxt32SMVPicZroKQ2sZA= golang.org/x/net v0.0.0-20200904194848-62affa334b73/go.mod h1:/O7V0waA8r7cgGh81Ro3o1hOxt32SMVPicZroKQ2sZA= golang.org/x/net v0.0.0-20201002202402-0a1ea396d57c/go.mod h1:iQL9McJNjoIa5mjH6nYTCTZXUN6RP+XW3eib7Ya3XcI= -golang.org/x/net v0.0.0-20201202161906-c7110b5ffcbb h1:eBmm0M9fYhWpKZLjQUUKka/LtIxf46G4fxeEz5KJr9U= golang.org/x/net v0.0.0-20201202161906-c7110b5ffcbb/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4 h1:4nGaVu0QrbjT/AK2PRLuQfQuh6DJve+pELhqTdAj3x0= golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4/go.mod h1:p54w0d4576C0XHj96bSt6lcn1PtDYWL6XObtHCRCNQM= @@ -740,7 +736,6 @@ golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20190227155943-e225da77a7e6/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.0.0-20201207232520-09787c993a3a h1:DcqTD9SDLc+1P/r1EmRBwnVsrOwW+kk2vWf9n+1sGhs= golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20210220032951-036812b2e83c h1:5KslGYwFpkhGh+Q16bwMP3cOontH8FOep7tGV86Y7SQ= golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= @@ -781,7 +776,6 @@ golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20201005172224-997123666555/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.0.0-20210309074719-68d13333faf2 h1:46ULzRKLh1CwgRq2dC5SlBzEqqNCi8rreOZnNrbqcIY= golang.org/x/sys v0.0.0-20210309074719-68d13333faf2/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210330210617-4fbd30eecc44/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210510120138-977fb7262007 h1:gG67DSER+11cZvqIMb8S8bt0vZtiN6xWYARwirrOSfE= From 4d95dc0b646c913a3f2589bf95f54c93aaa8f48b Mon Sep 17 00:00:00 2001 From: Daniel Bily Date: Thu, 10 Feb 2022 11:37:31 +0100 Subject: [PATCH 4/4] update recommendation list endpoint to return a list of clusters instead of number of clusters --- openapi.json | 15 ++++++++++----- server/server_test.go | 29 ++++++++++++++++++----------- storage/storage.go | 26 ++++++++++++-------------- storage/storage_test.go | 28 ++++++++++++++-------------- 4 files changed, 54 insertions(+), 44 deletions(-) diff --git a/openapi.json b/openapi.json index 6cc75124..a46349d3 100644 --- a/openapi.json +++ b/openapi.json @@ -1385,7 +1385,7 @@ }, "/recommendations/organizations/{org_id}/users/{user_id}/list": { "post": { - "summary": "Returns a list of recommendations and a number of clusters each one is hitting.", + "summary": "Returns a list of recommendations and a list of clusters each one is hitting.", "operationId": "getRecommendationsPost", "description": "Recommendations will be retrieved based on the list of cluster IDs that is part of request body.", "parameters": [ @@ -1438,10 +1438,15 @@ "description": "The rule ID in the | format.", "example": "rule.module|ERROR_KEY" }, - "impacted_clusters_cnt": { - "type": "int", - "description": "The number of clusters impacted by this rule.", - "example": 42 + "cluster_id": { + "type": "array", + "items": { + "description": "Cluster ID", + "type": "string", + "minLength": 36, + "maxLength": 36, + "format": "uuid" + } } } } diff --git a/server/server_test.go b/server/server_test.go index 5c33e07f..2dfb599d 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -1180,11 +1180,11 @@ func TestHTTPServer_RecommendationsListEndpoint_3Recs1Cluster(t *testing.T) { clusterList := []types.ClusterName{testdata.ClusterName} reqBody, _ := json.Marshal(clusterList) - respBody := `{"recommendations":{"%v":%v,"%v":%v,"%v":%v},"status":"ok"}` + respBody := `{"recommendations":{"%v":["%v"],"%v":["%v"],"%v":["%v"]},"status":"ok"}` respBody = fmt.Sprintf(respBody, - testdata.Rule1CompositeID, 1, - testdata.Rule2CompositeID, 1, - testdata.Rule3CompositeID, 1, + testdata.Rule1CompositeID, testdata.ClusterName, + testdata.Rule2CompositeID, testdata.ClusterName, + testdata.Rule3CompositeID, testdata.ClusterName, ) helpers.AssertAPIRequest(t, mockStorage, nil, &helpers.APIRequest{ @@ -1219,11 +1219,18 @@ func TestHTTPServer_RecommendationsListEndpoint_3Recs2Clusters(t *testing.T) { reqBody, _ := json.Marshal(clusterList) - respBody := `{"recommendations":{"%v":%v,"%v":%v,"%v":%v},"status":"ok"}` + respBody := `{ + "recommendations":{ + "%v":["%v", "%v"], + "%v":["%v", "%v"], + "%v":["%v"] + }, + "status":"ok" + }` respBody = fmt.Sprintf(respBody, - testdata.Rule1CompositeID, 2, - testdata.Rule2CompositeID, 2, - testdata.Rule3CompositeID, 1, + testdata.Rule1CompositeID, clusterList[0], clusterList[1], + testdata.Rule2CompositeID, clusterList[0], clusterList[1], + testdata.Rule3CompositeID, clusterList[1], ) helpers.AssertAPIRequest(t, mockStorage, nil, &helpers.APIRequest{ @@ -1490,11 +1497,11 @@ func TestHTTPServer_ClustersRecommendationsListEndpoint_2Recs1Cluster(t *testing mockStorage, closer := helpers.MustGetMockStorage(t, true) defer closer() - respBody := `{"recommendations":{"%v":%v,"%v":%v},"status":"ok"}` + respBody := `{"recommendations":{"%v":["%v"],"%v":["%v"]},"status":"ok"}` expected := fmt.Sprintf(respBody, - testdata.Rule1CompositeID, 1, - testdata.Rule2CompositeID, 1, + testdata.Rule1CompositeID, testdata.ClusterName, + testdata.Rule2CompositeID, testdata.ClusterName, ) err := mockStorage.WriteRecommendationsForCluster( diff --git a/storage/storage.go b/storage/storage.go index cd07c715..bb9a6869 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -1045,10 +1045,10 @@ func (storage DBStorage) ReadRecommendationsForClusters( orgID types.OrgID, ) (ctypes.RecommendationImpactedClusters, error) { - recommendationsMap := make(ctypes.RecommendationImpactedClusters, 0) + impactedClusters := make(ctypes.RecommendationImpactedClusters, 0) if len(clusterList) < 1 { - return recommendationsMap, nil + return impactedClusters, nil } // #nosec G201 @@ -1058,38 +1058,36 @@ func (storage DBStorage) ReadRecommendationsForClusters( // #nosec G202 query := ` SELECT - rule_id, count(*) as impacted_clusters_c + rule_id, cluster_id FROM recommendation - ` + whereClause + ` - GROUP BY - rule_id - ` + ` + whereClause + rows, err := storage.connection.Query(query, orgID) if err != nil { log.Error().Err(err).Msg("query to get recommendations") - return recommendationsMap, err + return impactedClusters, err } for rows.Next() { var ( - ruleID types.RuleID - impactedCnt ctypes.ImpactedClustersCnt + ruleID types.RuleID + clusterID types.ClusterName ) err := rows.Scan( &ruleID, - &impactedCnt, + &clusterID, ) if err != nil { log.Error().Err(err).Msg("read one recommendation") - return recommendationsMap, err + return impactedClusters, err } - recommendationsMap[ruleID] = impactedCnt + impactedClusters[ruleID] = append(impactedClusters[ruleID], clusterID) } - return recommendationsMap, nil + return impactedClusters, nil } // ReadClusterListRecommendations retrieves cluster IDs and a list of hitting rules for each one diff --git a/storage/storage_test.go b/storage/storage_test.go index 597c2695..681bea45 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -1186,11 +1186,11 @@ func TestDBStorageReadRecommendationsForClusters(t *testing.T) { ) helpers.FailOnError(t, err) - expectingImpactedC := ctypes.ImpactedClustersCnt(1) + expected := []types.ClusterName{testdata.ClusterName} expect := ctypes.RecommendationImpactedClusters{ - testdata.Rule1CompositeID: expectingImpactedC, - testdata.Rule2CompositeID: expectingImpactedC, - testdata.Rule3CompositeID: expectingImpactedC, + testdata.Rule1CompositeID: expected, + testdata.Rule2CompositeID: expected, + testdata.Rule3CompositeID: expected, } res, err := mockStorage.ReadRecommendationsForClusters([]string{string(testdata.ClusterName)}, testdata.OrgID) @@ -1228,11 +1228,11 @@ func TestDBStorageReadRecommendationsForClustersMoreClusters(t *testing.T) { ) helpers.FailOnError(t, err) - expect2Impacted := ctypes.ImpactedClustersCnt(2) + expect2 := []types.ClusterName{types.ClusterName(clusterList[1]), types.ClusterName(clusterList[2])} expect := ctypes.RecommendationImpactedClusters{ - testdata.Rule1CompositeID: expect2Impacted, - testdata.Rule2CompositeID: expect2Impacted, - testdata.Rule3CompositeID: ctypes.ImpactedClustersCnt(1), + testdata.Rule1CompositeID: expect2, + testdata.Rule2CompositeID: expect2, + testdata.Rule3CompositeID: []types.ClusterName{types.ClusterName(clusterList[2])}, } res, err := mockStorage.ReadRecommendationsForClusters(clusterList, testdata.OrgID) @@ -1300,14 +1300,14 @@ func TestDBStorageReadRecommendationsGetSelectedClusters(t *testing.T) { res, err := mockStorage.ReadRecommendationsForClusters([]string{string(clusterList[0])}, testdata.OrgID) helpers.FailOnError(t, err) - expect1Impacted := ctypes.ImpactedClustersCnt(1) - expect := ctypes.RecommendationImpactedClusters{ - testdata.Rule1CompositeID: expect1Impacted, - testdata.Rule2CompositeID: expect1Impacted, - testdata.Rule3CompositeID: expect1Impacted, + expect := []types.ClusterName{types.ClusterName(clusterList[0])} + expectResp := ctypes.RecommendationImpactedClusters{ + testdata.Rule1CompositeID: expect, + testdata.Rule2CompositeID: expect, + testdata.Rule3CompositeID: expect, } - assert.Equal(t, expect, res) + assert.Equal(t, expectResp, res) } // TestDBStorageReadRecommendationsForNonexistingClusters simulates getting a list of clusters where