From 6ca8783730035c6b0efbb0b9e839ec1703360aea Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Tue, 21 May 2019 16:50:12 +0000 Subject: [PATCH 1/2] cli-plugins: add concept of experimental plugin, only enabled in experimental mode To test, add $(pwd)/build/plugins-linux-amd64 to "cliPluginsExtraDirs" config and run: make plugins make binary HELLO_EXPERIMENTAL=1 docker helloworld To show it enabled: HELLO_EXPERIMENTAL=1 DOCKER_CLI_EXPERIMENTAL=enabled docker helloworld Signed-off-by: Tibor Vass --- cli-plugins/examples/helloworld/main.go | 1 + cli-plugins/manager/candidate_test.go | 12 ++++++---- cli-plugins/manager/manager.go | 31 +++++++++++++++++++++---- cli-plugins/manager/metadata.go | 3 +++ cli-plugins/manager/plugin.go | 9 +++++-- 5 files changed, 46 insertions(+), 10 deletions(-) diff --git a/cli-plugins/examples/helloworld/main.go b/cli-plugins/examples/helloworld/main.go index ba23f4954ecf..597dc42b0179 100644 --- a/cli-plugins/examples/helloworld/main.go +++ b/cli-plugins/examples/helloworld/main.go @@ -101,5 +101,6 @@ func main() { SchemaVersion: "0.1.0", Vendor: "Docker Inc.", Version: "testing", + Experimental: os.Getenv("HELLO_EXPERIMENTAL") != "", }) } diff --git a/cli-plugins/manager/candidate_test.go b/cli-plugins/manager/candidate_test.go index b5ed06453f76..b802caaa0db1 100644 --- a/cli-plugins/manager/candidate_test.go +++ b/cli-plugins/manager/candidate_test.go @@ -12,9 +12,10 @@ import ( ) type fakeCandidate struct { - path string - exec bool - meta string + path string + exec bool + meta string + allowExperimental bool } func (c *fakeCandidate) Path() string { @@ -67,10 +68,13 @@ func TestValidateCandidate(t *testing.T) { {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "xyzzy"}`}, invalid: `plugin SchemaVersion "xyzzy" is not valid`}, {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0"}`}, invalid: "plugin metadata does not define a vendor"}, {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": ""}`}, invalid: "plugin metadata does not define a vendor"}, + {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing", "Experimental": true}`}, invalid: "requires experimental CLI"}, // This one should work {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing"}`}}, + {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing"}`, allowExperimental: true}}, + {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing", "Experimental": true}`, allowExperimental: true}}, } { - p, err := newPlugin(tc.c, fakeroot) + p, err := newPlugin(tc.c, fakeroot, tc.c.allowExperimental) if tc.err != "" { assert.ErrorContains(t, err, tc.err) } else if tc.invalid != "" { diff --git a/cli-plugins/manager/manager.go b/cli-plugins/manager/manager.go index e7cd2de40aa2..e06286e9c46d 100644 --- a/cli-plugins/manager/manager.go +++ b/cli-plugins/manager/manager.go @@ -1,6 +1,7 @@ package manager import ( + "fmt" "io/ioutil" "os" "os/exec" @@ -27,10 +28,23 @@ func (e errPluginNotFound) Error() string { return "Error: No such CLI plugin: " + string(e) } +type errPluginRequireExperimental string + +// Note: errPluginRequireExperimental implements notFound so that the plugin +// is skipped when listing the plugins. +func (e errPluginRequireExperimental) NotFound() {} + +func (e errPluginRequireExperimental) Error() string { + return fmt.Sprintf("plugin candidate %q: requires experimental CLI", string(e)) +} + type notFound interface{ NotFound() } // IsNotFound is true if the given error is due to a plugin not being found. func IsNotFound(err error) bool { + if e, ok := err.(*pluginError); ok { + err = e.Cause() + } _, ok := err.(notFound) return ok } @@ -117,12 +131,14 @@ func ListPlugins(dockerCli command.Cli, rootcmd *cobra.Command) ([]Plugin, error continue } c := &candidate{paths[0]} - p, err := newPlugin(c, rootcmd) + p, err := newPlugin(c, rootcmd, dockerCli.ClientInfo().HasExperimental) if err != nil { return nil, err } - p.ShadowedPaths = paths[1:] - plugins = append(plugins, p) + if !IsNotFound(p.Err) { + p.ShadowedPaths = paths[1:] + plugins = append(plugins, p) + } } return plugins, nil @@ -159,12 +175,19 @@ func PluginRunCommand(dockerCli command.Cli, name string, rootcmd *cobra.Command } c := &candidate{path: path} - plugin, err := newPlugin(c, rootcmd) + plugin, err := newPlugin(c, rootcmd, dockerCli.ClientInfo().HasExperimental) if err != nil { return nil, err } if plugin.Err != nil { // TODO: why are we not returning plugin.Err? + + err := plugin.Err.(*pluginError).Cause() + // if an experimental plugin was invoked directly while experimental mode is off + // provide a more useful error message than "not found". + if err, ok := err.(errPluginRequireExperimental); ok { + return nil, err + } return nil, errPluginNotFound(name) } cmd := exec.Command(plugin.Path, args...) diff --git a/cli-plugins/manager/metadata.go b/cli-plugins/manager/metadata.go index d3de778141f6..19379034ea7a 100644 --- a/cli-plugins/manager/metadata.go +++ b/cli-plugins/manager/metadata.go @@ -22,4 +22,7 @@ type Metadata struct { ShortDescription string `json:",omitempty"` // URL is a pointer to the plugin's homepage. URL string `json:",omitempty"` + // Experimental specifies whether the plugin is experimental. + // Experimental plugins are not displayed on non-experimental CLIs. + Experimental bool `json:",omitempty"` } diff --git a/cli-plugins/manager/plugin.go b/cli-plugins/manager/plugin.go index a8ac4fa3a399..fc3ad693cf6d 100644 --- a/cli-plugins/manager/plugin.go +++ b/cli-plugins/manager/plugin.go @@ -33,7 +33,9 @@ type Plugin struct { // 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, rootcmd *cobra.Command) (Plugin, error) { +// +// nolint: gocyclo +func newPlugin(c Candidate, rootcmd *cobra.Command, allowExperimental bool) (Plugin, error) { path := c.Path() if path == "" { return Plugin{}, errors.New("plugin candidate path cannot be empty") @@ -94,7 +96,10 @@ func newPlugin(c Candidate, rootcmd *cobra.Command) (Plugin, error) { p.Err = wrapAsPluginError(err, "invalid metadata") return p, nil } - + if p.Experimental && !allowExperimental { + p.Err = &pluginError{errPluginRequireExperimental(p.Name)} + 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) return p, nil From bb8e89bb2eb6743f8bbafe481272598192bbecf0 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Tue, 21 May 2019 22:24:00 +0000 Subject: [PATCH 2/2] cli-plugins: add test names for easier debugging Signed-off-by: Tibor Vass --- cli-plugins/manager/candidate_test.go | 68 ++++++++++++++------------- 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/cli-plugins/manager/candidate_test.go b/cli-plugins/manager/candidate_test.go index b802caaa0db1..a8cf2ab49888 100644 --- a/cli-plugins/manager/candidate_test.go +++ b/cli-plugins/manager/candidate_test.go @@ -36,9 +36,10 @@ func TestValidateCandidate(t *testing.T) { builtinName = NamePrefix + "builtin" builtinAlias = NamePrefix + "alias" - badPrefixPath = "/usr/local/libexec/cli-plugins/wobble" - badNamePath = "/usr/local/libexec/cli-plugins/docker-123456" - goodPluginPath = "/usr/local/libexec/cli-plugins/" + goodPluginName + badPrefixPath = "/usr/local/libexec/cli-plugins/wobble" + badNamePath = "/usr/local/libexec/cli-plugins/docker-123456" + goodPluginPath = "/usr/local/libexec/cli-plugins/" + goodPluginName + metaExperimental = `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing", "Experimental": true}` ) fakeroot := &cobra.Command{Use: "docker"} @@ -50,43 +51,46 @@ func TestValidateCandidate(t *testing.T) { }) for _, tc := range []struct { - c *fakeCandidate + name string + c *fakeCandidate // Either err or invalid may be non-empty, but not both (both can be empty for a good plugin). err string invalid string }{ /* Each failing one of the tests */ - {c: &fakeCandidate{path: ""}, err: "plugin candidate path cannot be empty"}, - {c: &fakeCandidate{path: badPrefixPath}, err: fmt.Sprintf("does not have %q prefix", NamePrefix)}, - {c: &fakeCandidate{path: badNamePath}, invalid: "did not match"}, - {c: &fakeCandidate{path: builtinName}, invalid: `plugin "builtin" duplicates builtin command`}, - {c: &fakeCandidate{path: builtinAlias}, invalid: `plugin "alias" duplicates an alias of builtin command "builtin"`}, - {c: &fakeCandidate{path: goodPluginPath, exec: false}, invalid: fmt.Sprintf("failed to fetch metadata: faked a failure to exec %q", goodPluginPath)}, - {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `xyzzy`}, invalid: "invalid character"}, - {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{}`}, invalid: `plugin SchemaVersion "" is not valid`}, - {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "xyzzy"}`}, invalid: `plugin SchemaVersion "xyzzy" is not valid`}, - {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0"}`}, invalid: "plugin metadata does not define a vendor"}, - {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": ""}`}, invalid: "plugin metadata does not define a vendor"}, - {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing", "Experimental": true}`}, invalid: "requires experimental CLI"}, + {name: "empty path", c: &fakeCandidate{path: ""}, err: "plugin candidate path cannot be empty"}, + {name: "bad prefix", c: &fakeCandidate{path: badPrefixPath}, err: fmt.Sprintf("does not have %q prefix", NamePrefix)}, + {name: "bad path", c: &fakeCandidate{path: badNamePath}, invalid: "did not match"}, + {name: "builtin command", c: &fakeCandidate{path: builtinName}, invalid: `plugin "builtin" duplicates builtin command`}, + {name: "builtin alias", c: &fakeCandidate{path: builtinAlias}, invalid: `plugin "alias" duplicates an alias of builtin command "builtin"`}, + {name: "fetch failure", c: &fakeCandidate{path: goodPluginPath, exec: false}, invalid: fmt.Sprintf("failed to fetch metadata: faked a failure to exec %q", goodPluginPath)}, + {name: "metadata not json", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `xyzzy`}, invalid: "invalid character"}, + {name: "empty schemaversion", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{}`}, invalid: `plugin SchemaVersion "" is not valid`}, + {name: "invalid schemaversion", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "xyzzy"}`}, invalid: `plugin SchemaVersion "xyzzy" is not valid`}, + {name: "no vendor", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0"}`}, invalid: "plugin metadata does not define a vendor"}, + {name: "empty vendor", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": ""}`}, invalid: "plugin metadata does not define a vendor"}, + {name: "experimental required", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: metaExperimental}, invalid: "requires experimental CLI"}, // This one should work - {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing"}`}}, - {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing"}`, allowExperimental: true}}, - {c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing", "Experimental": true}`, allowExperimental: true}}, + {name: "valid", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing"}`}}, + {name: "valid + allowing experimental", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing"}`, allowExperimental: true}}, + {name: "experimental + allowing experimental", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: metaExperimental, allowExperimental: true}}, } { - p, err := newPlugin(tc.c, fakeroot, tc.c.allowExperimental) - if tc.err != "" { - assert.ErrorContains(t, err, tc.err) - } else if tc.invalid != "" { - assert.NilError(t, err) - assert.Assert(t, cmp.ErrorType(p.Err, reflect.TypeOf(&pluginError{}))) - assert.ErrorContains(t, p.Err, tc.invalid) - } else { - assert.NilError(t, err) - assert.Equal(t, NamePrefix+p.Name, goodPluginName) - assert.Equal(t, p.SchemaVersion, "0.1.0") - assert.Equal(t, p.Vendor, "e2e-testing") - } + t.Run(tc.name, func(t *testing.T) { + p, err := newPlugin(tc.c, fakeroot, tc.c.allowExperimental) + if tc.err != "" { + assert.ErrorContains(t, err, tc.err) + } else if tc.invalid != "" { + assert.NilError(t, err) + assert.Assert(t, cmp.ErrorType(p.Err, reflect.TypeOf(&pluginError{}))) + assert.ErrorContains(t, p.Err, tc.invalid) + } else { + assert.NilError(t, err) + assert.Equal(t, NamePrefix+p.Name, goodPluginName) + assert.Equal(t, p.SchemaVersion, "0.1.0") + assert.Equal(t, p.Vendor, "e2e-testing") + } + }) } }