Use rule instance table in executor #3899

dmjb · 2024-07-16T09:40:12Z

This PR changes the executor to load rule instances from the
rule_instances table instead of using the serialized protobufs in
entity_profiles.

Using the rule_instances table, a new querying method is added to load
all profiles relevant to evaluation, along with the rule instances for
the entity type being evaluated. Two domain-level types -
ProfileAggregate and RuleInstance are defined to represent the
result of these queries. I have removed a lot of the protobuf usage from
the evaluation code since converting to and from protobufs just adds
complexity. The fields in the new domain types match up better with
the types expected by the engine code.

Tested locally, but definitely needs some smoke testing before rollout.

Summary

Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.

Fixes #(related issue)

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

coveralls · 2024-07-16T16:04:35Z

coverage: 54.187% (-0.2%) from 54.407%
when pulling f1e4cb9 on rule-instance-store
into dccc11a on main.

dmjb · 2024-07-16T16:36:34Z

internal/profiles/models/models.go

+)
+
+// ProfileAggregate represents a profile along with rule instances
+type ProfileAggregate struct {


These new types are intended to address the fact that neither the protobuf or the database schema representations of Profile match up neatly with what a lot of the code in minder actually needs. These will be used elsewhere in the codebase in future.

dmjb · 2024-07-16T16:37:15Z

internal/profiles/models/models.go

+
+// ActionConfiguration stores the configuration state for a profile
+type ActionConfiguration struct {
+	Remediate ActionOpt


These fields are isolated into an inner struct because various parts of the engine code previously passed around the whole profile to just get these two values.

dmjb · 2024-07-16T16:39:39Z

internal/profiles/models/models.go

+	ID           uuid.UUID
+	Name         string
+	ActionConfig ActionConfiguration
+	Rules        []RuleInstance


This type has Aggregate in the name because it contains a list of rule instances as well as just representing the profile itself.

internal/profiles/store.go

dmjb · 2024-07-16T16:45:26Z

internal/profiles/store.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package profiles


This file could do with a test suite, but in practice most of it is already covered by existing unit tests.

This PR changes the executor to load rule instances from the `rule_instances` table instead of using the serialized protobufs in `entity_profiles`. Using the `rule_instances` table, a new querying method is added to load all profiles relevant to evaluation, along with the rule instances for the entity type being evaluated. Two domain-level types - `ProfileAggregate` and `RuleInstance` are defined to represent the result of these queries. I have removed a lot of the protobuf usage from the evaluation code since converting to and from protobufs just adds complexity, and the new types line up with the values and types expected by the evaluation code. Tested locally, but definitely needs some smoke testing before rollout.

dmjb · 2024-07-17T16:14:52Z

internal/profiles/store.go

+	// The query is written in such a way that if a profile is deleted between
+	// the rule instance query and this query, it will simply be omitted from
+	// the results.
+	profiles, err := p.store.BulkGetProfilesByID(ctx, maps.Keys(rulesByProfileID))


One advantage of this new approach is that we do not have to query all profiles in the projects and then filter in the code - we can filter out irrelevant profiles at query time.

this is very nice.

What do you think would be the best way to also retrieve selectors at the same time? For the old way of listing profiles by ID and project, we added a helper select and a join in 881c672 do you think we should use a similar approach here?

Alternatively we could add another query in the loop over profiles to get the profile selectors.

jhrozek · 2024-07-19T11:10:39Z

@dmjb I think this can be taken out of draft and unless you want more eyes, I'm fine acking the PR, too. I've tried different combinations of profiles with different entities and both evaluation, alerts and remediations work fine. The code is also looking much nicer.

jhrozek

You'll probably want to remove the DNM from the PR subject so that it doesn't appear in git history

PR #3899 accidentally removed a call to GetOnOffState, which allows individual actions to override the remediation and alerting status in the profile. This leads to some unusual behaviour, such as attempting to run the noop alerts and remediations instead of skipping them. This PR reinstates the calls to GetOnOffState, but changes the function to accept the ActionOpt status for that action type instead of taking a pointer to the entire profile.

dmjb changed the title ~~Use rule instance table in executor~~ [WIP] Use rule instance table in executor Jul 16, 2024

dmjb force-pushed the rule-instance-store branch from c137cec to 60bbaa6 Compare July 16, 2024 14:31

dmjb changed the title ~~[WIP] Use rule instance table in executor~~ [DO NOT MERGE] Use rule instance table in executor Jul 16, 2024

dmjb force-pushed the rule-instance-store branch from c8f7e7b to 8cbbbce Compare July 16, 2024 16:29

dmjb marked this pull request as ready for review July 16, 2024 16:29

dmjb requested a review from a team as a code owner July 16, 2024 16:29

dmjb assigned dmjb, JAORMX and jhrozek and unassigned dmjb Jul 16, 2024

dmjb commented Jul 16, 2024

View reviewed changes

internal/profiles/store.go Outdated Show resolved Hide resolved

dmjb commented Jul 16, 2024

View reviewed changes

dmjb marked this pull request as draft July 16, 2024 17:06

dmjb force-pushed the rule-instance-store branch from d140cec to bb47df5 Compare July 17, 2024 12:17

dmjb added 3 commits July 17, 2024 15:14

remove unnecessary comment

b7e5583

clarify comment

6c3e420

dmjb force-pushed the rule-instance-store branch from bb47df5 to 6c3e420 Compare July 17, 2024 14:15

select profiles based on rule instances

9e5b6a2

dmjb commented Jul 17, 2024

View reviewed changes

dmjb marked this pull request as ready for review July 19, 2024 10:15

Merge branch 'main' into rule-instance-store

528c7cb

dmjb marked this pull request as draft July 19, 2024 10:16

remove redundant comment

5add116

dmjb marked this pull request as ready for review July 19, 2024 12:29

Merge branch 'main' into rule-instance-store

f950a55

jhrozek approved these changes Jul 19, 2024

View reviewed changes

dmjb changed the title ~~[DO NOT MERGE] Use rule instance table in executor~~ Use rule instance table in executor Jul 19, 2024

Merge branch 'main' into rule-instance-store

f1e4cb9

dmjb merged commit ca039f9 into main Jul 19, 2024
20 checks passed

dmjb deleted the rule-instance-store branch July 19, 2024 18:23

dmjb mentioned this pull request Jul 22, 2024

Respect the action overrides #3946

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use rule instance table in executor #3899

Use rule instance table in executor #3899

dmjb commented Jul 16, 2024 •

edited

Loading

coveralls commented Jul 16, 2024 •

edited

Loading

dmjb Jul 16, 2024

dmjb Jul 16, 2024

dmjb Jul 16, 2024

dmjb Jul 16, 2024

dmjb Jul 17, 2024

jhrozek Jul 18, 2024

jhrozek commented Jul 19, 2024

jhrozek left a comment

Use rule instance table in executor #3899

Use rule instance table in executor #3899

Conversation

dmjb commented Jul 16, 2024 • edited Loading

Summary

Change Type

Testing

Review Checklist:

coveralls commented Jul 16, 2024 • edited Loading

dmjb Jul 16, 2024

Choose a reason for hiding this comment

dmjb Jul 16, 2024

Choose a reason for hiding this comment

dmjb Jul 16, 2024

Choose a reason for hiding this comment

dmjb Jul 16, 2024

Choose a reason for hiding this comment

dmjb Jul 17, 2024

Choose a reason for hiding this comment

jhrozek Jul 18, 2024

Choose a reason for hiding this comment

jhrozek commented Jul 19, 2024

jhrozek left a comment

Choose a reason for hiding this comment

dmjb commented Jul 16, 2024 •

edited

Loading

coveralls commented Jul 16, 2024 •

edited

Loading