EVEREST-855 Namespaces validation improvements (#299)

EVEREST-855 ns validation improvements

(cherry picked from commit 5ab6e5f)

.golangci.yml

@@ -48,6 +48,7 @@ linters:
     - wsl              # too many empty lines makes methods too long
     - wrapcheck        # forces to wrap errors everywhere
     - lll              # Just useless in the most cases
+    - perfsprint       # to keep errors consistent
 
 
 issues:

commands/install.go

@@ -29,7 +29,13 @@ import (
 
 func newInstallCmd(l *zap.SugaredLogger) *cobra.Command {
 	cmd := &cobra.Command{
-		Use:     "install",
+		Use: "install",
+		// The command expects no arguments. So to prevent users from misspelling and confusion
+		// in cases with unexpected spaces like
+		//       ./everestctl install --namespaces=aaa, a
+		// it will return
+		//        Error: unknown command "a" for "everestctl install"
+		Args:    cobra.NoArgs,
 		Example: "everestctl install --namespaces dev,staging,prod --operator.mongodb=true --operator.postgresql=true --operator.xtradb-cluster=true --skip-wizard",
 		Run: func(cmd *cobra.Command, args []string) {
 			initInstallViperFlags(cmd)

commands/upgrade.go

@@ -31,6 +31,12 @@ import (
 func newUpgradeCmd(l *zap.SugaredLogger) *cobra.Command {
 	cmd := &cobra.Command{
 		Use: "upgrade",
+		// The command expects no arguments. So to prevent users from misspelling and confusion
+		// in cases with unexpected spaces like
+		//       ./everestctl upgrade --namespaces=aaa, a
+		// it will return
+		//        Error: unknown command "a" for "everestctl upgrade"
+		Args: cobra.NoArgs,
 		Run: func(cmd *cobra.Command, args []string) {
 			initUpgradeViperFlags(cmd)
 

pkg/install/install.go

@@ -23,6 +23,7 @@ import (
 	"fmt"
 	"net/url"
 	"os"
+	"regexp"
 	"strings"
 
 	"github.com/AlecAivazis/survey/v2"
@@ -85,11 +86,29 @@ const (
 	disableTelemetryEnvVar = "DISABLE_TELEMETRY"
 )
 
+//nolint:gochecknoglobals
+var (
+	// ErrNSEmpty appears when the provided list of the namespaces is considered empty.
+	ErrNSEmpty = errors.New("namespace list is empty. Specify at least one namespace")
+	// ErrNSReserved appears when some of the provided names are forbidden to use.
+	ErrNSReserved = func(ns string) error {
+		return fmt.Errorf("'%s' namespace is reserved for Everest internals. Please specify another namespace", ns)
+	}
+	// ErrNameNotRFC1035Compatible appears when some of the provided names are not RFC1035 compatible.
+	ErrNameNotRFC1035Compatible = func(fieldName string) error {
+		return fmt.Errorf(`'%s' is not RFC 1035 compatible. The name should contain only lowercase alphanumeric characters or '-', start with an alphabetic character, end with an alphanumeric character`,
+			fieldName,
+		)
+	}
+)
+
 type (
 	// Config stores configuration for the operators.
 	Config struct {
-		// Namespaces defines comma-separated list of namespaces that everest can operate in.
+		// Namespaces is a user-defined string represents raw non-validated comma-separated list of namespaces for everest to operate in.
 		Namespaces string `mapstructure:"namespaces"`
+		// NamespacesList validated list of namespaces that everest can operate in.
+		NamespacesList []string `mapstructure:"namespaces-map"`
 		// SkipWizard skips wizard during installation.
 		SkipWizard bool `mapstructure:"skip-wizard"`
 		// KubeconfigPath is a path to a kubeconfig
@@ -109,14 +128,6 @@ type (
 	}
 )
 
-// NamespacesList returns list of the namespaces that everest can operate in.
-func (c Config) NamespacesList() []string {
-	if len(c.Namespaces) == 0 {
-		return []string{}
-	}
-	return strings.Split(c.Namespaces, ",")
-}
-
 // NewInstall returns a new Install struct.
 func NewInstall(c Config, l *zap.SugaredLogger) (*Install, error) {
 	cli := &Install{
@@ -184,15 +195,11 @@ func (o *Install) populateConfig() error {
 		}
 	}
 
-	if len(o.config.NamespacesList()) == 0 {
-		return errors.New("namespace list is empty. Specify the comma-separated list of namespaces using the --namespaces flag, at least one namespace is required")
-	}
-
-	for _, ns := range o.config.NamespacesList() {
-		if ns == SystemNamespace || ns == MonitoringNamespace {
-			return fmt.Errorf("'%s' namespace is reserved for Everest internals. Please specify another namespace", ns)
-		}
+	l, err := ValidateNamespaces(o.config.Namespaces)
+	if err != nil {
+		return err
 	}
+	o.config.NamespacesList = l
 
 	return nil
 }
@@ -246,7 +253,7 @@ func (o *Install) provisionEverestOperator(ctx context.Context) error {
 	}
 
 	o.l.Info("Creating operator group for everest")
-	if err := o.kubeClient.CreateOperatorGroup(ctx, systemOperatorGroup, SystemNamespace, o.config.NamespacesList()); err != nil {
+	if err := o.kubeClient.CreateOperatorGroup(ctx, systemOperatorGroup, SystemNamespace, o.config.NamespacesList); err != nil {
 		return err
 	}
 
@@ -284,15 +291,15 @@ func (o *Install) provisionEverest(ctx context.Context) error {
 	}
 
 	o.l.Info("Updating cluster role bindings for everest-admin")
-	if err := o.kubeClient.UpdateClusterRoleBinding(ctx, everestServiceAccountClusterRoleBinding, o.config.NamespacesList()); err != nil {
+	if err := o.kubeClient.UpdateClusterRoleBinding(ctx, everestServiceAccountClusterRoleBinding, o.config.NamespacesList); err != nil {
 		return err
 	}
 
 	return nil
 }
 
 func (o *Install) provisionDBNamespaces(ctx context.Context) error {
-	for _, namespace := range o.config.NamespacesList() {
+	for _, namespace := range o.config.NamespacesList {
 		namespace := namespace
 		if err := o.createNamespace(namespace); err != nil {
 			return err
@@ -345,27 +352,11 @@ func (o *Install) runEverestWizard() error {
 		return err
 	}
 
-	nsList := strings.Split(namespaces, ",")
-	for _, ns := range nsList {
-		ns = strings.TrimSpace(ns)
-		if ns == "" {
-			continue
-		}
-
-		if ns == SystemNamespace {
-			return fmt.Errorf("'%s' namespace is reserved for Everest internals. Please specify another namespace", ns)
-		}
-
-		if o.config.Namespaces != "" {
-			o.config.Namespaces += ","
-		}
-
-		o.config.Namespaces += ns
-	}
-
-	if len(o.config.NamespacesList()) == 0 {
-		return errors.New("namespace list is empty. Specify at least one namespace")
+	list, err := ValidateNamespaces(namespaces)
+	if err != nil {
+		return err
 	}
+	o.config.NamespacesList = list
 
 	return nil
 }
@@ -508,15 +499,15 @@ func (o *Install) installOperator(ctx context.Context, channel, operatorName, na
 			},
 		}
 		if operatorName == everestOperatorName {
-			params.TargetNamespaces = o.config.NamespacesList()
+			params.TargetNamespaces = o.config.NamespacesList
 			params.SubscriptionConfig.Env = append(params.SubscriptionConfig.Env, []corev1.EnvVar{
 				{
 					Name:  EverestMonitoringNamespaceEnvVar,
 					Value: MonitoringNamespace,
 				},
 				{
 					Name:  kubernetes.EverestDBNamespacesEnvVar,
-					Value: o.config.Namespaces,
+					Value: strings.Join(o.config.NamespacesList, ","),
 				},
 			}...)
 		}
@@ -602,3 +593,46 @@ func (o *Install) generateToken(ctx context.Context) (*token.ResetResponse, erro
 
 	return res, nil
 }
+
+// ValidateNamespaces validates a comma-separated namespaces string.
+func ValidateNamespaces(str string) ([]string, error) {
+	nsList := strings.Split(str, ",")
+	m := make(map[string]struct{})
+	for _, ns := range nsList {
+		ns = strings.TrimSpace(ns)
+		if ns == "" {
+			continue
+		}
+
+		if ns == SystemNamespace || ns == MonitoringNamespace {
+			return nil, ErrNSReserved(ns)
+		}
+
+		if err := validateRFC1035(ns); err != nil {
+			return nil, err
+		}
+
+		m[ns] = struct{}{}
+	}
+
+	list := make([]string, 0, len(m))
+	for k := range m {
+		list = append(list, k)
+	}
+
+	if len(list) == 0 {
+		return nil, ErrNSEmpty
+	}
+	return list, nil
+}
+
+// validates names to be RFC-1035 compatible  https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#rfc-1035-label-names
+func validateRFC1035(s string) error {
+	rfc1035Regex := "^[a-z]([-a-z0-9]{0,61}[a-z0-9])?$"
+	re := regexp.MustCompile(rfc1035Regex)
+	if !re.MatchString(s) {
+		return ErrNameNotRFC1035Compatible(s)
+	}
+
+	return nil
+}

pkg/install/install_test.go

@@ -0,0 +1,98 @@
+package install
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestValidateNamespaces(t *testing.T) {
+	t.Parallel()
+
+	type tcase struct {
+		name   string
+		input  string
+		output []string
+		error  error
+	}
+
+	tcases := []tcase{
+		{
+			name:   "empty string",
+			input:  "",
+			output: nil,
+			error:  ErrNSEmpty,
+		},
+		{
+			name:   "several empty strings",
+			input:  "   ,   ,",
+			output: nil,
+			error:  ErrNSEmpty,
+		},
+		{
+			name:   "correct",
+			input:  "aaa,bbb,ccc",
+			output: []string{"aaa", "bbb", "ccc"},
+			error:  nil,
+		},
+		{
+			name: "correct with spaces",
+			input: `    aaa, bbb 
+,ccc   `,
+			output: []string{"aaa", "bbb", "ccc"},
+			error:  nil,
+		},
+		{
+			name:   "reserved system ns",
+			input:  "everest-system",
+			output: nil,
+			error:  ErrNSReserved("everest-system"),
+		},
+		{
+			name:   "reserved system ns and empty ns",
+			input:  "everest-system,    ",
+			output: nil,
+			error:  ErrNSReserved("everest-system"),
+		},
+		{
+			name:   "reserved monitoring ns",
+			input:  "everest-monitoring",
+			output: nil,
+			error:  ErrNSReserved("everest-monitoring"),
+		},
+		{
+			name:   "duplicated ns",
+			input:  "aaa,bbb,aaa",
+			output: []string{"aaa", "bbb"},
+			error:  nil,
+		},
+		{
+			name:   "name is too long",
+			input:  "e1234567890123456789012345678901234567890123456789012345678901234567890,bbb",
+			output: nil,
+			error:  ErrNameNotRFC1035Compatible("e1234567890123456789012345678901234567890123456789012345678901234567890"),
+		},
+		{
+			name:   "name starts with number",
+			input:  "1aaa,bbb",
+			output: nil,
+			error:  ErrNameNotRFC1035Compatible("1aaa"),
+		},
+		{
+			name:   "name contains special characters",
+			input:  "aa12a,b$s",
+			output: nil,
+			error:  ErrNameNotRFC1035Compatible("b$s"),
+		},
+	}
+
+	for _, tc := range tcases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			output, err := ValidateNamespaces(tc.input)
+			assert.Equal(t, tc.error, err)
+			assert.ElementsMatch(t, tc.output, output)
+		})
+	}
+}

pkg/kubernetes/backup_storage.go

@@ -79,21 +79,21 @@ func (k *Kubernetes) IsBackupStorageUsed(ctx context.Context, namespace, backupS
 		if err != nil {
 			return false, err
 		}
-		if len(list.Items) > 0 {
+		if len(list.Items) != 0 {
 			return true, nil
 		}
 		bList, err := k.client.ListDatabaseClusterBackups(ctx, namespace, options)
 		if err != nil {
 			return false, err
 		}
-		if len(bList.Items) > 0 {
+		if len(bList.Items) != 0 {
 			return true, nil
 		}
 		rList, err := k.client.ListDatabaseClusterRestores(ctx, namespace, options)
 		if err != nil {
 			return false, err
 		}
-		if len(rList.Items) > 0 {
+		if len(rList.Items) != 0 {
 			return true, nil
 		}
 	}

pkg/kubernetes/client/client.go

@@ -71,7 +71,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 
 	"github.com/percona/percona-everest-cli/pkg/kubernetes/client/customresources"
-	"github.com/percona/percona-everest-cli/pkg/kubernetes/client/database"
 )
 
 const (
@@ -104,7 +103,6 @@ type Client struct {
 	customClientSet  *customresources.Client
 	apiextClientset  apiextv1clientset.Interface
 	dynamicClientset dynamic.Interface
-	dbClusterClient  *database.DBClusterClient
 	rcLock           *sync.Mutex
 	restConfig       *rest.Config
 	namespace        string

pkg/uninstall/uninstall.go

@@ -164,7 +164,7 @@ func (u *Uninstall) getDBs(ctx context.Context) (map[string]*everestv1alpha1.Dat
 		return nil, err
 	}
 
-	allDBs := map[string]*everestv1alpha1.DatabaseClusterList{}
+	allDBs := make(map[string]*everestv1alpha1.DatabaseClusterList)
 	for _, ns := range namespaces {
 		dbs, err := u.kubeClient.ListDatabaseClusters(ctx, ns)
 		if err != nil {