Skip to content
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

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

Closed
anderseknert opened this issue Jun 6, 2024 · 3 comments · Fixed by #6963
Closed

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

anderseknert opened this issue Jun 6, 2024 · 3 comments · Fixed by #6963

Comments

@anderseknert
Copy link
Member

Having worked on some Regal rules around metadata annotations recently, and the ambiguous-scope rule specifically, I was surprised to see that the entrypoint attribute worked for annotations scoped to rule. AFAIK, an entrypoint will always point to the "whole" rule — in other words the document. I thought it was just an oversight, and a (by all means, harmless) bug that something silly like defining an incremental rule where one is entrypoint: true and the other is entrypoint: false was allowed, as clearly both will be evaluated when one of them is marked as an entrypoint?

But now that I tried to change the scope to document, that's apparently a parser error 🤔

package policy

import rego.v1

# METADATA
# entrypoint: true
# scope: document
allow = true
1 error occurred: p.rego:5: rego_parse_error: annotation entrypoint applied to non-rule or package scope 'document'

Which had me check the documentation on entrypoint, and the docs confirm this too:

This value is false by default, and can only be used at rule or package scope.

So while this isn't a bug in the sense that something isn't working as intended or documented — this doesn't seem right to me? Isn't an entrypoint on a rule always scoped to the whole rule (i.e. the document) rather than a specific rule definition?

@tsandall
Copy link
Member

tsandall commented Jun 14, 2024

Agreed that entrypoint is applicable to document or package (or I suppose, subpackages) but rule does not make sense (and similarly, if we had a file scope, it wouldn't make sense either.) I'm guessing this was just an oversight in the original implementation.

After having used annotations a fair amount recently I would say that having to specify the scope is a bit of a pain and most often felt with the document scope. It's just another thing that users can get wrong.

I wonder if we could default the scope differently than we already do today... the current defaults are based on the place in the file, e.g., preceeding a package makes it package scope and preceeding a rule makes it rule scope. Instead of using the file location, what if each type of annotation had a required scope... then the default/inferred scope would be the maximum scope of all the annotations in the METADATA snippet.

@johanfylling
Copy link
Contributor

Changing the default scope of annotations is a semantic change. Would this be a v1 feature? Or, to not risk pushing that release back even further with additional changes, even v2 (assuming we tighten the major version cadence)?

Copy link

stale bot commented Aug 10, 2024

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

@stale stale bot added the inactive label Aug 10, 2024
anderseknert added a commit to anderseknert/opa that referenced this issue Aug 26, 2024
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 added a commit to anderseknert/opa that referenced this issue Aug 26, 2024
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 added a commit to anderseknert/opa that referenced this issue Aug 27, 2024
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 added a commit to anderseknert/opa that referenced this issue Aug 27, 2024
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 added a commit to anderseknert/opa that referenced this issue Aug 28, 2024
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 added a commit to anderseknert/opa that referenced this issue Aug 28, 2024
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 added a commit to anderseknert/opa that referenced this issue Aug 28, 2024
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>
johanfylling pushed a commit that referenced this issue Aug 28, 2024
And automatically change implied `scope` from `rule` to `document` when
no `scope` is provided (on rule metadata).

Fixes #6798

Signed-off-by: Anders Eknert <anders@styra.com>
This was referenced Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants