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

download: Fix saving OCI bundles on disk #6945

Merged

Conversation

Sergey-Kizimov
Copy link
Contributor

This commit fixes an issue related to zero-sized bundles being saved to disk, which can cause OPA to fail to start if a remote OCI repository is unavailable.

Fixes: #6939

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.

Thanks @Sergey-Kizimov! The fix looks fine. If you could add some unit tests that would be great.

@anderseknert
Copy link
Member

Hi there! And thanks for contributing to OPA! Can you please add a test to verify the fix?

@Sergey-Kizimov
Copy link
Contributor Author

Ok, will add.

@Sergey-Kizimov
Copy link
Contributor Author

I added unit test

t.Fatal("expected bundle reader to be non-nil")
}

if buf.Len() == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably check u1.Size == 0 here. Also for completeness can we do something like this test.

Copy link
Contributor Author

@Sergey-Kizimov Sergey-Kizimov Aug 21, 2024

Choose a reason for hiding this comment

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

I see, looks like another bug - the Size is not being filled in the downloaderResponse.
Let me fix that

Copy link
Contributor Author

@Sergey-Kizimov Sergey-Kizimov Aug 21, 2024

Choose a reason for hiding this comment

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

I've checked code in oci_download and download files, and oci_download has several dependencies with the download package, Update struct as an example, not sure how best to proceed here.
Just leave count in download then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed changes

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

This commit fixes an issue related to zero-sized bundles being saved to disk,
which can cause OPA to fail to start if a remote OCI repository is unavailable.

Fixes: open-policy-agent#6939

Signed-off-by: Sergey-Kizimov <[email protected]>
Signed-off-by: Sergey-Kizimov <[email protected]>
Signed-off-by: Sergey-Kizimov <[email protected]>
@ashutosh-narkar ashutosh-narkar merged commit f7b1552 into open-policy-agent:main Aug 21, 2024
28 checks passed
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.

OPA does not save bundle files to disk
3 participants