From 79dd3a3c794ec5eafe7794107c41487197a40991 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 4 Aug 2025 09:14:13 +0200 Subject: [PATCH 1/8] cli-plugins/manager: un-export "Candidate" interface It is for internal use for mocking purposes, and is not part of any public interface / signature. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 54367b32837e30dc3c4fe08584552812e959500d) Signed-off-by: Sebastiaan van Stijn --- cli-plugins/manager/candidate.go | 6 ------ cli-plugins/manager/plugin.go | 8 +++++++- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cli-plugins/manager/candidate.go b/cli-plugins/manager/candidate.go index d809926cf62b..a4253968dcca 100644 --- a/cli-plugins/manager/candidate.go +++ b/cli-plugins/manager/candidate.go @@ -6,12 +6,6 @@ import ( "github.com/docker/cli/cli-plugins/metadata" ) -// Candidate represents a possible plugin candidate, for mocking purposes -type Candidate interface { - Path() string - Metadata() ([]byte, error) -} - type candidate struct { path string } diff --git a/cli-plugins/manager/plugin.go b/cli-plugins/manager/plugin.go index fa846452b548..ed4dac54e6a4 100644 --- a/cli-plugins/manager/plugin.go +++ b/cli-plugins/manager/plugin.go @@ -31,12 +31,18 @@ type Plugin struct { ShadowedPaths []string `json:",omitempty"` } +// pluginCandidate represents a possible plugin candidate, for mocking purposes. +type pluginCandidate interface { + Path() string + Metadata() ([]byte, error) +} + // newPlugin determines if the given candidate is valid and returns a // Plugin. If the candidate fails one of the tests then `Plugin.Err` // is set, and is always a `pluginError`, but the `Plugin` is still // returned with no error. An error is only returned due to a // non-recoverable error. -func newPlugin(c Candidate, cmds []*cobra.Command) (Plugin, error) { +func newPlugin(c pluginCandidate, cmds []*cobra.Command) (Plugin, error) { path := c.Path() if path == "" { return Plugin{}, errors.New("plugin candidate path cannot be empty") From 09efe3f408342133aee8b95335059a8ad4dc55a4 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 4 Aug 2025 10:45:02 +0200 Subject: [PATCH 2/8] cli-plugins/manager: fix Plugin marshaling with regular errors Go does not by default marshal `error` type fields to JSON. The manager package therefore implemented a `pluginError` type that implements [encoding.TextMarshaler]. However, the field was marked as a regular `error`, which made it brittle; assining any other type of error would result in the error being discarded in the marshaled JSON (as used in `docker info` output), resulting in the error being marshaled as `{}`. This patch adds a custom `MarshalJSON()` on the `Plugin` type itself so that any error is rendered. It checks if the error used already implements [encoding.TextMarshaler], otherwise wraps the error in a `pluginError`. [encoding.TextMarshaler]: https://pkg.go.dev/encoding#TextMarshaler Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 549d39a89fe3e6546c55af2a00c9ea0b77305ef3) Signed-off-by: Sebastiaan van Stijn --- cli-plugins/manager/plugin.go | 17 ++++++++++++ cli-plugins/manager/plugin_test.go | 43 ++++++++++++++++++++++++++++++ cli/command/system/info_test.go | 3 ++- 3 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 cli-plugins/manager/plugin_test.go diff --git a/cli-plugins/manager/plugin.go b/cli-plugins/manager/plugin.go index ed4dac54e6a4..e72d9039b895 100644 --- a/cli-plugins/manager/plugin.go +++ b/cli-plugins/manager/plugin.go @@ -2,6 +2,7 @@ package manager import ( "context" + "encoding" "encoding/json" "errors" "fmt" @@ -31,6 +32,22 @@ type Plugin struct { ShadowedPaths []string `json:",omitempty"` } +// MarshalJSON implements [json.Marshaler] to handle marshaling the +// [Plugin.Err] field (Go doesn't marshal errors by default). +func (p *Plugin) MarshalJSON() ([]byte, error) { + type Alias Plugin // avoid recursion + + cp := *p // shallow copy to avoid mutating original + + if cp.Err != nil { + if _, ok := cp.Err.(encoding.TextMarshaler); !ok { + cp.Err = &pluginError{cp.Err} + } + } + + return json.Marshal((*Alias)(&cp)) +} + // pluginCandidate represents a possible plugin candidate, for mocking purposes. type pluginCandidate interface { Path() string diff --git a/cli-plugins/manager/plugin_test.go b/cli-plugins/manager/plugin_test.go new file mode 100644 index 000000000000..84d71d5187d6 --- /dev/null +++ b/cli-plugins/manager/plugin_test.go @@ -0,0 +1,43 @@ +package manager + +import ( + "encoding/json" + "errors" + "testing" + + "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" +) + +func TestPluginMarshal(t *testing.T) { + const jsonWithError = `{"Name":"some-plugin","Err":"something went wrong"}` + const jsonNoError = `{"Name":"some-plugin"}` + + tests := []struct { + doc string + error error + expected string + }{ + { + doc: "no error", + expected: jsonNoError, + }, + { + doc: "regular error", + error: errors.New("something went wrong"), + expected: jsonWithError, + }, + { + doc: "custom error", + error: NewPluginError("something went wrong"), + expected: jsonWithError, + }, + } + for _, tc := range tests { + t.Run(tc.doc, func(t *testing.T) { + actual, err := json.Marshal(&Plugin{Name: "some-plugin", Err: tc.error}) + assert.NilError(t, err) + assert.Check(t, is.Equal(string(actual), tc.expected)) + }) + } +} diff --git a/cli/command/system/info_test.go b/cli/command/system/info_test.go index 726d1ea5a2d4..6b193b7fe921 100644 --- a/cli/command/system/info_test.go +++ b/cli/command/system/info_test.go @@ -2,6 +2,7 @@ package system import ( "encoding/base64" + "errors" "net" "testing" "time" @@ -220,7 +221,7 @@ var samplePluginsInfo = []pluginmanager.Plugin{ { Name: "badplugin", Path: "/path/to/docker-badplugin", - Err: pluginmanager.NewPluginError("something wrong"), + Err: errors.New("something wrong"), }, } From 5e29918a44eba0940baef98150e78965bbd58b62 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 4 Aug 2025 11:01:24 +0200 Subject: [PATCH 3/8] cli-plugins/manager: un-export "NewPluginError" It is for internal use, and no longer needed for testing, now that the `Plugin` type handles marshalling errors. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 1cc698c68fdacb3cc8ec9933dde4360a0e108de4) Signed-off-by: Sebastiaan van Stijn --- cli-plugins/manager/error.go | 4 ++-- cli-plugins/manager/error_test.go | 2 +- cli-plugins/manager/plugin.go | 10 +++++----- cli-plugins/manager/plugin_test.go | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/cli-plugins/manager/error.go b/cli-plugins/manager/error.go index 1b5f4f60e54e..04bf3decb480 100644 --- a/cli-plugins/manager/error.go +++ b/cli-plugins/manager/error.go @@ -47,8 +47,8 @@ func wrapAsPluginError(err error, msg string) error { return &pluginError{cause: fmt.Errorf("%s: %w", msg, err)} } -// NewPluginError creates a new pluginError, analogous to +// newPluginError creates a new pluginError, analogous to // errors.Errorf. -func NewPluginError(msg string, args ...any) error { +func newPluginError(msg string, args ...any) error { return &pluginError{cause: fmt.Errorf(msg, args...)} } diff --git a/cli-plugins/manager/error_test.go b/cli-plugins/manager/error_test.go index c4cb19bd55d5..682860daadb0 100644 --- a/cli-plugins/manager/error_test.go +++ b/cli-plugins/manager/error_test.go @@ -10,7 +10,7 @@ import ( ) func TestPluginError(t *testing.T) { - err := NewPluginError("new error") + err := newPluginError("new error") assert.Check(t, is.Error(err, "new error")) inner := errors.New("testing") diff --git a/cli-plugins/manager/plugin.go b/cli-plugins/manager/plugin.go index e72d9039b895..2caae39b0292 100644 --- a/cli-plugins/manager/plugin.go +++ b/cli-plugins/manager/plugin.go @@ -86,7 +86,7 @@ func newPlugin(c pluginCandidate, cmds []*cobra.Command) (Plugin, error) { // Now apply the candidate tests, so these update p.Err. if !pluginNameRe.MatchString(p.Name) { - p.Err = NewPluginError("plugin candidate %q did not match %q", p.Name, pluginNameRe.String()) + p.Err = newPluginError("plugin candidate %q did not match %q", p.Name, pluginNameRe.String()) return p, nil } @@ -98,11 +98,11 @@ func newPlugin(c pluginCandidate, cmds []*cobra.Command) (Plugin, error) { continue } if cmd.Name() == p.Name { - p.Err = NewPluginError("plugin %q duplicates builtin command", p.Name) + p.Err = newPluginError("plugin %q duplicates builtin command", p.Name) return p, nil } if cmd.HasAlias(p.Name) { - p.Err = NewPluginError("plugin %q duplicates an alias of builtin command %q", p.Name, cmd.Name()) + p.Err = newPluginError("plugin %q duplicates an alias of builtin command %q", p.Name, cmd.Name()) return p, nil } } @@ -119,11 +119,11 @@ func newPlugin(c pluginCandidate, cmds []*cobra.Command) (Plugin, error) { return p, nil } if p.Metadata.SchemaVersion != "0.1.0" { - p.Err = NewPluginError("plugin SchemaVersion %q is not valid, must be 0.1.0", p.Metadata.SchemaVersion) + p.Err = newPluginError("plugin SchemaVersion %q is not valid, must be 0.1.0", p.Metadata.SchemaVersion) return p, nil } if p.Metadata.Vendor == "" { - p.Err = NewPluginError("plugin metadata does not define a vendor") + p.Err = newPluginError("plugin metadata does not define a vendor") return p, nil } return p, nil diff --git a/cli-plugins/manager/plugin_test.go b/cli-plugins/manager/plugin_test.go index 84d71d5187d6..ace51c12c630 100644 --- a/cli-plugins/manager/plugin_test.go +++ b/cli-plugins/manager/plugin_test.go @@ -29,7 +29,7 @@ func TestPluginMarshal(t *testing.T) { }, { doc: "custom error", - error: NewPluginError("something went wrong"), + error: newPluginError("something went wrong"), expected: jsonWithError, }, } From fe1d790d8edf6dd347ee993bed57fad41566f673 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 4 Aug 2025 11:06:49 +0200 Subject: [PATCH 4/8] cli-plugins/manager: deprecate "IsNotFound" These errors satisfy errdefs.IsNotFound, so make it a wrapper, and deprecate it. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 71460215d3f41ae449bbd5e32618fc33b638159e) Signed-off-by: Sebastiaan van Stijn --- cli-plugins/manager/manager.go | 19 ++++++++----------- cli-plugins/manager/manager_test.go | 7 ++++--- cmd/docker/builder.go | 3 ++- cmd/docker/docker.go | 6 +++--- 4 files changed, 17 insertions(+), 18 deletions(-) diff --git a/cli-plugins/manager/manager.go b/cli-plugins/manager/manager.go index 25515bccb868..52be4da40b5c 100644 --- a/cli-plugins/manager/manager.go +++ b/cli-plugins/manager/manager.go @@ -9,6 +9,7 @@ import ( "strings" "sync" + "github.com/containerd/errdefs" "github.com/docker/cli/cli-plugins/metadata" "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/config/configfile" @@ -40,15 +41,11 @@ func (e errPluginNotFound) Error() string { return "Error: No such CLI plugin: " + string(e) } -type notFound interface{ NotFound() } - // IsNotFound is true if the given error is due to a plugin not being found. +// +// Deprecated: use [errdefs.IsNotFound]. func IsNotFound(err error) bool { - if e, ok := err.(*pluginError); ok { - err = e.Cause() - } - _, ok := err.(notFound) - return ok + return errdefs.IsNotFound(err) } // getPluginDirs returns the platform-specific locations to search for plugins @@ -127,7 +124,7 @@ func getPlugin(name string, pluginDirs []string, rootcmd *cobra.Command) (*Plugi if err != nil { return nil, err } - if !IsNotFound(p.Err) { + if !errdefs.IsNotFound(p.Err) { p.ShadowedPaths = paths[1:] } return &p, nil @@ -164,7 +161,7 @@ func ListPlugins(dockerCli config.Provider, rootcmd *cobra.Command) ([]Plugin, e if err != nil { return err } - if !IsNotFound(p.Err) { + if !errdefs.IsNotFound(p.Err) { p.ShadowedPaths = paths[1:] mu.Lock() defer mu.Unlock() @@ -185,9 +182,9 @@ func ListPlugins(dockerCli config.Provider, rootcmd *cobra.Command) ([]Plugin, e return plugins, nil } -// PluginRunCommand returns an "os/exec".Cmd which when .Run() will execute the named plugin. +// PluginRunCommand returns an [os/exec.Cmd] which when [os/exec.Cmd.Run] will execute the named plugin. // The rootcmd argument is referenced to determine the set of builtin commands in order to detect conficts. -// The error returned satisfies the IsNotFound() predicate if no plugin was found or if the first candidate plugin was invalid somehow. +// The error returned satisfies the [errdefs.IsNotFound] predicate if no plugin was found or if the first candidate plugin was invalid somehow. func PluginRunCommand(dockerCli config.Provider, name string, rootcmd *cobra.Command) (*exec.Cmd, error) { // This uses the full original args, not the args which may // have been provided by cobra to our caller. This is because diff --git a/cli-plugins/manager/manager_test.go b/cli-plugins/manager/manager_test.go index 64609fbbad39..d1223ed4ec86 100644 --- a/cli-plugins/manager/manager_test.go +++ b/cli-plugins/manager/manager_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" + "github.com/containerd/errdefs" "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/internal/test" @@ -131,7 +132,7 @@ echo '{"SchemaVersion":"0.1.0"}'`, fs.WithMode(0o777)), _, err = GetPlugin("ccc", cli, &cobra.Command{}) assert.Error(t, err, "Error: No such CLI plugin: ccc") - assert.Assert(t, IsNotFound(err)) + assert.Assert(t, errdefs.IsNotFound(err)) } func TestListPluginsIsSorted(t *testing.T) { @@ -166,8 +167,8 @@ func TestErrPluginNotFound(t *testing.T) { var err error = errPluginNotFound("test") err.(errPluginNotFound).NotFound() assert.Error(t, err, "Error: No such CLI plugin: test") - assert.Assert(t, IsNotFound(err)) - assert.Assert(t, !IsNotFound(nil)) + assert.Assert(t, errdefs.IsNotFound(err)) + assert.Assert(t, !errdefs.IsNotFound(nil)) } func TestGetPluginDirs(t *testing.T) { diff --git a/cmd/docker/builder.go b/cmd/docker/builder.go index cccae304fe5e..00fc1b40f1ab 100644 --- a/cmd/docker/builder.go +++ b/cmd/docker/builder.go @@ -8,6 +8,7 @@ import ( "strconv" "strings" + "github.com/containerd/errdefs" pluginmanager "github.com/docker/cli/cli-plugins/manager" "github.com/docker/cli/cli-plugins/metadata" "github.com/docker/cli/cli/command" @@ -36,7 +37,7 @@ const ( ) func newBuilderError(errorMsg string, pluginLoadErr error) error { - if pluginmanager.IsNotFound(pluginLoadErr) { + if errdefs.IsNotFound(pluginLoadErr) { return errors.New(errorMsg) } if pluginLoadErr != nil { diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 32091106314b..b8e57add4afd 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -201,7 +201,7 @@ func setupHelpCommand(dockerCli command.Cli, rootCmd, helpCmd *cobra.Command) { if err == nil { return helpcmd.Run() } - if !pluginmanager.IsNotFound(err) { + if !errdefs.IsNotFound(err) { return fmt.Errorf("unknown help topic: %v", strings.Join(args, " ")) } } @@ -240,7 +240,7 @@ func setHelpFunc(dockerCli command.Cli, cmd *cobra.Command) { if err == nil { return } - if !pluginmanager.IsNotFound(err) { + if !errdefs.IsNotFound(err) { ccmd.Println(err) return } @@ -473,7 +473,7 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { } return nil } - if !pluginmanager.IsNotFound(err) { + if !errdefs.IsNotFound(err) { // For plugin not found we fall through to // cmd.Execute() which deals with reporting // "command not found" in a consistent way. From 7f699066bd047c2aee5236f625597d1a9cc53c44 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 4 Aug 2025 11:11:52 +0200 Subject: [PATCH 5/8] cli-plugins/manager: pluginError: remove Causer interface We no longer depend on this interface and it implements Unwrap for native handling by go stdlib. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit d789bac04af184bd11496f5ef6d34fe7e64949cf) Signed-off-by: Sebastiaan van Stijn --- cli-plugins/manager/error.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cli-plugins/manager/error.go b/cli-plugins/manager/error.go index 04bf3decb480..709149151161 100644 --- a/cli-plugins/manager/error.go +++ b/cli-plugins/manager/error.go @@ -23,11 +23,6 @@ func (e *pluginError) Error() string { return e.cause.Error() } -// Cause satisfies the errors.causer interface for pluginError. -func (e *pluginError) Cause() error { - return e.cause -} - // Unwrap provides compatibility for Go 1.13 error chains. func (e *pluginError) Unwrap() error { return e.cause From ac88b74462ba6c8f720e6e6d182f93bdb74cc2f2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 4 Aug 2025 11:15:35 +0200 Subject: [PATCH 6/8] cli-plugins/manager: wrapAsPluginError: don't special-case nil This was a pattern inheritted from pkg/errors.Wrapf, which ignored nil errors for convenience. However, it is error-prone, as it is not obvious when returning a nil-error. All call-sites using `wrapAsPluginError` already do a check for nil errors, so remove this code to prevent hard to find bugs. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 50963accec79a87652014d6b40a1d1cee4054f51) Signed-off-by: Sebastiaan van Stijn --- cli-plugins/manager/error.go | 3 --- cli-plugins/manager/error_test.go | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cli-plugins/manager/error.go b/cli-plugins/manager/error.go index 709149151161..aaedae14fad3 100644 --- a/cli-plugins/manager/error.go +++ b/cli-plugins/manager/error.go @@ -36,9 +36,6 @@ func (e *pluginError) MarshalText() (text []byte, err error) { // wrapAsPluginError wraps an error in a pluginError with an // additional message. func wrapAsPluginError(err error, msg string) error { - if err == nil { - return nil - } return &pluginError{cause: fmt.Errorf("%s: %w", msg, err)} } diff --git a/cli-plugins/manager/error_test.go b/cli-plugins/manager/error_test.go index 682860daadb0..2e92001c20ed 100644 --- a/cli-plugins/manager/error_test.go +++ b/cli-plugins/manager/error_test.go @@ -21,4 +21,7 @@ func TestPluginError(t *testing.T) { actual, err := json.Marshal(err) assert.Check(t, err) assert.Check(t, is.Equal(`"wrapping: testing"`, string(actual))) + + err = wrapAsPluginError(nil, "wrapping") + assert.Check(t, is.Error(err, "wrapping: %!w()")) } From 099820d8cafd8b1db627683552dcb3762c92e609 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 4 Aug 2025 11:25:01 +0200 Subject: [PATCH 7/8] cli-plugins/manager: deprecate metadata aliases These aliases were added in 4321293972a4ed804e8c063868cc5da6147ce73b (part of v28.0), but did not deprecate them. They are no longer used in the CLI itself, but may be used by cli-plugin implementations. This deprecates the aliases in `cli-plugins/manager` in favor of their equivalent in `cli-plugins/manager/metadata`: - `NamePrefix` - `MetadataSubcommandName` - `HookSubcommandName` - `Metadata` - `ReexecEnvvar` Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 5876b2941c8b0cbefade16d6f857f1899a43f51b) Signed-off-by: Sebastiaan van Stijn --- cli-plugins/manager/metadata.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cli-plugins/manager/metadata.go b/cli-plugins/manager/metadata.go index 9bddb121422d..6fda7fa1af05 100644 --- a/cli-plugins/manager/metadata.go +++ b/cli-plugins/manager/metadata.go @@ -6,18 +6,26 @@ import ( const ( // NamePrefix is the prefix required on all plugin binary names + // + // Deprecated: use [metadata.NamePrefix]. This alias will be removed in a future release. NamePrefix = metadata.NamePrefix // MetadataSubcommandName is the name of the plugin subcommand // which must be supported by every plugin and returns the // plugin metadata. + // + // Deprecated: use [metadata.MetadataSubcommandName]. This alias will be removed in a future release. MetadataSubcommandName = metadata.MetadataSubcommandName // HookSubcommandName is the name of the plugin subcommand // which must be implemented by plugins declaring support // for hooks in their metadata. + // + // Deprecated: use [metadata.HookSubcommandName]. This alias will be removed in a future release. HookSubcommandName = metadata.HookSubcommandName ) // Metadata provided by the plugin. +// +// Deprecated: use [metadata.Metadata]. This alias will be removed in a future release. type Metadata = metadata.Metadata From 79e0b1d7d9f1fd1d54e037c2c69765feb7f917e4 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 4 Aug 2025 11:27:07 +0200 Subject: [PATCH 8/8] cli-plugins/manager: remove deprecated ResourceAttributesEnvvar This const was deprecated in 9dc175d6ef7057c0e86d3a097b86012676fe4a95, which is part of v28.0, so let's remove it. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 513ceeec0a4cb73722a4f2ea6e06ffb4ee6b46fb) Signed-off-by: Sebastiaan van Stijn --- cli-plugins/manager/manager.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/cli-plugins/manager/manager.go b/cli-plugins/manager/manager.go index 52be4da40b5c..b76dbbe37667 100644 --- a/cli-plugins/manager/manager.go +++ b/cli-plugins/manager/manager.go @@ -24,12 +24,6 @@ const ( // plugin. Assuming $PATH and $CWD remain unchanged this should allow // the plugin to re-execute the original CLI. ReexecEnvvar = metadata.ReexecEnvvar - - // ResourceAttributesEnvvar is the name of the envvar that includes additional - // resource attributes for OTEL. - // - // Deprecated: The "OTEL_RESOURCE_ATTRIBUTES" env-var is part of the OpenTelemetry specification; users should define their own const for this. This const will be removed in the next release. - ResourceAttributesEnvvar = "OTEL_RESOURCE_ATTRIBUTES" ) // errPluginNotFound is the error returned when a plugin could not be found.