opa icon indicating copy to clipboard operation
opa copied to clipboard

`inspect` command does not always detect undefined functions

Open nikpivkin opened this issue 1 year ago • 9 comments

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

nikpivkin avatar Aug 21 '24 05:08 nikpivkin

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 avatar Aug 21 '24 12:08 anderseknert

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 🤔

anderseknert avatar Aug 21 '24 12:08 anderseknert

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.

ashutosh-narkar avatar Aug 21 '24 19:08 ashutosh-narkar

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

  • Custom functions written in Go
  • Custom functions written in Rego, but not defined in the bundle inspected

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.

anderseknert avatar Aug 21 '24 20:08 anderseknert

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.

ashutosh-narkar avatar Aug 22 '24 17:08 ashutosh-narkar

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)

anderseknert avatar Aug 22 '24 21:08 anderseknert

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.

stale[bot] avatar Sep 22 '24 17:09 stale[bot]

I think I have just run under this issue with the following code:

bug.rego

package bug

import data.lib
import rego.v1

dispatcher(arg) := dispatcher_impl(arg.type, arg)

dispatcher_impl("type1", arg) := lib.f1(arg)

lib.rego

package lib

import rego.v1

f1(arg) := arg.value

It does work fine if I inspect the whole bundle at once.

andrioni avatar Oct 30 '24 13:10 andrioni

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.

stale[bot] avatar Nov 30 '24 18:11 stale[bot]