Skip to content

Commit

Permalink
Adding --rego-v1 flag to check cmd
Browse files Browse the repository at this point in the history
When enabled, checked module must be compliant with OPA 1.0 Rego.

Fixes: open-policy-agent#6429
Signed-off-by: Johan Fylling <[email protected]>
  • Loading branch information
johanfylling authored and ashutosh-narkar committed Nov 29, 2023
1 parent 7a32e8f commit 123c055
Show file tree
Hide file tree
Showing 6 changed files with 161 additions and 6 deletions.
17 changes: 14 additions & 3 deletions ast/compile.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ type Compiler struct {
inputType types.Type // global input type retrieved from schema set
annotationSet *AnnotationSet // hierarchical set of annotations
strict bool // enforce strict compilation checks
regoV1Compatible bool // indicates if compiler should enforce Rego v1 compatibility
keepModules bool // whether to keep the unprocessed, parse modules (below)
parsedModules map[string]*Module // parsed, but otherwise unprocessed modules, kept track of when keepModules is true
useTypeCheckAnnotations bool // whether to provide annotated information (schemas) to the type checker
Expand Down Expand Up @@ -442,6 +443,12 @@ func (c *Compiler) WithStrict(strict bool) *Compiler {
return c
}

// WithRegoV1Compatible enables Rego v1 compatibility mode in the compiler.
func (c *Compiler) WithRegoV1Compatible(yes bool) *Compiler {
c.regoV1Compatible = yes
return c
}

// WithKeepModules enables retaining unprocessed modules in the compiler.
// Note that the modules aren't copied on the way in or out -- so when
// accessing them via ParsedModules(), mutations will occur in the module
Expand Down Expand Up @@ -1537,7 +1544,7 @@ func (c *Compiler) checkUnsafeBuiltins() {
func (c *Compiler) checkDeprecatedBuiltins() {
for _, name := range c.sorted {
mod := c.Modules[name]
errs := checkDeprecatedBuiltins(c.deprecatedBuiltinsMap, mod, c.strict || mod.regoV1Compatible)
errs := checkDeprecatedBuiltins(c.deprecatedBuiltinsMap, mod, c.strict || c.isRegoV1Compatible(mod))
for _, err := range errs {
c.err(err)
}
Expand Down Expand Up @@ -1679,7 +1686,7 @@ func (c *Compiler) GetAnnotationSet() *AnnotationSet {
func (c *Compiler) checkDuplicateImports() {
for _, name := range c.sorted {
mod := c.Modules[name]
if !c.strict && !mod.regoV1Compatible {
if !c.strict && !c.isRegoV1Compatible(mod) {
continue
}

Expand All @@ -1700,7 +1707,7 @@ func (c *Compiler) checkDuplicateImports() {
func (c *Compiler) checkKeywordOverrides() {
for _, name := range c.sorted {
mod := c.Modules[name]
errs := checkKeywordOverrides(mod, c.strict || mod.regoV1Compatible)
errs := checkKeywordOverrides(mod, c.strict || c.isRegoV1Compatible(mod))
for _, err := range errs {
c.err(err)
}
Expand Down Expand Up @@ -2706,6 +2713,10 @@ func (c *Compiler) setGraph() {
c.Graph = NewGraph(c.Modules, list)
}

func (c *Compiler) isRegoV1Compatible(m *Module) bool {
return c.regoV1Compatible || m.regoV1Compatible
}

type queryCompiler struct {
compiler *Compiler
qctx *QueryContext
Expand Down
1 change: 1 addition & 0 deletions ast/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions ast/parser_ext.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -693,7 +693,7 @@ func parseModule(filename string, stmts []Statement, comments []*Comment) (*Modu
}
}

if mod.regoV1Compatible {
if mod.regoV1Compatible || regoV1Compatible {
for _, rule := range mod.Rules {
for r := rule; r != nil; r = r.Else {
var t string
Expand Down
5 changes: 5 additions & 0 deletions cmd/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ type checkParams struct {
capabilities *capabilitiesFlag
schema *schemaFlags
strict bool
regoV1 bool
}

func newCheckParams() checkParams {
Expand Down Expand Up @@ -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).
Expand All @@ -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)
Expand All @@ -100,6 +103,7 @@ func checkModules(params checkParams, args []string) error {
WithSchemas(ss).
WithEnablePrintStatements(true).
WithStrict(params.strict).
WithRegoV1Compatible(params.regoV1).
WithUseTypeCheckAnnotations(true)

compiler.Compile(modules)
Expand Down Expand Up @@ -165,5 +169,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")
RootCommand.AddCommand(checkCommand)
}
131 changes: 131 additions & 0 deletions cmd/check_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
})
}
}
7 changes: 7 additions & 0 deletions loader/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 123c055

Please sign in to comment.