From 3b5dff27832e5a0124198fa0024c0dedd46bf2a0 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 1 Mar 2025 02:33:48 +0100 Subject: [PATCH 1/4] cli/command: internalize constructing ManifestStore The CLI.ManifestStore method is a shallow wrapper around manifeststore.NewStore and has no dependency on the CLI itself. However, due to its signature resulted in various dependencies becoming a dependency of the "command" package. Consequence of this was that cli-plugins, which need the cli/command package, would also get those dependencies. - This patch inlines the code to produce the store, skipping the wrapper. - Define a local interface for some tests where a dummy store was used. Signed-off-by: Sebastiaan van Stijn --- cli/command/manifest/annotate.go | 23 +++++++++++++++++++++-- cli/command/manifest/create_list.go | 2 +- cli/command/manifest/inspect.go | 4 ++-- cli/command/manifest/push.go | 4 ++-- cli/command/manifest/rm.go | 2 +- cli/command/manifest/util.go | 2 +- 6 files changed, 28 insertions(+), 9 deletions(-) diff --git a/cli/command/manifest/annotate.go b/cli/command/manifest/annotate.go index c12b69d823e9..2a0a49893bd1 100644 --- a/cli/command/manifest/annotate.go +++ b/cli/command/manifest/annotate.go @@ -2,9 +2,11 @@ package manifest import ( "fmt" + "path/filepath" "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" + "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/manifest/store" ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" @@ -21,6 +23,23 @@ type annotateOptions struct { osVersion string } +// manifestStoreProvider is used in tests to provide a dummy store. +type manifestStoreProvider interface { + // ManifestStore returns a store for local manifests + ManifestStore() store.Store +} + +// newManifestStore returns a store for local manifests +func newManifestStore(dockerCLI command.Cli) store.Store { + if msp, ok := dockerCLI.(manifestStoreProvider); ok { + // manifestStoreProvider is used in tests to provide a dummy store. + return msp.ManifestStore() + } + + // TODO: support override default location from config file + return store.NewStore(filepath.Join(config.Dir(), "manifests")) +} + // NewAnnotateCommand creates a new `docker manifest annotate` command func newAnnotateCommand(dockerCli command.Cli) *cobra.Command { var opts annotateOptions @@ -47,7 +66,7 @@ func newAnnotateCommand(dockerCli command.Cli) *cobra.Command { return cmd } -func runManifestAnnotate(dockerCli command.Cli, opts annotateOptions) error { +func runManifestAnnotate(dockerCLI command.Cli, opts annotateOptions) error { targetRef, err := normalizeReference(opts.target) if err != nil { return errors.Wrapf(err, "annotate: error parsing name for manifest list %s", opts.target) @@ -57,7 +76,7 @@ func runManifestAnnotate(dockerCli command.Cli, opts annotateOptions) error { return errors.Wrapf(err, "annotate: error parsing name for manifest %s", opts.image) } - manifestStore := dockerCli.ManifestStore() + manifestStore := newManifestStore(dockerCLI) imageManifest, err := manifestStore.Get(targetRef, imgRef) switch { case store.IsNotFound(err): diff --git a/cli/command/manifest/create_list.go b/cli/command/manifest/create_list.go index 5a399d5758de..58c18124dfce 100644 --- a/cli/command/manifest/create_list.go +++ b/cli/command/manifest/create_list.go @@ -41,7 +41,7 @@ func createManifestList(ctx context.Context, dockerCLI command.Cli, args []strin return errors.Wrapf(err, "error parsing name for manifest list %s", newRef) } - manifestStore := dockerCLI.ManifestStore() + manifestStore := newManifestStore(dockerCLI) _, err = manifestStore.GetList(targetRef) switch { case store.IsNotFound(err): diff --git a/cli/command/manifest/inspect.go b/cli/command/manifest/inspect.go index 23ea2e69c62b..ee68acf3c95e 100644 --- a/cli/command/manifest/inspect.go +++ b/cli/command/manifest/inspect.go @@ -61,7 +61,7 @@ func runInspect(ctx context.Context, dockerCli command.Cli, opts inspectOptions) return err } - imageManifest, err := dockerCli.ManifestStore().Get(listRef, namedRef) + imageManifest, err := newManifestStore(dockerCli).Get(listRef, namedRef) if err != nil { return err } @@ -69,7 +69,7 @@ func runInspect(ctx context.Context, dockerCli command.Cli, opts inspectOptions) } // Try a local manifest list first - localManifestList, err := dockerCli.ManifestStore().GetList(namedRef) + localManifestList, err := newManifestStore(dockerCli).GetList(namedRef) if err == nil { return printManifestList(dockerCli, namedRef, localManifestList, opts) } diff --git a/cli/command/manifest/push.go b/cli/command/manifest/push.go index 9b214eeaa721..97bdab6cbc01 100644 --- a/cli/command/manifest/push.go +++ b/cli/command/manifest/push.go @@ -68,7 +68,7 @@ func runPush(ctx context.Context, dockerCli command.Cli, opts pushOpts) error { return err } - manifests, err := dockerCli.ManifestStore().GetList(targetRef) + manifests, err := newManifestStore(dockerCli).GetList(targetRef) if err != nil { return err } @@ -85,7 +85,7 @@ func runPush(ctx context.Context, dockerCli command.Cli, opts pushOpts) error { return err } if opts.purge { - return dockerCli.ManifestStore().Remove(targetRef) + return newManifestStore(dockerCli).Remove(targetRef) } return nil } diff --git a/cli/command/manifest/rm.go b/cli/command/manifest/rm.go index e59b716cd3cb..dda4e3e489dd 100644 --- a/cli/command/manifest/rm.go +++ b/cli/command/manifest/rm.go @@ -16,7 +16,7 @@ func newRmManifestListCommand(dockerCLI command.Cli) *cobra.Command { Short: "Delete one or more manifest lists from local storage", Args: cli.RequiresMinArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - return runRemove(cmd.Context(), dockerCLI.ManifestStore(), args) + return runRemove(cmd.Context(), newManifestStore(dockerCLI), args) }, } diff --git a/cli/command/manifest/util.go b/cli/command/manifest/util.go index f89116d96c3e..929f646bcc1c 100644 --- a/cli/command/manifest/util.go +++ b/cli/command/manifest/util.go @@ -70,7 +70,7 @@ func normalizeReference(ref string) (reference.Named, error) { // getManifest from the local store, and fallback to the remote registry if it // doesn't exist locally func getManifest(ctx context.Context, dockerCli command.Cli, listRef, namedRef reference.Named, insecure bool) (types.ImageManifest, error) { - data, err := dockerCli.ManifestStore().Get(listRef, namedRef) + data, err := newManifestStore(dockerCli).Get(listRef, namedRef) switch { case store.IsNotFound(err): return dockerCli.RegistryClient(insecure).GetManifest(ctx, namedRef) From 985b58e7e1a0428f9d945c0447aca8e6d67c9daf Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 1 Mar 2025 02:45:42 +0100 Subject: [PATCH 2/4] cli/command: internalize constructing RegistryClient The CLI.RegistryClient method is a shallow wrapper around registryclient.NewRegistryClient but due to its signature resulted in various dependencies becoming a dependency of the "command" package. Consequence of this was that cli-plugins, which need the cli/command package, would also get those dependencies. This patch inlines the code where needed, skipping the wrapper Signed-off-by: Sebastiaan van Stijn --- cli/command/manifest/annotate.go | 17 +++++++++++++++++ cli/command/manifest/inspect.go | 2 +- cli/command/manifest/push.go | 2 +- cli/command/manifest/util.go | 8 ++++---- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/cli/command/manifest/annotate.go b/cli/command/manifest/annotate.go index 2a0a49893bd1..0f2a10aade1e 100644 --- a/cli/command/manifest/annotate.go +++ b/cli/command/manifest/annotate.go @@ -1,6 +1,7 @@ package manifest import ( + "context" "fmt" "path/filepath" @@ -8,6 +9,8 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/manifest/store" + registryclient "github.com/docker/cli/cli/registry/client" + "github.com/docker/docker/api/types/registry" ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" "github.com/spf13/cobra" @@ -27,6 +30,7 @@ type annotateOptions struct { type manifestStoreProvider interface { // ManifestStore returns a store for local manifests ManifestStore() store.Store + RegistryClient(bool) registryclient.RegistryClient } // newManifestStore returns a store for local manifests @@ -40,6 +44,19 @@ func newManifestStore(dockerCLI command.Cli) store.Store { return store.NewStore(filepath.Join(config.Dir(), "manifests")) } +// newRegistryClient returns a client for communicating with a Docker distribution +// registry +func newRegistryClient(dockerCLI command.Cli, allowInsecure bool) registryclient.RegistryClient { + if msp, ok := dockerCLI.(manifestStoreProvider); ok { + // manifestStoreProvider is used in tests to provide a dummy store. + return msp.RegistryClient(allowInsecure) + } + resolver := func(ctx context.Context, index *registry.IndexInfo) registry.AuthConfig { + return command.ResolveAuthConfig(dockerCLI.ConfigFile(), index) + } + return registryclient.NewRegistryClient(resolver, command.UserAgent(), allowInsecure) +} + // NewAnnotateCommand creates a new `docker manifest annotate` command func newAnnotateCommand(dockerCli command.Cli) *cobra.Command { var opts annotateOptions diff --git a/cli/command/manifest/inspect.go b/cli/command/manifest/inspect.go index ee68acf3c95e..217383a07f61 100644 --- a/cli/command/manifest/inspect.go +++ b/cli/command/manifest/inspect.go @@ -75,7 +75,7 @@ func runInspect(ctx context.Context, dockerCli command.Cli, opts inspectOptions) } // Next try a remote manifest - registryClient := dockerCli.RegistryClient(opts.insecure) + registryClient := newRegistryClient(dockerCli, opts.insecure) imageManifest, err := registryClient.GetManifest(ctx, namedRef) if err == nil { return printManifest(dockerCli, imageManifest, opts) diff --git a/cli/command/manifest/push.go b/cli/command/manifest/push.go index 97bdab6cbc01..ccfa84cd6c5b 100644 --- a/cli/command/manifest/push.go +++ b/cli/command/manifest/push.go @@ -248,7 +248,7 @@ func buildPutManifestRequest(imageManifest types.ImageManifest, targetRef refere } func pushList(ctx context.Context, dockerCLI command.Cli, req pushRequest) error { - rclient := dockerCLI.RegistryClient(req.insecure) + rclient := newRegistryClient(dockerCLI, req.insecure) if err := mountBlobs(ctx, rclient, req.targetRef, req.manifestBlobs); err != nil { return err diff --git a/cli/command/manifest/util.go b/cli/command/manifest/util.go index 929f646bcc1c..be9fe9ea1ba4 100644 --- a/cli/command/manifest/util.go +++ b/cli/command/manifest/util.go @@ -69,15 +69,15 @@ func normalizeReference(ref string) (reference.Named, error) { // getManifest from the local store, and fallback to the remote registry if it // doesn't exist locally -func getManifest(ctx context.Context, dockerCli command.Cli, listRef, namedRef reference.Named, insecure bool) (types.ImageManifest, error) { - data, err := newManifestStore(dockerCli).Get(listRef, namedRef) +func getManifest(ctx context.Context, dockerCLI command.Cli, listRef, namedRef reference.Named, insecure bool) (types.ImageManifest, error) { + data, err := newManifestStore(dockerCLI).Get(listRef, namedRef) switch { case store.IsNotFound(err): - return dockerCli.RegistryClient(insecure).GetManifest(ctx, namedRef) + return newRegistryClient(dockerCLI, insecure).GetManifest(ctx, namedRef) case err != nil: return types.ImageManifest{}, err case len(data.Raw) == 0: - return dockerCli.RegistryClient(insecure).GetManifest(ctx, namedRef) + return newRegistryClient(dockerCLI, insecure).GetManifest(ctx, namedRef) default: return data, nil } From e32d5d56f5fea7cdfb91a317d517cd163c4954d7 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 4 Mar 2025 14:49:18 +0100 Subject: [PATCH 3/4] cli/command: deprecate Cli.ManifestStore This method is a shallow wrapper around manifeststore.NewStore, but due to its signature resulted in various dependencies becoming a dependency of the "command" package. Consequence of this was that cli-plugins, which need the cli/command package, would also get those dependencies. It is no longer used in our code, which constructs the client in packages that need it, so we can deprecate this method. Signed-off-by: Sebastiaan van Stijn --- cli/command/cli.go | 10 +--------- cli/command/cli_deprecated.go | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/cli/command/cli.go b/cli/command/cli.go index dfb4fba69a4e..7d1560215691 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -8,7 +8,6 @@ import ( "fmt" "io" "os" - "path/filepath" "runtime" "strconv" "strings" @@ -22,7 +21,6 @@ import ( "github.com/docker/cli/cli/context/store" "github.com/docker/cli/cli/debug" cliflags "github.com/docker/cli/cli/flags" - manifeststore "github.com/docker/cli/cli/manifest/store" registryclient "github.com/docker/cli/cli/registry/client" "github.com/docker/cli/cli/streams" "github.com/docker/cli/cli/version" @@ -56,7 +54,6 @@ type Cli interface { ServerInfo() ServerInfo DefaultVersion() string CurrentVersion() string - ManifestStore() manifeststore.Store RegistryClient(bool) registryclient.RegistryClient ContentTrustEnabled() bool BuildKitEnabled() (bool, error) @@ -65,6 +62,7 @@ type Cli interface { DockerEndpoint() docker.Endpoint TelemetryClient DeprecatedNotaryClient + DeprecatedManifestClient } // DockerCli is an instance the docker command line client. @@ -229,12 +227,6 @@ func (cli *DockerCli) HooksEnabled() bool { return false } -// ManifestStore returns a store for local manifests -func (*DockerCli) ManifestStore() manifeststore.Store { - // TODO: support override default location from config file - return manifeststore.NewStore(filepath.Join(config.Dir(), "manifests")) -} - // RegistryClient returns a client for communicating with a Docker distribution // registry func (cli *DockerCli) RegistryClient(allowInsecure bool) registryclient.RegistryClient { diff --git a/cli/command/cli_deprecated.go b/cli/command/cli_deprecated.go index d179f42f229d..d6e38a607a31 100644 --- a/cli/command/cli_deprecated.go +++ b/cli/command/cli_deprecated.go @@ -1,6 +1,10 @@ package command import ( + "path/filepath" + + "github.com/docker/cli/cli/config" + manifeststore "github.com/docker/cli/cli/manifest/store" "github.com/docker/cli/cli/trust" notaryclient "github.com/theupdateframework/notary/client" ) @@ -12,7 +16,21 @@ type DeprecatedNotaryClient interface { NotaryClient(imgRefAndAuth trust.ImageRefAndAuth, actions []string) (notaryclient.Repository, error) } +type DeprecatedManifestClient interface { + // ManifestStore returns a store for local manifests + // + // Deprecated: use [manifeststore.NewStore] instead. This method is no longer used and will be removed in the next release. + ManifestStore() manifeststore.Store +} + // NotaryClient provides a Notary Repository to interact with signed metadata for an image func (cli *DockerCli) NotaryClient(imgRefAndAuth trust.ImageRefAndAuth, actions []string) (notaryclient.Repository, error) { return trust.GetNotaryRepository(cli.In(), cli.Out(), UserAgent(), imgRefAndAuth.RepoInfo(), imgRefAndAuth.AuthConfig(), actions...) } + +// ManifestStore returns a store for local manifests +// +// Deprecated: use [manifeststore.NewStore] instead. This method is no longer used and will be removed in the next release. +func (*DockerCli) ManifestStore() manifeststore.Store { + return manifeststore.NewStore(filepath.Join(config.Dir(), "manifests")) +} From 8ad07217dc92fa81806d4eb64a4eb04cf5c99b15 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 4 Mar 2025 14:54:51 +0100 Subject: [PATCH 4/4] cli/command: deprecate Cli.RegistryClient This method was a shallow wrapper around registryclient.NewRegistryClient but due to its signature resulted in various dependencies becoming a dependency of the "command" package. Consequence of this was that cli-plugins, which need the cli/command package, would also get those dependencies. It is no longer used in our code, which constructs the client in packages that need it, so we can deprecate this method. Signed-off-by: Sebastiaan van Stijn --- cli/command/cli.go | 12 ------------ cli/command/cli_deprecated.go | 20 ++++++++++++++++++++ 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/cli/command/cli.go b/cli/command/cli.go index 7d1560215691..e056334f46e0 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -21,13 +21,11 @@ import ( "github.com/docker/cli/cli/context/store" "github.com/docker/cli/cli/debug" cliflags "github.com/docker/cli/cli/flags" - registryclient "github.com/docker/cli/cli/registry/client" "github.com/docker/cli/cli/streams" "github.com/docker/cli/cli/version" dopts "github.com/docker/cli/opts" "github.com/docker/docker/api" "github.com/docker/docker/api/types" - "github.com/docker/docker/api/types/registry" "github.com/docker/docker/api/types/swarm" "github.com/docker/docker/client" "github.com/docker/go-connections/tlsconfig" @@ -54,7 +52,6 @@ type Cli interface { ServerInfo() ServerInfo DefaultVersion() string CurrentVersion() string - RegistryClient(bool) registryclient.RegistryClient ContentTrustEnabled() bool BuildKitEnabled() (bool, error) ContextStore() store.Store @@ -227,15 +224,6 @@ func (cli *DockerCli) HooksEnabled() bool { return false } -// RegistryClient returns a client for communicating with a Docker distribution -// registry -func (cli *DockerCli) RegistryClient(allowInsecure bool) registryclient.RegistryClient { - resolver := func(ctx context.Context, index *registry.IndexInfo) registry.AuthConfig { - return ResolveAuthConfig(cli.ConfigFile(), index) - } - return registryclient.NewRegistryClient(resolver, UserAgent(), allowInsecure) -} - // WithInitializeClient is passed to DockerCli.Initialize by callers who wish to set a particular API Client for use by the CLI. func WithInitializeClient(makeClient func(dockerCli *DockerCli) (client.APIClient, error)) CLIOption { return func(dockerCli *DockerCli) error { diff --git a/cli/command/cli_deprecated.go b/cli/command/cli_deprecated.go index d6e38a607a31..15fac1a6e327 100644 --- a/cli/command/cli_deprecated.go +++ b/cli/command/cli_deprecated.go @@ -1,11 +1,14 @@ package command import ( + "context" "path/filepath" "github.com/docker/cli/cli/config" manifeststore "github.com/docker/cli/cli/manifest/store" + registryclient "github.com/docker/cli/cli/registry/client" "github.com/docker/cli/cli/trust" + "github.com/docker/docker/api/types/registry" notaryclient "github.com/theupdateframework/notary/client" ) @@ -21,6 +24,12 @@ type DeprecatedManifestClient interface { // // Deprecated: use [manifeststore.NewStore] instead. This method is no longer used and will be removed in the next release. ManifestStore() manifeststore.Store + + // RegistryClient returns a client for communicating with a Docker distribution + // registry. + // + // Deprecated: use [registryclient.NewRegistryClient]. This method is no longer used and will be removed in the next release. + RegistryClient(bool) registryclient.RegistryClient } // NotaryClient provides a Notary Repository to interact with signed metadata for an image @@ -34,3 +43,14 @@ func (cli *DockerCli) NotaryClient(imgRefAndAuth trust.ImageRefAndAuth, actions func (*DockerCli) ManifestStore() manifeststore.Store { return manifeststore.NewStore(filepath.Join(config.Dir(), "manifests")) } + +// RegistryClient returns a client for communicating with a Docker distribution +// registry +// +// Deprecated: use [registryclient.NewRegistryClient]. This method is no longer used and will be removed in the next release. +func (cli *DockerCli) RegistryClient(allowInsecure bool) registryclient.RegistryClient { + resolver := func(ctx context.Context, index *registry.IndexInfo) registry.AuthConfig { + return ResolveAuthConfig(cli.ConfigFile(), index) + } + return registryclient.NewRegistryClient(resolver, UserAgent(), allowInsecure) +}