Fix Rule Evaluation Logic for Handling Multiple Rules of the Same Type - Part 2 (Data Backfilling)

[Vyom-Yadav]

Fixes #1609

---

Changes and implications:

• entity_profiles.contextual_rules are modified to have rule name key with rule_type or rule_type-rand_uuid in case of collisions

• Existing null rule_evaluations.rule_name columns are filled by referring to name key in entity_profiles.contextual_rules

• 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

• The rules are reconciled by the subsequent InternalProfileInitEventTopic, and ExecuteEntityEventTopic events. Profiles/Repositories not receiving these types of events won't be reconciled to update them to the latest state.

• Reconciliation can potentially open new PRs/Security Advisories, etc., as the duplicate rule's row (entry) was overridden by the other rule of the same type.

[evankanderson]

I like this approach, thank you!

[evankanderson]

It took me a bit to verify. this, so I'm going to record it. I think this is safe because the migration script runs before the server update.

Because migrations run before the server code update, we could have the server attempting to insert rule_name == NULL from https://github.com/stacklok/minder/blob/b7fb73a53159a74dd6307a4dd7de4c614ec8aef1/internal/engine/eval_status.go#L129 unless we've backfilled the table. Once the table is backfilled, that shouldn't happen, and all the following rule evaluations should have name set:

Path:

• EvalStatusParams is set here in createEvalStatusParams from the Profile.Rule and not otherwise.
• getEvaluator is the only caller of createEvalStatusParams, which is a pass-through from the TraverseRules in evalEntityEvent.
• TraverseRules runs the supplied function over each rule in the relevant set computed by GetRulesForEntity, which selects one of repository, build_environment, pull_requests, or artifacts from the lists of rules in the Profile and returns them without edit.
• We get the set of Profiles from ListProfileByProjectID passed through MergeDatabaseListIntoProfiles
• While ListProfilesByProjectID is auto-generated, MergeDatabaseListIntoProfiles calls rowInfoToProfileMap to fill out the rules in the proto.
• rowInfoToProfileMap unpacks the list of rules stored as JSON in the database, and returns them, which doesn't set rule names if not already present.
• The migration is selecting the rows FOR UPDATE, which should hopefully block any attempts to evaluate rules while the migration is in progress.

[Vyom-Yadav]

I think this is safe because the migration script runs before the server update.
...
The migration is selecting the rows FOR UPDATE, which should hopefully block any attempts to evaluate rules while the migration is in progress.

Exactly. Even if some rule is under evaluation and has fetched all required data from DB before the migration script began, the migration script would lock the tables and set the NOT NULL constraint on the column. So after the migration is complete and the engine gets access to the table, upserts would fail if they don't have a rule_name.

To prevent losing those events to some extent, we have retry mechanisms. In subsequent tries, the server would fetch rule_name from the DB to perform a legal operation.