Change required scope of `entrypoint` from `rule` to `document` #6963

anderseknert · 2024-08-26T19:08:04Z

And automatically change implied scope from rule to document when no scope is provided (on rule metadata).

Whether this can be included in a "normal" release, or will have to wait until OPA 1.0, I'll let others decide. But I think it's worth pointing out that this is a breaking change to address a bug, and that a scope of rule makes no sense for an entrypoint.

One alternative could perhaps be to only change this behavior when import rego.v1 is included in a file, as that's meant to be a glimpse of the future anyway. But at this time, no such considerations have been taken.

netlify · 2024-08-26T19:15:50Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`8b558a1`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/66cefc80bf5b4d000896fa02
😎 Deploy Preview	https://deploy-preview-6963--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

johanfylling

LGTM 👍

I think we should address the possible confusion of an entrypoint: false annotation, though.

johanfylling · 2024-08-28T08:59:01Z

ast/annotations.go

@@ -556,7 +556,11 @@ func attachAnnotationsNodes(mod *Module) Errors {
 if a.Scope == "" {
 switch a.node.(type) {
 case *Rule:
- a.Scope = annotationScopeRule
+ if a.Entrypoint {
+ a.Scope = annotationScopeDocument


If we left this out, then this breaking change would be more overt, and not hidden. The drawback would be a more verbose annotation block whenever the entrypoint annotation is used, as we also need to declare the scope.

Personally, I prefer when breaking changes are more obvious.
The impact area will probably be pretty small, though, so I'm not going to put my foot down on this.

I think that's missing the larger point, which is that without this behavior, this change will be breaking every current policy where an entrypoint annotation is currently used today. Here's a list of public repos making use of that: https://github.com/search?q=language%3A%22Open+Policy+Agent%22+%22%23+entrypoint%3A%22&type=code

And users of those projects would effectively have to use the latest version of OPA as soon as "their project" bumps the OPA version, as setting scope: document on an entrypoint is currently a parser error.

Only one of the projects in that list would AFAICS "break" with this change as currently desigend, and that's a single place in Regal. I can deal with that :)

So if we want to minimize the impact area, this is the way.

I understand that, which is why I'm not making a big fuss ;).

Generally though, I think it's better to break user projects by compile-time errors, than to change the semantic behavior and rely on users either reading the changelog (and realizing the impact) or not being adversely affected by the change without noticing.
Just doing the compile-time error would require more upfront work by users to upgrade, but it also guarantees that none is unknowingly affected by the breaking change (it is still breaking, after all).

johanfylling · 2024-08-28T09:02:15Z

ast/annotations_test.go

+ tests := []struct {
+ note string
+ module string
+ expectError bool


Could we also assert that the scope is set as expected?

Sure, I'll amend the tests to do that 👍

johanfylling · 2024-08-28T09:12:31Z

docs/content/policy-language.md

@@ -2890,7 +2894,8 @@ allow if {
 ### Entrypoint

 The `entrypoint` annotation is a boolean used to mark rules and packages that should be used as entrypoints for a policy.
-This value is false by default, and can only be used at `rule` or `package` scope.
+This value is false by default, and can only be used at `document` or `package` scope. When used on a rule with no
+explicit `scope` set, the presence of an `entrypoint` annotation will automatically set the scope to `document`.


I just noticed this section doesn't have an example. Should we add one since we're here already? Would be an opportunity to make the scope implications even more obvious.

# METADATA # entrypoint: true allow if { ... } # NOTE: The METADATA block on the above rule has an implicit scope of 'document' and also applies to this rule allow if { ... }

Sure! I'll add an example

johanfylling · 2024-08-28T09:24:07Z

ast/annotations.go

@@ -556,7 +556,11 @@ func attachAnnotationsNodes(mod *Module) Errors {
 if a.Scope == "" {
 switch a.node.(type) {
 case *Rule:
- a.Scope = annotationScopeRule
+ if a.Entrypoint {


So the document scope is only implied if entrypoint is explicitly set to true?

# METADATA # entrypoint: false # custom: # only_applicable_to_doc_scope: I hope this doesn't show up in my rule annotations allow if { ... }

Only going by the docs, someone authoring the above would probably expect the metadata to be scoped to document. We should either make this explicit in the docs, or enforce the document scope regardless of its value (make it a pointer to a bool, so we can check for nil? 🤔).

I can add to the docs that it applies when entrypoint is set to true.

Changing to a pointer is more of a breaking change than anything in here so far, IMHO :)

johanfylling

👍

And automatically change implied `scope` from `rule` to `document` when no `scope` is provided (on rule metadata). Whether this can be included in a "normal" release, or will have to wait until OPA 1.0, I'll let others decide. But I think it's worth pointing out that this is a breaking change to address a **bug**, and that a scope of `rule` makes no sense for an entrypoint. One alternative could perhaps be to only change this behavior when `import rego.v1` is included in a file, as that's meant to be a glimpse of the future anyway. But at this time, no such considerations have been taken. Fixes open-policy-agent#6798 Signed-off-by: Anders Eknert <anders@styra.com>

anderseknert force-pushed the entrypoint-document branch from d5bb986 to 56004bd Compare August 26, 2024 19:08

anderseknert linked an issue Aug 26, 2024 that may be closed by this pull request

Not clear why metadata attribute entrypoint requires scope: rule #6798

Closed

anderseknert force-pushed the entrypoint-document branch from 56004bd to 0083ee8 Compare August 26, 2024 19:14

anderseknert force-pushed the entrypoint-document branch 2 times, most recently from 42da21f to 735aea2 Compare August 27, 2024 23:45

johanfylling reviewed Aug 28, 2024

View reviewed changes

anderseknert force-pushed the entrypoint-document branch 2 times, most recently from c3cb34f to f318581 Compare August 28, 2024 10:15

johanfylling previously approved these changes Aug 28, 2024

View reviewed changes

anderseknert dismissed johanfylling’s stale review via 8b558a1 August 28, 2024 10:31

anderseknert force-pushed the entrypoint-document branch from f318581 to 8b558a1 Compare August 28, 2024 10:31

johanfylling approved these changes Aug 28, 2024

View reviewed changes

johanfylling merged commit f10cc1f into open-policy-agent:main Aug 28, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change required scope of `entrypoint` from `rule` to `document` #6963

Change required scope of `entrypoint` from `rule` to `document` #6963

anderseknert commented Aug 26, 2024 •

edited

Loading

netlify bot commented Aug 26, 2024 •

edited

Loading

johanfylling left a comment

johanfylling Aug 28, 2024

anderseknert Aug 28, 2024

johanfylling Aug 28, 2024

johanfylling Aug 28, 2024

anderseknert Aug 28, 2024

johanfylling Aug 28, 2024

anderseknert Aug 28, 2024

johanfylling Aug 28, 2024

anderseknert Aug 28, 2024

johanfylling left a comment

Change required scope of entrypoint from rule to document #6963

Change required scope of entrypoint from rule to document #6963

Conversation

anderseknert commented Aug 26, 2024 • edited Loading

netlify bot commented Aug 26, 2024 • edited Loading

✅ Deploy Preview for openpolicyagent ready!

johanfylling left a 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

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

johanfylling left a comment

Choose a reason for hiding this comment

Change required scope of `entrypoint` from `rule` to `document` #6963

Change required scope of `entrypoint` from `rule` to `document` #6963

anderseknert commented Aug 26, 2024 •

edited

Loading

netlify bot commented Aug 26, 2024 •

edited

Loading