-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Remove registry Service use #4111
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
Conversation
Looks like the service is not needed, as we always pass an empty config, so we can use ParseRepositoryInfo instead. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
| repoInfo, err := registry.ParseRepositoryInfo(name) | ||
| if repoInfo != nil { | ||
| repoInfo.Class = "plugin" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to check if this is still needed; ISTR this was some snafu on Docker Hub, and may not even be needed anymore. /cc @tianon (ISTR you were on that discussion, but I don't recall the status 🤔)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct! Docker Hub still allows the auth class, but it works fine either way (ie, it is no longer required like it used to be).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the case, perhaps we can remove all of this altogether 🤔 🎉
I tried to be a bit more careful as I'm not even sure this code-path is tested 😬 (plugins are a bit of a corner-case, so it wouldn't surprise me if there isn't a e2e test for this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave that a quick go in #4114 (still in draft; if that variant would work, I can update this PR 😅)
| func (s pluginRegistryService) ResolveRepository(name reference.Named) (*registry.RepositoryInfo, error) { | ||
| repoInfo, err := s.Service.ResolveRepository(name) | ||
| func resolvePlugnRepository(name reference.Named) (*registry.RepositoryInfo, error) { | ||
| repoInfo, err := registry.ParseRepositoryInfo(name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference; this is what ParseRepositoryInfo does;
cli/vendor/github.com/docker/docker/registry/config.go
Lines 413 to 432 in 1448258
| // newRepositoryInfo validates and breaks down a repository name into a RepositoryInfo | |
| func newRepositoryInfo(config *serviceConfig, name reference.Named) (*RepositoryInfo, error) { | |
| index, err := newIndexInfo(config, reference.Domain(name)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| official := !strings.ContainsRune(reference.FamiliarName(name), '/') | |
| return &RepositoryInfo{ | |
| Name: reference.TrimNamed(name), | |
| Index: index, | |
| Official: official, | |
| }, nil | |
| } | |
| // ParseRepositoryInfo performs the breakdown of a repository name into a RepositoryInfo, but | |
| // lacks registry configuration. | |
| func ParseRepositoryInfo(reposName reference.Named) (*RepositoryInfo, error) { | |
| return newRepositoryInfo(emptyServiceConfig, reposName) | |
| } |
It uses an emptyServiceConfig, which is needed to mark localhost as "insecure", and docker.io as "official"
| emptyServiceConfig, _ = newServiceConfig(ServiceOptions{}) |
| // RepositoryInfoResolver returns repository info for the given reference. | ||
| type RepositoryInfoResolver func(name reference.Named) (*registry.RepositoryInfo, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit on the fence wether to define this as a type, or to just inline the signature in the function below (like we do for authResolver).
|
For testing this... if only I could find a plugin with content trust 😂 DOCKER_CONTENT_TRUST=1 docker plugin install --disable vieux/sshfs
Error: remote trust data does not exist for docker.io/vieux/sshfs: notary.docker.io does not have trust data for docker.io/vieux/sshfs
DOCKER_CONTENT_TRUST=1 docker plugin install --disable tiborvass/sample-volume-plugin
Error: remote trust data does not exist for docker.io/tiborvass/sample-volume-plugin: notary.docker.io does not have trust data for docker.io/tiborvass/sample-volume-plugin
DOCKER_CONTENT_TRUST=1 docker plugin install --disable grafana/loki-docker-driver:main
Error: remote trust data does not exist for docker.io/grafana/loki-docker-driver: notary.docker.io does not have trust data for docker.io/grafana/loki-docker-driver
DOCKER_CONTENT_TRUST=1 docker plugin install --disable docker/telemetry
Error: remote trust data does not exist for docker.io/docker/telemetry: notary.docker.io does not have trust data for docker.io/docker/telemetry
DOCKER_CONTENT_TRUST=1 docker plugin install --disable vmware/vsphere-storage-for-docker
Error: remote trust data does not exist for docker.io/vmware/vsphere-storage-for-docker: notary.docker.io does not have trust data for docker.io/vmware/vsphere-storage-for-docker |
|
Closing this in favour of #4114 |
Looks like the service is not needed, as we always pass an empty config, so we can use ParseRepositoryInfo instead.
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)