Skip to content

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.

Sign up for GitHub

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

Closed
thaJeztah wants to merge 1 commit into from
Closed

Remove registry Service use #4111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/command/image/trust.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ func imagePullPrivileged(ctx context.Context, cli command.Cli, imgRefAndAuth tru
}

// TrustedReference returns the canonical trusted reference for an image reference
func TrustedReference(ctx context.Context, cli command.Cli, ref reference.NamedTagged, rs registry.Service) (reference.Canonical, error) {
func TrustedReference(ctx context.Context, cli command.Cli, ref reference.NamedTagged, rs trust.RepositoryInfoResolver) (reference.Canonical, error) {
imgRefAndAuth, err := trust.GetImageReferencesAndAuth(ctx, rs, AuthResolver(cli), ref.String())
if err != nil {
return nil, err
Expand Down
23 changes: 3 additions & 20 deletions cli/command/plugin/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,26 +54,14 @@ func newInstallCommand(dockerCli command.Cli) *cobra.Command {
return cmd
}

type pluginRegistryService struct {
registry.Service
}

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)
Copy link
Member Author

@thaJeztah thaJeztah Mar 22, 2023

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"

cli/vendor/github.com/docker/docker/registry/config.go

Line 57 in 1448258

emptyServiceConfig, _ = newServiceConfig(ServiceOptions{})

if repoInfo != nil {
repoInfo.Class = "plugin"
}
Comment on lines +58 to 61
Copy link
Member Author

@thaJeztah thaJeztah Mar 22, 2023

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 🤔)

Copy link
Contributor

@tianon tianon Mar 22, 2023

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).

Copy link
Member Author

@thaJeztah thaJeztah Mar 22, 2023

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).

Copy link
Member Author

@thaJeztah thaJeztah Mar 22, 2023

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 😅)

return repoInfo, err
}

func newRegistryService() (registry.Service, error) {
svc, err := registry.NewService(registry.ServiceOptions{})
if err != nil {
return nil, err
}
return pluginRegistryService{Service: svc}, nil
}

func buildPullConfig(ctx context.Context, dockerCli command.Cli, opts pluginOptions, cmdName string) (types.PluginInstallOptions, error) {
// Names with both tag and digest will be treated by the daemon
// as a pull by digest with a local name for the tag
Expand All @@ -98,12 +86,7 @@ func buildPullConfig(ctx context.Context, dockerCli command.Cli, opts pluginOpti
return types.PluginInstallOptions{}, errors.Errorf("invalid name: %s", ref.String())
}

ctx := context.Background()
svc, err := newRegistryService()
if err != nil {
return types.PluginInstallOptions{}, err
}
trusted, err := image.TrustedReference(ctx, dockerCli, nt, svc)
trusted, err := image.TrustedReference(context.Background(), dockerCli, nt, resolvePlugnRepository)
if err != nil {
return types.PluginInstallOptions{}, err
}
Expand Down
9 changes: 6 additions & 3 deletions cli/trust/trust.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,9 +299,12 @@ type ImageRefAndAuth struct {
digest digest.Digest
}

// RepositoryInfoResolver returns repository info for the given reference.
type RepositoryInfoResolver func(name reference.Named) (*registry.RepositoryInfo, error)
Comment on lines +302 to +303
Copy link
Member Author

@thaJeztah thaJeztah Mar 22, 2023

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).


// GetImageReferencesAndAuth retrieves the necessary reference and auth information for an image name
// as an ImageRefAndAuth struct
func GetImageReferencesAndAuth(ctx context.Context, rs registry.Service,
func GetImageReferencesAndAuth(ctx context.Context, resolveFn RepositoryInfoResolver,
authResolver func(ctx context.Context, index *registrytypes.IndexInfo) types.AuthConfig,
imgName string,
) (ImageRefAndAuth, error) {
Expand All @@ -312,8 +315,8 @@ func GetImageReferencesAndAuth(ctx context.Context, rs registry.Service,

// Resolve the Repository name from fqn to RepositoryInfo
var repoInfo *registry.RepositoryInfo
if rs != nil {
repoInfo, err = rs.ResolveRepository(ref)
if resolveFn != nil {
repoInfo, err = resolveFn(ref)
} else {
repoInfo, err = registry.ParseRepositoryInfo(ref)
}
Expand Down