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

Add OCI documentation #4781

Merged
merged 3 commits into from
Aug 12, 2022
Merged

Add OCI documentation #4781

merged 3 commits into from
Aug 12, 2022

Conversation

carabasdaniel
Copy link
Contributor

Closes: #4638

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.

Thanks for picking this up! I've left a bunch of comments inline 🙃 👇

docs/content/management-bundles.md Outdated Show resolved Hide resolved
docs/content/management-bundles.md Outdated Show resolved Hide resolved
docs/content/management-bundles.md Outdated Show resolved Hide resolved
docs/content/management-bundles.md Outdated Show resolved Hide resolved
docs/content/management-bundles.md Outdated Show resolved Hide resolved
docs/content/management-bundles.md Outdated Show resolved Hide resolved
@carabasdaniel carabasdaniel requested a review from srenatus June 16, 2022 13:57
@carabasdaniel carabasdaniel marked this pull request as ready for review June 17, 2022 07:25
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.

Thanks for bearing with me! I've added a few more comments.

- the bundle tarball layer - the actual bundle tarball
- the configuration layer - currently empty

For OCI compatible registries an ***oci*** folder is created in the [persistence directory](../configuration/#miscellaneous). If this value is not set, because the OCI downloader plugin requires a storage path, the system's temporary folder location will be used instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the user care about this folder? Should it be backed-up or cleaned out regularly? 💭

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment that this folder should be maintained by the user and it should be backed-up/cleaned periodically as this also acts as a cache for the OCI downloader

docs/content/management-bundles.md Outdated Show resolved Hide resolved
docs/content/management-bundles.md Outdated Show resolved Hide resolved
@srenatus
Copy link
Contributor

This is also a good moment to drop docs/devel/OCI.md.

@carabasdaniel
Copy link
Contributor Author

@srenatus I talked with the team about the policy CLI. This is an OSS tool similar to oras within it's own Github organization so we don't think this is really a vendor-specific tool as we wanted it to be independent.

@carabasdaniel
Copy link
Contributor Author

This is also a good moment to drop docs/devel/OCI.md.

Updated devel doc, I left only debugging information there in case anyone wants to do a step-by-step debugging session on the OCI downloader

@srenatus
Copy link
Contributor

Thanks! I think what's left is pretty standard procedure -- let's remove the entire file, please 😃

@carabasdaniel carabasdaniel requested a review from srenatus June 20, 2022 13:14
@srenatus
Copy link
Contributor

Hah, the failing test is #4748 🙃

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.

Some questions/notes/requests. Thanks for bearing with me

docs/content/management-bundles.md Outdated Show resolved Hide resolved
docs/content/management-bundles.md Outdated Show resolved Hide resolved
docs/content/management-bundles.md Outdated Show resolved Hide resolved
- the bundle tarball layer - the actual bundle tarball
- the configuration layer - currently empty

For OCI compatible registries an ***oci*** folder is created in the [persistence directory](../configuration/#miscellaneous). If this value is not set, because the OCI downloader plugin requires a storage path, the system's temporary folder location will be used instead. This folder should be maintained by the user. We recommend backing-up or cleaning up this folder periodically as this acts as a local cache for the OCI downloader.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I have bundles pulled from multiple registries? I suppose so. But one, shared, oci folder is still OK?

Copy link
Contributor Author

@carabasdaniel carabasdaniel Jun 22, 2022

Choose a reason for hiding this comment

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

Yes, it should still be ok. You can configure multiple services with oci typing and add in the bundles in you configuration.

@srenatus
Copy link
Contributor

@srenatus I talked with the team about the policy CLI. This is an OSS tool similar to oras within it's own Github organization so we don't think this is really a vendor-specific tool as we wanted it to be independent.

The policy CLI doesn't compare to ORAS, I think. ORAS is not specific in any way: it's a generic OCI tool, like curl is for HTTP, as far as I am concerned. On the one hand, that link goes to the OPCR docs, not the github repo for policy. The github repo also states that it's "for OPCR" (which it talks to by default) which is not an OSS tool.

Both OPCR and the policy tool (and Aserto, for that matter) are on the ecosystem page already. I propose we keep it at that. Perhaps it would make sense to add the word "OCI" somewhere there so it's more obvious what it's about?

test/e2e/oci/oci_test.go Outdated Show resolved Hide resolved
@ogazitt
Copy link
Contributor

ogazitt commented Jun 28, 2022

@srenatus I talked with the team about the policy CLI. This is an OSS tool similar to oras within it's own Github organization so we don't think this is really a vendor-specific tool as we wanted it to be independent.

The policy CLI doesn't compare to ORAS, I think. ORAS is not specific in any way: it's a generic OCI tool, like curl is for HTTP, as far as I am concerned. On the one hand, that link goes to the OPCR docs, not the github repo for policy. The github repo also states that it's "for OPCR" (which it talks to by default) which is not an OSS tool.

Both OPCR and the policy tool (and Aserto, for that matter) are on the ecosystem page already. I propose we keep it at that. Perhaps it would make sense to add the word "OCI" somewhere there so it's more obvious what it's about?

Hi @srenatus, OPCR is meant to be a reference implementation of a "policy registry" - it is single-user, free to use, and has no vendor angle (or upsell). And the policy CLI is built to work with any OCI-compatible registry, and AFAIK the github repo doesn't contain any reference to OPCR in particular.

Stepping back, the entire point of the policy CLI is to make it easy to build OPA policies into OCI images, which hopefully we agree is a good thing. It was built as an OSS tool, in service to the broader OPA community, to support any use-case involving OPA (gatekeeper, conftest, etc). It's in a vendor-neutral GH organization (opcr-io). We would gladly house it under the OPA project if that would remove the concern.

@srenatus
Copy link
Contributor

srenatus commented Jul 4, 2022

👋 hello @ogazitt welcome to this discussion.

OPCR is meant to be a reference implementation of a "policy registry" - it is single-user, free to use, and has no vendor angle (or upsell). And the policy CLI is built to work with any OCI-compatible registry, and AFAIK the github repo doesn't contain any reference to OPCR in particular.

OPCR is the default server that policy uses, and the github repo says "Policy CLI for the Open Policy Registry (opcr.io)".

But really, I believe OPCR doesn't need this extra mention. Besides being on the ecosystem page already, we have no evidence that our users care for it, and I don't think we should push them into using the "reference implementation". Frankly, since OPCR isn't open source, I'm not sure I see the "reference" angle here -- we don't learn anything from using OPCR that we wouldn't learn from using AWS' or GitHub's OCI registries.

Also, the docs here show that there's not much that it does that a shell script using oras or any other standard OCI tool wouldn't be able to accomplish. If there was much effort, we'd need to provide that in opa push, or some similar kind of out-of-the-box experience 🤔

@ogazitt
Copy link
Contributor

ogazitt commented Jul 5, 2022

👋 hello @ogazitt welcome to this discussion.

OPCR is meant to be a reference implementation of a "policy registry" - it is single-user, free to use, and has no vendor angle (or upsell). And the policy CLI is built to work with any OCI-compatible registry, and AFAIK the github repo doesn't contain any reference to OPCR in particular.

OPCR is the default server that policy uses, and the github repo says "Policy CLI for the Open Policy Registry (opcr.io)".

But really, I believe OPCR doesn't need this extra mention. Besides being on the ecosystem page already, we have no evidence that our users care for it, and I don't think we should push them into using the "reference implementation". Frankly, since OPCR isn't open source, I'm not sure I see the "reference" angle here -- we don't learn anything from using OPCR that we wouldn't learn from using AWS' or GitHub's OCI registries.

Also, the docs here show that there's not much that it does that a shell script using oras or any other standard OCI tool wouldn't be able to accomplish. If there was much effort, we'd need to provide that in opa push, or some similar kind of out-of-the-box experience 🤔

Ah, I see the description of the policy repo does contain a mention of Open Policy Registry, and I can see why that would be a concern, because policy was designed to work with any oras-compatible container registry. I assuming revising the description to "CLI for building OPA policies into OCI images" would alleviate this concern?

@philipaconrad
Copy link
Contributor

After taking a look at the docs text, I can see what @srenatus is getting at. policy isn't a particularly necessary part of that documentation addition. I think if we axe the policy-specific section, that should be sufficient for getting this PR finally shipped, because the rest looks pretty good.

Related: I think that if we find users often want to be building OCI images for OPA, that ought to become part of OPA itself, maybe as part of what a sub-command on the CLI (like opa push?) handles. 🤔

Signed-off-by: carabasdaniel <dani@aserto.com>
Signed-off-by: carabasdaniel <dani@aserto.com>
@carabasdaniel carabasdaniel requested a review from srenatus August 10, 2022 07:32
@carabasdaniel
Copy link
Contributor Author

Hi @philipaconrad @srenatus,
I've updated the PR but the failing test does not seem related to these changes, can you please let me know if that is the case ?

@philipaconrad
Copy link
Contributor

@carabasdaniel You're correct. The test failure is unrelated to these changes. Our MacOS tests have been a bit flaky recently. We'll re-run the failing CI tests, and this PR should go green again.

@peteroneilljr peteroneilljr requested review from philipaconrad and removed request for srenatus August 12, 2022 17:02
Copy link
Contributor

@philipaconrad philipaconrad left a comment

Choose a reason for hiding this comment

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

These changes look ready for merge! 👍

@philipaconrad philipaconrad merged commit 295d251 into open-policy-agent:main Aug 12, 2022
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.

OCI: documentation
5 participants