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

loader: add support for fs.FS #5069

Merged
merged 1 commit into from Sep 6, 2022
Merged

Conversation

ear7h
Copy link
Contributor

@ear7h ear7h commented Aug 30, 2022

There's no correct way to provide the behavior of native os functions
with an fs.FS since fs.FSs should reject rooted paths and only use
unix path separators ("/"). Initially I created a rawFS that directly
forwarded calls to the os package but it felt more wrong the more I
looked at it.

Relevant issues:

closes #5066

@ashutosh-narkar
Copy link
Member

@ear7h thanks for the contribution. It would be great if you could add some unit tests.

@ear7h
Copy link
Contributor Author

ear7h commented Aug 31, 2022

@ashutosh-narkar updated with tests and fixed the lint

@philipaconrad
Copy link
Contributor

philipaconrad commented Sep 1, 2022

Thanks @ear7h for the contributions! Please check the DCO requirements here.

@ear7h
Copy link
Contributor Author

ear7h commented Sep 2, 2022

Fixed DCO. Now CI is complaining about the JSON files I included. Running the failed command works fine locally (I copy/pasted in the json object in a local file). Any hints on this would be appreciated.

@ashutosh-narkar
Copy link
Member

Fixed DCO. Now CI is complaining about the JSON files I included. Running the failed command works fine locally (I copy/pasted in the json object in a local file). Any hints on this would be appreciated.

It looks like the files you added failed this check. The json.is_valid builtin thinks this is invalid json.

@ear7h
Copy link
Contributor Author

ear7h commented Sep 2, 2022

The json.is_valid builtin thinks this is invalid json

Yea I get that, but the files are valid JSON

@anderseknert
Copy link
Member

That's interesting / weird. If I run the same tests manually, there are no complaints:

curl --silent https://api.github.com/repos/open-policy-agent/opa/pulls/5069/files | opa eval -I -b build/policy --format values --fail-defined 'data.files.deny[message]

Perhaps there's some issue with the GITHUB_TOKEN being passed as an env var, and picked up by the http.send function inside of the tests for fetching file contents like the ones above 🤔 I'll see if I can get some error handling introduced in that policy.. it should probably have been there before, but hasn't been needed until now.

There's no correct way to provide the behavior of native `os` functions
with an `fs.FS` since `fs.FS`s should reject rooted paths and only use
unix path separators ("/"). Initially I created a `rawFS` that directly
forwarded calls to the `os` package but it felt more wrong the more I
looked at it.

Relevant issues:
* golang/go#47803
* golang/go#44279

closes open-policy-agent#5066

Signed-off-by: julio <julio.grillo98@gmail.com>
@anderseknert
Copy link
Member

I'm gonna have to throw in the towel here 😫

Spent hours trying to understand what's going on here, but no matter what I (and many good ideas from @srenatus too!) have tried, the error here boils down to this: no matter what URL we provide to http.send in this context (i.e. the github actions runner), we always get the same response back — which would be the first response retrieved. In this case that happens to be the yaml file, which is of course not valid as JSON, and as such fails the json.is_valid check.

For posterity, some things tried:

  • Printing every value imaginable, using --explain=full, etc - provided insights into what's wrong but not more.
  • Turn off caching in http.send - no effect.
  • Try to repeat the process but using curl instead - always works.
  • Try to repeat the process but using curl instead, with exact same headers as http.send - always works.
  • Try the exact same policy using the exact same input on any other machine - always works.

So, for whatever reason, the combination of http.send querying the raw_url pointers provided for the files in the PR using the GitHub API — from, and only from, the inside of the GitHub Actions runner, always returns the same response. No answers, but a few questions.

  • If this was a caching issue in http.send, or even rule evaluation, in OPA, why would it happen only in this context?
  • If this was an issue with HTTP requests in Github Actions, why does it work when repeating the process using curl?

Maybe one day, I'll get to know... but that day is not today. I'm sorry for the inconvenience here @ear7h .. we'll need to ignore the errors here for the scope of this PR. Not sure what to do for the future, but we can figure it out later. @srenatus if everything else checks out here, let's have this merged.

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Nice first contribution 👏 Thanks!

@anderseknert anderseknert merged commit ef55642 into open-policy-agent:main Sep 6, 2022
@anderseknert
Copy link
Member

Also for posterity — the problems described above turned out to be a real bug in OPA. Fixed in #5097

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

Successfully merging this pull request may close these issues.

Support fs.FS in FileLoader
5 participants