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

ast+cmd: Allowing bundle to contain calls to unknown functions when inspected #6462

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 17 additions & 8 deletions ast/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,15 @@ type exprChecker func(*TypeEnv, *Expr) *Error
// accumulated on the typeChecker so that a single run can report multiple
// issues.
type typeChecker struct {
builtins map[string]*Builtin
required *Capabilities
errs Errors
exprCheckers map[string]exprChecker
varRewriter varRewriter
ss *SchemaSet
allowNet []string
input types.Type
builtins map[string]*Builtin
required *Capabilities
errs Errors
exprCheckers map[string]exprChecker
varRewriter varRewriter
ss *SchemaSet
allowNet []string
input types.Type
allowUndefinedFuncs bool
}

// newTypeChecker returns a new typeChecker object that has no errors.
Expand Down Expand Up @@ -92,6 +93,11 @@ func (tc *typeChecker) WithInputType(tpe types.Type) *typeChecker {
return tc
}

func (tc *typeChecker) WithAllowUndefinedFunctionCalls(allow bool) *typeChecker {
tc.allowUndefinedFuncs = allow
return tc
}

// Env returns a type environment for the specified built-ins with any other
// global types configured on the checker. In practice, this is the default
// environment that other statements will be checked against.
Expand Down Expand Up @@ -347,6 +353,9 @@ func (tc *typeChecker) checkExprBuiltin(env *TypeEnv, expr *Expr) *Error {
tpe := env.Get(name)

if tpe == nil {
if tc.allowUndefinedFuncs {
return nil
}
return NewError(TypeErr, expr.Location, "undefined function %v", name)
}

Expand Down
14 changes: 13 additions & 1 deletion ast/compile.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ type Compiler struct {
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
allowUndefinedFuncCalls bool // don't error on calls to unknown functions.
evalMode CompilerEvalMode
}

Expand Down Expand Up @@ -457,6 +458,11 @@ func (c *Compiler) WithUseTypeCheckAnnotations(enabled bool) *Compiler {
return c
}

func (c *Compiler) WithAllowUndefinedFunctionCalls(allow bool) *Compiler {
c.allowUndefinedFuncCalls = allow
return c
}

// WithEvalMode allows setting the CompilerEvalMode of the compiler
func (c *Compiler) WithEvalMode(e CompilerEvalMode) *Compiler {
c.evalMode = e
Expand Down Expand Up @@ -1513,7 +1519,8 @@ func (c *Compiler) checkTypes() {
WithInputType(c.inputType).
WithBuiltins(c.builtins).
WithRequiredCapabilities(c.Required).
WithVarRewriter(rewriteVarsInRef(c.RewrittenVars))
WithVarRewriter(rewriteVarsInRef(c.RewrittenVars)).
WithAllowUndefinedFunctionCalls(c.allowUndefinedFuncCalls)
var as *AnnotationSet
if c.useTypeCheckAnnotations {
as = c.annotationSet
Expand Down Expand Up @@ -1577,6 +1584,11 @@ func (c *Compiler) compile() {
continue // skip these stages
}
}

if c.allowUndefinedFuncCalls && s.name == "CheckUndefinedFuncs" {
continue
}

c.runStage(s.metricName, s.f)
if c.Failed() {
return
Expand Down
82 changes: 82 additions & 0 deletions cmd/inspect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -565,3 +565,85 @@ Custom:

})
}

func TestCallToUnknownBuiltInFunction(t *testing.T) {
files := [][2]string{
{"/policy.rego", `package test
p {
foo.bar(42)
contains("foo", "o")
}
`},
}

buf := archive.MustWriteTarGz(files)

test.WithTempFS(nil, func(rootDir string) {
bundleFile := filepath.Join(rootDir, "bundle.tar.gz")

bf, err := os.Create(bundleFile)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}

_, err = bf.Write(buf.Bytes())
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}

var out bytes.Buffer
params := newInspectCommandParams()
err = params.outputFormat.Set(evalJSONOutput)
if err != nil {
t.Fatalf("Unexpected error: %s", err)
}

err = doInspect(params, bundleFile, &out)
if err != nil {
t.Fatalf("Unexpected error %v", err)
}

bs := out.Bytes()
output := strings.TrimSpace(string(bs))
// Note: unknown foo.bar() built-in doesn't appear in the output, but also didn't cause an error.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've taken care of the bug that will opa inspect fail with an error.

As a possible improvement, we could also include the unknown function in the capabilities/builtins listing. We wouldn't know the types other than what's in the call, though, so I think we'd need to set the result and args types to any.
The question is if this would be helpful or confusing to the user.

expected := strings.TrimSpace(`{
"manifest": {
"revision": "",
"roots": [
""
]
},
"signatures_config": {},
"namespaces": {
"data.test": [
"/policy.rego"
]
},
"capabilities": {
"builtins": [
{
"name": "contains",
"decl": {
"args": [
{
"type": "string"
},
{
"type": "string"
}
],
"result": {
"type": "boolean"
},
"type": "function"
}
}
]
}
}`)

if output != expected {
t.Fatalf("Unexpected output. Expected:\n\n%s\n\nGot:\n\n%s", expected, output)
}
})
}
3 changes: 2 additions & 1 deletion internal/bundle/inspect/inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ func File(path string, includeAnnotations bool) (*Info, error) {
moduleMap[f.URL] = f.Parsed
}

c := ast.NewCompiler()
c := ast.NewCompiler().
WithAllowUndefinedFunctionCalls(true)
c.Compile(moduleMap)
if c.Failed() {
return bi, c.Errors
Expand Down