Skip to content

Commit

Permalink
Change required scope of entrypoint from rule to document
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
anderseknert committed Aug 27, 2024
1 parent 7b535a7 commit 735aea2
Show file tree
Hide file tree
Showing 6 changed files with 105 additions and 21 deletions.
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 {
a.Scope = annotationScopeDocument
} 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
74 changes: 74 additions & 0 deletions ast/annotations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,80 @@ import (
"testing"
)

func TestEntrypointAnnotationScopeRequirements(t *testing.T) {
tests := []struct {
note string
module string
expectError bool
}{
{
note: "package scope explicit",
module: `# METADATA
# entrypoint: true
# scope: package
package foo`,
expectError: false,
},
{
note: "package scope implied",
module: `# METADATA
# entrypoint: true
package foo`,
expectError: false,
},
{
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,
},
{
note: "document scope implied",
module: `package foo
# METADATA
# entrypoint: true
foo := true`,
expectError: false,
},
{
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) {
_, 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")
}
})
}

}

// 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
7 changes: 6 additions & 1 deletion docs/content/policy-language.md
Original file line number Diff line number Diff line change
Expand Up @@ -2690,6 +2690,10 @@ As there is no ordering across files in the same package, the `document`, `packa
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 used 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

```live:rego/metadata/scope:module:read_only
Expand Down Expand Up @@ -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`.

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

0 comments on commit 735aea2

Please sign in to comment.