From 91f2197e6b65aae8a4267984561e582a22fd79ea Mon Sep 17 00:00:00 2001 From: FakePlasticTree Date: Fri, 13 Dec 2024 11:54:24 +0800 Subject: [PATCH] Fix(cli): bugs in cli first round testing (#664) * fix: version flag * fix: bugs in demo cmd and add addtional check when creating cluster and tenant * fix: update all the helm repos when helm<3.7.0 * fix: log cmd to show the passwd with specified namespace * fix: duplicate item in backup policy show cmd * fix: remove `Complete` function when creating a backup policy When using the same path for each tenant, will raise an NFS error "Error 9081 (HY000): format file does not match, try to set a new directory". This commit fixes the issue by ensuring the destination configuration correctly matches the current cluster/tenant/backup type. * fix: remove path complete in tenant 's `create` cmd * fix: lint --- internal/cli/backup/create.go | 14 +------ internal/cli/cli.go | 7 ++-- internal/cli/cmd/backup/create.go | 3 -- internal/cli/cmd/backup/show.go | 4 +- internal/cli/cmd/cluster/create.go | 2 +- internal/cli/cmd/demo/demo.go | 39 +++++++++++++------ internal/cli/cmd/tenant/create.go | 2 +- internal/cli/demo/prompt.go | 3 +- internal/cli/install/install.go | 2 +- internal/cli/tenant/create.go | 16 ++++---- internal/cli/utils/checker.go | 20 ++++++++++ internal/cli/utils/validator.go | 9 +++-- .../dashboard/business/oceanbase/obcluster.go | 7 ++-- 13 files changed, 74 insertions(+), 54 deletions(-) diff --git a/internal/cli/backup/create.go b/internal/cli/backup/create.go index bb6dea47a..7cd1987c5 100644 --- a/internal/cli/backup/create.go +++ b/internal/cli/backup/create.go @@ -16,7 +16,6 @@ package backup import ( "context" "errors" - "fmt" "github.com/robfig/cron/v3" "github.com/spf13/cobra" @@ -178,17 +177,6 @@ func CreateTenantBackupPolicy(ctx context.Context, o *CreateOptions) (*v1alpha1. return policy, nil } -func (o *CreateOptions) Complete() error { - // set default values for archive path and backup data path - if o.DestType == "NFS" && o.ArchivePath == "" { - o.ArchivePath = fmt.Sprintf("%s/%s", "archive", o.Name) - } - if o.DestType == "NFS" && o.BakDataPath == "" { - o.BakDataPath = fmt.Sprintf("%s/%s", "backup", o.Name) - } - return nil -} - func (o *CreateOptions) Validate() error { if o.Namespace == "" { return errors.New("Namespace is required") @@ -224,6 +212,8 @@ func (o *CreateOptions) AddFlags(cmd *cobra.Command) { func (o *CreateOptions) SetRequiredFlags(cmd *cobra.Command) { _ = cmd.MarkFlagRequired(FLAG_FULL) + _ = cmd.MarkFlagRequired(FLAG_ARCHIVE_PATH) + _ = cmd.MarkFlagRequired(FLAG_BAK_DATA_PATH) } // AddBaseFlags adds the base flags for the create command diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 87f08fc78..b806649d5 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -32,13 +32,12 @@ func NewCliCmd() *cobra.Command { Use: "obocli", Short: "OceanBase Operator Cli", Long: "OceanBase Operator Cli tool to manage OceanBase clusters, tenants, and backup policies.", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { if cmd.Flags().Changed("version") { versionCmd := version.NewCmd() - versionCmd.Run(cmd, args) - } else { - _ = cmd.Help() + return versionCmd.RunE(cmd, args) } + return cmd.Help() }, } cmd.AddCommand(version.NewCmd()) diff --git a/internal/cli/cmd/backup/create.go b/internal/cli/cmd/backup/create.go index 052c74fea..13aaecc73 100644 --- a/internal/cli/cmd/backup/create.go +++ b/internal/cli/cmd/backup/create.go @@ -31,9 +31,6 @@ func NewCreateCmd() *cobra.Command { Args: cobra.ExactArgs(1), PreRunE: o.Parse, Run: func(cmd *cobra.Command, args []string) { - if err := o.Complete(); err != nil { - logger.Fatalln(err) - } if err := o.Validate(); err != nil { logger.Fatalln(err) } diff --git a/internal/cli/cmd/backup/show.go b/internal/cli/cmd/backup/show.go index 799f5abbf..2f697e5bd 100644 --- a/internal/cli/cmd/backup/show.go +++ b/internal/cli/cmd/backup/show.go @@ -45,7 +45,7 @@ func NewShowCmd() *cobra.Command { logger.Fatalln(err, "failed to get backup policy") } if backupPolicy == nil || backupPolicy.Name == "" { - logger.Fatalln("no backup policy found") + logger.Fatalln("no backup policies found") } backupJobList, err := backup.ListBackupJobs(cmd.Context(), backupPolicy.Name, o) if err != nil { @@ -60,9 +60,9 @@ func NewShowCmd() *cobra.Command { sort.Slice(backupJobList.Items, func(i, j int) bool { return backupJobList.Items[i].CreationTimestamp.Before(&backupJobList.Items[j].CreationTimestamp) }) + tbLog.Println("JOBNAME \t STATUS \t TYPE \t STARTAT \t ENDAT \t") } for _, job := range backupJobList.Items { - tbLog.Println("JOBNAME \t STATUS \t TYPE \t STARTAT \t ENDAT \t") tbLog.Printf("%s \t %s \t %s \t %s \t %s \n", job.Name, job.Status.Status, job.Spec.Type, job.Status.StartedAt, job.Status.EndedAt) } if err = tbw.Flush(); err != nil { diff --git a/internal/cli/cmd/cluster/create.go b/internal/cli/cmd/cluster/create.go index 5b5251300..89981cfc0 100644 --- a/internal/cli/cmd/cluster/create.go +++ b/internal/cli/cmd/cluster/create.go @@ -41,7 +41,7 @@ func NewCreateCmd() *cobra.Command { logger.Fatalln(err) } logger.Printf("Create OBCluster instance: %s", o.ClusterName) - logger.Printf("Run `echo $(kubectl get secret %s -o jsonpath='{.data.password}'|base64 --decode)` to get the secrets", obcluster.Spec.UserSecrets.Root) + logger.Printf("Run `echo $(kubectl get secret %s -n %s -o jsonpath='{.data.password}'|base64 --decode)` to get the secrets", obcluster.Spec.UserSecrets.Root, obcluster.Namespace) }, } o.AddFlags(cmd) diff --git a/internal/cli/cmd/demo/demo.go b/internal/cli/cmd/demo/demo.go index 13c28d19b..361a5124c 100644 --- a/internal/cli/cmd/demo/demo.go +++ b/internal/cli/cmd/demo/demo.go @@ -40,6 +40,7 @@ func NewCmd() *cobra.Command { var clusterType string var wait bool var err error + var prompt any cmd := &cobra.Command{ Use: "demo ", Short: "deploy demo ob cluster and tenant in easier way", @@ -47,13 +48,19 @@ func NewCmd() *cobra.Command { Args: cobra.NoArgs, PreRun: func(cmd *cobra.Command, args []string) { // prompt for cluster create options - prompt := pf.CreatePrompt(cluster.FLAG_NAME) - if clusterOptions.Name, err = pf.RunPromptE(prompt); err != nil { - logger.Fatalln(err) - } - prompt = pf.CreatePrompt(cluster.FLAG_NAMESPACE) - if clusterOptions.Namespace, err = pf.RunPromptE(prompt); err != nil { - logger.Fatalln(err) + for { + prompt = pf.CreatePrompt(cluster.FLAG_NAME) + if clusterOptions.Name, err = pf.RunPromptE(prompt); err != nil { + logger.Fatalln(err) + } + prompt = pf.CreatePrompt(cluster.FLAG_NAMESPACE) + if clusterOptions.Namespace, err = pf.RunPromptE(prompt); err != nil { + logger.Fatalln(err) + } + if !utils.CheckIfClusterExists(cmd.Context(), clusterOptions.Name, clusterOptions.Namespace) { + break + } + logger.Printf("Cluster %s already exists in namespace %s, please input another cluster name", clusterOptions.Name, clusterOptions.Namespace) } prompt = pf.CreatePrompt(cluster.CLUSTER_TYPE) if clusterType, err = pf.RunPromptE(prompt); err != nil { @@ -63,9 +70,17 @@ func NewCmd() *cobra.Command { if clusterOptions.RootPassword, err = pf.RunPromptE(prompt); err != nil { logger.Fatalln(err) } - prompt = pf.CreatePrompt(tenant.FLAG_TENANT_NAME_IN_K8S) - if tenantOptions.Name, err = pf.RunPromptE(prompt); err != nil { - logger.Fatalln(err) + + // prompt for tenant create options + for { + prompt = pf.CreatePrompt(tenant.FLAG_TENANT_NAME_IN_K8S) + if tenantOptions.Name, err = pf.RunPromptE(prompt); err != nil { + logger.Fatalln(err) + } + if !utils.CheckIfTenantExists(cmd.Context(), tenantOptions.Name, clusterOptions.Namespace) { + break + } + logger.Printf("Tenant %s already exists in namespace %s, please input another tenant name", tenantOptions.Name, clusterOptions.Namespace) } prompt = pf.CreatePrompt(tenant.FLAG_TENANT_NAME) if tenantOptions.TenantName, err = pf.RunPromptE(prompt); err != nil { @@ -91,7 +106,7 @@ func NewCmd() *cobra.Command { } logger.Printf("Creating OBCluster instance: %s", clusterOptions.ClusterName) waitForClusterReady(cmd.Context(), obcluster, logger, 2*time.Second) - logger.Printf("Run `echo $(kubectl get secret %s -clusterOptions jsonpath='{.data.password}'|base64 --decode)` to get clsuter secrets", obcluster.Spec.UserSecrets.Root) + logger.Printf("Run `echo $(kubectl get secret %s -n %s -o jsonpath='{.data.password}'|base64 --decode)` to get cluster secrets", obcluster.Spec.UserSecrets.Root, obcluster.Namespace) }, PostRun: func(cmd *cobra.Command, args []string) { // create tenant after cluster ready @@ -101,7 +116,7 @@ func NewCmd() *cobra.Command { } logger.Printf("Creating OBTenant instance: %s", tenantOptions.TenantName) waitForTenantReady(cmd.Context(), obtenant, logger, 1*time.Second) - logger.Printf("Run `echo $(kubectl get secret %s -o jsonpath='{.data.password}'|base64 --decode)` to get tenant secrets", obtenant.Spec.Credentials.Root) + logger.Printf("Run `echo $(kubectl get secret %s -n %s -o jsonpath='{.data.password}'|base64 --decode)` to get tenant secrets", obtenant.Spec.Credentials.Root, obtenant.Namespace) }, } // TODO: if w is set, wait for the cluster and tenant ready diff --git a/internal/cli/cmd/tenant/create.go b/internal/cli/cmd/tenant/create.go index c67dafe4c..dfd608d75 100644 --- a/internal/cli/cmd/tenant/create.go +++ b/internal/cli/cmd/tenant/create.go @@ -41,7 +41,7 @@ func NewCreateCmd() *cobra.Command { logger.Fatalln(err) } logger.Printf("Create OBTenant instance: %s", o.TenantName) - logger.Printf("Run `echo $(kubectl get secret %s -o jsonpath='{.data.password}'|base64 --decode)` to get the secrets", obtenant.Spec.Credentials.Root) + logger.Printf("Run `echo $(kubectl get secret %s -n %s -o jsonpath='{.data.password}'|base64 --decode)` to get the secrets", obtenant.Spec.Credentials.Root, obtenant.Namespace) }, } o.AddFlags(cmd) diff --git a/internal/cli/demo/prompt.go b/internal/cli/demo/prompt.go index ef1627037..b8d10ab10 100644 --- a/internal/cli/demo/prompt.go +++ b/internal/cli/demo/prompt.go @@ -39,6 +39,7 @@ var ( } ) +// PromptFactory is a factory to create prompt type PromptFactory struct { promptTepl *promptui.PromptTemplates selectTepl *promptui.SelectTemplates @@ -114,7 +115,7 @@ func (pf *PromptFactory) CreatePrompt(promptType string) any { if input == "" { return nil } - if utils.CheckPassword(input) { + if !utils.CheckPassword(input) { return errors.New("invalid password") } return nil diff --git a/internal/cli/install/install.go b/internal/cli/install/install.go index 920565fd4..b2d6a1da0 100644 --- a/internal/cli/install/install.go +++ b/internal/cli/install/install.go @@ -129,7 +129,7 @@ func addHelmRepo() error { // updateHelmRepo update ob-operator helm repo func updateHelmRepo() error { - cmdUpdateRepo := exec.Command("helm", "repo", "update", "ob-operator") + cmdUpdateRepo := exec.Command("helm", "repo", "update") output, err := cmdUpdateRepo.CombinedOutput() if err != nil { return fmt.Errorf("updating repo failed: %s, %s", err, output) diff --git a/internal/cli/tenant/create.go b/internal/cli/tenant/create.go index 6eb69c966..269f59e80 100644 --- a/internal/cli/tenant/create.go +++ b/internal/cli/tenant/create.go @@ -116,12 +116,6 @@ func (o *CreateOptions) Complete() error { if o.RestoreType != "" { o.RestoreType = strings.ToUpper(o.RestoreType) } - if o.RestoreType == "NFS" && o.Source.Restore.BakDataSource == "" { - o.Source.Restore.BakDataSource = fmt.Sprintf("%s/%s", "backup", o.From) - } - if o.RestoreType == "NFS" && o.Source.Restore.ArchiveSource == "" { - o.Source.Restore.ArchiveSource = fmt.Sprintf("%s/%s", "archive", o.From) - } return nil } @@ -141,14 +135,17 @@ func (o *CreateOptions) Validate() error { if !utils.CheckTenantName(o.TenantName) { return fmt.Errorf("invalid tenant name: %s, the first letter must be a letter or an underscore and cannot contain -", o.TenantName) } + if o.Restore && o.Source == nil { + return errors.New("source tenant is not specified") + } if o.Restore && o.RestoreType != "OSS" && o.RestoreType != "NFS" { - return errors.New("Restore Type not supported") + return errors.New("restore Type is not supported") } if o.Restore && o.RestoreType == "OSS" && o.Source.Restore.OSSAccessKey == "" { - return errors.New("oss access key not specified") + return errors.New("oss access key is not specified") } if o.Restore && o.RestoreType == "NFS" && o.Source.Restore.BakEncryptionPassword == "" { - return errors.New("back encryption password not specified") + return errors.New("back encryption password is not specified") } return nil } @@ -401,6 +398,7 @@ func (o *CreateOptions) AddFlags(cmd *cobra.Command) { func (o *CreateOptions) SetRequiredFlags(cmd *cobra.Command) { _ = cmd.MarkFlagRequired(FLAG_CLUSTER_NAME) _ = cmd.MarkFlagRequired(FLAG_ZONE_PRIORITY) + cmd.MarkFlagsRequiredTogether(FLAG_RESTORE, FLAG_ARCHIVE_SOURCE, FLAG_BAK_DATA_SOURCE) } // AddBaseFlags add base flags diff --git a/internal/cli/utils/checker.go b/internal/cli/utils/checker.go index 81f43c329..abfd81293 100644 --- a/internal/cli/utils/checker.go +++ b/internal/cli/utils/checker.go @@ -15,11 +15,15 @@ package utils import ( "bytes" + "context" "fmt" "os/exec" + "k8s.io/apimachinery/pkg/types" + apitypes "github.com/oceanbase/ob-operator/api/types" "github.com/oceanbase/ob-operator/api/v1alpha1" + "github.com/oceanbase/ob-operator/internal/clients" clusterstatus "github.com/oceanbase/ob-operator/internal/const/status/obcluster" "github.com/oceanbase/ob-operator/internal/const/status/tenantstatus" ) @@ -59,6 +63,22 @@ var ( localPathProvisionerResources = "local-path-provisioner" ) +// CheckIfClusterExists checks if cluster exists in the environment +func CheckIfClusterExists(ctx context.Context, name string, namespace string) bool { + cluster, _ := clients.GetOBCluster(ctx, namespace, name) + return cluster != nil +} + +// CheckIfTenantExists checks if tenant exists in the environment +func CheckIfTenantExists(ctx context.Context, name string, namespace string) bool { + nn := types.NamespacedName{ + Name: name, + Namespace: namespace, + } + tenant, _ := clients.GetOBTenant(ctx, nn) + return tenant != nil +} + // CheckTenantStatus checks running status of obtenant func CheckTenantStatus(tenant *v1alpha1.OBTenant) error { if tenant.Status.Status != tenantstatus.Running { diff --git a/internal/cli/utils/validator.go b/internal/cli/utils/validator.go index 31cb75798..31346272c 100644 --- a/internal/cli/utils/validator.go +++ b/internal/cli/utils/validator.go @@ -16,6 +16,7 @@ package utils import ( "regexp" "strings" + "unicode" ) const ( @@ -42,11 +43,11 @@ func CheckPassword(password string) bool { for _, char := range password { if strings.ContainsRune(characters, char) { switch { - case strings.ContainsRune("ABCDEFGHIJKLMNOPQRSTUVWXYZ", char): + case unicode.IsUpper(char): countUppercase++ - case strings.ContainsRune("abcdefghijklmnopqrstuvwxyz", char): + case unicode.IsLower(char): countLowercase++ - case strings.ContainsRune("0123456789", char): + case unicode.IsNumber(char): countNumber++ default: countSpecialChar++ @@ -54,7 +55,7 @@ func CheckPassword(password string) bool { } else { return false } - // if satisfied + // if satisfied, early return if countUppercase >= 2 && countLowercase >= 2 && countNumber >= 2 && countSpecialChar >= 2 { return true } diff --git a/internal/dashboard/business/oceanbase/obcluster.go b/internal/dashboard/business/oceanbase/obcluster.go index 7f0c32e52..6c0251f67 100644 --- a/internal/dashboard/business/oceanbase/obcluster.go +++ b/internal/dashboard/business/oceanbase/obcluster.go @@ -20,7 +20,6 @@ import ( "github.com/google/uuid" "github.com/pkg/errors" - "github.com/sirupsen/logrus" logger "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" apiresource "k8s.io/apimachinery/pkg/api/resource" @@ -896,17 +895,17 @@ func ListOBClusterParameters(ctx context.Context, nn *param.K8sObjectIdentity) ( LabelSelector: fmt.Sprintf("%s=%s", oceanbaseconst.LabelRefOBCluster, nn.Name), }) if err != nil { - logrus.WithError(err).Error("Failed to list observers") + logger.WithError(err).Error("Failed to list observers") return nil, errors.Wrap(err, "List observers") } conn, err := utils.GetOBConnection(ctx, obcluster, "root", "sys", obcluster.Spec.UserSecrets.Root) if err != nil { - logrus.Info("Failed to get OceanBase database connection") + logger.Info("Failed to get OceanBase database connection") return nil, errors.Wrap(err, "Get OceanBase database connection") } parameters, err := conn.ListParametersWithTenantID(ctx, 1) if err != nil { - logrus.WithError(err).Error("Failed to query parameters") + logger.WithError(err).Error("Failed to query parameters") return nil, errors.Wrap(err, "Query parameters") } return parameters, nil