open-policy-agent / opa

Open Policy Agent (OPA) is an open source, general-purpose policy engine.
https://www.openpolicyagent.org
Apache License 2.0
9.64k stars 1.34k forks source link

`inspect` command does not always detect undefined functions #6946

Open nikpivkin opened 2 months ago

nikpivkin commented 2 months ago

Short description

The inspect command does not detect undefined functions directly called in a rule, but it does detect them in a list comprehension.

Opa: 0.67.1

Steps To Reproduce

File:

package lib.test

import rego.v1

some_rule if {
    res := [r |
        some r in input
        func()
    ]
}

some_rule if {
    func()
}
$ tar -C bundle -czvf bundle.tar.gz .

$ opa inspect bundle.tar.gz
error: 1 error occurred: ./test.rego:8: rego_type_error: undefined function func

Expected behavior

All cases of using the undefined function will be detected

anderseknert commented 2 months ago

Yeah, I saw the same running opa inspect on Regal's bundle directory the other day, but reported it only in person to my colleague 😅 Thanks for creating an issue for it! I managed to track the issue down to the copy method on the type checker, which does not copy the value for allowUndefinedFuncs. Fixing that and the undefined function error is gone.

I'm however still seeing other errors that are likely related, but harder to understand, like:

keywords_test.rego:102: rego_type_error: data.regal.ast.with_rego_v1: too many arguments
    have: (string, ???)
    want: (string)

and:

search.rego:163: rego_type_error: match error
    left  : ???
    right : ???
anderseknert commented 2 months ago

So these additional failing checks fail due to Rego-defined functions returning the return value of "undefined" built-in functions, and those have no known return type...

TBH, I think a better solution would be to have a proper --capabilities flag for opa inspect, where the compiler can be provided exactly what additional built-in functions expect and what they return. I have a dev branch where I've added that to opa inspect and it's working just as intended. I can have that submitted, of course... but the question is then if the "ignore undefined functions" function should remain 🤔

ashutosh-narkar commented 2 months ago

but the question is then if the "ignore undefined functions" function should remain 🤔

Not sure I understand how this relates to adding caps. The former seems like a bug and later would be good to have. Maybe I'm missing something.

anderseknert commented 2 months ago

Essentially, there could be two types of "missing" functions:

Both of these should arguably be covered by "allowing undefined functions".

The errors returned when running opa inspect on the bundle directory in the Regal project showed how at least custom Go functions was not fully accounted for. Adding a --capabilities flag to opa inspect would "solve" the problem by allowing the user to make these functions known to the compiler. I have already written the code for this, and can submit a PR if adding --capabilities to opa inspect sounds like a good addition to others.

My question was whether one should expect opa inspect to deal gracefully with uknown custom functions when custom --capabilities are not provided.

ashutosh-narkar commented 2 months ago

The command is configured to always allow undefined functions. So if --capabilities is not provided, I would not expect an error if an undefined function is encountered.

anderseknert commented 2 months ago

Agreed. This change fixes the "undefined function" error reported here. But there are still cases where that's not going to be enough for the type checker to be happy, as the return value type, or the arguments, of an unknown function are still uknknown. This can cause new and different issues. Example:

p.rego

package p

import rego.v1

f(x) := custom_function(x)

bug if x := f("foo")
opa inspect p.rego
error: 1 error occurred: p.rego:7: rego_type_error: data.p.f: too many arguments
    have: (string, ???)
    want: (any)
stale[bot] commented 1 month ago

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.