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

missing import should create error #491

Closed
jc-m opened this issue Oct 20, 2017 · 11 comments
Closed

missing import should create error #491

jc-m opened this issue Oct 20, 2017 · 11 comments

Comments

@jc-m
Copy link
Contributor

jc-m commented Oct 20, 2017

The import statement is not checking that the imported path actually exist in the loaded/parsed rules.

for example

package foo

import data.foo.bar

x = { true }

w = { data.foo.bar.w }

doesn't fail, and the only issue is w being undefined.

The expectation would be that the import statement would fail unless a package is loaded before the import.

@tsandall
Copy link
Member

Currently import only aliases the document referred to by the path. Since the path can refer to either raw JSON or a document defined by a rule, it's not clear we can fail on undefined (e.g., you can have raw JSON loaded at path a.b.c and a virtual document defined at path a.b.d without issue.)

@jc-m
Copy link
Contributor Author

jc-m commented Oct 23, 2017

I didn't realize that, but it makes sense. However, since data.foo.bar.w returns undefined, why wouldn't data.foo.bar at the time of import ? Is it related to the parsing versus evaluation time ?
Maybe adding an error for the import too would make it more user friendly (the only error today is where the import is used) ?

@autodidaddict
Copy link

I noticed this problem today. While I understand and like the idea that OPA's store can change over time, so we can't always enforce import consistency... I'd like to be able to set strict enforcement if I know ahead of time that the supporting data for the policy should be available (that way I can get a failure that indicates missing data rather than policy evaluation failure).

One approach might be to provide an alternative import! syntax that would require the source of the alias to exist at compilation time.

Another approach might be to give an option when evaluating a rule in the Go library via Eval to indicate that we want strict import checking.

My goal here would be to tell the difference between a policy failure because of true policy failure versus a policy failure because of accidentally missing supporting data.

@timothyhinrichs
Copy link
Member

This came up again today. I could imagine a compiler flag that throws an error when there's a dangling import. It'd restrict usage of import, but I think 99% of policies would work properly with this flag.

@jceresini
Copy link

jceresini commented Jun 19, 2020

We load a config.yaml file into opa along with our policies. Some of those policies require data from the config, so if its missing our policies report bad data. Due to a blunder on our part, we managed to run OPA without our config file, and had to track down an issue.

There are other ways to address the above issue, but one solution that would be nice, is if we could add a dependency to our rego file that would prevent opa from starting if the dependency was undefined. I played with imports and ended up finding this issue. It would be great if we could do something like

package app.policy.foo
import data.config as config

rule_1 {
    ## some check that relies on config.whatever...
}

and have the import fail if its undefined. Or maybe a new way to import like import strict data.config as config that would prevent opa from starting on undefined imports.

@grosser
Copy link

grosser commented Jun 29, 2020

This is also important for https://github.com/open-policy-agent/gatekeeper since it's easy to miss some libraries when deploying new opa rules and then nothing works 🤦

@stale
Copy link

stale bot commented Nov 22, 2021

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Nov 22, 2021
@alvarogomez93
Copy link
Contributor

Is there any plans to address this? Maybe a warning could be issued when an undefined import is detected during the parsing, just as a heads up

@stale stale bot removed the inactive label Dec 1, 2022
@anderseknert
Copy link
Member

I don't think we can or want to change the default behavior here, but maybe something like what's been suggested above would be possible, where a configuration option/flag could be used to force imports to be "resolved". There are no immediate plans to work on this, but contributions are always welcome 🙂

@stale
Copy link

stale bot commented Dec 31, 2022

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@anderseknert
Copy link
Member

There is now a rule in Regal to do this exactly 😃 That rule can also be configured to allow exceptions for paths that are meant to be dynamic.

Given that a solution exists and one that is unlikely to be included in OPA core, I'll close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants