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

Change required scope of entrypoint from rule to document #6963

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions ast/annotations.go
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ func (a *Annotations) Copy(node Node) *Annotations {
return &cpy
}

// toObject constructs an AST Object from a.
// toObject constructs an AST Object from the annotation.
func (a *Annotations) toObject() (*Object, *Error) {
obj := NewObject()

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

a.Scope = annotationScopeDocument
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

} else {
a.Scope = annotationScopeRule
}
case *Package:
a.Scope = annotationScopePackage
case *Import:
Expand Down Expand Up @@ -596,8 +600,9 @@ func validateAnnotationScopeAttachment(a *Annotations) *Error {
}

func validateAnnotationEntrypointAttachment(a *Annotations) *Error {
if a.Entrypoint && !(a.Scope == annotationScopeRule || a.Scope == annotationScopePackage) {
return NewError(ParseErr, a.Loc(), "annotation entrypoint applied to non-rule or package scope '%v'", a.Scope)
if a.Entrypoint && !(a.Scope == annotationScopeDocument || a.Scope == annotationScopePackage) {
return NewError(
ParseErr, a.Loc(), "annotation entrypoint applied to non-document or package scope '%v'", a.Scope)
}
return nil
}
Expand Down
82 changes: 82 additions & 0 deletions ast/annotations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,88 @@ import (
"testing"
)

func TestEntrypointAnnotationScopeRequirements(t *testing.T) {
tests := []struct {
note string
module string
expectError bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

expectScope string
}{
{
note: "package scope explicit",
module: `# METADATA
# entrypoint: true
# scope: package
package foo`,
expectError: false,
expectScope: "package",
},
{
note: "package scope implied",
module: `# METADATA
# entrypoint: true
package foo`,
expectError: false,
expectScope: "package",
},
{
note: "subpackages scope explicit",
module: `# METADATA
# entrypoint: true
# scope: subpackages
package foo`,
expectError: true,
},
{
note: "document scope explicit",
module: `package foo
# METADATA
# entrypoint: true
# scope: document
foo := true`,
expectError: false,
expectScope: "document",
},
{
note: "document scope implied",
module: `package foo
# METADATA
# entrypoint: true
foo := true`,
expectError: false,
expectScope: "document",
},
{
note: "rule scope explicit",
module: `package foo
# METADATA
# entrypoint: true
# scope: rule
foo := true`,
expectError: true,
},
}

for _, tc := range tests {
t.Run(tc.note, func(t *testing.T) {
module, err := ParseModuleWithOpts("test.rego", tc.module, ParserOptions{ProcessAnnotation: true})
if err != nil {
if !tc.expectError {
t.Errorf("unexpected error: %v", err)
}
return
}
if tc.expectError {
t.Fatalf("expected error")
}
if tc.expectScope != module.Annotations[0].Scope {
t.Fatalf("expected scope %q, got %q", tc.expectScope, module.Annotations[0].Scope)
}
})
}

}

// Test of example code in docs/content/annotations.md
func ExampleAnnotationSet_Flatten() {
modules := [][]string{
Expand Down
14 changes: 7 additions & 7 deletions cmd/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,7 @@ p2 := 2
"entrypoint":"test/p2",
"module":"/policy.wasm",
"annotations":[{
"scope":"rule",
"scope":"document",
"title":"P2",
"entrypoint":true
}]
Expand Down Expand Up @@ -742,15 +742,15 @@ bar := "baz"
"entrypoint":"test/foo/bar",
"module":"/policy.wasm",
"annotations":[{
"scope":"rule",
"scope":"document",
"title":"BAR",
"entrypoint":true
}]
},{
"entrypoint":"test/p2",
"module":"/policy.wasm",
"annotations":[{
"scope":"rule",
"scope":"document",
"title":"P2",
"entrypoint":true
}]
Expand All @@ -767,10 +767,10 @@ package test
# METADATA
# title: P doc
# scope: document
# entrypoint: true

# METADATA
# title: P
# entrypoint: true
p := 1
`,
},
Expand All @@ -784,11 +784,11 @@ p := 1
"module":"/policy.wasm",
"annotations":[{
"scope":"document",
"title":"P doc"
"title":"P doc",
"entrypoint":true
},{
"scope":"rule",
"title":"P",
"entrypoint":true
"title":"P"
}]
}]
}
Expand Down
14 changes: 7 additions & 7 deletions compile/compile.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,20 +255,20 @@ func (c *Compiler) WithRegoVersion(v ast.RegoVersion) *Compiler {
return c
}

func addEntrypointsFromAnnotations(c *Compiler, ar []*ast.AnnotationsRef) error {
for _, ref := range ar {
func addEntrypointsFromAnnotations(c *Compiler, arefs []*ast.AnnotationsRef) error {
for _, aref := range arefs {
var entrypoint ast.Ref
scope := ref.Annotations.Scope
scope := aref.Annotations.Scope

if ref.Annotations.Entrypoint {
if aref.Annotations.Entrypoint {
// Build up the entrypoint path from either package path or rule.
switch scope {
case "package":
if p := ref.GetPackage(); p != nil {
if p := aref.GetPackage(); p != nil {
entrypoint = p.Path
}
case "rule":
if r := ref.GetRule(); r != nil {
case "document":
if r := aref.GetRule(); r != nil {
entrypoint = r.Ref().GroundPrefix()
}
default:
Expand Down
4 changes: 2 additions & 2 deletions compile/compile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2106,7 +2106,7 @@ q = true`,
Annotations: []*ast.Annotations{
{
Title: "My P rule",
Scope: "rule",
Scope: "document",
Entrypoint: true,
},
},
Expand Down Expand Up @@ -2366,7 +2366,7 @@ func TestCompilerRegoEntrypointAnnotations(t *testing.T) {
wantEntrypoints map[string]struct{}
}{
{
note: "rule annotation",
note: "implied document scope annotation",
entrypoints: []string{},
modules: map[string]string{
"test.rego": `
Expand Down
18 changes: 15 additions & 3 deletions docs/content/policy-language.md
Original file line number Diff line number Diff line change
Expand Up @@ -2687,8 +2687,12 @@ Since the `document` scope annotation applies to all rules with the same name in
and the `package` and `subpackages` scope annotations apply to all packages with a matching path, metadata blocks with
these scopes are applied over all files with applicable package- and rule paths.
As there is no ordering across files in the same package, the `document`, `package`, and `subpackages` scope annotations
can only be specified **once** per path.
The `document` scope annotation can be applied to any rule in the set (i.e., ordering does not matter.)
can only be specified **once** per path. The `document` scope annotation can be applied to any rule in the set (i.e.,
ordering does not matter.)

An `entrypoint` annotation implies a `scope` of either `package` or `document`. When `entrypoint` is set to `true` on a
rule, the `scope` is automatically set to `document` if not explicitly provided. Setting the `scope` to `rule` will
result in an error, as an entrypoint always applies to the whole document.

#### Example

Expand All @@ -2708,6 +2712,13 @@ allow if {
allow if {
x == 2
}
# METADATA
# entrypoint: true
# description: |
# `scope` annotation automatically set to `document`
# as that is required for entrypoints
message := "welcome!" if allow
```

### Title
Expand Down Expand Up @@ -2890,7 +2901,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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
  ...
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I'll add an example


The `build` and `eval` CLI commands will automatically pick up annotated entrypoints; you do not have to specify them with
[`--entrypoint`](../cli/#options-1).
Expand Down