From e148ddad8f7790387ff7830fa8d3cf6ab7bf8a2d Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Thu, 30 Apr 2020 14:19:55 -0600 Subject: [PATCH] Add backwards-compatibility for flags passed to plugins (#2479) * update plugin server to ignore unknown flags during parse Signed-off-by: Steve Kriss --- changelogs/unreleased/2479-skriss | 1 + pkg/plugin/clientmgmt/process.go | 56 ++++++++++++++++++++++++++- pkg/plugin/clientmgmt/process_test.go | 50 ++++++++++++++++++++++++ pkg/plugin/framework/server.go | 1 + 4 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/2479-skriss diff --git a/changelogs/unreleased/2479-skriss b/changelogs/unreleased/2479-skriss new file mode 100644 index 000000000..d574682f6 --- /dev/null +++ b/changelogs/unreleased/2479-skriss @@ -0,0 +1 @@ +If plugins don't support the `--features` flag, don't pass it to them. Also, update the standard plugin server to ignore unknown flags. diff --git a/pkg/plugin/clientmgmt/process.go b/pkg/plugin/clientmgmt/process.go index 0d57a7cce..f2fc8ae36 100644 --- a/pkg/plugin/clientmgmt/process.go +++ b/pkg/plugin/clientmgmt/process.go @@ -17,6 +17,8 @@ limitations under the License. package clientmgmt import ( + "strings" + plugin "github.com/hashicorp/go-plugin" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -59,7 +61,32 @@ func newProcess(command string, logger logrus.FieldLogger, logLevel logrus.Level // This launches the plugin process. protocolClient, err := client.Client() if err != nil { - return nil, err + if !strings.Contains(err.Error(), "unknown flag: --features") { + return nil, err + } + + // Velero started passing the --features flag to plugins in v1.2, however existing plugins + // may not support that flag and may not silently ignore unknown flags. The plugin server + // code that we make available to plugin authors has since been updated to ignore unknown + // flags, but to avoid breaking plugins that haven't updated to that code and don't support + // the --features flag, we specifically handle not passing the flag if we can detect that + // it's not supported. + + logger.Debug("Plugin process does not support the --features flag, removing it and trying again") + + builder.commandArgs = removeFeaturesFlag(builder.commandArgs) + + logger.Debugf("Updated command args after removing --features flag: %v", builder.commandArgs) + + // re-get the client and protocol client now that --features has been removed + // from the command args. + client = builder.client() + protocolClient, err = client.Client() + if err != nil { + return nil, err + } + + logger.Debug("Plugin process successfully started without the --features flag") } p := &process{ @@ -70,6 +97,33 @@ func newProcess(command string, logger logrus.FieldLogger, logLevel logrus.Level return p, nil } +// removeFeaturesFlag looks for and removes the '--features' arg +// as well as the arg immediately following it (the flag value). +func removeFeaturesFlag(args []string) []string { + var commandArgs []string + var featureFlag bool + + for _, arg := range args { + // if this arg is the flag name, skip it + if arg == "--features" { + featureFlag = true + continue + } + + // if the last arg we saw was the flag name, then + // this arg is the value for the flag, so skip it + if featureFlag { + featureFlag = false + continue + } + + // otherwise, keep the arg + commandArgs = append(commandArgs, arg) + } + + return commandArgs +} + func (r *process) dispense(key kindAndName) (interface{}, error) { // This calls GRPCClient(clientConn) on the plugin instance registered for key.name. dispensed, err := r.protocolClient.Dispense(key.kind.String()) diff --git a/pkg/plugin/clientmgmt/process_test.go b/pkg/plugin/clientmgmt/process_test.go index 37e2c1128..ac82ade87 100644 --- a/pkg/plugin/clientmgmt/process_test.go +++ b/pkg/plugin/clientmgmt/process_test.go @@ -121,3 +121,53 @@ func TestDispense(t *testing.T) { }) } } + +func Test_removeFeaturesFlag(t *testing.T) { + tests := []struct { + name string + commandArgs []string + want []string + }{ + { + name: "when commandArgs is nil, a nil slice is returned", + commandArgs: nil, + want: nil, + }, + { + name: "when commandArgs is empty, a nil slice is returned", + commandArgs: []string{}, + want: nil, + }, + { + name: "when commandArgs does not contain --features, it is returned as-is", + commandArgs: []string{"--log-level", "debug", "--another-flag", "foo"}, + want: []string{"--log-level", "debug", "--another-flag", "foo"}, + }, + { + name: "when --features is the only flag, a nil slice is returned", + commandArgs: []string{"--features", "EnableCSI"}, + want: nil, + }, + { + name: "when --features is the first flag, it's properly removed", + commandArgs: []string{"--features", "EnableCSI", "--log-level", "debug", "--another-flag", "foo"}, + want: []string{"--log-level", "debug", "--another-flag", "foo"}, + }, + { + name: "when --features is the last flag, it's properly removed", + commandArgs: []string{"--log-level", "debug", "--another-flag", "foo", "--features", "EnableCSI"}, + want: []string{"--log-level", "debug", "--another-flag", "foo"}, + }, + { + name: "when --features is neither the first nor last flag, it's properly removed", + commandArgs: []string{"--log-level", "debug", "--features", "EnableCSI", "--another-flag", "foo"}, + want: []string{"--log-level", "debug", "--another-flag", "foo"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, removeFeaturesFlag(tc.commandArgs)) + }) + } +} diff --git a/pkg/plugin/framework/server.go b/pkg/plugin/framework/server.go index 927414421..844315eb0 100644 --- a/pkg/plugin/framework/server.go +++ b/pkg/plugin/framework/server.go @@ -103,6 +103,7 @@ func (s *server) BindFlags(flags *pflag.FlagSet) Server { flags.Var(s.logLevelFlag, "log-level", fmt.Sprintf("the level at which to log. Valid values are %s.", strings.Join(s.logLevelFlag.AllowedValues(), ", "))) flags.Var(s.featureSet, "features", "list of feature flags for this plugin") s.flagSet = flags + s.flagSet.ParseErrorsWhitelist.UnknownFlags = true return s }