Take selectors into use in executor #4004

jhrozek · 2024-07-25T21:21:46Z

Summary

Change the BulkGetProfilesByID DB query to return an embedded profile - This will make it easier to extend what the query returns
Extend the BulkGetProfilesByID DB query with selectors - This is to return the selectors at the same time as querying the profiles from the DB and be able to use them in the executor.
Extend the ProfileAggregate structure with selectors - returns the selectors to the engine through the ProfileAggregate struct
Change the selector's NewSelectionFromProfile API to accept the structure from the models API - This will make it easier to use the method in the executor
Integrate the selectors into the executor - When an entity is not selected for a profile, let's save that into a profile-global status override and use that for all the statuses instead of calling eval
Include which selector did unselect the entity - In addition to returning a bool, let's also return which selector did shortcut the evaluation when returning false

Fixes: #3724
Fixes: #3725

Change Type

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

manual

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.

jhrozek · 2024-07-25T21:22:45Z

database/query/profiles.sql

+    GROUP BY pr.id
+)
+SELECT sqlc.embed(profiles),
+       helper.selectors::profile_selector[] AS profiles_with_selectors


If this is too hairy, we can instead just loop over the profiles and do a get for each profile

on the other hand, having everything in a single call is probably safer in the long run

jhrozek · 2024-07-25T21:25:02Z

internal/engine/selectors/selectors.go

@@ -107,6 +107,7 @@ func newEnvForEntity(varName string, typ any, typName string) (*cel.Env, error)
 }

 type compiledSelector struct {
+	orig    string


there is ast.ToString but it doesn't guarantee that the result will be exactly the same, only that it will have the same meaning..

jhrozek · 2024-07-25T21:25:52Z

internal/engine/selectors/selectors.go

@@ -258,9 +260,9 @@ type EntitySelection struct {
 }

 // Select return true if the entity matches all the compiled expressions and false otherwise
-func (s *EntitySelection) Select(se *internalpb.SelectorEntity, userOpts ...SelectOption) (bool, error) {
+func (s *EntitySelection) Select(se *internalpb.SelectorEntity, userOpts ...SelectOption) (bool, string, error) {


I started wondering if it would be better to return a struct that would contain bool and string for now. Three return values from a function already starts to smell especially if the function is used in an interface.

I think having a dedicated struct would be cleaner in terms of returning it and using it. A bool and a string can get confusing. However, this is not blocking IMO and can be done later.

Alternatively, you could use presence or absence of the error as part of the boolean (and a particular error interface to indicate "not selected"), but I'm happy with this interface.

coveralls · 2024-07-25T21:39:25Z

coverage: 54.504% (+0.06%) from 54.441%
when pulling 1d88d5b on jhrozek:selectors_executor
into b9170e5 on stacklok:main.

JAORMX · 2024-07-26T06:56:56Z

internal/engine/executor.go

@@ -131,10 +136,14 @@ func (e *executor) EvalEntityEvent(ctx context.Context, inf *entities.EntityInfo
 		return fmt.Errorf("error while retrieving profiles and rule instances: %w", err)
 	}

-	// For each profile, evaluate each rule and store the outcome in the database
+	// For each profile, get the profile-override status. Then, if there is no profile-override status,
+	// evaluate each rule and store the outcome in the database or store the override status for all rules


remind me what the "profile override status" is

JAORMX

logic LGTM

jhrozek · 2024-07-26T07:14:13Z

internal/engine/executor.go

 	for _, profile := range profileAggregates {
+
+		profileEvalStatus := e.profileEvalStatus(ctx, provider, inf, profile)


@JAORMX I called the variable actually profileEvalStatus, but "profile-override status" in the comment. I'll fix the naming since it's confusing.

This will make it easier to extend what the query returns

This is to return the selectors at the same time as querying the profiles from the DB and be able to use them in the executor.

This returns the selectors to the engine through the ProfileAggregate structure

…ture from the models API This will make it easier to use the method in the executor

When an entity is not selected for a profile, let's save that into a profile-global status override and use that for all the statuses instead calling eval. Fixes: mindersec#3724

In addition to returning a bool, let's also return which selector did shortcut the evaluation when returning false

jhrozek requested a review from a team as a code owner July 25, 2024 21:21

jhrozek commented Jul 25, 2024

View reviewed changes

jhrozek force-pushed the selectors_executor branch from f0b6675 to e6b1e24 Compare July 25, 2024 21:29

JAORMX reviewed Jul 26, 2024

View reviewed changes

JAORMX previously approved these changes Jul 26, 2024

View reviewed changes

jhrozek commented Jul 26, 2024

View reviewed changes

jhrozek added 7 commits July 26, 2024 12:45

Change the BulkGetProfilesByID DB query to return an embedded profile

00334cb

This will make it easier to extend what the query returns

Extend the BulkGetProfilesByID DB query with selectors

c7793ed

This is to return the selectors at the same time as querying the profiles from the DB and be able to use them in the executor.

Extend the ProfileAggregate structure with selectors

6fb293e

This returns the selectors to the engine through the ProfileAggregate structure

Change the selector's NewSelectionFromProfile API to accept the struc…

0156245

…ture from the models API This will make it easier to use the method in the executor

Integrate the selectors into the executor

f23e56c

When an entity is not selected for a profile, let's save that into a profile-global status override and use that for all the statuses instead calling eval. Fixes: mindersec#3724

Include which selector did unselect the entity

66c6987

In addition to returning a bool, let's also return which selector did shortcut the evaluation when returning false

Review feedback: reword confusing comment

1d88d5b

jhrozek dismissed JAORMX’s stale review via 1d88d5b July 26, 2024 10:49

jhrozek force-pushed the selectors_executor branch from e6b1e24 to 1d88d5b Compare July 26, 2024 10:49

JAORMX approved these changes Jul 26, 2024

View reviewed changes

jhrozek merged commit c9688e6 into mindersec:main Jul 26, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take selectors into use in executor #4004

Take selectors into use in executor #4004

jhrozek commented Jul 25, 2024 •

edited

Loading

jhrozek Jul 25, 2024

jhrozek Jul 25, 2024

jhrozek Jul 25, 2024

jhrozek Jul 25, 2024

JAORMX Jul 26, 2024

evankanderson Jul 26, 2024

coveralls commented Jul 25, 2024 •

edited

Loading

JAORMX Jul 26, 2024

JAORMX left a comment

jhrozek Jul 26, 2024

		for _, profile := range profileAggregates {

		profileEvalStatus := e.profileEvalStatus(ctx, provider, inf, profile)

Take selectors into use in executor #4004

Take selectors into use in executor #4004

Conversation

jhrozek commented Jul 25, 2024 • edited Loading

Summary

Change Type

Testing

Review Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

JAORMX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhrozek commented Jul 25, 2024 •

edited

Loading

coveralls commented Jul 25, 2024 •

edited

Loading