-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Take selectors into use in executor #4004
Changes from all commits
00334cb
c7793ed
6fb293e
0156245
f23e56c
66c6987
1d88d5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,11 +32,13 @@ import ( | |
"github.com/stacklok/minder/internal/engine/ingestcache" | ||
engif "github.com/stacklok/minder/internal/engine/interfaces" | ||
"github.com/stacklok/minder/internal/engine/rtengine" | ||
"github.com/stacklok/minder/internal/engine/selectors" | ||
"github.com/stacklok/minder/internal/history" | ||
minderlogger "github.com/stacklok/minder/internal/logger" | ||
"github.com/stacklok/minder/internal/profiles" | ||
"github.com/stacklok/minder/internal/profiles/models" | ||
"github.com/stacklok/minder/internal/providers/manager" | ||
provsel "github.com/stacklok/minder/internal/providers/selectors" | ||
pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" | ||
provinfv1 "github.com/stacklok/minder/pkg/providers/v1" | ||
) | ||
|
@@ -55,6 +57,7 @@ type executor struct { | |
historyService history.EvaluationHistoryService | ||
featureFlags openfeature.IClient | ||
profileStore profiles.ProfileStore | ||
selBuilder selectors.SelectionBuilder | ||
} | ||
|
||
// NewExecutor creates a new executor | ||
|
@@ -65,6 +68,7 @@ func NewExecutor( | |
historyService history.EvaluationHistoryService, | ||
featureFlags openfeature.IClient, | ||
profileStore profiles.ProfileStore, | ||
selBuilder selectors.SelectionBuilder, | ||
) Executor { | ||
return &executor{ | ||
querier: querier, | ||
|
@@ -73,6 +77,7 @@ func NewExecutor( | |
historyService: historyService, | ||
featureFlags: featureFlags, | ||
profileStore: profileStore, | ||
selBuilder: selBuilder, | ||
} | ||
} | ||
|
||
|
@@ -131,10 +136,15 @@ 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 profileEvalStatus first. Then, if the profileEvalStatus is nil | ||
// evaluate each rule and store the outcome in the database. If profileEvalStatus is non-nil, | ||
// just store it for all rules without evaluation. | ||
for _, profile := range profileAggregates { | ||
|
||
profileEvalStatus := e.profileEvalStatus(ctx, provider, inf, profile) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JAORMX I called the variable actually |
||
|
||
for _, rule := range profile.Rules { | ||
if err := e.evaluateRule(ctx, inf, provider, &profile, &rule, ruleEngineCache); err != nil { | ||
if err := e.evaluateRule(ctx, inf, provider, &profile, &rule, ruleEngineCache, profileEvalStatus); err != nil { | ||
return fmt.Errorf("error evaluating entity event: %w", err) | ||
} | ||
} | ||
|
@@ -150,6 +160,7 @@ func (e *executor) evaluateRule( | |
profile *models.ProfileAggregate, | ||
rule *models.RuleInstance, | ||
ruleEngineCache rtengine.Cache, | ||
profileEvalStatus error, | ||
) error { | ||
// Create eval status params | ||
evalParams, err := e.createEvalStatusParams(ctx, inf, profile, rule) | ||
|
@@ -176,7 +187,12 @@ func (e *executor) evaluateRule( | |
defer e.updateLockLease(ctx, *inf.ExecutionID, evalParams) | ||
|
||
// Evaluate the rule | ||
evalErr := ruleEngine.Eval(ctx, inf, evalParams) | ||
var evalErr error | ||
if profileEvalStatus != nil { | ||
evalErr = profileEvalStatus | ||
} else { | ||
evalErr = ruleEngine.Eval(ctx, inf, evalParams) | ||
} | ||
evalParams.SetEvalErr(evalErr) | ||
|
||
// Perform actionEngine, if any | ||
|
@@ -190,6 +206,37 @@ func (e *executor) evaluateRule( | |
return e.createOrUpdateEvalStatus(ctx, evalParams) | ||
} | ||
|
||
func (e *executor) profileEvalStatus( | ||
ctx context.Context, | ||
provider provinfv1.Provider, | ||
eiw *entities.EntityInfoWrapper, | ||
aggregate models.ProfileAggregate, | ||
) error { | ||
// so far this function only handles selectors. In the future we can extend it to handle other | ||
// profile-global evaluations | ||
|
||
selection, err := e.selBuilder.NewSelectionFromProfile(eiw.Type, aggregate.Selectors) | ||
if err != nil { | ||
return fmt.Errorf("error creating selection from profile: %w", err) | ||
} | ||
|
||
selEnt := provsel.EntityToSelectorEntity(ctx, provider, eiw.Type, eiw.Entity) | ||
if selEnt == nil { | ||
return fmt.Errorf("error converting entity to selector entity") | ||
} | ||
|
||
selected, matchedSelector, err := selection.Select(selEnt) | ||
if err != nil { | ||
return fmt.Errorf("error selecting entity: %w", err) | ||
} | ||
|
||
if !selected { | ||
return evalerrors.NewErrEvaluationSkipped("entity not applicable due to profile selector %s", matchedSelector) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (e *executor) updateLockLease( | ||
ctx context.Context, | ||
executionID uuid.UUID, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is too hairy, we can instead just loop over the profiles and do a get for each profile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the other hand, having everything in a single call is probably safer in the long run