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

inspect command does not always detect undefined functions #6946

Open
nikpivkin opened this issue Aug 21, 2024 · 8 comments
Open

inspect command does not always detect undefined functions #6946

nikpivkin opened this issue Aug 21, 2024 · 8 comments
Labels

Comments

@nikpivkin
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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 added a commit to anderseknert/opa that referenced this issue Aug 21, 2024
Just a small improvement from looking into open-policy-agent#6946

This doesn't necessarily solve all issues reported there, but I figured
I might as well commit this anyway.

Signed-off-by: Anders Eknert <[email protected]>
anderseknert added a commit to anderseknert/opa that referenced this issue Aug 21, 2024
Just a small improvement from looking into open-policy-agent#6946

This doesn't necessarily solve all issues reported there, but I figured
I might as well commit this anyway.

Signed-off-by: Anders Eknert <[email protected]>
anderseknert added a commit that referenced this issue Aug 21, 2024
Just a small improvement from looking into #6946

This doesn't necessarily solve all issues reported there, but I figured
I might as well commit this anyway.

Signed-off-by: Anders Eknert <[email protected]>
@ashutosh-narkar
Copy link
Member

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
Copy link
Member

anderseknert commented Aug 21, 2024

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.

@ashutosh-narkar
Copy link
Member

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
Copy link
Member

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)

Copy link

stale bot commented Sep 22, 2024

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 stale bot added the inactive label Sep 22, 2024
@andrioni
Copy link

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.

@stale stale bot removed the inactive label Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants