From f485a3db1c77482046ade2dbc606045a2ffbb956 Mon Sep 17 00:00:00 2001 From: Vyom Yadav <73882557+Vyom-Yadav@users.noreply.github.com> Date: Mon, 5 Feb 2024 23:19:19 +0530 Subject: [PATCH] Fix Rule Evaluation Logic for Handling Multiple Rules of the Same Type - Part 2 (Data Backfilling) (#2206) * entity_profiles.contextual_rules are modified to have rule name key with rule_type or 'rule_type-rand_uuid' in case of collisions * Existing rule_evaluations.rule_name columns are filled by referring to entity_profiles.contextual_rules[*].name key * Handlers and Sql queries are updated to reflect not null constraint on rule_evaluations.rule_name column * migration_profile_backfill_log table is created to support migrating down Signed-off-by: Vyom-Yadav --- cmd/cli/app/profile/table_render.go | 10 +- ...evaluations_rule_name_backfilling.down.sql | 80 +++++++++++ ...e_evaluations_rule_name_backfilling.up.sql | 129 ++++++++++++++++++ database/query/profile_status.sql | 7 +- internal/controlplane/handlers_profile.go | 15 +- internal/db/models.go | 20 +-- internal/db/profile_status.sql.go | 25 ++-- .../security_advisory/security_advisory.go | 2 +- internal/engine/eval_status.go | 11 +- internal/engine/executor_test.go | 3 +- 10 files changed, 245 insertions(+), 57 deletions(-) create mode 100644 database/migrations/000017_rule_evaluations_rule_name_backfilling.down.sql create mode 100644 database/migrations/000017_rule_evaluations_rule_name_backfilling.up.sql diff --git a/cmd/cli/app/profile/table_render.go b/cmd/cli/app/profile/table_render.go index 2f0f29f1a6..c4ae04ca09 100644 --- a/cmd/cli/app/profile/table_render.go +++ b/cmd/cli/app/profile/table_render.go @@ -156,17 +156,9 @@ func RenderRuleEvaluationStatusTable( t table.Table, ) { for _, eval := range statuses { - ruleName := eval.RuleDescriptionName - - // TODO: Remove this after migration, ruleName would be valid after updating existing evaluations (#1609) - //nolint:staticcheck // ignore SA1019: Deprecated field supported for backward compatibility - if ruleName == "" { - ruleName = eval.RuleName - } - t.AddRowWithColor( layouts.NoColor(eval.RuleId), - layouts.NoColor(ruleName), + layouts.NoColor(eval.RuleDescriptionName), layouts.NoColor(eval.Entity), getColoredEvalStatus(eval.Status), getRemediateStatusColor(eval.RemediationStatus), diff --git a/database/migrations/000017_rule_evaluations_rule_name_backfilling.down.sql b/database/migrations/000017_rule_evaluations_rule_name_backfilling.down.sql new file mode 100644 index 0000000000..800a6d6911 --- /dev/null +++ b/database/migrations/000017_rule_evaluations_rule_name_backfilling.down.sql @@ -0,0 +1,80 @@ +-- Copyright 2024 Stacklok, 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. + +-- Begin transaction +BEGIN; + +-- Function to remove the "name" field from a JSONB array. Function is immutable as it will return same results +-- for same input arguments forever. +CREATE OR REPLACE FUNCTION remove_name_from_jsonb_array(input_jsonb jsonb) RETURNS jsonb AS +$$ +DECLARE + updated_array jsonb; + element jsonb; +BEGIN + updated_array := '[]'::jsonb; + + FOR element IN SELECT * FROM jsonb_array_elements(input_jsonb) + LOOP + element := element - 'name'; + updated_array := updated_array || jsonb_build_array(element); + END LOOP; + + RETURN updated_array; +END; +$$ LANGUAGE plpgsql IMMUTABLE; + +-- Prevent concurrent updates to entity_profiles +SELECT * +FROM entity_profiles FOR UPDATE; + +-- Update the entity_profiles table to remove the "name" key from the "contextual_rules" JSONB array +UPDATE entity_profiles +SET contextual_rules = remove_name_from_jsonb_array(contextual_rules), + updated_at = now() +WHERE entity_profiles.profile_id IN (SELECT profile_id FROM migration_profile_backfill_log); + +-- Prevent concurrent updates to rule_evaluations +SELECT * +FROM rule_evaluations FOR UPDATE; + +-- Update the rule_name column to remove not null constraint +ALTER TABLE rule_evaluations + ALTER COLUMN rule_name DROP NOT NULL; + +-- Delete duplicate rule evaluation results without considering rule_name +-- Using CTID as postgres doesn't have min, max aggregators for uuid (too much code to add one) +DELETE +FROM rule_evaluations +WHERE CTID IN (SELECT MIN(CTID) AS CTID + FROM rule_evaluations + GROUP BY entity, profile_id, repository_id, rule_type_id, + COALESCE(pull_request_id, '00000000-0000-0000-0000-000000000000'::UUID), + COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID) + HAVING COUNT(*) > 1) + AND profile_id IN (SELECT profile_id FROM migration_profile_backfill_log); + +-- Set rule_name column to null +UPDATE rule_evaluations +SET rule_name = null +WHERE profile_id IN (SELECT profile_id FROM migration_profile_backfill_log); + +-- Drop the created function +DROP FUNCTION IF EXISTS remove_name_from_jsonb_array(input_jsonb jsonb); + +-- Drop the migration log table +DROP TABLE IF EXISTS migration_profile_backfill_log; + +-- transaction commit +COMMIT; diff --git a/database/migrations/000017_rule_evaluations_rule_name_backfilling.up.sql b/database/migrations/000017_rule_evaluations_rule_name_backfilling.up.sql new file mode 100644 index 0000000000..1388404d7a --- /dev/null +++ b/database/migrations/000017_rule_evaluations_rule_name_backfilling.up.sql @@ -0,0 +1,129 @@ +-- Copyright 2024 Stacklok, 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. + +-- Begin transaction +BEGIN; + +-- Function to check whether jsonb array element contain a name key +CREATE OR REPLACE FUNCTION contains_name_key(input_jsonb jsonb) RETURNS boolean AS +$$ +DECLARE + element jsonb; +BEGIN + FOR element IN SELECT * FROM jsonb_array_elements(input_jsonb) + LOOP + IF element ? 'name' THEN + RETURN true; + END IF; + END LOOP; + + RETURN false; +END; +$$ LANGUAGE plpgsql IMMUTABLE; + +-- Function to add a descriptive name to each element in a JSONB array +CREATE OR REPLACE FUNCTION add_descriptive_name_to_jsonb_array(input_jsonb jsonb) RETURNS jsonb AS +$$ +DECLARE + updated_array jsonb; + element jsonb; + element_type text; +BEGIN + updated_array := '[]'::jsonb; + + FOR element IN SELECT * FROM jsonb_array_elements(input_jsonb) + LOOP + element_type := element ->> 'type'; + + IF (SELECT COUNT(*) FROM jsonb_array_elements(input_jsonb) WHERE value ->> 'type' = element_type) > 1 THEN + element := jsonb_set(element, '{name}', ('"' || element_type || '_' || gen_random_uuid() || '"')::jsonb); + ELSE + element := jsonb_set(element, '{name}', ('"' || element_type || '"')::jsonb); + END IF; + + updated_array := updated_array || jsonb_build_array(element); + END LOOP; + + RETURN updated_array; +END; +$$ LANGUAGE plpgsql; + +-- Function to get the rule name for a given entity, profile and rule id. +CREATE OR REPLACE FUNCTION get_rule_name(entity entities, profile_id uuid, rule_type_id uuid) RETURNS text AS +$$ +DECLARE + rule_name text; + rule_type_name text; + rules jsonb; +BEGIN + SELECT entity_profiles.contextual_rules + INTO rules + FROM entity_profiles + WHERE entity_profiles.profile_id = get_rule_name.profile_id + AND entity_profiles.entity = get_rule_name.entity; + + SELECT rule_type.name INTO rule_type_name FROM rule_type WHERE id = get_rule_name.rule_type_id; + + SELECT rule_element ->> 'name' + INTO rule_name + FROM jsonb_array_elements(rules) rule_element + WHERE rule_element ->> 'type' = rule_type_name; + + RETURN rule_name; +END; +$$ LANGUAGE plpgsql STABLE; + +CREATE TABLE migration_profile_backfill_log ( + profile_id UUID PRIMARY KEY, + FOREIGN KEY (profile_id) REFERENCES profiles (id) ON DELETE CASCADE +); + +-- Prevent entity_profiles to be updated outside the transaction +SELECT * +FROM entity_profiles FOR UPDATE; + +-- Add updated profile_id to migration_profile_backfill_log, don't add if already exists +WITH updated_profile_ids AS ( + -- Update existing rules to have the name field, internally this field is mandatory + UPDATE entity_profiles + SET contextual_rules = add_descriptive_name_to_jsonb_array(entity_profiles.contextual_rules), + updated_at = now() + WHERE contains_name_key(entity_profiles.contextual_rules) = false + RETURNING entity_profiles.profile_id) +INSERT +INTO migration_profile_backfill_log (profile_id) +SELECT DISTINCT profile_id +FROM updated_profile_ids +WHERE profile_id NOT IN (SELECT profile_id FROM migration_profile_backfill_log); + +-- Prevent rule_evaluations to be updated outside the transaction +SELECT * +FROM rule_evaluations FOR UPDATE; + +-- Update rule evaluations +UPDATE rule_evaluations +SET rule_name = get_rule_name(rule_evaluations.entity, rule_evaluations.profile_id, rule_evaluations.rule_type_id) +WHERE rule_name IS NULL; + +-- Add non null constraint on rule_name +ALTER TABLE rule_evaluations + ALTER COLUMN rule_name SET NOT NULL; + +-- Drop the created functions +DROP FUNCTION IF EXISTS contains_name_key(input_jsonb jsonb); +DROP FUNCTION IF EXISTS add_descriptive_name_to_jsonb_array(input_jsonb jsonb); +DROP FUNCTION IF EXISTS get_rule_name(entity entities, profile_id uuid, rule_type_id uuid); + +-- transaction commit +COMMIT; diff --git a/database/query/profile_status.sql b/database/query/profile_status.sql index fcc0a7ae54..0bb86dc1e4 100644 --- a/database/query/profile_status.sql +++ b/database/query/profile_status.sql @@ -2,7 +2,7 @@ -- name: UpsertRuleEvaluations :one INSERT INTO rule_evaluations ( profile_id, repository_id, artifact_id, pull_request_id, rule_type_id, entity, rule_name -) VALUES ($1, $2, $3, $4, $5, $6, sqlc.narg(rule_name)) +) VALUES ($1, $2, $3, $4, $5, $6, $7) ON CONFLICT (profile_id, repository_id, COALESCE(artifact_id, '00000000-0000-0000-0000-000000000000'::UUID), COALESCE(pull_request_id, '00000000-0000-0000-0000-000000000000'::UUID), entity, rule_type_id, rule_name) DO UPDATE SET profile_id = $1 RETURNING id; @@ -79,8 +79,7 @@ WHERE p.project_id = $1; DELETE FROM rule_evaluations WHERE id IN ( SELECT id FROM rule_evaluations as re - WHERE re.profile_id = $1 AND re.rule_type_id = $2 AND - (re.rule_name = sqlc.narg(rule_name) OR sqlc.narg(rule_name) IS NULL) FOR UPDATE); + WHERE re.profile_id = $1 AND re.rule_type_id = $2 AND re.rule_name = $3 FOR UPDATE); -- name: ListRuleEvaluationsByProfileId :many WITH @@ -123,7 +122,7 @@ SELECT ad.alert_last_updated, res.repository_id, res.entity, - COALESCE(res.rule_name, ''), + res.rule_name, repo.repo_name, repo.repo_owner, repo.provider, diff --git a/internal/controlplane/handlers_profile.go b/internal/controlplane/handlers_profile.go index e0836d5f6e..d554842536 100644 --- a/internal/controlplane/handlers_profile.go +++ b/internal/controlplane/handlers_profile.go @@ -662,7 +662,7 @@ func (s *Server) UpdateProfile(ctx context.Context, return nil, status.Errorf(codes.Internal, "failed to get profile: %s", err) } - oldRules, err := s.getRulesFromOldProfile(ctx, oldProfile, entityCtx) + oldRules, err := s.getRulesFromProfile(ctx, oldProfile, entityCtx) if err != nil { if errors.Is(err, sql.ErrNoRows) { return nil, util.UserVisibleError(codes.NotFound, "profile not found") @@ -813,7 +813,7 @@ func (s *Server) getAndValidateRulesFromProfile( return rulesInProf, nil } -func (s *Server) getRulesFromOldProfile( +func (s *Server) getRulesFromProfile( ctx context.Context, prof *minderv1.Profile, entityCtx engine.EntityContext, @@ -839,11 +839,9 @@ func (s *Server) getRulesFromOldProfile( return fmt.Errorf("cannot convert rule type %s to pb: %w", rtdb.Name, err) } - // TODO: Remove r.Name and replace with computeRuleName(..) after migration is complete for #1609 - // Existing rules (before migration) would have r.Name == "" key := ruleTypeAndNamePair{ RuleType: r.GetType(), - RuleName: r.Name, + RuleName: computeRuleName(r), } rulesInProf[key] = entityAndRuleTuple{ @@ -1057,16 +1055,11 @@ func deleteRuleStatusesForProfile( ) error { for ruleTypeAndName, rule := range unusedRuleStatuses { log.Printf("deleting rule evaluations for rule %s in profile %s", rule.RuleID, profile.ID) - // TODO: Remove this after migration, ruleName would be valid after updating existing evaluations (#1609) - ruleName := sql.NullString{ - String: ruleTypeAndName.RuleName, - Valid: ruleTypeAndName.RuleName != "", - } if err := querier.DeleteRuleStatusesForProfileAndRuleType(ctx, db.DeleteRuleStatusesForProfileAndRuleTypeParams{ ProfileID: profile.ID, RuleTypeID: rule.RuleID, - RuleName: ruleName, + RuleName: ruleTypeAndName.RuleName, }); err != nil { log.Printf("error deleting rule evaluations: %v", err) return fmt.Errorf("error deleting rule evaluations: %w", err) diff --git a/internal/db/models.go b/internal/db/models.go index 839e0cc7eb..1ab10234f6 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -352,6 +352,10 @@ type FlushCache struct { QueuedAt time.Time `json:"queued_at"` } +type MigrationProfileBackfillLog struct { + ProfileID uuid.UUID `json:"profile_id"` +} + type Profile struct { ID uuid.UUID `json:"id"` Name string `json:"name"` @@ -454,14 +458,14 @@ type RuleDetailsRemediate struct { } type RuleEvaluation struct { - ID uuid.UUID `json:"id"` - Entity Entities `json:"entity"` - ProfileID uuid.UUID `json:"profile_id"` - RuleTypeID uuid.UUID `json:"rule_type_id"` - RepositoryID uuid.NullUUID `json:"repository_id"` - ArtifactID uuid.NullUUID `json:"artifact_id"` - PullRequestID uuid.NullUUID `json:"pull_request_id"` - RuleName sql.NullString `json:"rule_name"` + ID uuid.UUID `json:"id"` + Entity Entities `json:"entity"` + ProfileID uuid.UUID `json:"profile_id"` + RuleTypeID uuid.UUID `json:"rule_type_id"` + RepositoryID uuid.NullUUID `json:"repository_id"` + ArtifactID uuid.NullUUID `json:"artifact_id"` + PullRequestID uuid.NullUUID `json:"pull_request_id"` + RuleName string `json:"rule_name"` } type RuleType struct { diff --git a/internal/db/profile_status.sql.go b/internal/db/profile_status.sql.go index fa2909c7c0..9ef70c7b6a 100644 --- a/internal/db/profile_status.sql.go +++ b/internal/db/profile_status.sql.go @@ -20,14 +20,13 @@ const deleteRuleStatusesForProfileAndRuleType = `-- name: DeleteRuleStatusesForP DELETE FROM rule_evaluations WHERE id IN ( SELECT id FROM rule_evaluations as re - WHERE re.profile_id = $1 AND re.rule_type_id = $2 AND - (re.rule_name = $3 OR $3 IS NULL) FOR UPDATE) + WHERE re.profile_id = $1 AND re.rule_type_id = $2 AND re.rule_name = $3 FOR UPDATE) ` type DeleteRuleStatusesForProfileAndRuleTypeParams struct { - ProfileID uuid.UUID `json:"profile_id"` - RuleTypeID uuid.UUID `json:"rule_type_id"` - RuleName sql.NullString `json:"rule_name"` + ProfileID uuid.UUID `json:"profile_id"` + RuleTypeID uuid.UUID `json:"rule_type_id"` + RuleName string `json:"rule_name"` } // DeleteRuleStatusesForProfileAndRuleType deletes a rule evaluation @@ -179,7 +178,7 @@ SELECT ad.alert_last_updated, res.repository_id, res.entity, - COALESCE(res.rule_name, ''), + res.rule_name, repo.repo_name, repo.repo_owner, repo.provider, @@ -390,13 +389,13 @@ RETURNING id ` type UpsertRuleEvaluationsParams struct { - ProfileID uuid.UUID `json:"profile_id"` - RepositoryID uuid.NullUUID `json:"repository_id"` - ArtifactID uuid.NullUUID `json:"artifact_id"` - PullRequestID uuid.NullUUID `json:"pull_request_id"` - RuleTypeID uuid.UUID `json:"rule_type_id"` - Entity Entities `json:"entity"` - RuleName sql.NullString `json:"rule_name"` + ProfileID uuid.UUID `json:"profile_id"` + RepositoryID uuid.NullUUID `json:"repository_id"` + ArtifactID uuid.NullUUID `json:"artifact_id"` + PullRequestID uuid.NullUUID `json:"pull_request_id"` + RuleTypeID uuid.UUID `json:"rule_type_id"` + Entity Entities `json:"entity"` + RuleName string `json:"rule_name"` } func (q *Queries) UpsertRuleEvaluations(ctx context.Context, arg UpsertRuleEvaluationsParams) (uuid.UUID, error) { diff --git a/internal/engine/actions/alert/security_advisory/security_advisory.go b/internal/engine/actions/alert/security_advisory/security_advisory.go index b16e32f72f..46dc680ba3 100644 --- a/internal/engine/actions/alert/security_advisory/security_advisory.go +++ b/internal/engine/actions/alert/security_advisory/security_advisory.go @@ -81,7 +81,7 @@ To address this security exposure, we recommend taking the following actions: - Profile: {{.Profile}} - Rule: {{.Rule}} -{{if and (not (eq .Name "")) (ne .Name .Rule) -}} +{{if (ne .Name .Rule) -}} - Name: {{.Name}} {{end -}} - Repository: {{.Repository}} diff --git a/internal/engine/eval_status.go b/internal/engine/eval_status.go index 6efb43fd62..2f4fff8807 100644 --- a/internal/engine/eval_status.go +++ b/internal/engine/eval_status.go @@ -78,10 +78,9 @@ func (e *Executor) createEvalStatusParams( Valid: true, } - // TODO: Remove this after migration, ruleName would be valid after updating existing evaluations (#1609) ruleName := sql.NullString{ String: rule.Name, - Valid: rule.Name != "", + Valid: true, } // Get the current rule evaluation from the database @@ -123,12 +122,6 @@ func (e *Executor) createOrUpdateEvalStatus( return nil } - // TODO: Remove this after migration, ruleName would be valid after updating existing evaluations (#1609) - ruleName := sql.NullString{ - String: params.Rule.Name, - Valid: params.Rule.Name != "", - } - // Upsert evaluation id, err := e.querier.UpsertRuleEvaluations(ctx, db.UpsertRuleEvaluationsParams{ ProfileID: params.ProfileID, @@ -140,7 +133,7 @@ func (e *Executor) createOrUpdateEvalStatus( Entity: params.EntityType, RuleTypeID: params.RuleTypeID, PullRequestID: params.PullRequestID, - RuleName: ruleName, + RuleName: params.Rule.Name, }) if err != nil { diff --git a/internal/engine/executor_test.go b/internal/engine/executor_test.go index c9d8954219..d87f0906c6 100644 --- a/internal/engine/executor_test.go +++ b/internal/engine/executor_test.go @@ -16,7 +16,6 @@ package engine_test import ( "context" - "database/sql" "encoding/base64" "encoding/json" "os" @@ -206,7 +205,7 @@ default allow = true`, ArtifactID: uuid.NullUUID{}, RuleTypeID: ruleTypeID, Entity: db.EntitiesRepository, - RuleName: sql.NullString{String: passthroughRuleType, Valid: true}, + RuleName: passthroughRuleType, }).Return(ruleEvalId, nil) // Mock upserting eval details status