Add validation for plugin name format and dups (#1339)

* Add validation for plugin name format and dups

Signed-off-by: Carlisia <carlisiac@vmware.com>

* Bug fix

Signed-off-by: Carlisia <carlisiac@vmware.com>

* Add changelog

Signed-off-by: Carlisia <carlisiac@vmware.com>

* Address code reviews

Signed-off-by: Carlisia <carlisiac@vmware.com>

* Fix code

Signed-off-by: Carlisia <carlisiac@vmware.com>

* Address code reviews

Signed-off-by: Carlisia <carlisiac@vmware.com>

* Add documentation

Signed-off-by: Carlisia <carlisiac@vmware.com>

* Update godoc

Signed-off-by: Carlisia <carlisiac@vmware.com>

* More doc

Signed-off-by: Carlisia <carlisiac@vmware.com>

pkg/plugin/framework/examples_test.go

@@ -23,8 +23,8 @@ import (
 
 func ExampleNewServer_volumeSnapshotter() {
 	NewServer(). // call the server
-			RegisterVolumeSnapshotter("example-volumesnapshotter", newVolumeSnapshotter). // register the plugin
-			Serve()                                                                       // serve the plugin
+			RegisterVolumeSnapshotter("example.io/volumesnapshotter", newVolumeSnapshotter). // register the plugin with a valid name
+			Serve()                                                                          // serve the plugin
 }
 
 func newVolumeSnapshotter(logger logrus.FieldLogger) (interface{}, error) {

pkg/plugin/framework/server.go

@@ -38,26 +38,30 @@ type Server interface {
 	// This method must be called prior to calling .Serve().
 	BindFlags(flags *pflag.FlagSet) Server
 
-	// RegisterBackupItemAction registers a backup item action.
-	RegisterBackupItemAction(name string, initializer HandlerInitializer) Server
+	// RegisterBackupItemAction registers a backup item action. Accepted format
+	// for the plugin name is <DNS subdomain>/<non-empty name>.
+	RegisterBackupItemAction(pluginName string, initializer HandlerInitializer) Server
 
 	// RegisterBackupItemActions registers multiple backup item actions.
 	RegisterBackupItemActions(map[string]HandlerInitializer) Server
 
-	// RegisterVolumeSnapshotter registers a volume snapshotter.
-	RegisterVolumeSnapshotter(name string, initializer HandlerInitializer) Server
+	// RegisterVolumeSnapshotter registers a volume snapshotter. Accepted format
+	// for the plugin name is <DNS subdomain>/<non-empty name>.
+	RegisterVolumeSnapshotter(pluginName string, initializer HandlerInitializer) Server
 
 	// RegisterVolumeSnapshotters registers multiple volume snapshotters.
 	RegisterVolumeSnapshotters(map[string]HandlerInitializer) Server
 
-	// RegisterObjectStore registers an object store.
-	RegisterObjectStore(name string, initializer HandlerInitializer) Server
+	// RegisterObjectStore registers an object store. Accepted format
+	// for the plugin name is <DNS subdomain>/<non-empty name>.
+	RegisterObjectStore(pluginName string, initializer HandlerInitializer) Server
 
 	// RegisterObjectStores registers multiple object stores.
 	RegisterObjectStores(map[string]HandlerInitializer) Server
 
-	// RegisterRestoreItemAction registers a restore item action.
-	RegisterRestoreItemAction(name string, initializer HandlerInitializer) Server
+	// RegisterRestoreItemAction registers a restore item action. Accepted format
+	// for the plugin name is <DNS subdomain>/<non-empty name>.
+	RegisterRestoreItemAction(pluginName string, initializer HandlerInitializer) Server
 
 	// RegisterRestoreItemActions registers multiple restore item actions.
 	RegisterRestoreItemActions(map[string]HandlerInitializer) Server

pkg/plugin/framework/server_errors.go

@@ -52,7 +52,7 @@ func newGRPCErrorWithCode(err error, code codes.Code, details ...goproto.Message
 	return statusErr.Err()
 }
 
-// newGRPCError is a convenience functino for creating a new gRPC error
+// newGRPCError is a convenience function for creating a new gRPC error
 // with code = codes.Unknown
 func newGRPCError(err error, details ...goproto.Message) error {
 	return newGRPCErrorWithCode(err, codes.Unknown, details...)

pkg/plugin/framework/server_mux.go

@@ -17,9 +17,12 @@ limitations under the License.
 package framework
 
 import (
+	"strings"
+
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/validation"
 )
 
 // HandlerInitializer is a function that initializes and returns a new instance of one of Velero's plugin interfaces
@@ -43,9 +46,13 @@ func newServerMux(logger logrus.FieldLogger) *serverMux {
 	}
 }
 
-// register registers the initializer for name.
+// register validates the plugin name and registers the
+// initializer for the given name.
 func (m *serverMux) register(name string, f HandlerInitializer) {
-	// TODO(ncdc): return an error on duplicate registrations for the same name.
+	if err := ValidatePluginName(name, m.names()); err != nil {
+		m.serverLog.Errorf("invalid plugin name %q: %s", name, err)
+		return
+	}
 	m.initializers[name] = f
 }
 
@@ -63,7 +70,7 @@ func (m *serverMux) getHandler(name string) (interface{}, error) {
 
 	initializer, found := m.initializers[name]
 	if !found {
-		return nil, errors.Errorf("unknown %v plugin: %s", m.kind, name)
+		return nil, errors.Errorf("%v plugin: %s was not found or has an invalid name format", m.kind, name)
 	}
 
 	instance, err := initializer(m.serverLog)
@@ -75,3 +82,34 @@ func (m *serverMux) getHandler(name string) (interface{}, error) {
 
 	return m.handlers[name], nil
 }
+
+// ValidatePluginName checks if the given name:
+// - the plugin name has two parts separated by '/'
+// - non of the above parts is empty
+// - the prefix is a valid DNS subdomain name
+// - a plugin with the same name does not already exist (if list of existing names is passed in)
+func ValidatePluginName(name string, existingNames []string) error {
+	// validate there is one "/" and two parts
+	parts := strings.Split(name, "/")
+	if len(parts) != 2 {
+		return errors.Errorf("plugin name must have exactly two parts separated by a `/`. Accepted format: <DNS subdomain>/<non-empty name>. %s is invalid", name)
+	}
+
+	// validate both prefix and name are non-empty
+	if parts[0] == "" || parts[1] == "" {
+		return errors.Errorf("both parts of the plugin name must be non-empty. Accepted format: <DNS subdomain>/<non-empty name>. %s is invalid", name)
+	}
+
+	// validate that the prefix is a DNS subdomain
+	if errs := validation.IsDNS1123Subdomain(parts[0]); len(errs) != 0 {
+		return errors.Errorf("first part of the plugin name must be a valid DNS subdomain. Accepted format: <DNS subdomain>/<non-empty name>. first part %q is invalid: %s", parts[0], strings.Join(errs, "; "))
+	}
+
+	for _, existingName := range existingNames {
+		if strings.Compare(name, existingName) == 0 {
+			return errors.New("plugin name " + existingName + " already exists")
+		}
+	}
+
+	return nil
+}

pkg/plugin/framework/server_mux_test.go

@@ -0,0 +1,71 @@
+/*
+Copyright 2019 the Velero contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package framework
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestValidPluginName(t *testing.T) {
+	successCases := []struct {
+		pluginName    string
+		existingNames []string
+	}{
+		{"example.io/azure", []string{"velero.io/aws"}},
+		{"with-dashes/name", []string{"velero.io/aws"}},
+		{"prefix/Uppercase_Is_OK_123", []string{"velero.io/aws"}},
+		{"example-with-dash.io/azure", []string{"velero.io/aws"}},
+		{"1.2.3.4/5678", []string{"velero.io/aws"}},
+		{"example.io/azure", []string{"velero.io/aws"}},
+		{"example.io/azure", []string{""}},
+		{"example.io/azure", nil},
+		{strings.Repeat("a", 253) + "/name", []string{"velero.io/aws"}},
+	}
+	for i, tt := range successCases {
+		t.Run(tt.pluginName, func(t *testing.T) {
+			if err := ValidatePluginName(tt.pluginName, tt.existingNames); err != nil {
+				t.Errorf("case[%d]: %q: expected success: %v", i, successCases[i], err)
+			}
+		})
+	}
+
+	errorCases := []struct {
+		pluginName    string
+		existingNames []string
+	}{
+		{"", []string{"velero.io/aws"}},
+		{"single", []string{"velero.io/aws"}},
+		{"/", []string{"velero.io/aws"}},
+		{"//", []string{"velero.io/aws"}},
+		{"///", []string{"velero.io/aws"}},
+		{"a/", []string{"velero.io/aws"}},
+		{"/a", []string{"velero.io/aws"}},
+		{"velero.io/aws", []string{"velero.io/aws"}},
+		{"Uppercase_Is_OK_123/name", []string{"velero.io/aws"}},
+		{strings.Repeat("a", 254) + "/name", []string{"velero.io/aws"}},
+		{"ospecialchars%^=@", []string{"velero.io/aws"}},
+	}
+
+	for i, tt := range errorCases {
+		t.Run(tt.pluginName, func(t *testing.T) {
+			if err := ValidatePluginName(tt.pluginName, tt.existingNames); err == nil {
+				t.Errorf("case[%d]: %q: expected failure.", i, errorCases[i])
+			}
+		})
+	}
+}