engine: Add rego evaluation engine #784
Conversation
| // jq is only used if the `jq` type is selected. | ||
| // It defines the comparisons that are made between | ||
| // the ingested data and the policy rule. | ||
| repeated JQComparison jq = 2; | ||
|
|
||
| // rego is only used if the `rego` type is selected. | ||
| optional Rego rego = 3; |
There was a problem hiding this comment.
Would we want to make these a oneof to ensure that only one type is set?
There was a problem hiding this comment.
We need to switch out parsing to using protojson for this to work. I asked a question here https://stacklok.slack.com/archives/C057KR4DT0W/p1693378177378369 so I can look at that in a separate PR
| } | ||
|
|
||
| // NewRegoEvaluator creates a new rego evaluator | ||
| func NewRegoEvaluator(cfg *pb.RuleType_Definition_Eval_Rego) (*Evaluator, error) { |
There was a problem hiding this comment.
If we used the enclosing pb.RuleType_Definition_Eval message here, we could define a common "create me one" interface for evaluators. I don't know if that's important, but it would make it slightly easier to plug in additional evaluation engines if we want.
There was a problem hiding this comment.
true! Let me write myself an action point to look into this.
| r := e.newRegoFromOptions( | ||
| rego.Dump(&buf), | ||
| ) | ||
| pq, err := r.PrepareForEval(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not prepare Rego: %w", err) | ||
| } |
There was a problem hiding this comment.
Is this a future caching opportunity (i.e. run this in NewRegoEvaluator)?
There was a problem hiding this comment.
if we're evaluating the same data, yeah, we could probably cache the data and/or the result.
| return fmt.Errorf("missing details in result") | ||
| } | ||
|
|
||
| detmap, ok := det.(map[string]interface{}) |
There was a problem hiding this comment.
Isn't det a rego.Vars, which is an alias of map[string]interface{}? i.e. do we need this line at all?
There was a problem hiding this comment.
It seems that we do, we can't access the contents of the rego.Vars otherwise. Seems to me like it was kind of an unnecessary aliasing 😕
JAORMX
force-pushed
the
rego
branch
2 times, most recently
from
8afe8bc
to
0e54514
Compare
This adds rego as an engine for rules, it takes whatever input from the ingester and is able to evaluate it against OPA/rego policies. We also pass in the policy from mediator so we could use that as input if needed. There are two modes of operations: - `deny-by-default`: Which is more secure and expects policy writers to only set an `allow` variable to true if the policy matches expectations - `constraints`: will deny the request if a violation is found. It can return a message with further explanations for the denial. the reason why I didn't just use the `deny-by-default` as the only engine is that it's non-trivial to return meaningful messages to the user with such a model in rego.
This adds rego as an engine for rules, it takes whatever input
from the ingester and is able to evaluate it against OPA/rego policies.
We also pass in the policy from mediator so we could use that as input
if needed.
There are two modes of operations:
deny-by-default: Which is more secure and expects policy writers to only set anallowvariable to true if the policy matches expectationsconstraints: will deny the request if a violation is found. It can return a message with further explanations for the denial.the reason why I didn't just use the
deny-by-defaultas the only engine is that it's non-trivial to returnmeaningful messages to the user with such a model in rego.