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

compile/compile: Fix panic from CLI + metadata entrypoint overlaps. #6667

Conversation

philipaconrad
Copy link
Contributor

This PR fixes a panic that could occur when opa build was provided an entrypoint from both a CLI flag, and via entrypoint metadata annotation.

The fix is simple: deduplicate the slice of entrypoint refs that the compiler uses, before compiling WASM or Plan targets.

Fixes: #6661

Thanks to @dxh9845 for reporting this one, and for putting together the initial repro test case!

@@ -978,9 +978,7 @@ update {

func modulesToString(modules []bundle.ModuleFile) string {
var buf bytes.Buffer
//result := make([]string, len(modules))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This appears to be an unfinished cleanup from a few months back, so I took the liberty of completing the job. 😅

@philipaconrad
Copy link
Contributor Author

Ah nuts, so deduplicating the entrypoint refs brought about a new issue: the order of the refs can be changed by the particular way I was deduplicating things with a map type, and this causes test failures because the results are ordered randomly. I'll rework things shortly...

@philipaconrad philipaconrad force-pushed the fix-opa-build-entrypoint-panic branch 5 times, most recently from a6ec082 to 6860f61 Compare April 3, 2024 19:57
@philipaconrad
Copy link
Contributor Author

Added @dxh9845 as a co-author on the PR commit, since he did the majority of the heavy-lifting to narrow down the bug site. The actual fix was pretty easy once I had a good repro case to work with.

srenatus
srenatus previously approved these changes Apr 3, 2024
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.

LGTM, thanks

@dxh9845
Copy link
Contributor

dxh9845 commented Apr 3, 2024

Thanks for the fix!

This commit fixes a panic that could occur when `opa build` was provided
an entrypoint from both a CLI flag, and via entrypoint metadata
annotation.

The fix is simple: deduplicate the slice of entrypoint refs that the
compiler uses, before compiling WASM or Plan targets.

Fixes: open-policy-agent#6661

Co-authored-by: Daniel Herzig <danielherzig96@gmail.com>
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
@philipaconrad philipaconrad force-pushed the fix-opa-build-entrypoint-panic branch from 82005bf to c9763bd Compare April 3, 2024 22:46
@philipaconrad
Copy link
Contributor Author

There were some more silly test failures, which led me to do some refactoring + a rebase or two was needed to catch up with the state of OPA's main branch. I think we're finally in a good-for-merge state. 😅

@philipaconrad
Copy link
Contributor Author

philipaconrad commented Apr 3, 2024

First time I've seen a GH Actions runner fail this way:
https://github.com/open-policy-agent/opa/actions/runs/8546499900/job/23417029263?pr=6667

I believe it's a spurious failure, and will re-run the failing actions once the other CI checks complete.

@anderseknert
Copy link
Member

@philipaconrad it's been failing that way in a lot of builds recently :( I think someone is about to look into it. Just retry the job in the meantime.

@philipaconrad philipaconrad requested a review from srenatus April 3, 2024 23:07
@philipaconrad
Copy link
Contributor Author

philipaconrad commented Apr 3, 2024

...And we're good! 😄

@ashutosh-narkar
Copy link
Member

We have #6650 to track the CI updates. We need to look into that.

Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

LGTM

@ashutosh-narkar ashutosh-narkar merged commit 88eaaa9 into open-policy-agent:main Apr 3, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building a WASM Bundle with multiple entrypoints specified via metadata annotation will result in a panic
5 participants