Skip to content

Commit

Permalink
Include which selector did unselect the entity
Browse files Browse the repository at this point in the history
In addition to returning a bool, let's also return which selector did
shortcut the evaluation when returning false
  • Loading branch information
jhrozek committed Jul 25, 2024
1 parent b668c93 commit f0b6675
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 17 deletions.
4 changes: 2 additions & 2 deletions cmd/dev/app/rule_type/rttst.go
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ func selectAndEval(
return fmt.Errorf("error converting entity to selector entity")
}

selected, err := profileSelectors.Select(selEnt)
selected, matchedSelector, err := profileSelectors.Select(selEnt)
if err != nil {
return fmt.Errorf("error selecting entity: %w", err)
}
Expand All @@ -323,7 +323,7 @@ func selectAndEval(
if selected {
evalErr = eng.Eval(ctx, inf, evalStatus)
} else {
evalErr = errors.NewErrEvaluationSkipped("entity not selected by selectors")
evalErr = errors.NewErrEvaluationSkipped("entity not selected by selector %s", matchedSelector)
}

return evalErr
Expand Down
4 changes: 2 additions & 2 deletions internal/engine/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,13 @@ func (e *executor) profileEvalStatus(
return fmt.Errorf("error converting entity to selector entity")
}

selected, err := selection.Select(selEnt)
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")
return evalerrors.NewErrEvaluationSkipped("entity not applicable due to profile selector %s", matchedSelector)
}

return nil
Expand Down
22 changes: 12 additions & 10 deletions internal/engine/selectors/selectors.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ func newEnvForEntity(varName string, typ any, typName string) (*cel.Env, error)
}

type compiledSelector struct {
orig string
ast *cel.Ast
program cel.Program
}
Expand All @@ -133,6 +134,7 @@ func compileSelectorForEntity(env *cel.Env, selector string) (*compiledSelector,

return &compiledSelector{
ast: checked,
orig: selector,
program: program,
}, nil
}
Expand Down Expand Up @@ -246,7 +248,7 @@ func WithUnknownPaths(paths ...string) SelectOption {

// Selection is an interface for selecting entities based on a profile
type Selection interface {
Select(*internalpb.SelectorEntity, ...SelectOption) (bool, error)
Select(*internalpb.SelectorEntity, ...SelectOption) (bool, string, error)
}

// EntitySelection is a struct that holds the compiled CEL expressions for a given entity type
Expand All @@ -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) {
if se == nil {
return false, fmt.Errorf("input entity is nil")
return false, "", fmt.Errorf("input entity is nil")
}

var opts selectionOptions
Expand All @@ -271,34 +273,34 @@ func (s *EntitySelection) Select(se *internalpb.SelectorEntity, userOpts ...Sele
for _, sel := range s.selector {
entityMap, err := inputAsMap(se)
if err != nil {
return false, fmt.Errorf("failed to convert input to map: %w", err)
return false, "", fmt.Errorf("failed to convert input to map: %w", err)
}

out, details, err := s.evalWithOpts(&opts, sel, entityMap)
// check unknowns /before/ an error. Maybe we should try to special-case the one
// error we get from the CEL library in this case and check for the rest?
if s.detailHasUnknowns(sel, details) {
return false, ErrResultUnknown
return false, "", ErrResultUnknown
}

if err != nil {
return false, fmt.Errorf("failed to evaluate Expression: %w", err)
return false, "", fmt.Errorf("failed to evaluate Expression: %w", err)
}

if types.IsUnknown(out) {
return false, ErrResultUnknown
return false, "", ErrResultUnknown
}

if out.Type() != cel.BoolType {
return false, fmt.Errorf("expression did not evaluate to a boolean: %v", out)
return false, "", fmt.Errorf("expression did not evaluate to a boolean: %v", out)
}

if !out.Value().(bool) {
return false, nil
return false, sel.orig, nil
}
}

return true, nil
return true, "", nil
}

func unknownAttributesFromOpts(unknownPaths []string) []*interpreter.AttributePattern {
Expand Down
11 changes: 8 additions & 3 deletions internal/engine/selectors/selectors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ func TestSelectSelectorEntity(t *testing.T) {
require.NoError(t, err)
require.NotNil(t, sels)

selected, err := sels.Select(se, scenario.selectOptions...)
selected, matchedSelector, err := sels.Select(se, scenario.selectOptions...)
if scenario.expectedSelectErr != nil {
require.Error(t, err)
require.Equal(t, scenario.expectedSelectErr, err)
Expand All @@ -516,6 +516,11 @@ func TestSelectSelectorEntity(t *testing.T) {

require.NoError(t, err)
require.Equal(t, scenario.selected, selected)
if !selected {
// TODO(jakub): Add tests with more selectors. If we have more than one selector, we should
// also match against an expected index
require.Equal(t, scenario.exprs[0].Selector, matchedSelector)
}
})
}
}
Expand Down Expand Up @@ -597,13 +602,13 @@ func TestSelectorEntityFillProperties(t *testing.T) {
require.NoError(t, err)
require.NotNil(t, sels)

_, err = sels.Select(se, WithUnknownPaths("repository.properties"))
_, _, err = sels.Select(se, WithUnknownPaths("repository.properties"))
require.ErrorIs(t, err, ErrResultUnknown)

// simulate fetching properties
scenario.mockFetch(se)

selected, err := sels.Select(se)
selected, _, err := sels.Select(se)
if scenario.secondSucceeds {
require.NoError(t, err)
require.True(t, selected)
Expand Down

0 comments on commit f0b6675

Please sign in to comment.