From 8497550f34e09b2ee2d982cda993755ec42e1aba Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Thu, 30 Nov 2023 11:36:51 +0100 Subject: [PATCH] Adding `--rego-v1` flag to `check` cmd (#6430) When enabled, checked module(s) must be compliant with OPA 1.0 Rego. Fixes: #6429 Signed-off-by: Johan Fylling --- ast/parser.go | 1 + ast/parser_ext.go | 5 +- cmd/check.go | 4 ++ cmd/check_test.go | 131 ++++++++++++++++++++++++++++++++++++++++++++++ loader/loader.go | 7 +++ 5 files changed, 146 insertions(+), 2 deletions(-) diff --git a/ast/parser.go b/ast/parser.go index ee1299098f..a2a3088952 100644 --- a/ast/parser.go +++ b/ast/parser.go @@ -106,6 +106,7 @@ type ParserOptions struct { SkipRules bool JSONOptions *astJSON.Options unreleasedKeywords bool // TODO(sr): cleanup + RegoV1Compatible bool } // NewParser creates and initializes a Parser. diff --git a/ast/parser_ext.go b/ast/parser_ext.go index 2dbd9f332e..c255915234 100644 --- a/ast/parser_ext.go +++ b/ast/parser_ext.go @@ -477,7 +477,7 @@ func ParseModuleWithOpts(filename, input string, popts ParserOptions) (*Module, if err != nil { return nil, err } - return parseModule(filename, stmts, comments) + return parseModule(filename, stmts, comments, popts.RegoV1Compatible) } // ParseBody returns exactly one body. @@ -637,7 +637,7 @@ func ParseStatementsWithOpts(filename, input string, popts ParserOptions) ([]Sta return stmts, comments, nil } -func parseModule(filename string, stmts []Statement, comments []*Comment) (*Module, error) { +func parseModule(filename string, stmts []Statement, comments []*Comment, regoV1Compatible bool) (*Module, error) { if len(stmts) == 0 { return nil, NewError(ParseErr, &Location{File: filename}, "empty module") @@ -658,6 +658,7 @@ func parseModule(filename string, stmts []Statement, comments []*Comment) (*Modu // The comments slice only holds comments that were not their own statements. mod.Comments = append(mod.Comments, comments...) + mod.regoV1Compatible = regoV1Compatible for i, stmt := range stmts[1:] { switch stmt := stmt.(type) { diff --git a/cmd/check.go b/cmd/check.go index db8528143f..2e07721404 100644 --- a/cmd/check.go +++ b/cmd/check.go @@ -25,6 +25,7 @@ type checkParams struct { capabilities *capabilitiesFlag schema *schemaFlags strict bool + regoV1 bool } func newCheckParams() checkParams { @@ -64,6 +65,7 @@ func checkModules(params checkParams, args []string) error { if params.bundleMode { for _, path := range args { b, err := loader.NewFileLoader(). + WithRegoV1Compatible(params.regoV1). WithSkipBundleVerification(true). WithProcessAnnotation(true). WithCapabilities(capabilities). @@ -82,6 +84,7 @@ func checkModules(params checkParams, args []string) error { } result, err := loader.NewFileLoader(). + WithRegoV1Compatible(params.regoV1). WithProcessAnnotation(true). WithCapabilities(capabilities). Filtered(args, f.Apply) @@ -165,5 +168,6 @@ func init() { addCapabilitiesFlag(checkCommand.Flags(), checkParams.capabilities) addSchemaFlags(checkCommand.Flags(), checkParams.schema) addStrictFlag(checkCommand.Flags(), &checkParams.strict, false) + checkCommand.Flags().BoolVar(&checkParams.regoV1, "rego-v1", false, "check for Rego v1 compatibility (policies must also be compatible with current OPA version)") RootCommand.AddCommand(checkCommand) } diff --git a/cmd/check_test.go b/cmd/check_test.go index b140644785..bbc9501e95 100644 --- a/cmd/check_test.go +++ b/cmd/check_test.go @@ -236,3 +236,134 @@ p { t.Fatalf("unexpected error from eval with inlined schema, got: %v", err) } } + +func TestCheckRegoV1(t *testing.T) { + cases := []struct { + note string + policy string + expErrs []string + }{ + { + note: "rego.v1 imported, v1 compliant", + policy: `package test +import rego.v1 +p contains x if { + x := [1,2,3] +}`, + }, + { + note: "rego.v1 imported, NOT v1 compliant (parser)", + policy: `package test +import rego.v1 +p contains x { + x := [1,2,3] +} + +q.r`, + expErrs: []string{ + "test.rego:3: rego_parse_error: `if` keyword is required before rule body", + "test.rego:7: rego_parse_error: `contains` keyword is required for partial set rules", + }, + }, + { + note: "rego.v1 imported, NOT v1 compliant (compiler)", + policy: `package test +import rego.v1 + +import data.foo +import data.bar as foo +`, + expErrs: []string{ + "test.rego:5: rego_compile_error: import must not shadow import data.foo", + }, + }, + { + note: "keywords imported, v1 compliant", + policy: `package test +import future.keywords.if +import future.keywords.contains +p contains x if { + x := [1,2,3] +}`, + }, + { + note: "keywords imported, NOT v1 compliant", + policy: `package test +import future.keywords.contains +p contains x { + x := [1,2,3] +} + +q.r`, + expErrs: []string{ + "test.rego:3: rego_parse_error: `if` keyword is required before rule body", + "test.rego:7: rego_parse_error: `contains` keyword is required for partial set rules", + }, + }, + { + note: "keywords imported, NOT v1 compliant (compiler)", + policy: `package test +import future.keywords.if + +input := 1 if { + 1 == 2 +}`, + expErrs: []string{ + "test.rego:4: rego_compile_error: rules must not shadow input (use a different rule name)", + }, + }, + { + note: "no imports, v1 compliant", + policy: `package test +p := 1 +`, + }, + { + note: "no imports, NOT v1 compliant but v0 compliant (compiler)", + policy: `package test +p.x`, + expErrs: []string{ + "test.rego:2: rego_parse_error: `contains` keyword is required for partial set rules", + }, + }, + { + note: "no imports, v1 compliant but NOT v0 compliant", + policy: `package test +p contains x if { + x := [1,2,3] +}`, + expErrs: []string{ + "test.rego:2: rego_parse_error: var cannot be used for rule name", // This error actually appears three times: once for 'p'; once for 'contains'; and once for 'x'. All are interpreted as [invalid] rule declarations with no value and body. + "test.rego:2: rego_parse_error: `if` keyword is required before rule body", + }, + }, + } + + for _, tc := range cases { + t.Run(tc.note, func(t *testing.T) { + files := map[string]string{ + "test.rego": tc.policy, + } + + test.WithTempFS(files, func(root string) { + params := newCheckParams() + params.regoV1 = true + + err := checkModules(params, []string{root}) + switch { + case err != nil && len(tc.expErrs) > 0: + for _, expErr := range tc.expErrs { + if !strings.Contains(err.Error(), expErr) { + t.Fatalf("expected err:\n\n%v\n\ngot:\n\n%v", expErr, err) + } + } + return // don't read back bundle below + case err != nil && len(tc.expErrs) == 0: + t.Fatalf("unexpected error: %v", err) + case err == nil && len(tc.expErrs) > 0: + t.Fatalf("expected error:\n\n%v\n\ngot: none", tc.expErrs) + } + }) + }) + } +} diff --git a/loader/loader.go b/loader/loader.go index 4583f21b44..7b3eac40a5 100644 --- a/loader/loader.go +++ b/loader/loader.go @@ -102,6 +102,8 @@ type FileLoader interface { WithProcessAnnotation(bool) FileLoader WithCapabilities(*ast.Capabilities) FileLoader WithJSONOptions(*astJSON.Options) FileLoader + + WithRegoV1Compatible(bool) FileLoader } // NewFileLoader returns a new FileLoader instance. @@ -181,6 +183,11 @@ func (fl *fileLoader) WithJSONOptions(opts *astJSON.Options) FileLoader { return fl } +func (fl *fileLoader) WithRegoV1Compatible(compatible bool) FileLoader { + fl.opts.RegoV1Compatible = compatible + return fl +} + // All returns a Result object loaded (recursively) from the specified paths. func (fl fileLoader) All(paths []string) (*Result, error) { return fl.Filtered(paths, nil)