-
Notifications
You must be signed in to change notification settings - Fork 39
feat(policies): add rego boilerplate injection #2515
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sylwester Piskozub <[email protected]>
Signed-off-by: Sylwester Piskozub <[email protected]>
Signed-off-by: Sylwester Piskozub <[email protected]>
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.
Great, @Piskoo. Just please consider adding the ignore rule.
Signed-off-by: Sylwester Piskozub <[email protected]>
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.
Overall, it looks good to me, and I don’t want to block the PR. However, I have a question: instead of injecting the template into the incoming policy source, why don’t we create an external rego package that we automatically import into the file? Then, both the template and the package would be loaded into the registry engine.
I went with injecting the template mainly to keep the setup straightforward and avoid changing how policies are loaded right now. Providing it in another package would require policy authors to refer to these rules by a namespace, which would change how existing policies are written and potentially break compatibility with the current structure. |
@javirln has a very good point here, what's the best practice in general w.r.t extensibility of rego code? i.e
right, but we could migrate little by little now? I am not saying we should do it like this, I am more interested in knowing what's the idiomatic way of doing this, specially since we'll be providing an SDK methods. |
The idiomatic OPA way is like @javirln mentioned to define shared helpers in separate packages and import them, since that supports versioning and reuse. |
|
|
||
| // buildBoilerplate constructs the boilerplate template based on what's missing | ||
| func buildBoilerplate(existingRules map[string]bool) (string, error) { | ||
| data := boilerplateData{ |
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.
Be careful, we still need the default rules. So maybe we should also look if there is already a "default" rule for each
default skipped := true
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.
please check my comment about default rules
There are two different topics mentioned in these comments:
In the policy we would inject something like this (because we want to keep it simple for users so that they don't worry about boilerplate): As you can see, we are not improving the UX at all, just replacing one rule by an import and another rule.
|
Thanks for the breakdown, I understand now, the current approach LGTM! |
Signed-off-by: Sylwester Piskozub <[email protected]>
Summary
Implemented automatic missing boilerplate injection for rego policies in the engine before policy evaluation.
Changes
packageandimportdeclarations.policy devel init.policy devel lintsince we no longer requireresultrule to be present in the policy.Example
Before policy required declaration of all rules used internally
Now these rules are injected into policy if they are missing, so above policy can be simplified to: