From 0083ee8723d2414875ab4ed93e0a46737a92f2a3 Mon Sep 17 00:00:00 2001 From: Anders Eknert Date: Mon, 26 Aug 2024 21:00:22 +0200 Subject: [PATCH] Change required scope of `entrypoint` from `rule` to `document` 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 #6798 Signed-off-by: Anders Eknert --- ast/annotations.go | 13 ++++-- ast/annotations_test.go | 74 +++++++++++++++++++++++++++++++++ cmd/build_test.go | 14 +++---- compile/compile.go | 14 +++---- compile/compile_test.go | 4 +- docs/content/policy-language.md | 7 +++- 6 files changed, 105 insertions(+), 21 deletions(-) diff --git a/ast/annotations.go b/ast/annotations.go index 9663b0cc67..7d09379fd5 100644 --- a/ast/annotations.go +++ b/ast/annotations.go @@ -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() @@ -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: @@ -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 } diff --git a/ast/annotations_test.go b/ast/annotations_test.go index 05563d5197..a8bae89a62 100644 --- a/ast/annotations_test.go +++ b/ast/annotations_test.go @@ -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{ diff --git a/cmd/build_test.go b/cmd/build_test.go index 3dc1bcfbb6..0b8178acde 100644 --- a/cmd/build_test.go +++ b/cmd/build_test.go @@ -653,7 +653,7 @@ p2 := 2 "entrypoint":"test/p2", "module":"/policy.wasm", "annotations":[{ - "scope":"rule", + "scope":"document", "title":"P2", "entrypoint":true }] @@ -742,7 +742,7 @@ bar := "baz" "entrypoint":"test/foo/bar", "module":"/policy.wasm", "annotations":[{ - "scope":"rule", + "scope":"document", "title":"BAR", "entrypoint":true }] @@ -750,7 +750,7 @@ bar := "baz" "entrypoint":"test/p2", "module":"/policy.wasm", "annotations":[{ - "scope":"rule", + "scope":"document", "title":"P2", "entrypoint":true }] @@ -767,10 +767,10 @@ package test # METADATA # title: P doc # scope: document +# entrypoint: true # METADATA # title: P -# entrypoint: true p := 1 `, }, @@ -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" }] }] } diff --git a/compile/compile.go b/compile/compile.go index a0ac6fc4be..142cb87716 100644 --- a/compile/compile.go +++ b/compile/compile.go @@ -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: diff --git a/compile/compile_test.go b/compile/compile_test.go index b3db2646b8..a5d1201e71 100644 --- a/compile/compile_test.go +++ b/compile/compile_test.go @@ -2106,7 +2106,7 @@ q = true`, Annotations: []*ast.Annotations{ { Title: "My P rule", - Scope: "rule", + Scope: "document", Entrypoint: true, }, }, @@ -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": ` diff --git a/docs/content/policy-language.md b/docs/content/policy-language.md index dd4d25dc2e..919530292d 100644 --- a/docs/content/policy-language.md +++ b/docs/content/policy-language.md @@ -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 @@ -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).