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

OCI fallback #159

Merged
merged 3 commits into from
Jul 4, 2024
Merged

OCI fallback #159

merged 3 commits into from
Jul 4, 2024

Conversation

aidy
Copy link
Contributor

@aidy aidy commented Feb 27, 2024

connects #121

This isn't a full implementation of a OCI registry client at this point, and doesn't support auth etc. Only intended as a last resort fallback for now, and as such I've not wired it in to be selectable by host via command line flags.

pkg/client/oci/oci.go Outdated Show resolved Hide resolved
Comment on lines +15 to +17
type Client struct{}

func New() (*Client, error) {
return &Client{}, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we could construct a *remote.Puller here and use it for the remote operations in Tags. This will cache auth across different calls, reducing the potential number of requests made for each operation.

See:
https://github.com/google/go-containerregistry/blob/main/pkg/v1/remote/puller.go

Copy link
Contributor Author

@aidy aidy Apr 12, 2024

Choose a reason for hiding this comment

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

Yeah, I had this in mind for the future in building this out to be a fully fledged client in it's own right, but I didn't want to do it here as a first pass - mostly I try to work in incremental value, rather than hitting too many things at once.

pkg/client/oci/oci.go Outdated Show resolved Hide resolved
pkg/client/oci/oci.go Outdated Show resolved Hide resolved
pkg/client/oci/oci.go Outdated Show resolved Hide resolved
Copy link
Member

@ribbybibby ribbybibby left a comment

Choose a reason for hiding this comment

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

Left some comments.

Two other things worth mentioning:

  1. Auth. By default, ggcr will try to find credentials in the environment, for instance in ~/.docker/config.json. I reckon we should document this, so users are aware that they can make creds available like that.
  2. Tests. It's pretty straightforward and powerful to use the the ggcr registry for testing. You can see an example of that in paranoia.

aidy added 2 commits May 1, 2024 12:20
OCI registries don't provide a way to retrieve metadata in the same call
as listing tags.

This means that you have to do a separate API request for each tag if
you want the metadata, which could be very slow for a large number of
tags.

As such, preferably use the selfhosted client as a fallback, but add a
fallback fallback to the oci handler for registries that are
incompatible with the selfhosted API implementation.
As we might not have retrieved a SHA (dependent on registry type), don't
try to include one if it's empty.
@davidcollom davidcollom added this to the v1 release milestone May 16, 2024
@hawksight hawksight added the enhancement New feature or request label May 23, 2024
@aidy
Copy link
Contributor Author

aidy commented May 23, 2024

I'll probably look to add tests in a separate PR - As there's not currently a comprehensive testing framework, I think bringing tests into this PR might make it difficult to review.

Copy link
Collaborator

@davidcollom davidcollom left a comment

Choose a reason for hiding this comment

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

Now that I've had chance to review.. Looks good to me - Appreciate the approach of falling back and keeping others - I may have some time to complete the auth and caching... Will look to include this in the next minor release.

@davidcollom davidcollom merged commit 13647c8 into jetstack:main Jul 4, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants