From 7f434f8b637a4f676a777f6051944d2c29e78ab9 Mon Sep 17 00:00:00 2001 From: Oksana Grishchenko Date: Thu, 15 Feb 2024 15:35:04 +0200 Subject: [PATCH 1/7] EVEREST-855 ns validation improvements --- commands/install.go | 8 ++- commands/upgrade.go | 6 ++ pkg/install/install.go | 114 +++++++++++++++++++------------ pkg/install/install_test.go | 97 ++++++++++++++++++++++++++ pkg/kubernetes/backup_storage.go | 6 +- pkg/uninstall/uninstall.go | 2 +- pkg/upgrade/upgrade.go | 21 +++--- 7 files changed, 195 insertions(+), 59 deletions(-) create mode 100644 pkg/install/install_test.go diff --git a/commands/install.go b/commands/install.go index 7c15ca0e..8404b565 100644 --- a/commands/install.go +++ b/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) diff --git a/commands/upgrade.go b/commands/upgrade.go index 7380ddb1..1864efc4 100644 --- a/commands/upgrade.go +++ b/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) diff --git a/pkg/install/install.go b/pkg/install/install.go index 7c868f1c..915cbb04 100644 --- a/pkg/install/install.go +++ b/pkg/install/install.go @@ -23,6 +23,7 @@ import ( "fmt" "net/url" "os" + "regexp" "strings" "github.com/AlecAivazis/survey/v2" @@ -85,11 +86,25 @@ const ( disableTelemetryEnvVar = "DISABLE_TELEMETRY" ) +var ( + ErrNSEmpty = errors.New("namespace list is empty. Specify at least one namespace") + ErrNSReserved = func(ns string) error { + return fmt.Errorf("'%s' namespace is reserved for Everest internals. Please specify another namespace", ns) + } + 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 +124,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 +191,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 +249,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,7 +287,7 @@ 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 } @@ -292,7 +295,7 @@ func (o *Install) provisionEverest(ctx context.Context) error { } 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 +348,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,7 +495,7 @@ 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, @@ -516,7 +503,7 @@ func (o *Install) installOperator(ctx context.Context, channel, operatorName, na }, { Name: kubernetes.EverestDBNamespacesEnvVar, - Value: o.config.Namespaces, + Value: strings.Join(o.config.NamespacesList, ","), }, }...) } @@ -602,3 +589,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{}{} + } + + var list []string + 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 +} diff --git a/pkg/install/install_test.go b/pkg/install/install_test.go new file mode 100644 index 00000000..0092a58e --- /dev/null +++ b/pkg/install/install_test.go @@ -0,0 +1,97 @@ +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 { + 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) + }) + } +} diff --git a/pkg/kubernetes/backup_storage.go b/pkg/kubernetes/backup_storage.go index 93a01b5f..b490e258 100644 --- a/pkg/kubernetes/backup_storage.go +++ b/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 } } diff --git a/pkg/uninstall/uninstall.go b/pkg/uninstall/uninstall.go index c81d4e67..3a8d9520 100644 --- a/pkg/uninstall/uninstall.go +++ b/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 { diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index d2d95797..7aa02319 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -20,14 +20,12 @@ import ( "context" "errors" "fmt" - "net/url" - "os" - "strings" - "github.com/AlecAivazis/survey/v2" goversion "github.com/hashicorp/go-version" "go.uber.org/zap" "k8s.io/apimachinery/pkg/types" + "net/url" + "os" "github.com/percona/percona-everest-cli/data" "github.com/percona/percona-everest-cli/pkg/install" @@ -39,6 +37,8 @@ type ( Config struct { // Namespaces defines comma-separated list of namespaces that everest can operate in. Namespaces string `mapstructure:"namespaces"` + // NamespacesList validated list of namespaces that everest can operate in. + NamespacesList []string `mapstructure:"namespaces-map"` // KubeconfigPath is a path to a kubeconfig KubeconfigPath string `mapstructure:"kubeconfig"` // UpgradeOLM defines do we need to upgrade OLM or not. @@ -55,11 +55,6 @@ type ( } ) -// NamespacesList returns list of the namespaces that everest can operate in. -func (c Config) NamespacesList() []string { - return strings.Split(c.Namespaces, ",") -} - // NewUpgrade returns a new Upgrade struct. func NewUpgrade(c Config, l *zap.SugaredLogger) (*Upgrade, error) { cli := &Upgrade{ @@ -85,9 +80,11 @@ func (u *Upgrade) Run(ctx context.Context) error { if err := u.runEverestWizard(ctx); err != nil { return err } - if len(u.config.NamespacesList()) == 0 { - return errors.New("namespace list is empty. Specify at least one namespace") + l, err := install.ValidateNamespaces(u.config.Namespaces) + if err != nil { + return err } + u.config.NamespacesList = l if err := u.upgradeOLM(ctx); err != nil { return err } @@ -132,7 +129,7 @@ func (u *Upgrade) runEverestWizard(ctx context.Context) error { } func (u *Upgrade) patchSubscriptions(ctx context.Context) error { - for _, namespace := range u.config.NamespacesList() { + for _, namespace := range u.config.NamespacesList { namespace := namespace subList, err := u.kubeClient.ListSubscriptions(ctx, namespace) if err != nil { From 14e8ff60eb6a4353993cdccfdf25b32ad83b8e4a Mon Sep 17 00:00:00 2001 From: Oksana Grishchenko Date: Thu, 15 Feb 2024 15:38:03 +0200 Subject: [PATCH 2/7] comment --- pkg/upgrade/upgrade.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index 7aa02319..a6d6cf99 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -35,7 +35,7 @@ import ( type ( // Config defines configuration required for upgrade command. 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"` From c30e2c53ae3411f4f3fa44ce678d904b94892a87 Mon Sep 17 00:00:00 2001 From: Oksana Grishchenko Date: Thu, 15 Feb 2024 15:56:37 +0200 Subject: [PATCH 3/7] linter --- pkg/install/install.go | 8 ++++++-- pkg/install/install_test.go | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/install/install.go b/pkg/install/install.go index 915cbb04..d043a912 100644 --- a/pkg/install/install.go +++ b/pkg/install/install.go @@ -86,11 +86,15 @@ const ( disableTelemetryEnvVar = "DISABLE_TELEMETRY" ) +//nolint:gochecknoglobals var ( - ErrNSEmpty = errors.New("namespace list is empty. Specify at least one namespace") + // 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, @@ -590,7 +594,7 @@ func (o *Install) generateToken(ctx context.Context) (*token.ResetResponse, erro return res, nil } -// ValidateNamespaces validates a comma-separated namespaces string +// ValidateNamespaces validates a comma-separated namespaces string. func ValidateNamespaces(str string) ([]string, error) { nsList := strings.Split(str, ",") m := make(map[string]struct{}) diff --git a/pkg/install/install_test.go b/pkg/install/install_test.go index 0092a58e..7598dff1 100644 --- a/pkg/install/install_test.go +++ b/pkg/install/install_test.go @@ -87,6 +87,7 @@ func TestValidateNamespaces(t *testing.T) { } for _, tc := range tcases { + tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() output, err := ValidateNamespaces(tc.input) From c3eb92271b78c5183924b39d60f44981a042bcde Mon Sep 17 00:00:00 2001 From: Oksana Grishchenko Date: Thu, 15 Feb 2024 15:58:30 +0200 Subject: [PATCH 4/7] linter --- .golangci.yml | 1 + pkg/install/install.go | 2 +- pkg/kubernetes/client/client.go | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index b5fa706d..0c2df182 100644 --- a/.golangci.yml +++ b/.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: diff --git a/pkg/install/install.go b/pkg/install/install.go index d043a912..c2a48b25 100644 --- a/pkg/install/install.go +++ b/pkg/install/install.go @@ -615,7 +615,7 @@ func ValidateNamespaces(str string) ([]string, error) { m[ns] = struct{}{} } - var list []string + list := make([]string, 0, len(m)) for k := range m { list = append(list, k) } diff --git a/pkg/kubernetes/client/client.go b/pkg/kubernetes/client/client.go index e140d659..7017a94e 100644 --- a/pkg/kubernetes/client/client.go +++ b/pkg/kubernetes/client/client.go @@ -104,7 +104,7 @@ type Client struct { customClientSet *customresources.Client apiextClientset apiextv1clientset.Interface dynamicClientset dynamic.Interface - dbClusterClient *database.DBClusterClient + dbClusterClient *database.DBClusterClient //nolint:structcheck rcLock *sync.Mutex restConfig *rest.Config namespace string From ab0b9b3498683f582fab697c7ced8d8a6ecf8c54 Mon Sep 17 00:00:00 2001 From: Oksana Grishchenko Date: Thu, 15 Feb 2024 16:05:02 +0200 Subject: [PATCH 5/7] format --- pkg/upgrade/upgrade.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index a6d6cf99..3cbd1d28 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -20,12 +20,13 @@ import ( "context" "errors" "fmt" + "net/url" + "os" + "github.com/AlecAivazis/survey/v2" goversion "github.com/hashicorp/go-version" "go.uber.org/zap" "k8s.io/apimachinery/pkg/types" - "net/url" - "os" "github.com/percona/percona-everest-cli/data" "github.com/percona/percona-everest-cli/pkg/install" From a20ea570a4f8721d530fc7063330cb3b617a071b Mon Sep 17 00:00:00 2001 From: Oksana Grishchenko Date: Thu, 15 Feb 2024 16:14:00 +0200 Subject: [PATCH 6/7] linter --- pkg/kubernetes/client/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kubernetes/client/client.go b/pkg/kubernetes/client/client.go index 7017a94e..8c2dba1a 100644 --- a/pkg/kubernetes/client/client.go +++ b/pkg/kubernetes/client/client.go @@ -104,7 +104,7 @@ type Client struct { customClientSet *customresources.Client apiextClientset apiextv1clientset.Interface dynamicClientset dynamic.Interface - dbClusterClient *database.DBClusterClient //nolint:structcheck + dbClusterClient *database.DBClusterClient //nolint:structcheck,unused rcLock *sync.Mutex restConfig *rest.Config namespace string From bf6de15ab4872c73d6f7d73cdf809729e1289eb7 Mon Sep 17 00:00:00 2001 From: Oksana Grishchenko Date: Thu, 15 Feb 2024 18:26:38 +0200 Subject: [PATCH 7/7] remove unused --- pkg/kubernetes/client/client.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/kubernetes/client/client.go b/pkg/kubernetes/client/client.go index 8c2dba1a..84c2e73a 100644 --- a/pkg/kubernetes/client/client.go +++ b/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 //nolint:structcheck,unused rcLock *sync.Mutex restConfig *rest.Config namespace string