OCI fallback #159
OCI fallback #159
Conversation
pkg/client/oci/oci.go
Outdated
| if err != nil { | ||
| continue | ||
| } | ||
| image, err := remote.Image(img, remote.WithContext(ctx)) |
There was a problem hiding this comment.
I believe Image will resolve manifest lists (multi-arch images) to a platform-specific manifest. The default is linux/amd64. Is this the desired behaviour?
Looking at the selfhosted client, I think it will return the tag.SHA and tag.Tag for the manifest list, it won't resolve down to a particular platform.
Based on that, I think this should be a remote.Get and then a check on desc.MediaType to figure out whether it's an image or an index. If it's an image then we can call remote.Image and extract the arch, OS and created time.
You can see here in the cosign codebase how they perform a similar check: https://github.com/sigstore/cosign/blob/main/pkg/oci/remote/remote.go#L71-L95.
There was a problem hiding this comment.
I think this isn't relevant now that we're not looking to retrieve metadata in this first pass.
| type Client struct{} | ||
|
|
||
| func New() (*Client, error) { | ||
| return &Client{}, nil | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| } | ||
|
|
||
| bareTags, err := remote.List(rpo, remote.WithContext(ctx)) | ||
|
|
There was a problem hiding this comment.
nit: rm line
pkg/client/oci/oci.go
Outdated
| func (c *Client) Tags(ctx context.Context, host, repo, image string) ([]api.ImageTag, error) { | ||
| src := fmt.Sprintf("%s/%s/%s", host, repo, image) | ||
| rpo, err := name.NewRepository(src) | ||
|
|
There was a problem hiding this comment.
nit: rm line
pkg/client/oci/oci.go
Outdated
| // Only fetch metadata for the highest version | ||
| // Could be very slow otherwise if there are a lot of tags, and | ||
| // we only really care about the highest version. |
There was a problem hiding this comment.
Forgive my ignorance of version-checker here, but is this definitely true? Isn't it possible that there will be some custom sorting of the []api.ImageTag this function returns that will require full details? For instance, finding the latest image by Timestamp?
The cost of fetching the config for every tag may just be the cost of doing business here?
I suppose if we want to make it more efficient, we could cache details by digest? That way we'd only ever have to fetch the config for each digest once?
There was a problem hiding this comment.
Hm, looking at it harder, I missed that there's a use-sha option, where it checks against the latest sha by date. I kinda dismissed that as something that people might want to do, because the latest by date might often be a hotfix against an older version. But I guess there is a use-case for if people are using latest or something - although then I'd expect them to either flush their pods regularly, or have some CI around it.
The cost of fetching the config for every tag may just be the cost of doing business here?
I tested against the fluentbit repo, I think it hadn't finished after an hour. I think if we were talking a few minutes I'd accept the cost, but potentially for repos with large numbers of tags and api rate limits, we could be talking days.
There was a problem hiding this comment.
Hm, in that case, maybe we just don't fetch the config at all?
There was a problem hiding this comment.
The prom metrics output wants to give you the sha of the latest version.
I think the best thing to do here is to fetch the metadata for the latest semver, and for any currently deployed tags. Still a gap for empty tags, but that seems like an edge case.
There was a problem hiding this comment.
Maybe we load the highest semver, and any non-semver tags - that'd be easier to implement, hopefully should be a reasonably small set, and I think would cater for all use cases.
There was a problem hiding this comment.
Had a chat with @ribbybibby and think it's better to skip the metadata for now. There's a few different ways in which we consider a version to be "current", and it's not trivial to pull that logic in here.
There was a problem hiding this comment.
Left some comments.
Two other things worth mentioning:
- 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. - Tests. It's pretty straightforward and powerful to use the the ggcr registry for testing. You can see an example of that in paranoia.
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.
|
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. |
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.