Add backwards-compatibility for flags passed to plugins (#2479)

* update plugin server to ignore unknown flags during parse

Signed-off-by: Steve Kriss <krisss@vmware.com>

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.

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

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))
+		})
+	}
+}

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
 }