From 00f3c39695ae2a5f5e957de2971087edfe0be748 Mon Sep 17 00:00:00 2001 From: Joseph Date: Mon, 4 Aug 2025 14:11:46 -0400 Subject: [PATCH 1/9] Refactor non-admin backup storage location commands - Introduced new subcommands for managing backup storage location requests: approve, deny, describe, and get. - Updated the create command to enhance its functionality and examples. - Improved help text for commands to provide clearer guidance on usage. - Added tests to ensure help messages are accurate and comprehensive. - Enhanced the overall structure and organization of the non-admin command set. --- cmd/non-admin/bsl/bsl.go | 7 +- cmd/non-admin/bsl/create.go | 138 ++++++++++------ cmd/non-admin/bsl/request.go | 40 +++++ cmd/non-admin/bsl/request_approve.go | 169 ++++++++++++++++++++ cmd/non-admin/bsl/request_describe.go | 196 +++++++++++++++++++++++ cmd/non-admin/bsl/request_get.go | 221 ++++++++++++++++++++++++++ cmd/non-admin/bsl/request_reject.go | 171 ++++++++++++++++++++ cmd/non-admin/nonadmin.go | 3 +- tests/help_test.go | 69 ++++++++ 9 files changed, 960 insertions(+), 54 deletions(-) create mode 100644 cmd/non-admin/bsl/request.go create mode 100644 cmd/non-admin/bsl/request_approve.go create mode 100644 cmd/non-admin/bsl/request_describe.go create mode 100644 cmd/non-admin/bsl/request_get.go create mode 100644 cmd/non-admin/bsl/request_reject.go diff --git a/cmd/non-admin/bsl/bsl.go b/cmd/non-admin/bsl/bsl.go index 3de5b796..a593c3b9 100644 --- a/cmd/non-admin/bsl/bsl.go +++ b/cmd/non-admin/bsl/bsl.go @@ -25,12 +25,13 @@ import ( func NewBSLCommand(f client.Factory) *cobra.Command { c := &cobra.Command{ Use: "bsl", - Short: "Work with non-admin backup storage locations", - Long: "Work with non-admin backup storage locations", + Short: "Create and manage backup storage locations", + Long: "Create and manage non-admin backup storage locations and their approval requests", } c.AddCommand( - NewCreateCommand(f, "create"), + NewCreateCommand(f), + NewRequestCommand(f), ) return c diff --git a/cmd/non-admin/bsl/create.go b/cmd/non-admin/bsl/create.go index 5c571944..868a6894 100644 --- a/cmd/non-admin/bsl/create.go +++ b/cmd/non-admin/bsl/create.go @@ -30,14 +30,15 @@ import ( "github.com/vmware-tanzu/velero/pkg/client" "github.com/vmware-tanzu/velero/pkg/cmd" "github.com/vmware-tanzu/velero/pkg/cmd/util/output" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func NewCreateCommand(f client.Factory, use string) *cobra.Command { +func NewCreateCommand(f client.Factory) *cobra.Command { o := NewCreateOptions() c := &cobra.Command{ - Use: use + " NAME", + Use: "create NAME", Short: "Create a non-admin backup storage location", Args: cobra.ExactArgs(1), Run: func(c *cobra.Command, args []string) { @@ -45,17 +46,28 @@ func NewCreateCommand(f client.Factory, use string) *cobra.Command { cmd.CheckError(o.Validate(c, args, f)) cmd.CheckError(o.Run(c, f)) }, - Example: ` # Create a non-admin backup storage location - kubectl oadp nonadmin bsl create my-bsl --backup-storage-location default - - # Create a non-admin backup storage location with specific namespace - kubectl oadp nonadmin bsl create my-bsl --backup-storage-location aws-bsl --namespace my-namespace - - # Create with custom BSL namespace (if OADP operator is not in openshift-adp) - kubectl oadp nonadmin bsl create my-bsl --backup-storage-location default --bsl-namespace velero - - # View the YAML for a non-admin backup storage location without sending it to the server - kubectl oadp nonadmin bsl create my-bsl --backup-storage-location default -o yaml`, + Example: ` # Create a non-admin backup storage location for AWS + kubectl oadp nonadmin bsl create my-storage \ + --provider aws \ + --bucket my-velero-bucket \ + --credential-name cloud-credentials \ + --region us-east-1 + + # Create with prefix for organizing backups + kubectl oadp nonadmin bsl create my-storage \ + --provider aws \ + --bucket my-velero-bucket \ + --prefix velero-backups \ + --credential-name cloud-credentials \ + --region us-east-1 + + # View the YAML without creating the resource + kubectl oadp nonadmin bsl create my-storage \ + --provider aws \ + --bucket my-bucket \ + --credential-name cloud-credentials \ + --region us-east-1 \ + -o yaml`, } o.BindFlags(c.Flags()) @@ -66,89 +78,115 @@ func NewCreateCommand(f client.Factory, use string) *cobra.Command { } type CreateOptions struct { - Name string - BackupStorageLocation string - NonAdminNamespace string - BSLNamespace string - client kbclient.WithWatch + Name string + Namespace string + Provider string + Bucket string + Prefix string + CredentialName string + Region string + Config map[string]string + client kbclient.WithWatch } func NewCreateOptions() *CreateOptions { return &CreateOptions{ - BSLNamespace: "openshift-adp", // Default OADP operator namespace + Config: make(map[string]string), } } func (o *CreateOptions) BindFlags(flags *pflag.FlagSet) { - flags.StringVar(&o.BackupStorageLocation, "backup-storage-location", "", "Name of the BackupStorageLocation to reference.") - flags.StringVar(&o.NonAdminNamespace, "namespace", "", "Namespace for the NonAdminBackupStorageLocation (defaults to current context namespace).") - flags.StringVar(&o.BSLNamespace, "bsl-namespace", "openshift-adp", "Namespace where the BackupStorageLocation exists.") + flags.StringVar(&o.Provider, "provider", "", "Storage provider (required). Examples: aws, azure, gcp") + flags.StringVar(&o.Bucket, "bucket", "", "Object storage bucket name (required)") + flags.StringVar(&o.Prefix, "prefix", "", "Prefix for backup objects in the bucket") + flags.StringVar(&o.CredentialName, "credential-name", "", "Name of the credential secret (required)") + flags.StringVar(&o.Region, "region", "", "Storage region (required for some providers like AWS)") + flags.StringToStringVar(&o.Config, "config", nil, "Additional provider-specific configuration (key=value pairs)") } func (o *CreateOptions) Complete(args []string, f client.Factory) error { o.Name = args[0] - // Create client with Velero scheme for BackupStorageLocation access - client, err := shared.NewClientWithScheme(f, shared.ClientOptions{ - IncludeVeleroTypes: true, - }) + // Create client with full scheme including NonAdmin and Velero types + client, err := shared.NewClientWithFullScheme(f) if err != nil { return err } o.client = client - if o.NonAdminNamespace == "" { - namespace := f.Namespace() - o.NonAdminNamespace = namespace + // Get the current namespace + currentNS, err := shared.GetCurrentNamespace() + if err != nil { + return fmt.Errorf("failed to determine current namespace: %w", err) } + o.Namespace = currentNS return nil } func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { - if o.BackupStorageLocation == "" { - return fmt.Errorf("--backup-storage-location is required") + if o.Provider == "" { + return fmt.Errorf("--provider is required") + } + if o.Bucket == "" { + return fmt.Errorf("--bucket is required") + } + if o.CredentialName == "" { + return fmt.Errorf("--credential-name is required") } return nil } func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error { - // If we have a BackupStorageLocation name, we need to fetch its spec - var bslSpec *velerov1.BackupStorageLocationSpec - if o.BackupStorageLocation != "" { - // Get the existing BackupStorageLocation to copy its spec - existingBSL := &velerov1.BackupStorageLocation{} - err := o.client.Get(context.Background(), kbclient.ObjectKey{ - Name: o.BackupStorageLocation, - Namespace: o.BSLNamespace, // Use the BSLNamespace flag - }, existingBSL) - if err != nil { - return fmt.Errorf("failed to get BackupStorageLocation %q: %w", o.BackupStorageLocation, err) - } - bslSpec = &existingBSL.Spec + // Build config map + config := make(map[string]string) + if o.Region != "" { + config["region"] = o.Region + } + // Add any additional config provided via --config flag + for k, v := range o.Config { + config[k] = v } - bsl := &nacv1alpha1.NonAdminBackupStorageLocation{ + // Create the NABSL + nabsl := &nacv1alpha1.NonAdminBackupStorageLocation{ ObjectMeta: metav1.ObjectMeta{ Name: o.Name, - Namespace: o.NonAdminNamespace, + Namespace: o.Namespace, }, Spec: nacv1alpha1.NonAdminBackupStorageLocationSpec{ - BackupStorageLocationSpec: bslSpec, + BackupStorageLocationSpec: &velerov1.BackupStorageLocationSpec{ + Provider: o.Provider, + Config: config, + Credential: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: o.CredentialName, + }, + Key: "cloud", // Standard key name for cloud credentials + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: o.Bucket, + Prefix: o.Prefix, + }, + }, + }, }, } - if printed, err := output.PrintWithFormat(c, bsl); printed || err != nil { + if printed, err := output.PrintWithFormat(c, nabsl); printed || err != nil { return err } - err := o.client.Create(context.Background(), bsl) + err := o.client.Create(context.Background(), nabsl) if err != nil { return err } - fmt.Printf("NonAdminBackupStorageLocation %q created successfully.\n", bsl.Name) + fmt.Printf("NonAdminBackupStorageLocation %q created successfully.\n", nabsl.Name) + fmt.Printf("The controller will create a request for admin approval.\n") + fmt.Printf("Use 'kubectl oadp nonadmin bsl request get' to view auto-created requests.\n") return nil } diff --git a/cmd/non-admin/bsl/request.go b/cmd/non-admin/bsl/request.go new file mode 100644 index 00000000..2889ee72 --- /dev/null +++ b/cmd/non-admin/bsl/request.go @@ -0,0 +1,40 @@ +/* +Copyright 2025 The OADP CLI 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 bsl + +import ( + "github.com/spf13/cobra" + "github.com/vmware-tanzu/velero/pkg/client" +) + +// NewRequestCommand creates the "request" subcommand under bsl +func NewRequestCommand(f client.Factory) *cobra.Command { + c := &cobra.Command{ + Use: "request", + Short: "Manage backup storage location approval requests", + Long: "View and manage approval requests for backup storage locations. Requests are automatically created when users create backup storage locations and require admin approval.", + } + + c.AddCommand( + NewRequestGetCommand(f), + NewRequestDescribeCommand(f), + NewRequestApproveCommand(f), + NewRequestDenyCommand(f), + ) + + return c +} diff --git a/cmd/non-admin/bsl/request_approve.go b/cmd/non-admin/bsl/request_approve.go new file mode 100644 index 00000000..e1d8a124 --- /dev/null +++ b/cmd/non-admin/bsl/request_approve.go @@ -0,0 +1,169 @@ +/* +Copyright 2025 The OADP CLI 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 bsl + +import ( + "context" + "fmt" + + "github.com/spf13/cobra" + "github.com/spf13/pflag" + kbclient "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/migtools/oadp-cli/cmd/shared" + nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" + "github.com/vmware-tanzu/velero/pkg/client" + "github.com/vmware-tanzu/velero/pkg/cmd" +) + +// NewRequestApproveCommand creates the "approve" subcommand under bsl request +func NewRequestApproveCommand(f client.Factory) *cobra.Command { + o := NewRequestApproveOptions() + + c := &cobra.Command{ + Use: "approve REQUEST_NAME", + Short: "Approve a pending backup storage location request", + Long: "Approve a pending backup storage location request to allow the controller to create the corresponding BackupStorageLocation", + Args: cobra.ExactArgs(1), + Example: ` # Approve a request by NABSL name (admin access required) + kubectl oadp nonadmin bsl request approve user-test-bsl + + # Approve a request by UUID with reason + kubectl oadp nonadmin bsl request approve nacuser01-user-test-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 --reason "Approved for production use"`, + Run: func(c *cobra.Command, args []string) { + cmd.CheckError(o.Complete(args, f)) + cmd.CheckError(o.Validate(c, args, f)) + cmd.CheckError(o.Run(c, f)) + }, + } + + o.BindFlags(c.Flags()) + + return c +} + +type RequestApproveOptions struct { + RequestName string + Reason string + client kbclient.WithWatch +} + +func NewRequestApproveOptions() *RequestApproveOptions { + return &RequestApproveOptions{} +} + +func (o *RequestApproveOptions) BindFlags(flags *pflag.FlagSet) { + flags.StringVar(&o.Reason, "reason", "", "Reason for approval (optional)") +} + +func (o *RequestApproveOptions) Complete(args []string, f client.Factory) error { + o.RequestName = args[0] + + client, err := shared.NewClientWithScheme(f, shared.ClientOptions{ + IncludeVeleroTypes: true, + IncludeNonAdminTypes: true, + }) + if err != nil { + return err + } + + o.client = client + return nil +} + +func (o *RequestApproveOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { + return nil +} + +func (o *RequestApproveOptions) Run(c *cobra.Command, f client.Factory) error { + // Find the request either by UUID or by looking up NABSL name + requestName, err := o.findRequestName() + if err != nil { + return err + } + + // Get the current request + var request nacv1alpha1.NonAdminBackupStorageLocationRequest + err = o.client.Get(context.Background(), kbclient.ObjectKey{ + Name: requestName, + Namespace: "openshift-adp", + }, &request) + if err != nil { + return fmt.Errorf("failed to get request %q: %w", requestName, err) + } + + // Check if already approved + if request.Spec.ApprovalDecision == "approve" { + fmt.Printf("Request %q is already approved.\n", o.RequestName) + return nil + } + + // Update the approval decision + request.Spec.ApprovalDecision = "approve" + if o.Reason != "" { + if request.Annotations == nil { + request.Annotations = make(map[string]string) + } + request.Annotations["openshift.io/oadp-approval-reason"] = o.Reason + } + + err = o.client.Update(context.Background(), &request) + if err != nil { + return fmt.Errorf("failed to approve request: %w", err) + } + + // Get the NABSL name for user-friendly output + nabslName := o.RequestName + if request.Status.SourceNonAdminBSL != nil { + nabslName = request.Status.SourceNonAdminBSL.Name + } + + fmt.Printf("Request for NonAdminBackupStorageLocation %q has been approved.\n", nabslName) + fmt.Printf("The controller will now create the corresponding BackupStorageLocation.\n") + + return nil +} + +func (o *RequestApproveOptions) findRequestName() (string, error) { + // First check if o.RequestName is already a UUID by trying to get it directly + var testRequest nacv1alpha1.NonAdminBackupStorageLocationRequest + err := o.client.Get(context.Background(), kbclient.ObjectKey{ + Name: o.RequestName, + Namespace: "openshift-adp", + }, &testRequest) + if err == nil { + // Found it directly, it's a UUID + return o.RequestName, nil + } + + // Not found directly, so o.RequestName might be a NABSL name + // We need to search through all requests to find one with matching source NABSL name + var requestList nacv1alpha1.NonAdminBackupStorageLocationRequestList + err = o.client.List(context.Background(), &requestList, kbclient.InNamespace("openshift-adp")) + if err != nil { + return "", fmt.Errorf("failed to list requests: %w", err) + } + + for _, request := range requestList.Items { + if request.Status.SourceNonAdminBSL != nil && + request.Status.SourceNonAdminBSL.Name == o.RequestName { + return request.Name, nil + } + } + + return "", fmt.Errorf("request for NABSL %q not found", o.RequestName) +} diff --git a/cmd/non-admin/bsl/request_describe.go b/cmd/non-admin/bsl/request_describe.go new file mode 100644 index 00000000..81fe34a4 --- /dev/null +++ b/cmd/non-admin/bsl/request_describe.go @@ -0,0 +1,196 @@ +/* +Copyright 2025 The OADP CLI 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 bsl + +import ( + "context" + "fmt" + "sort" + "strings" + + "github.com/spf13/cobra" + kbclient "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/migtools/oadp-cli/cmd/shared" + nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" + "github.com/vmware-tanzu/velero/pkg/client" + "github.com/vmware-tanzu/velero/pkg/cmd" +) + +func NewRequestDescribeCommand(f client.Factory) *cobra.Command { + o := NewRequestDescribeOptions() + + c := &cobra.Command{ + Use: "describe NAME", + Short: "Describe a non-admin backup storage location request", + Args: cobra.ExactArgs(1), + Run: func(c *cobra.Command, args []string) { + cmd.CheckError(o.Complete(args, f)) + cmd.CheckError(o.Validate(c, args, f)) + cmd.CheckError(o.Run(c, f)) + }, + Example: ` # Describe a request by NABSL name + kubectl oadp nonadmin bsl request describe my-bsl-request + + # Describe a request by UUID + kubectl oadp nonadmin bsl request describe nacuser01-my-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8`, + } + + return c +} + +type RequestDescribeOptions struct { + Name string + client kbclient.WithWatch +} + +func NewRequestDescribeOptions() *RequestDescribeOptions { + return &RequestDescribeOptions{} +} + +func (o *RequestDescribeOptions) Complete(args []string, f client.Factory) error { + o.Name = args[0] + + client, err := shared.NewClientWithScheme(f, shared.ClientOptions{ + IncludeVeleroTypes: true, + IncludeNonAdminTypes: true, + }) + if err != nil { + return err + } + + o.client = client + return nil +} + +func (o *RequestDescribeOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { + return nil +} + +func (o *RequestDescribeOptions) Run(c *cobra.Command, f client.Factory) error { + // Get the current namespace to find user's NABSLs + currentNS, err := shared.GetCurrentNamespace() + if err != nil { + return fmt.Errorf("failed to determine current namespace: %w", err) + } + + // First get all NABSLs in user's namespace to find related requests + var nabslList nacv1alpha1.NonAdminBackupStorageLocationList + err = o.client.List(context.Background(), &nabslList, kbclient.InNamespace(currentNS)) + if err != nil { + return fmt.Errorf("failed to list NABSLs: %w", err) + } + + // Find the target UUID for the request + var targetUUID string + for _, nabsl := range nabslList.Items { + if nabsl.Status.VeleroBackupStorageLocation != nil && nabsl.Status.VeleroBackupStorageLocation.NACUUID != "" { + uuid := nabsl.Status.VeleroBackupStorageLocation.NACUUID + // Check if o.Name matches the UUID or NABSL name + if uuid == o.Name || nabsl.Name == o.Name { + targetUUID = uuid + break + } + } + } + + if targetUUID == "" { + return fmt.Errorf("request %q not found for NABSLs in namespace %s", o.Name, currentNS) + } + + // Get the request from openshift-adp namespace using the UUID + var request nacv1alpha1.NonAdminBackupStorageLocationRequest + err = o.client.Get(context.Background(), kbclient.ObjectKey{ + Name: targetUUID, + Namespace: "openshift-adp", + }, &request) + if err != nil { + return fmt.Errorf("failed to get request for %q: %w", o.Name, err) + } + + return describeRequest(&request) +} + +func describeRequest(request *nacv1alpha1.NonAdminBackupStorageLocationRequest) error { + fmt.Printf("Name:\t%s\n", request.Name) + fmt.Printf("Namespace:\t%s\n", request.Namespace) + + fmt.Printf("Labels:\t%s\n", formatLabels(request.Labels)) + fmt.Printf("Annotations:\t%s\n", formatLabels(request.Annotations)) + + fmt.Printf("Phase:\t%s\n", request.Status.Phase) + + if request.Spec.ApprovalDecision != "" { + fmt.Printf("Approval Decision:\t%s\n", request.Spec.ApprovalDecision) + } + + if request.Status.SourceNonAdminBSL != nil { + source := request.Status.SourceNonAdminBSL + fmt.Printf("Requested NonAdminBackupStorageLocation:\n") + fmt.Printf(" Name:\t%s\n", source.Name) + fmt.Printf(" Namespace:\t%s\n", source.Namespace) + + if source.NACUUID != "" { + fmt.Printf(" NACUUID:\t%s\n", source.NACUUID) + } + + if source.RequestedSpec != nil { + spec := source.RequestedSpec + fmt.Printf("Requested BackupStorageLocation Spec:\n") + fmt.Printf(" Provider:\t%s\n", spec.Provider) + fmt.Printf(" Object Storage Bucket:\t%s\n", spec.ObjectStorage.Bucket) + + if spec.ObjectStorage.Prefix != "" { + fmt.Printf(" Prefix:\t%s\n", spec.ObjectStorage.Prefix) + } + + if len(spec.Config) > 0 { + fmt.Printf(" Config:\t%s\n", formatLabels(spec.Config)) + } + + if spec.AccessMode != "" { + fmt.Printf(" Access Mode:\t%s\n", spec.AccessMode) + } + + if spec.BackupSyncPeriod != nil { + fmt.Printf(" Backup Sync Period:\t%s\n", spec.BackupSyncPeriod.String()) + } + + if spec.ValidationFrequency != nil { + fmt.Printf(" Validation Frequency:\t%s\n", spec.ValidationFrequency.String()) + } + } + } + + fmt.Printf("Creation Timestamp:\t%s\n", request.CreationTimestamp.String()) + + return nil +} + +// formatLabels formats a map of labels into a string +func formatLabels(labels map[string]string) string { + if len(labels) == 0 { + return "" + } + + var pairs []string + for key, value := range labels { + pairs = append(pairs, fmt.Sprintf("%s=%s", key, value)) + } + sort.Strings(pairs) + return strings.Join(pairs, ",") +} diff --git a/cmd/non-admin/bsl/request_get.go b/cmd/non-admin/bsl/request_get.go new file mode 100644 index 00000000..ac6be78a --- /dev/null +++ b/cmd/non-admin/bsl/request_get.go @@ -0,0 +1,221 @@ +/* +Copyright 2025 The OADP CLI 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 bsl + +import ( + "context" + "fmt" + "os" + "text/tabwriter" + + "github.com/spf13/cobra" + "github.com/spf13/pflag" + kbclient "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/migtools/oadp-cli/cmd/shared" + nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" + "github.com/vmware-tanzu/velero/pkg/client" + "github.com/vmware-tanzu/velero/pkg/cmd" + "github.com/vmware-tanzu/velero/pkg/cmd/util/output" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func NewRequestGetCommand(f client.Factory) *cobra.Command { + o := NewRequestGetOptions() + + c := &cobra.Command{ + Use: "get [NAME]", + Short: "Get non-admin backup storage location requests", + Args: cobra.MaximumNArgs(1), + Run: func(c *cobra.Command, args []string) { + cmd.CheckError(o.Complete(args, f)) + cmd.CheckError(o.Validate(c, args, f)) + cmd.CheckError(o.Run(c, f)) + }, + Example: ` # Get all your backup storage location requests + kubectl oadp nonadmin bsl request get + + # Get a specific request by NABSL name + kubectl oadp nonadmin bsl request get my-bsl-request + + # Get a specific request by UUID + kubectl oadp nonadmin bsl request get nacuser01-my-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 + + # Get output in YAML format + kubectl oadp nonadmin bsl request get my-bsl-request -o yaml`, + } + + o.BindFlags(c.Flags()) + output.BindFlags(c.Flags()) + output.ClearOutputFlagDefault(c) + + return c +} + +type RequestGetOptions struct { + Name string + AllNamespaces bool + client kbclient.WithWatch +} + +func NewRequestGetOptions() *RequestGetOptions { + return &RequestGetOptions{} +} + +func (o *RequestGetOptions) BindFlags(flags *pflag.FlagSet) { + flags.BoolVar(&o.AllNamespaces, "all-namespaces", false, "If present, list requests across all namespaces") +} + +func (o *RequestGetOptions) Complete(args []string, f client.Factory) error { + if len(args) > 0 { + o.Name = args[0] + } + + client, err := shared.NewClientWithScheme(f, shared.ClientOptions{ + IncludeVeleroTypes: true, + IncludeNonAdminTypes: true, + }) + if err != nil { + return err + } + + o.client = client + return nil +} + +func (o *RequestGetOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { + return nil +} + +func (o *RequestGetOptions) Run(c *cobra.Command, f client.Factory) error { + // Get the current namespace to find user's NABSLs + currentNS, err := shared.GetCurrentNamespace() + if err != nil { + return fmt.Errorf("failed to determine current namespace: %w", err) + } + + // First get all NABSLs in user's namespace to find related requests + var nabslList nacv1alpha1.NonAdminBackupStorageLocationList + err = o.client.List(context.Background(), &nabslList, kbclient.InNamespace(currentNS)) + if err != nil { + return fmt.Errorf("failed to list NABSLs: %w", err) + } + + // Collect request UUIDs from NABSL statuses + requestUUIDs := make(map[string]string) // UUID -> NABSL name + for _, nabsl := range nabslList.Items { + if nabsl.Status.VeleroBackupStorageLocation != nil && nabsl.Status.VeleroBackupStorageLocation.NACUUID != "" { + requestUUIDs[nabsl.Status.VeleroBackupStorageLocation.NACUUID] = nabsl.Name + } + } + + if o.Name != "" { + // Get specific request by UUID or NABSL name + var targetUUID string + + // Check if o.Name is a UUID or NABSL name + if _, exists := requestUUIDs[o.Name]; exists { + // o.Name is a UUID + targetUUID = o.Name + } else { + // o.Name might be a NABSL name, find its UUID + for uuid, nabslName := range requestUUIDs { + if nabslName == o.Name { + targetUUID = uuid + break + } + } + } + + if targetUUID != "" { + var request nacv1alpha1.NonAdminBackupStorageLocationRequest + err := o.client.Get(context.Background(), kbclient.ObjectKey{ + Name: targetUUID, + Namespace: "openshift-adp", + }, &request) + if err != nil { + return fmt.Errorf("failed to get request for %q: %w", o.Name, err) + } + + if printed, err := output.PrintWithFormat(c, &request); printed || err != nil { + return err + } + + list := &nacv1alpha1.NonAdminBackupStorageLocationRequestList{ + Items: []nacv1alpha1.NonAdminBackupStorageLocationRequest{request}, + } + return printRequestTable(list) + } + + return fmt.Errorf("request %q not found for NABSLs in namespace %s", o.Name, currentNS) + } + + // List all requests related to user's NABSLs + var userRequests []nacv1alpha1.NonAdminBackupStorageLocationRequest + for uuid := range requestUUIDs { + var request nacv1alpha1.NonAdminBackupStorageLocationRequest + err := o.client.Get(context.Background(), kbclient.ObjectKey{ + Name: uuid, + Namespace: "openshift-adp", + }, &request) + if err != nil { + // Request might not exist yet, skip + continue + } + userRequests = append(userRequests, request) + } + + requestList := &nacv1alpha1.NonAdminBackupStorageLocationRequestList{ + Items: userRequests, + } + + if printed, err := output.PrintWithFormat(c, requestList); printed || err != nil { + return err + } + + return printRequestTable(requestList) +} + +func printRequestTable(requestList *nacv1alpha1.NonAdminBackupStorageLocationRequestList) error { + w := tabwriter.NewWriter(os.Stdout, 0, 8, 2, ' ', 0) + defer w.Flush() + + // Print header + fmt.Fprintln(w, "NAME\tNAMESPACE\tPHASE\tREQUESTED-NABSL\tREQUESTED-NAMESPACE\tAGE") + + for _, request := range requestList.Items { + age := metav1.Now().Sub(request.CreationTimestamp.Time) + + requestedNABSL := "" + requestedNamespace := "" + if request.Status.SourceNonAdminBSL != nil { + requestedNABSL = request.Status.SourceNonAdminBSL.Name + requestedNamespace = request.Status.SourceNonAdminBSL.Namespace + } + + fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%s\n", + request.Name, + request.Namespace, + request.Status.Phase, + requestedNABSL, + requestedNamespace, + age.Round(1e9).String(), + ) + } + + return nil +} diff --git a/cmd/non-admin/bsl/request_reject.go b/cmd/non-admin/bsl/request_reject.go new file mode 100644 index 00000000..24194ec4 --- /dev/null +++ b/cmd/non-admin/bsl/request_reject.go @@ -0,0 +1,171 @@ +/* +Copyright 2025 The OADP CLI 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 bsl + +import ( + "context" + "fmt" + + "github.com/spf13/cobra" + "github.com/spf13/pflag" + kbclient "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/migtools/oadp-cli/cmd/shared" + nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" + "github.com/vmware-tanzu/velero/pkg/client" + "github.com/vmware-tanzu/velero/pkg/cmd" +) + +// NewRequestDenyCommand creates the "deny" subcommand under bsl request +func NewRequestDenyCommand(f client.Factory) *cobra.Command { + o := NewRequestDenyOptions() + + c := &cobra.Command{ + Use: "deny REQUEST_NAME", + Short: "Deny a pending backup storage location request", + Long: "Deny a pending backup storage location request to reject the user's request for a backup storage location", + Args: cobra.ExactArgs(1), + Example: ` # Deny a request by NABSL name (admin access required) + kubectl oadp nonadmin bsl request deny user-test-bsl --reason "Invalid configuration" + + # Deny a request by UUID with detailed reason + kubectl oadp nonadmin bsl request deny nacuser01-user-test-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 --reason "Bucket does not exist in specified region"`, + Run: func(c *cobra.Command, args []string) { + cmd.CheckError(o.Complete(args, f)) + cmd.CheckError(o.Validate(c, args, f)) + cmd.CheckError(o.Run(c, f)) + }, + } + + o.BindFlags(c.Flags()) + + return c +} + +type RequestDenyOptions struct { + RequestName string + Reason string + client kbclient.WithWatch +} + +func NewRequestDenyOptions() *RequestDenyOptions { + return &RequestDenyOptions{} +} + +func (o *RequestDenyOptions) BindFlags(flags *pflag.FlagSet) { + flags.StringVar(&o.Reason, "reason", "", "Reason for denial (recommended)") +} + +func (o *RequestDenyOptions) Complete(args []string, f client.Factory) error { + o.RequestName = args[0] + + client, err := shared.NewClientWithScheme(f, shared.ClientOptions{ + IncludeVeleroTypes: true, + IncludeNonAdminTypes: true, + }) + if err != nil { + return err + } + + o.client = client + return nil +} + +func (o *RequestDenyOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { + return nil +} + +func (o *RequestDenyOptions) Run(c *cobra.Command, f client.Factory) error { + // Find the request either by UUID or by looking up NABSL name + requestName, err := o.findRequestName() + if err != nil { + return err + } + + // Get the current request + var request nacv1alpha1.NonAdminBackupStorageLocationRequest + err = o.client.Get(context.Background(), kbclient.ObjectKey{ + Name: requestName, + Namespace: "openshift-adp", + }, &request) + if err != nil { + return fmt.Errorf("failed to get request %q: %w", requestName, err) + } + + // Check if already rejected + if request.Spec.ApprovalDecision == "reject" { + fmt.Printf("Request %q is already rejected.\n", o.RequestName) + return nil + } + + // Update the approval decision + request.Spec.ApprovalDecision = "reject" + if o.Reason != "" { + if request.Annotations == nil { + request.Annotations = make(map[string]string) + } + request.Annotations["openshift.io/oadp-rejection-reason"] = o.Reason + } + + err = o.client.Update(context.Background(), &request) + if err != nil { + return fmt.Errorf("failed to deny request: %w", err) + } + + // Get the NABSL name for user-friendly output + nabslName := o.RequestName + if request.Status.SourceNonAdminBSL != nil { + nabslName = request.Status.SourceNonAdminBSL.Name + } + + fmt.Printf("Request for NonAdminBackupStorageLocation %q has been rejected.\n", nabslName) + if o.Reason != "" { + fmt.Printf("Reason: %s\n", o.Reason) + } + + return nil +} + +func (o *RequestDenyOptions) findRequestName() (string, error) { + // First check if o.RequestName is already a UUID by trying to get it directly + var testRequest nacv1alpha1.NonAdminBackupStorageLocationRequest + err := o.client.Get(context.Background(), kbclient.ObjectKey{ + Name: o.RequestName, + Namespace: "openshift-adp", + }, &testRequest) + if err == nil { + // Found it directly, it's a UUID + return o.RequestName, nil + } + + // Not found directly, so o.RequestName might be a NABSL name + // We need to search through all requests to find one with matching source NABSL name + var requestList nacv1alpha1.NonAdminBackupStorageLocationRequestList + err = o.client.List(context.Background(), &requestList, kbclient.InNamespace("openshift-adp")) + if err != nil { + return "", fmt.Errorf("failed to list requests: %w", err) + } + + for _, request := range requestList.Items { + if request.Status.SourceNonAdminBSL != nil && + request.Status.SourceNonAdminBSL.Name == o.RequestName { + return request.Name, nil + } + } + + return "", fmt.Errorf("request for NABSL %q not found", o.RequestName) +} diff --git a/cmd/non-admin/nonadmin.go b/cmd/non-admin/nonadmin.go index b42c1023..b1953c1a 100644 --- a/cmd/non-admin/nonadmin.go +++ b/cmd/non-admin/nonadmin.go @@ -18,6 +18,7 @@ package nonadmin import ( "github.com/migtools/oadp-cli/cmd/non-admin/backup" + "github.com/migtools/oadp-cli/cmd/non-admin/bsl" "github.com/spf13/cobra" "github.com/vmware-tanzu/velero/pkg/client" ) @@ -35,7 +36,7 @@ func NewNonAdminCommand(f client.Factory) *cobra.Command { c.AddCommand(backup.NewBackupCommand(f)) // Add backup storage location subcommand - //c.AddCommand(bsl.NewBSLCommand(f)) + c.AddCommand(bsl.NewBSLCommand(f)) return c } diff --git a/tests/help_test.go b/tests/help_test.go index a3bd4fe3..d31ba631 100644 --- a/tests/help_test.go +++ b/tests/help_test.go @@ -63,6 +63,7 @@ func TestCLIHelpCommands(t *testing.T) { "Work with non-admin resources", "Work with non-admin resources like backups", "backup", + "bsl", }, }, { @@ -80,6 +81,66 @@ func TestCLIHelpCommands(t *testing.T) { "Create a non-admin backup", }, }, + { + name: "nonadmin bsl help", + args: []string{"nonadmin", "bsl", "--help"}, + expectContains: []string{ + "Create and manage non-admin backup storage locations", + "create", + "request", + }, + }, + { + name: "nonadmin bsl create help", + args: []string{"nonadmin", "bsl", "create", "--help"}, + expectContains: []string{ + "Create a non-admin backup storage location", + "--provider", + "--bucket", + "--credential-name", + }, + }, + { + name: "nonadmin bsl request help", + args: []string{"nonadmin", "bsl", "request", "--help"}, + expectContains: []string{ + "View and manage approval requests", + "approve", + "deny", + "describe", + "get", + }, + }, + { + name: "nonadmin bsl request approve help", + args: []string{"nonadmin", "bsl", "request", "approve", "--help"}, + expectContains: []string{ + "Approve a pending backup storage location request", + "--reason", + }, + }, + { + name: "nonadmin bsl request deny help", + args: []string{"nonadmin", "bsl", "request", "deny", "--help"}, + expectContains: []string{ + "Deny a pending backup storage location request", + "--reason", + }, + }, + { + name: "nonadmin bsl request get help", + args: []string{"nonadmin", "bsl", "request", "get", "--help"}, + expectContains: []string{ + "Get non-admin backup storage location requests", + }, + }, + { + name: "nonadmin bsl request describe help", + args: []string{"nonadmin", "bsl", "request", "describe", "--help"}, + expectContains: []string{ + "Describe a non-admin backup storage location request", + }, + }, } // Run tests for each command path @@ -103,6 +164,14 @@ func TestCLIHelpFlags(t *testing.T) { {"nonadmin", "-h"}, {"backup", "--help"}, {"backup", "-h"}, + {"nonadmin", "bsl", "--help"}, + {"nonadmin", "bsl", "-h"}, + {"nonadmin", "bsl", "request", "--help"}, + {"nonadmin", "bsl", "request", "-h"}, + {"nonadmin", "bsl", "request", "approve", "--help"}, + {"nonadmin", "bsl", "request", "approve", "-h"}, + {"nonadmin", "bsl", "request", "deny", "--help"}, + {"nonadmin", "bsl", "request", "deny", "-h"}, } for _, cmd := range commands { From 2c7cc6437ed69ad4182e9163f52451a1512a8c5f Mon Sep 17 00:00:00 2001 From: Joseph Date: Wed, 6 Aug 2025 13:58:17 -0400 Subject: [PATCH 2/9] Enhance non-admin backup storage location management - Introduced new commands for managing non-admin backup storage location requests: approve, reject, describe, and get. - Updated the create command to support custom credential specifications and improved help text for clarity. - Refactored the Makefile to include new installation options for interactive namespace prompts and default settings. - Removed outdated demo outline documentation to streamline project structure. - Added utility functions for reading client configuration and managing namespaces, improving overall command functionality. --- Docs/demo.outline | 18 --- Makefile | 37 +++++- .../request_approve.go => nabsl/approve.go} | 39 ++++--- .../request_describe.go => nabsl/describe.go} | 27 +++-- .../bsl/request_get.go => nabsl/get.go} | 37 +++--- cmd/nabsl/nabsl.go | 56 +++++++++ .../bsl/request_reject.go => nabsl/reject.go} | 45 ++++---- cmd/non-admin/bsl/bsl.go | 1 - cmd/non-admin/bsl/create.go | 59 ++++++---- cmd/non-admin/bsl/request.go | 40 ------- cmd/non-admin/nonadmin.go | 7 -- cmd/root.go | 32 ++--- cmd/utils.go | 109 ++++++++++++++++++ cmd/utls.go | 32 ----- 14 files changed, 321 insertions(+), 218 deletions(-) delete mode 100644 Docs/demo.outline rename cmd/{non-admin/bsl/request_approve.go => nabsl/approve.go} (79%) rename cmd/{non-admin/bsl/request_describe.go => nabsl/describe.go} (87%) rename cmd/{non-admin/bsl/request_get.go => nabsl/get.go} (85%) create mode 100644 cmd/nabsl/nabsl.go rename cmd/{non-admin/bsl/request_reject.go => nabsl/reject.go} (75%) delete mode 100644 cmd/non-admin/bsl/request.go create mode 100644 cmd/utils.go delete mode 100644 cmd/utls.go diff --git a/Docs/demo.outline b/Docs/demo.outline deleted file mode 100644 index 00cde135..00000000 --- a/Docs/demo.outline +++ /dev/null @@ -1,18 +0,0 @@ -Demo outline to be recorded by team: - -1: on a clean system, show the system is clean -2: run through cli install - * show help for various commands -3: on a openshift cluster, w/ oadp already installed, dpa configed and sample app ready. Have non-admin user prepared w/ non-admin app deployed -4. execute admin backup - * show cli logs / describe -4. nuke app namespace -5. restore app - * show cli logs / describe -6. WIN -7. Now log out of cluster as admin, login as non-admin -8. Show app, add data to the to-dolist -9. run non-admin cli for backup -10. run non-admin cli logs for backup -11. run non-admin describe for backup -12. run non-admin backup delete diff --git a/Makefile b/Makefile index 81dcda0b..5566d9c4 100644 --- a/Makefile +++ b/Makefile @@ -6,6 +6,8 @@ BINARY_NAME = kubectl-oadp INSTALL_PATH ?= $(HOME)/.local/bin VERSION ?= $(shell git describe --tags --always --dirty 2>/dev/null || echo "dev") +VELERO_NAMESPACE ?= openshift-adp +ASSUME_DEFAULT ?= false # Centralized platform definitions to avoid duplication # Matches architectures supported by Kubernetes: https://kubernetes.io/releases/download/#binaries @@ -31,10 +33,12 @@ help: ## Show this help message @awk 'BEGIN {FS = ":.*?## "} /^[a-zA-Z_-]+:.*?## / {printf " \033[36m%-15s\033[0m %s\n", $$1, $$2}' $(MAKEFILE_LIST) @echo "" @echo "Installation options:" - @echo " \033[36mmake install\033[0m # Install to ~/.local/bin (recommended, no sudo)" - @echo " \033[36mmake install-user\033[0m # Same as install (legacy alias)" - @echo " \033[36mmake install-bin\033[0m # Install to ~/bin (alternative, no sudo)" - @echo " \033[36mmake install-system\033[0m # Install to /usr/local/bin (requires sudo)" + @echo " \033[36mmake install\033[0m # Install with interactive namespace prompt" + @echo " \033[36mmake install ASSUME_DEFAULT=true\033[0m # Install with default namespace (no prompt)" + @echo " \033[36mmake install VELERO_NAMESPACE=velero\033[0m # Install with custom namespace (no prompt)" + @echo " \033[36mmake install-user\033[0m # Same as install (legacy alias)" + @echo " \033[36mmake install-bin\033[0m # Install to ~/bin (alternative, no sudo)" + @echo " \033[36mmake install-system\033[0m # Install to /usr/local/bin (requires sudo)" @echo "" @echo "Uninstall options:" @echo " \033[36mmake uninstall\033[0m # Remove from user locations (no sudo)" @@ -108,7 +112,30 @@ install: build ## Build and install the kubectl plugin to ~/.local/bin (no sudo if [[ "$$PATH_UPDATED" == "true" ]] || [[ "$$PATH_IN_CONFIG" == "true" ]]; then \ echo "🔄 Restart terminal or run: source ~/.zshrc"; \ fi; \ - echo "Test: kubectl oadp --help" + echo ""; \ + echo "📋 Configuration:"; \ + NAMESPACE=$(VELERO_NAMESPACE); \ + if [[ "$(ASSUME_DEFAULT)" != "true" && "$(VELERO_NAMESPACE)" == "openshift-adp" ]]; then \ + echo ""; \ + echo "🤔 Which namespace should admin commands use for Velero resources?"; \ + echo " (Common options: openshift-adp, velero, oadp)"; \ + echo ""; \ + printf "Enter namespace [default: $(VELERO_NAMESPACE)]: "; \ + read -r user_input; \ + if [[ -n "$$user_input" ]]; then \ + NAMESPACE=$$user_input; \ + fi; \ + echo ""; \ + fi; \ + echo "Setting Velero namespace to: $$NAMESPACE"; \ + $(INSTALL_PATH)/$(BINARY_NAME) client config set namespace=$$NAMESPACE 2>/dev/null || true; \ + echo "✅ Client config initialized"; \ + echo ""; \ + echo "📋 Next steps:"; \ + echo " 1. Test admin commands: kubectl oadp backup get"; \ + echo " 2. Test non-admin commands: kubectl oadp nonadmin backup get"; \ + echo " 3. Manage NABSL requests: kubectl oadp nabsl get"; \ + echo " 4. Change namespace: kubectl oadp client config set namespace=" .PHONY: install-user install-user: build ## Build and install the kubectl plugin to ~/.local/bin (no sudo required) diff --git a/cmd/non-admin/bsl/request_approve.go b/cmd/nabsl/approve.go similarity index 79% rename from cmd/non-admin/bsl/request_approve.go rename to cmd/nabsl/approve.go index e1d8a124..3fda708c 100644 --- a/cmd/non-admin/bsl/request_approve.go +++ b/cmd/nabsl/approve.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package bsl +package nabsl import ( "context" @@ -30,9 +30,9 @@ import ( "github.com/vmware-tanzu/velero/pkg/cmd" ) -// NewRequestApproveCommand creates the "approve" subcommand under bsl request -func NewRequestApproveCommand(f client.Factory) *cobra.Command { - o := NewRequestApproveOptions() +// NewApproveCommand creates the "approve" subcommand under bsl request +func NewApproveCommand(f client.Factory) *cobra.Command { + o := NewApproveOptions() c := &cobra.Command{ Use: "approve REQUEST_NAME", @@ -40,10 +40,10 @@ func NewRequestApproveCommand(f client.Factory) *cobra.Command { Long: "Approve a pending backup storage location request to allow the controller to create the corresponding BackupStorageLocation", Args: cobra.ExactArgs(1), Example: ` # Approve a request by NABSL name (admin access required) - kubectl oadp nonadmin bsl request approve user-test-bsl + kubectl oadp nabsl approve user-test-bsl # Approve a request by UUID with reason - kubectl oadp nonadmin bsl request approve nacuser01-user-test-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 --reason "Approved for production use"`, + kubectl oadp nabsl approve nacuser01-user-test-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 --reason "Approved for production use"`, Run: func(c *cobra.Command, args []string) { cmd.CheckError(o.Complete(args, f)) cmd.CheckError(o.Validate(c, args, f)) @@ -56,21 +56,21 @@ func NewRequestApproveCommand(f client.Factory) *cobra.Command { return c } -type RequestApproveOptions struct { +type ApproveOptions struct { RequestName string Reason string client kbclient.WithWatch } -func NewRequestApproveOptions() *RequestApproveOptions { - return &RequestApproveOptions{} +func NewApproveOptions() *ApproveOptions { + return &ApproveOptions{} } -func (o *RequestApproveOptions) BindFlags(flags *pflag.FlagSet) { +func (o *ApproveOptions) BindFlags(flags *pflag.FlagSet) { flags.StringVar(&o.Reason, "reason", "", "Reason for approval (optional)") } -func (o *RequestApproveOptions) Complete(args []string, f client.Factory) error { +func (o *ApproveOptions) Complete(args []string, f client.Factory) error { o.RequestName = args[0] client, err := shared.NewClientWithScheme(f, shared.ClientOptions{ @@ -85,13 +85,16 @@ func (o *RequestApproveOptions) Complete(args []string, f client.Factory) error return nil } -func (o *RequestApproveOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { +func (o *ApproveOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { return nil } -func (o *RequestApproveOptions) Run(c *cobra.Command, f client.Factory) error { +func (o *ApproveOptions) Run(c *cobra.Command, f client.Factory) error { + // Get the admin namespace (from client config) where requests are stored + adminNS := f.Namespace() + // Find the request either by UUID or by looking up NABSL name - requestName, err := o.findRequestName() + requestName, err := o.findRequestName(adminNS) if err != nil { return err } @@ -100,7 +103,7 @@ func (o *RequestApproveOptions) Run(c *cobra.Command, f client.Factory) error { var request nacv1alpha1.NonAdminBackupStorageLocationRequest err = o.client.Get(context.Background(), kbclient.ObjectKey{ Name: requestName, - Namespace: "openshift-adp", + Namespace: adminNS, }, &request) if err != nil { return fmt.Errorf("failed to get request %q: %w", requestName, err) @@ -138,12 +141,12 @@ func (o *RequestApproveOptions) Run(c *cobra.Command, f client.Factory) error { return nil } -func (o *RequestApproveOptions) findRequestName() (string, error) { +func (o *ApproveOptions) findRequestName(adminNS string) (string, error) { // First check if o.RequestName is already a UUID by trying to get it directly var testRequest nacv1alpha1.NonAdminBackupStorageLocationRequest err := o.client.Get(context.Background(), kbclient.ObjectKey{ Name: o.RequestName, - Namespace: "openshift-adp", + Namespace: adminNS, }, &testRequest) if err == nil { // Found it directly, it's a UUID @@ -153,7 +156,7 @@ func (o *RequestApproveOptions) findRequestName() (string, error) { // Not found directly, so o.RequestName might be a NABSL name // We need to search through all requests to find one with matching source NABSL name var requestList nacv1alpha1.NonAdminBackupStorageLocationRequestList - err = o.client.List(context.Background(), &requestList, kbclient.InNamespace("openshift-adp")) + err = o.client.List(context.Background(), &requestList, kbclient.InNamespace(adminNS)) if err != nil { return "", fmt.Errorf("failed to list requests: %w", err) } diff --git a/cmd/non-admin/bsl/request_describe.go b/cmd/nabsl/describe.go similarity index 87% rename from cmd/non-admin/bsl/request_describe.go rename to cmd/nabsl/describe.go index 81fe34a4..ddc25da4 100644 --- a/cmd/non-admin/bsl/request_describe.go +++ b/cmd/nabsl/describe.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package bsl +package nabsl import ( "context" @@ -31,8 +31,8 @@ import ( "github.com/vmware-tanzu/velero/pkg/cmd" ) -func NewRequestDescribeCommand(f client.Factory) *cobra.Command { - o := NewRequestDescribeOptions() +func NewDescribeCommand(f client.Factory) *cobra.Command { + o := NewDescribeOptions() c := &cobra.Command{ Use: "describe NAME", @@ -44,25 +44,25 @@ func NewRequestDescribeCommand(f client.Factory) *cobra.Command { cmd.CheckError(o.Run(c, f)) }, Example: ` # Describe a request by NABSL name - kubectl oadp nonadmin bsl request describe my-bsl-request + kubectl oadp nabsl describe my-bsl-request # Describe a request by UUID - kubectl oadp nonadmin bsl request describe nacuser01-my-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8`, + kubectl oadp nabsl describe nacuser01-my-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8`, } return c } -type RequestDescribeOptions struct { +type DescribeOptions struct { Name string client kbclient.WithWatch } -func NewRequestDescribeOptions() *RequestDescribeOptions { - return &RequestDescribeOptions{} +func NewDescribeOptions() *DescribeOptions { + return &DescribeOptions{} } -func (o *RequestDescribeOptions) Complete(args []string, f client.Factory) error { +func (o *DescribeOptions) Complete(args []string, f client.Factory) error { o.Name = args[0] client, err := shared.NewClientWithScheme(f, shared.ClientOptions{ @@ -77,11 +77,14 @@ func (o *RequestDescribeOptions) Complete(args []string, f client.Factory) error return nil } -func (o *RequestDescribeOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { +func (o *DescribeOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { return nil } -func (o *RequestDescribeOptions) Run(c *cobra.Command, f client.Factory) error { +func (o *DescribeOptions) Run(c *cobra.Command, f client.Factory) error { + // Get the admin namespace (from client config) where requests are stored + adminNS := f.Namespace() + // Get the current namespace to find user's NABSLs currentNS, err := shared.GetCurrentNamespace() if err != nil { @@ -116,7 +119,7 @@ func (o *RequestDescribeOptions) Run(c *cobra.Command, f client.Factory) error { var request nacv1alpha1.NonAdminBackupStorageLocationRequest err = o.client.Get(context.Background(), kbclient.ObjectKey{ Name: targetUUID, - Namespace: "openshift-adp", + Namespace: adminNS, }, &request) if err != nil { return fmt.Errorf("failed to get request for %q: %w", o.Name, err) diff --git a/cmd/non-admin/bsl/request_get.go b/cmd/nabsl/get.go similarity index 85% rename from cmd/non-admin/bsl/request_get.go rename to cmd/nabsl/get.go index ac6be78a..6a800658 100644 --- a/cmd/non-admin/bsl/request_get.go +++ b/cmd/nabsl/get.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package bsl +package nabsl import ( "context" @@ -34,8 +34,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func NewRequestGetCommand(f client.Factory) *cobra.Command { - o := NewRequestGetOptions() +func NewGetCommand(f client.Factory) *cobra.Command { + o := NewGetOptions() c := &cobra.Command{ Use: "get [NAME]", @@ -46,17 +46,17 @@ func NewRequestGetCommand(f client.Factory) *cobra.Command { cmd.CheckError(o.Validate(c, args, f)) cmd.CheckError(o.Run(c, f)) }, - Example: ` # Get all your backup storage location requests - kubectl oadp nonadmin bsl request get + Example: ` # Get all backup storage location requests (admin access required) + kubectl oadp nabsl get # Get a specific request by NABSL name - kubectl oadp nonadmin bsl request get my-bsl-request + kubectl oadp nabsl get my-bsl-request # Get a specific request by UUID - kubectl oadp nonadmin bsl request get nacuser01-my-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 + kubectl oadp nabsl get nacuser01-my-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 # Get output in YAML format - kubectl oadp nonadmin bsl request get my-bsl-request -o yaml`, + kubectl oadp nabsl get my-bsl-request -o yaml`, } o.BindFlags(c.Flags()) @@ -66,21 +66,21 @@ func NewRequestGetCommand(f client.Factory) *cobra.Command { return c } -type RequestGetOptions struct { +type GetOptions struct { Name string AllNamespaces bool client kbclient.WithWatch } -func NewRequestGetOptions() *RequestGetOptions { - return &RequestGetOptions{} +func NewGetOptions() *GetOptions { + return &GetOptions{} } -func (o *RequestGetOptions) BindFlags(flags *pflag.FlagSet) { +func (o *GetOptions) BindFlags(flags *pflag.FlagSet) { flags.BoolVar(&o.AllNamespaces, "all-namespaces", false, "If present, list requests across all namespaces") } -func (o *RequestGetOptions) Complete(args []string, f client.Factory) error { +func (o *GetOptions) Complete(args []string, f client.Factory) error { if len(args) > 0 { o.Name = args[0] } @@ -97,11 +97,14 @@ func (o *RequestGetOptions) Complete(args []string, f client.Factory) error { return nil } -func (o *RequestGetOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { +func (o *GetOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { return nil } -func (o *RequestGetOptions) Run(c *cobra.Command, f client.Factory) error { +func (o *GetOptions) Run(c *cobra.Command, f client.Factory) error { + // Get the admin namespace (from client config) where requests are stored + adminNS := f.Namespace() + // Get the current namespace to find user's NABSLs currentNS, err := shared.GetCurrentNamespace() if err != nil { @@ -145,7 +148,7 @@ func (o *RequestGetOptions) Run(c *cobra.Command, f client.Factory) error { var request nacv1alpha1.NonAdminBackupStorageLocationRequest err := o.client.Get(context.Background(), kbclient.ObjectKey{ Name: targetUUID, - Namespace: "openshift-adp", + Namespace: adminNS, }, &request) if err != nil { return fmt.Errorf("failed to get request for %q: %w", o.Name, err) @@ -170,7 +173,7 @@ func (o *RequestGetOptions) Run(c *cobra.Command, f client.Factory) error { var request nacv1alpha1.NonAdminBackupStorageLocationRequest err := o.client.Get(context.Background(), kbclient.ObjectKey{ Name: uuid, - Namespace: "openshift-adp", + Namespace: adminNS, }, &request) if err != nil { // Request might not exist yet, skip diff --git a/cmd/nabsl/nabsl.go b/cmd/nabsl/nabsl.go new file mode 100644 index 00000000..87788576 --- /dev/null +++ b/cmd/nabsl/nabsl.go @@ -0,0 +1,56 @@ +/* +Copyright 2025 The OADP CLI 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 nabsl + +import ( + "github.com/spf13/cobra" + "github.com/vmware-tanzu/velero/pkg/client" +) + +// NewNABSLCommand creates the "nabsl" command for managing non-admin backup storage location requests +func NewNABSLCommand(f client.Factory) *cobra.Command { + c := &cobra.Command{ + Use: "nabsl", + Short: "Manage non-admin backup storage location requests", + Long: `Manage approval requests for non-admin backup storage locations. + +Non-admin backup storage locations (NABSL) require admin approval before they can be used. +When users create NABSLs, approval requests are automatically generated for admin review. + +Use these commands to view, approve, or reject pending NABSL requests.`, + Example: ` # List all pending NABSL requests + kubectl oadp nabsl get + + # Describe a specific NABSL request + kubectl oadp nabsl describe my-storage-request + + # Approve a NABSL request + kubectl oadp nabsl approve my-storage-request + + # Reject a NABSL request + kubectl oadp nabsl reject my-storage-request`, + } + + c.AddCommand( + NewGetCommand(f), + NewDescribeCommand(f), + NewApproveCommand(f), + NewRejectCommand(f), + ) + + return c +} diff --git a/cmd/non-admin/bsl/request_reject.go b/cmd/nabsl/reject.go similarity index 75% rename from cmd/non-admin/bsl/request_reject.go rename to cmd/nabsl/reject.go index 24194ec4..1d904fb3 100644 --- a/cmd/non-admin/bsl/request_reject.go +++ b/cmd/nabsl/reject.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package bsl +package nabsl import ( "context" @@ -30,20 +30,20 @@ import ( "github.com/vmware-tanzu/velero/pkg/cmd" ) -// NewRequestDenyCommand creates the "deny" subcommand under bsl request -func NewRequestDenyCommand(f client.Factory) *cobra.Command { - o := NewRequestDenyOptions() +// NewRejectCommand creates the "deny" subcommand under bsl request +func NewRejectCommand(f client.Factory) *cobra.Command { + o := NewRejectOptions() c := &cobra.Command{ - Use: "deny REQUEST_NAME", - Short: "Deny a pending backup storage location request", - Long: "Deny a pending backup storage location request to reject the user's request for a backup storage location", + Use: "reject REQUEST_NAME", + Short: "Reject a pending backup storage location request", + Long: "Reject a pending backup storage location request to deny the user's request for a backup storage location", Args: cobra.ExactArgs(1), Example: ` # Deny a request by NABSL name (admin access required) - kubectl oadp nonadmin bsl request deny user-test-bsl --reason "Invalid configuration" + kubectl oadp nabsl reject user-test-bsl --reason "Invalid configuration" # Deny a request by UUID with detailed reason - kubectl oadp nonadmin bsl request deny nacuser01-user-test-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 --reason "Bucket does not exist in specified region"`, + kubectl oadp nabsl reject nacuser01-user-test-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 --reason "Bucket does not exist in specified region"`, Run: func(c *cobra.Command, args []string) { cmd.CheckError(o.Complete(args, f)) cmd.CheckError(o.Validate(c, args, f)) @@ -56,21 +56,21 @@ func NewRequestDenyCommand(f client.Factory) *cobra.Command { return c } -type RequestDenyOptions struct { +type RejectOptions struct { RequestName string Reason string client kbclient.WithWatch } -func NewRequestDenyOptions() *RequestDenyOptions { - return &RequestDenyOptions{} +func NewRejectOptions() *RejectOptions { + return &RejectOptions{} } -func (o *RequestDenyOptions) BindFlags(flags *pflag.FlagSet) { +func (o *RejectOptions) BindFlags(flags *pflag.FlagSet) { flags.StringVar(&o.Reason, "reason", "", "Reason for denial (recommended)") } -func (o *RequestDenyOptions) Complete(args []string, f client.Factory) error { +func (o *RejectOptions) Complete(args []string, f client.Factory) error { o.RequestName = args[0] client, err := shared.NewClientWithScheme(f, shared.ClientOptions{ @@ -85,13 +85,16 @@ func (o *RequestDenyOptions) Complete(args []string, f client.Factory) error { return nil } -func (o *RequestDenyOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { +func (o *RejectOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { return nil } -func (o *RequestDenyOptions) Run(c *cobra.Command, f client.Factory) error { +func (o *RejectOptions) Run(c *cobra.Command, f client.Factory) error { + // Get the admin namespace (from client config) where requests are stored + adminNS := f.Namespace() + // Find the request either by UUID or by looking up NABSL name - requestName, err := o.findRequestName() + requestName, err := o.findRequestName(adminNS) if err != nil { return err } @@ -100,7 +103,7 @@ func (o *RequestDenyOptions) Run(c *cobra.Command, f client.Factory) error { var request nacv1alpha1.NonAdminBackupStorageLocationRequest err = o.client.Get(context.Background(), kbclient.ObjectKey{ Name: requestName, - Namespace: "openshift-adp", + Namespace: adminNS, }, &request) if err != nil { return fmt.Errorf("failed to get request %q: %w", requestName, err) @@ -140,12 +143,12 @@ func (o *RequestDenyOptions) Run(c *cobra.Command, f client.Factory) error { return nil } -func (o *RequestDenyOptions) findRequestName() (string, error) { +func (o *RejectOptions) findRequestName(adminNS string) (string, error) { // First check if o.RequestName is already a UUID by trying to get it directly var testRequest nacv1alpha1.NonAdminBackupStorageLocationRequest err := o.client.Get(context.Background(), kbclient.ObjectKey{ Name: o.RequestName, - Namespace: "openshift-adp", + Namespace: adminNS, }, &testRequest) if err == nil { // Found it directly, it's a UUID @@ -155,7 +158,7 @@ func (o *RequestDenyOptions) findRequestName() (string, error) { // Not found directly, so o.RequestName might be a NABSL name // We need to search through all requests to find one with matching source NABSL name var requestList nacv1alpha1.NonAdminBackupStorageLocationRequestList - err = o.client.List(context.Background(), &requestList, kbclient.InNamespace("openshift-adp")) + err = o.client.List(context.Background(), &requestList, kbclient.InNamespace(adminNS)) if err != nil { return "", fmt.Errorf("failed to list requests: %w", err) } diff --git a/cmd/non-admin/bsl/bsl.go b/cmd/non-admin/bsl/bsl.go index a593c3b9..5c9d48df 100644 --- a/cmd/non-admin/bsl/bsl.go +++ b/cmd/non-admin/bsl/bsl.go @@ -31,7 +31,6 @@ func NewBSLCommand(f client.Factory) *cobra.Command { c.AddCommand( NewCreateCommand(f), - NewRequestCommand(f), ) return c diff --git a/cmd/non-admin/bsl/create.go b/cmd/non-admin/bsl/create.go index 868a6894..083ceefb 100644 --- a/cmd/non-admin/bsl/create.go +++ b/cmd/non-admin/bsl/create.go @@ -20,6 +20,7 @@ import ( "context" "fmt" + "github.com/pkg/errors" "github.com/spf13/cobra" "github.com/spf13/pflag" kbclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -27,10 +28,11 @@ import ( "github.com/migtools/oadp-cli/cmd/shared" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + "github.com/vmware-tanzu/velero/pkg/builder" "github.com/vmware-tanzu/velero/pkg/client" "github.com/vmware-tanzu/velero/pkg/cmd" + "github.com/vmware-tanzu/velero/pkg/cmd/util/flag" "github.com/vmware-tanzu/velero/pkg/cmd/util/output" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -50,7 +52,7 @@ func NewCreateCommand(f client.Factory) *cobra.Command { kubectl oadp nonadmin bsl create my-storage \ --provider aws \ --bucket my-velero-bucket \ - --credential-name cloud-credentials \ + --credential cloud-credentials=cloud \ --region us-east-1 # Create with prefix for organizing backups @@ -58,14 +60,21 @@ func NewCreateCommand(f client.Factory) *cobra.Command { --provider aws \ --bucket my-velero-bucket \ --prefix velero-backups \ - --credential-name cloud-credentials \ + --credential cloud-credentials=cloud \ + --region us-east-1 + + # Create with custom credential key + kubectl oadp nonadmin bsl create my-storage \ + --provider aws \ + --bucket my-velero-bucket \ + --credential my-secret=service-account-key \ --region us-east-1 # View the YAML without creating the resource kubectl oadp nonadmin bsl create my-storage \ --provider aws \ --bucket my-bucket \ - --credential-name cloud-credentials \ + --credential cloud-credentials=cloud \ --region us-east-1 \ -o yaml`, } @@ -78,20 +87,21 @@ func NewCreateCommand(f client.Factory) *cobra.Command { } type CreateOptions struct { - Name string - Namespace string - Provider string - Bucket string - Prefix string - CredentialName string - Region string - Config map[string]string - client kbclient.WithWatch + Name string + Namespace string + Provider string + Bucket string + Prefix string + Credential flag.Map + Region string + Config map[string]string + client kbclient.WithWatch } func NewCreateOptions() *CreateOptions { return &CreateOptions{ - Config: make(map[string]string), + Credential: flag.NewMap(), + Config: make(map[string]string), } } @@ -99,7 +109,7 @@ func (o *CreateOptions) BindFlags(flags *pflag.FlagSet) { flags.StringVar(&o.Provider, "provider", "", "Storage provider (required). Examples: aws, azure, gcp") flags.StringVar(&o.Bucket, "bucket", "", "Object storage bucket name (required)") flags.StringVar(&o.Prefix, "prefix", "", "Prefix for backup objects in the bucket") - flags.StringVar(&o.CredentialName, "credential-name", "", "Name of the credential secret (required)") + flags.Var(&o.Credential, "credential", "The credential to be used by this location as a key-value pair, where the key is the Kubernetes Secret name, and the value is the data key name within the Secret. Required, one value only.") flags.StringVar(&o.Region, "region", "", "Storage region (required for some providers like AWS)") flags.StringToStringVar(&o.Config, "config", nil, "Additional provider-specific configuration (key=value pairs)") } @@ -132,8 +142,11 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto if o.Bucket == "" { return fmt.Errorf("--bucket is required") } - if o.CredentialName == "" { - return fmt.Errorf("--credential-name is required") + if len(o.Credential.Data()) == 0 { + return errors.New("--credential is required") + } + if len(o.Credential.Data()) > 1 { + return errors.New("--credential can only contain 1 key/value pair") } return nil @@ -160,12 +173,6 @@ func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error { BackupStorageLocationSpec: &velerov1.BackupStorageLocationSpec{ Provider: o.Provider, Config: config, - Credential: &corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: o.CredentialName, - }, - Key: "cloud", // Standard key name for cloud credentials - }, StorageType: velerov1.StorageType{ ObjectStorage: &velerov1.ObjectStorageLocation{ Bucket: o.Bucket, @@ -176,6 +183,12 @@ func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error { }, } + // Set credential from user-provided key-value pair + for secretName, secretKey := range o.Credential.Data() { + nabsl.Spec.BackupStorageLocationSpec.Credential = builder.ForSecretKeySelector(secretName, secretKey).Result() + break + } + if printed, err := output.PrintWithFormat(c, nabsl); printed || err != nil { return err } diff --git a/cmd/non-admin/bsl/request.go b/cmd/non-admin/bsl/request.go deleted file mode 100644 index 2889ee72..00000000 --- a/cmd/non-admin/bsl/request.go +++ /dev/null @@ -1,40 +0,0 @@ -/* -Copyright 2025 The OADP CLI 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 bsl - -import ( - "github.com/spf13/cobra" - "github.com/vmware-tanzu/velero/pkg/client" -) - -// NewRequestCommand creates the "request" subcommand under bsl -func NewRequestCommand(f client.Factory) *cobra.Command { - c := &cobra.Command{ - Use: "request", - Short: "Manage backup storage location approval requests", - Long: "View and manage approval requests for backup storage locations. Requests are automatically created when users create backup storage locations and require admin approval.", - } - - c.AddCommand( - NewRequestGetCommand(f), - NewRequestDescribeCommand(f), - NewRequestApproveCommand(f), - NewRequestDenyCommand(f), - ) - - return c -} diff --git a/cmd/non-admin/nonadmin.go b/cmd/non-admin/nonadmin.go index b1953c1a..72d84b3f 100644 --- a/cmd/non-admin/nonadmin.go +++ b/cmd/non-admin/nonadmin.go @@ -41,10 +41,3 @@ func NewNonAdminCommand(f client.Factory) *cobra.Command { return c } -// NewNonAdminFactory creates a client factory for NonAdminBackup operations -// that uses the current kubeconfig context namespace instead of hardcoded openshift-adp -func NewNonAdminFactory() client.Factory { - // Don't set a default namespace, let it use the kubeconfig context - cfg := client.VeleroConfig{} - return client.NewFactory("oadp-nonadmin-cli", cfg) -} diff --git a/cmd/root.go b/cmd/root.go index 53c95f65..447b1e01 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -22,9 +22,11 @@ import ( "path/filepath" "strings" + "github.com/migtools/oadp-cli/cmd/nabsl" nonadmin "github.com/migtools/oadp-cli/cmd/non-admin" "github.com/spf13/cobra" "github.com/vmware-tanzu/velero/pkg/cmd/cli/backup" + "github.com/vmware-tanzu/velero/pkg/cmd/cli/client" "github.com/vmware-tanzu/velero/pkg/cmd/cli/restore" "github.com/vmware-tanzu/velero/pkg/cmd/cli/version" ) @@ -65,11 +67,12 @@ func NewVeleroRootCommand() *cobra.Command { // Create NonAdmin client factory for NonAdminBackup commands // This factory uses the current kubeconfig context namespace instead of hardcoded openshift-adp - nonAdminFactory := nonadmin.NewNonAdminFactory() + nonAdminFactory := NewNonAdminFactory() // Create the commands and modify their help text before adding them backupCmd := backup.NewCommand(veleroFactory) restoreCmd := restore.NewCommand(veleroFactory) + clientCmd := client.NewCommand() // Modify help text to replace "velero" with "oadp" updateCommandHelpText(backupCmd, usagePrefix) @@ -79,6 +82,10 @@ func NewVeleroRootCommand() *cobra.Command { rootCmd.AddCommand(version.NewCommand(veleroFactory)) rootCmd.AddCommand(backupCmd) rootCmd.AddCommand(restoreCmd) + rootCmd.AddCommand(clientCmd) + + // Admin NABSL commands - use Velero factory (admin namespace) + rootCmd.AddCommand(nabsl.NewNABSLCommand(veleroFactory)) // Custom subcommands - use NonAdmin factory rootCmd.AddCommand(nonadmin.NewNonAdminCommand(nonAdminFactory)) @@ -86,29 +93,6 @@ func NewVeleroRootCommand() *cobra.Command { return rootCmd } -// updateCommandHelpText recursively updates help text in commands and subcommands -func updateCommandHelpText(cmd *cobra.Command, usagePrefix string) { - // Update examples that contain "velero" - if strings.Contains(cmd.Example, "velero") { - cmd.Example = strings.ReplaceAll(cmd.Example, "velero", usagePrefix) - } - - // Update long description if it contains "velero" - if strings.Contains(cmd.Long, "velero") { - cmd.Long = strings.ReplaceAll(cmd.Long, "velero", "oadp") - } - - // Update short description if it contains "velero" - if strings.Contains(cmd.Short, "velero") { - cmd.Short = strings.ReplaceAll(cmd.Short, "velero", "oadp") - } - - // Recursively update subcommands - for _, subCmd := range cmd.Commands() { - updateCommandHelpText(subCmd, usagePrefix) - } -} - func Execute() { if err := NewVeleroRootCommand().Execute(); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) diff --git a/cmd/utils.go b/cmd/utils.go new file mode 100644 index 00000000..0366c4c8 --- /dev/null +++ b/cmd/utils.go @@ -0,0 +1,109 @@ +/* +Copyright 2025 The OADP CLI 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 cmd + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + + "github.com/spf13/cobra" + "github.com/vmware-tanzu/velero/pkg/client" +) + +// ClientConfig represents the structure of Velero's client configuration file +type ClientConfig struct { + Namespace string `json:"namespace,omitempty"` + Features string `json:"features,omitempty"` +} + +// readVeleroClientConfig reads the Velero client configuration from ~/.config/velero/config.json +func readVeleroClientConfig() (*ClientConfig, error) { + homeDir, err := os.UserHomeDir() + if err != nil { + return nil, err + } + + configPath := filepath.Join(homeDir, ".config", "velero", "config.json") + data, err := os.ReadFile(configPath) + if err != nil { + // If file doesn't exist, return empty config (no error) + if os.IsNotExist(err) { + return &ClientConfig{}, nil + } + return nil, err + } + + var config ClientConfig + if err := json.Unmarshal(data, &config); err != nil { + return nil, err + } + + return &config, nil +} + +// newVeleroFactory creates a Velero client factory that respects client configuration. +// This allows admin commands to follow the same namespace precedence as standard Velero: +// 1. Client config (oadp client config set namespace=...) +// 2. Kubeconfig context namespace +// 3. Velero default (usually "velero") +func newVeleroFactory() client.Factory { + cfg := client.VeleroConfig{} + + // Read client configuration to respect namespace settings + clientConfig, err := readVeleroClientConfig() + if err == nil && clientConfig.Namespace != "" { + // Use namespace from client config if set + cfg[client.ConfigKeyNamespace] = clientConfig.Namespace + } + // If no client config namespace, let Velero use its default resolution: + // kubeconfig context > velero default + + return client.NewFactory("oadp-velero-cli", cfg) +} + +// NewNonAdminFactory creates a client factory for NonAdminBackup operations +// that uses the current kubeconfig context namespace instead of hardcoded openshift-adp +func NewNonAdminFactory() client.Factory { + // Don't set a default namespace, let it use the kubeconfig context + cfg := client.VeleroConfig{} + return client.NewFactory("oadp-nonadmin-cli", cfg) +} + +// updateCommandHelpText recursively updates help text in commands and subcommands +func updateCommandHelpText(cmd *cobra.Command, usagePrefix string) { + // Update examples that contain "velero" + if strings.Contains(cmd.Example, "velero") { + cmd.Example = strings.ReplaceAll(cmd.Example, "velero", usagePrefix) + } + + // Update long description if it contains "velero" + if strings.Contains(cmd.Long, "velero") { + cmd.Long = strings.ReplaceAll(cmd.Long, "velero", "oadp") + } + + // Update short description if it contains "velero" + if strings.Contains(cmd.Short, "velero") { + cmd.Short = strings.ReplaceAll(cmd.Short, "velero", "oadp") + } + + // Recursively update subcommands + for _, subCmd := range cmd.Commands() { + updateCommandHelpText(subCmd, usagePrefix) + } +} diff --git a/cmd/utls.go b/cmd/utls.go deleted file mode 100644 index c19641d8..00000000 --- a/cmd/utls.go +++ /dev/null @@ -1,32 +0,0 @@ -/* -Copyright 2025 The OADP CLI 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 cmd - -import ( - "github.com/vmware-tanzu/velero/pkg/client" -) - -// Default namespace for Velero resources -const veleroNamespace = "openshift-adp" - -// newVeleroFactory creates a Velero client factory with the configured namespace. -func newVeleroFactory() client.Factory { - cfg := client.VeleroConfig{ - client.ConfigKeyNamespace: veleroNamespace, - } - return client.NewFactory("oadp-velero-cli", cfg) -} From 811cd004cebaa9d52df07b1f9f3183651c9abf26 Mon Sep 17 00:00:00 2001 From: Joseph Date: Wed, 6 Aug 2025 14:17:29 -0400 Subject: [PATCH 3/9] Add integration and unit tests for OADP CLI commands - Introduced a comprehensive suite of integration tests in `integration_test.go` to validate binary build, Makefile installation, client configuration, and command architecture. - Created unit tests for root, non-admin, and NABSL commands to ensure help text accuracy and command functionality. - Enhanced the Makefile to include dedicated targets for running unit and integration tests. - Added a `TESTING.md` document to outline the testing architecture and best practices for future contributions. - Removed outdated test files to streamline the testing structure. --- Makefile | 22 +++- TESTING.md | 162 +++++++++++++++++++++++++++++ cmd/nabsl/nabsl_test.go | 142 +++++++++++++++++++++++++ cmd/non-admin/bsl/bsl.go | 2 +- cmd/non-admin/bsl/bsl_test.go | 114 +++++++++++++++++++++ cmd/non-admin/nonadmin_test.go | 86 ++++++++++++++++ cmd/root_test.go | 125 ++++++++++++++++++++++ integration_test.go | 149 +++++++++++++++++++++++++++ internal/testutil/testutil.go | 156 ++++++++++++++++++++++++++++ tests/README.md | 143 -------------------------- tests/build_test.go | 79 -------------- tests/common.go | 158 ---------------------------- tests/help_test.go | 182 --------------------------------- tests/main_test.go | 50 --------- 14 files changed, 956 insertions(+), 614 deletions(-) create mode 100644 TESTING.md create mode 100644 cmd/nabsl/nabsl_test.go create mode 100644 cmd/non-admin/bsl/bsl_test.go create mode 100644 cmd/non-admin/nonadmin_test.go create mode 100644 cmd/root_test.go create mode 100644 integration_test.go create mode 100644 internal/testutil/testutil.go delete mode 100644 tests/README.md delete mode 100644 tests/build_test.go delete mode 100644 tests/common.go delete mode 100644 tests/help_test.go delete mode 100644 tests/main_test.go diff --git a/Makefile b/Makefile index 5566d9c4..29b9a5c1 100644 --- a/Makefile +++ b/Makefile @@ -55,6 +55,11 @@ help: ## Show this help message @echo " make build PLATFORM=windows/amd64" @echo " make build PLATFORM=windows/arm64" @echo "" + @echo "Testing commands:" + @echo " make test # Run all tests (unit + integration)" + @echo " make test-unit # Run unit tests only" + @echo " make test-integration # Run integration tests only" + @echo "" @echo "Release commands:" @echo " make release-build # Build binaries for all platforms" @echo " make release-archives # Create tar.gz archives for all platforms" @@ -212,9 +217,24 @@ uninstall-all: ## Uninstall the kubectl plugin from all locations (user + system .PHONY: test test: ## Run all tests @echo "Running tests..." - go test ./... + @echo "🧪 Running unit tests..." + go test ./cmd/... ./internal/... + @echo "🔗 Running integration tests..." + go test . -v @echo "✅ Tests completed!" +.PHONY: test-unit +test-unit: ## Run unit tests only + @echo "Running unit tests..." + go test ./cmd/... ./internal/... + @echo "✅ Unit tests completed!" + +.PHONY: test-integration +test-integration: ## Run integration tests only + @echo "Running integration tests..." + go test . -v + @echo "✅ Integration tests completed!" + # Cleanup targets .PHONY: clean clean: ## Remove built binaries diff --git a/TESTING.md b/TESTING.md new file mode 100644 index 00000000..c1b9452a --- /dev/null +++ b/TESTING.md @@ -0,0 +1,162 @@ +# OADP CLI Testing Guide + +This document describes the decentralized testing architecture for the OADP CLI. + +## Architecture Overview + +Tests are organized following Go best practices - they live next to the code they test: + +``` +├── cmd/ +│ ├── root_test.go # Root command tests +│ ├── nabsl/ +│ │ ├── nabsl.go +│ │ └── nabsl_test.go # NABSL command tests +│ └── non-admin/ +│ ├── nonadmin_test.go # Non-admin command tests +│ └── bsl/ +│ ├── bsl.go +│ └── bsl_test.go # BSL command tests +├── internal/ +│ └── testutil/ +│ └── testutil.go # Shared test utilities +└── integration_test.go # Integration tests +``` + +## Test Types + +### 1. Unit Tests +Located next to the source code they test: + +- **`cmd/root_test.go`**: Tests root command functionality, help text, basic structure +- **`cmd/nabsl/nabsl_test.go`**: Tests NABSL commands (approve, reject, get, describe) +- **`cmd/non-admin/nonadmin_test.go`**: Tests non-admin command structure +- **`cmd/non-admin/bsl/bsl_test.go`**: Tests BSL creation, credential handling + +### 2. Integration Tests +Located at the project root in `integration_test.go`: + +- **Binary Build**: Tests that the CLI binary builds successfully +- **Makefile Integration**: Tests installation options and build system +- **Client Config**: Tests end-to-end client configuration workflow +- **Command Architecture**: Tests overall command structure and relationships + +### 3. Shared Utilities +Located in `internal/testutil/`: + +- **`BuildCLIBinary()`**: Builds test binary with proper cleanup +- **`RunCommand()`**: Executes CLI commands with timeout and logging +- **`TestHelpCommand()`**: Validates help text contains expected content +- **`SetupTempHome()`**: Creates isolated test environment for client config + +## Running Tests + +### All Tests +```bash +make test +``` + +### Unit Tests Only +```bash +make test-unit +``` + +### Integration Tests Only +```bash +make test-integration +``` + +### Specific Package +```bash +# Test specific command +go test ./cmd/nabsl -v + +# Test with coverage +go test ./cmd/... -cover +``` + +## Test Coverage + +### Unit Tests Verify: +- ✅ Command help text and structure +- ✅ Flag definitions and validation +- ✅ Subcommand availability +- ✅ Help flag consistency (`--help` and `-h`) +- ✅ Command architecture changes + +### Integration Tests Verify: +- ✅ Binary builds successfully +- ✅ Makefile installation options work +- ✅ Client configuration end-to-end +- ✅ Cross-command functionality +- ✅ Overall system behavior + +## Benefits of Decentralized Testing + +### 1. **Locality** +- Tests live next to the code they test +- Easy to find and maintain +- Clear ownership and responsibility + +### 2. **Focused Scope** +- Each test file has a narrow, clear scope +- Faster test execution for specific areas +- Better isolation of test failures + +### 3. **Parallel Execution** +- Tests can run in parallel across packages +- Better CI/CD performance +- Independent test environments + +### 4. **Maintainability** +- When code changes, related tests are immediately visible +- Easier to keep tests in sync with code +- Reduced cognitive overhead + +## Adding New Tests + +### For New Commands +1. Create `*_test.go` file in the same package as your command +2. Import `"github.com/migtools/oadp-cli/internal/testutil"` +3. Follow existing patterns for help text validation + +### For Integration Scenarios +1. Add tests to `integration_test.go` +2. Use `testutil.BuildCLIBinary()` for binary-based tests +3. Focus on cross-package functionality + +### Example Test Structure +```go +func TestNewCommand(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + tests := []struct { + name string + args []string + expectContains []string + }{ + { + name: "command help", + args: []string{"newcommand", "--help"}, + expectContains: []string{"expected text"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testutil.TestHelpCommand(t, binaryPath, tt.args, tt.expectContains) + }) + } +} +``` + +## Best Practices + +1. **Use testutil helpers** for common operations +2. **Test help text** to verify command structure +3. **Use table-driven tests** for multiple scenarios +4. **Keep tests focused** on single responsibilities +5. **Mock external dependencies** when possible +6. **Use descriptive test names** that explain what's being tested + +This testing architecture ensures comprehensive coverage while maintaining clarity and ease of maintenance. \ No newline at end of file diff --git a/cmd/nabsl/nabsl_test.go b/cmd/nabsl/nabsl_test.go new file mode 100644 index 00000000..96bc4e57 --- /dev/null +++ b/cmd/nabsl/nabsl_test.go @@ -0,0 +1,142 @@ +/* +Copyright 2025 The OADP CLI 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 nabsl + +import ( + "testing" + + "github.com/migtools/oadp-cli/internal/testutil" +) + +// TestNABSLCommands tests the NABSL command functionality +func TestNABSLCommands(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + tests := []struct { + name string + args []string + expectContains []string + }{ + { + name: "nabsl help", + args: []string{"nabsl", "--help"}, + expectContains: []string{ + "Manage approval requests for non-admin backup storage locations", + "approve", + "reject", + "describe", + "get", + }, + }, + { + name: "nabsl approve help", + args: []string{"nabsl", "approve", "--help"}, + expectContains: []string{ + "Approve a pending backup storage location request", + "--reason", + }, + }, + { + name: "nabsl reject help", + args: []string{"nabsl", "reject", "--help"}, + expectContains: []string{ + "Reject a pending backup storage location request", + "--reason", + }, + }, + { + name: "nabsl get help", + args: []string{"nabsl", "get", "--help"}, + expectContains: []string{ + "Get non-admin backup storage location requests", + }, + }, + { + name: "nabsl describe help", + args: []string{"nabsl", "describe", "--help"}, + expectContains: []string{ + "Describe a non-admin backup storage location request", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testutil.TestHelpCommand(t, binaryPath, tt.args, tt.expectContains) + }) + } +} + +// TestNABSLHelpFlags tests that both --help and -h work for nabsl commands +func TestNABSLHelpFlags(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + commands := [][]string{ + {"nabsl", "--help"}, + {"nabsl", "-h"}, + {"nabsl", "approve", "--help"}, + {"nabsl", "approve", "-h"}, + {"nabsl", "reject", "--help"}, + {"nabsl", "reject", "-h"}, + {"nabsl", "get", "--help"}, + {"nabsl", "get", "-h"}, + {"nabsl", "describe", "--help"}, + {"nabsl", "describe", "-h"}, + } + + for _, cmd := range commands { + t.Run("help_flags_"+cmd[len(cmd)-1], func(t *testing.T) { + testutil.TestHelpCommand(t, binaryPath, cmd, []string{"Usage:"}) + }) + } +} + +// TestNABSLClientConfigIntegration tests that NABSL commands respect client config +func TestNABSLClientConfigIntegration(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + _, cleanup := testutil.SetupTempHome(t) + defer cleanup() + + t.Run("nabsl commands work with client config", func(t *testing.T) { + // Set a known namespace + _, err := testutil.RunCommand(t, binaryPath, "client", "config", "set", "namespace=admin-namespace") + if err != nil { + t.Fatalf("Failed to set client config: %v", err) + } + + // Test that nabsl commands can be invoked (they should respect the namespace) + // We test help commands since they don't require actual K8s resources + commands := [][]string{ + {"nabsl", "get", "--help"}, + {"nabsl", "approve", "--help"}, + {"nabsl", "reject", "--help"}, + {"nabsl", "describe", "--help"}, + } + + for _, cmd := range commands { + t.Run("config_test_"+cmd[1], func(t *testing.T) { + output, err := testutil.RunCommand(t, binaryPath, cmd...) + if err != nil { + t.Fatalf("NABSL command should work with client config: %v", err) + } + if output == "" { + t.Errorf("Expected help output for %v", cmd) + } + }) + } + }) +} diff --git a/cmd/non-admin/bsl/bsl.go b/cmd/non-admin/bsl/bsl.go index 5c9d48df..57011e23 100644 --- a/cmd/non-admin/bsl/bsl.go +++ b/cmd/non-admin/bsl/bsl.go @@ -26,7 +26,7 @@ func NewBSLCommand(f client.Factory) *cobra.Command { c := &cobra.Command{ Use: "bsl", Short: "Create and manage backup storage locations", - Long: "Create and manage non-admin backup storage locations and their approval requests", + Long: "Create and manage non-admin backup storage locations", } c.AddCommand( diff --git a/cmd/non-admin/bsl/bsl_test.go b/cmd/non-admin/bsl/bsl_test.go new file mode 100644 index 00000000..a6ce111e --- /dev/null +++ b/cmd/non-admin/bsl/bsl_test.go @@ -0,0 +1,114 @@ +/* +Copyright 2025 The OADP CLI 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 bsl + +import ( + "strings" + "testing" + + "github.com/migtools/oadp-cli/internal/testutil" +) + +// TestBSLCommands tests the BSL command functionality +func TestBSLCommands(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + tests := []struct { + name string + args []string + expectContains []string + }{ + { + name: "nonadmin bsl help", + args: []string{"nonadmin", "bsl", "--help"}, + expectContains: []string{ + "Create and manage non-admin backup storage locations", + "create", + }, + }, + { + name: "nonadmin bsl create help", + args: []string{"nonadmin", "bsl", "create", "--help"}, + expectContains: []string{ + "Create a non-admin backup storage location", + "--provider", + "--bucket", + "--credential", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testutil.TestHelpCommand(t, binaryPath, tt.args, tt.expectContains) + }) + } +} + +// TestBSLNoLongerHasRequestCommands verifies that request commands were moved to nabsl +func TestBSLNoLongerHasRequestCommands(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + t.Run("nonadmin bsl no longer has request", func(t *testing.T) { + output, err := testutil.RunCommand(t, binaryPath, "nonadmin", "bsl", "--help") + if err != nil { + t.Fatalf("nonadmin bsl command should exist: %v", err) + } + + // Should have create but not request + if !strings.Contains(output, "create") { + t.Errorf("Expected nonadmin bsl to still have create command") + } + + // Check that "request" doesn't appear as a subcommand in Available Commands section + lines := strings.Split(output, "\n") + inAvailableCommands := false + for _, line := range lines { + if strings.Contains(line, "Available Commands:") { + inAvailableCommands = true + continue + } + if inAvailableCommands && strings.Contains(line, "Flags:") { + inAvailableCommands = false + break + } + if inAvailableCommands && strings.Contains(strings.TrimSpace(line), "request") { + t.Errorf("Expected nonadmin bsl to NOT have request subcommand anymore, but found: %s", strings.TrimSpace(line)) + } + } + }) +} + +// TestBSLCreateUsesNewCredentialFlag verifies the credential flag format +func TestBSLCreateUsesNewCredentialFlag(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + t.Run("nonadmin bsl create uses new credential flag", func(t *testing.T) { + output, err := testutil.RunCommand(t, binaryPath, "nonadmin", "bsl", "create", "--help") + if err != nil { + t.Fatalf("nonadmin bsl create command should exist: %v", err) + } + + // Should use --credential not --credential-name + if !strings.Contains(output, "--credential") { + t.Errorf("Expected nonadmin bsl create to have --credential flag") + } + if strings.Contains(output, "--credential-name") { + t.Errorf("Expected nonadmin bsl create to NOT have --credential-name flag anymore") + } + }) +} diff --git a/cmd/non-admin/nonadmin_test.go b/cmd/non-admin/nonadmin_test.go new file mode 100644 index 00000000..8883fa10 --- /dev/null +++ b/cmd/non-admin/nonadmin_test.go @@ -0,0 +1,86 @@ +/* +Copyright 2025 The OADP CLI 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 nonadmin + +import ( + "testing" + + "github.com/migtools/oadp-cli/internal/testutil" +) + +// TestNonAdminCommands tests the non-admin command functionality +func TestNonAdminCommands(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + tests := []struct { + name string + args []string + expectContains []string + }{ + { + name: "nonadmin help", + args: []string{"nonadmin", "--help"}, + expectContains: []string{ + "Work with non-admin resources", + "Work with non-admin resources like backups", + "backup", + "bsl", + }, + }, + { + name: "nonadmin backup help", + args: []string{"nonadmin", "backup", "--help"}, + expectContains: []string{ + "Work with non-admin backups", + "create", + }, + }, + { + name: "nonadmin backup create help", + args: []string{"nonadmin", "backup", "create", "--help"}, + expectContains: []string{ + "Create a non-admin backup", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testutil.TestHelpCommand(t, binaryPath, tt.args, tt.expectContains) + }) + } +} + +// TestNonAdminHelpFlags tests that both --help and -h work for non-admin commands +func TestNonAdminHelpFlags(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + commands := [][]string{ + {"nonadmin", "--help"}, + {"nonadmin", "-h"}, + {"nonadmin", "backup", "--help"}, + {"nonadmin", "backup", "-h"}, + {"nonadmin", "bsl", "--help"}, + {"nonadmin", "bsl", "-h"}, + } + + for _, cmd := range commands { + t.Run("help_flags_"+cmd[len(cmd)-1], func(t *testing.T) { + testutil.TestHelpCommand(t, binaryPath, cmd, []string{"Usage:"}) + }) + } +} diff --git a/cmd/root_test.go b/cmd/root_test.go new file mode 100644 index 00000000..96f5d073 --- /dev/null +++ b/cmd/root_test.go @@ -0,0 +1,125 @@ +/* +Copyright 2025 The OADP CLI 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 cmd + +import ( + "testing" + + "github.com/migtools/oadp-cli/internal/testutil" +) + +// TestRootCommand tests the root command functionality +func TestRootCommand(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + tests := []struct { + name string + args []string + expectContains []string + }{ + { + name: "root help", + args: []string{"--help"}, + expectContains: []string{ + "OADP CLI commands", + "Available Commands:", + "version", + "backup", + "restore", + "nabsl", + "nonadmin", + }, + }, + { + name: "root help short", + args: []string{"-h"}, + expectContains: []string{ + "OADP CLI commands", + "Available Commands:", + }, + }, + { + name: "version help", + args: []string{"version", "--help"}, + expectContains: []string{ + "Print the velero version and associated image", + }, + }, + { + name: "backup help", + args: []string{"backup", "--help"}, + expectContains: []string{ + "Work with backups", + }, + }, + { + name: "restore help", + args: []string{"restore", "--help"}, + expectContains: []string{ + "Work with restores", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testutil.TestHelpCommand(t, binaryPath, tt.args, tt.expectContains) + }) + } +} + +// TestRootCommandHelpFlags tests that both --help and -h work consistently +func TestRootCommandHelpFlags(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + commands := [][]string{ + {"--help"}, + {"-h"}, + {"backup", "--help"}, + {"backup", "-h"}, + {"restore", "--help"}, + {"restore", "-h"}, + {"version", "--help"}, + {"version", "-h"}, + } + + for _, cmd := range commands { + t.Run("help_flags_"+cmd[len(cmd)-1], func(t *testing.T) { + testutil.TestHelpCommand(t, binaryPath, cmd, []string{"Usage:"}) + }) + } +} + +// TestRootCommandSmoke performs basic smoke tests +func TestRootCommandSmoke(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + smokeCommands := [][]string{ + {"--help"}, + {"-h"}, + {"backup", "--help"}, + {"restore", "--help"}, + {"version", "--help"}, + } + + for _, cmd := range smokeCommands { + t.Run("smoke_"+cmd[0], func(t *testing.T) { + // Just verify commands don't crash + _, _ = testutil.RunCommand(t, binaryPath, cmd...) + }) + } +} diff --git a/integration_test.go b/integration_test.go new file mode 100644 index 00000000..64c1cd4b --- /dev/null +++ b/integration_test.go @@ -0,0 +1,149 @@ +/* +Copyright 2025 The OADP CLI 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 main + +import ( + "os" + "os/exec" + "strings" + "testing" + + "github.com/migtools/oadp-cli/internal/testutil" +) + +// TestBinaryBuild tests that the binary can be built successfully +func TestBinaryBuild(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + // Test that the binary is executable + output, err := testutil.RunCommand(t, binaryPath, "--help") + + // Help command might exit with non-zero, but should produce output + if output == "" { + t.Errorf("Expected help output, but got empty string. Error: %v", err) + } +} + +// TestMakefileInstallation tests the Makefile installation functionality +func TestMakefileInstallation(t *testing.T) { + // Change to project root for make commands + projectRoot := testutil.GetProjectRoot(t) + originalDir, err := os.Getwd() + if err != nil { + t.Fatalf("Failed to get current directory: %v", err) + } + defer os.Chdir(originalDir) + + err = os.Chdir(projectRoot) + if err != nil { + t.Fatalf("Failed to change to project root: %v", err) + } + + t.Run("makefile help shows installation options", func(t *testing.T) { + cmd := exec.Command("make", "help") + output, err := cmd.Output() + if err != nil { + t.Fatalf("Failed to run make help: %v", err) + } + + outputStr := string(output) + expectedOptions := []string{ + "make install", + "ASSUME_DEFAULT=true", + "VELERO_NAMESPACE=velero", + } + + for _, option := range expectedOptions { + if !strings.Contains(outputStr, option) { + t.Errorf("Expected make help to contain %q, but it didn't.\nFull output:\n%s", option, outputStr) + } + } + }) + + t.Run("make build works", func(t *testing.T) { + // Clean first + exec.Command("make", "clean").Run() + + cmd := exec.Command("make", "build") + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("Failed to run make build: %v\nOutput: %s", err, string(output)) + } + + // Check binary was created + if _, err := os.Stat("kubectl-oadp"); os.IsNotExist(err) { + t.Errorf("Binary kubectl-oadp was not created") + } + + // Cleanup + os.Remove("kubectl-oadp") + }) +} + +// TestClientConfigIntegration tests end-to-end client configuration +func TestClientConfigIntegration(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + _, cleanup := testutil.SetupTempHome(t) + defer cleanup() + + t.Run("client config set and get", func(t *testing.T) { + // Set namespace + output, err := testutil.RunCommand(t, binaryPath, "client", "config", "set", "namespace=test-namespace") + if err != nil { + t.Fatalf("Failed to set client config: %v\nOutput: %s", err, output) + } + + // Get namespace + output, err = testutil.RunCommand(t, binaryPath, "client", "config", "get") + if err != nil { + t.Fatalf("Failed to get client config: %v", err) + } + + if !strings.Contains(output, "test-namespace") { + t.Errorf("Expected client config to contain 'test-namespace', got: %s", output) + } + }) +} + +// TestCommandArchitecture tests the overall command structure +func TestCommandArchitecture(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + t.Run("all major commands exist", func(t *testing.T) { + majorCommands := []string{"backup", "restore", "nabsl", "nonadmin", "client", "version"} + + output, _ := testutil.RunCommand(t, binaryPath, "--help") + + for _, cmd := range majorCommands { + if !strings.Contains(output, cmd) { + t.Errorf("Expected root help to contain %q command", cmd) + } + } + }) + + t.Run("nabsl command has correct subcommands", func(t *testing.T) { + expectedSubcommands := []string{"approve", "reject", "describe", "get"} + + output, _ := testutil.RunCommand(t, binaryPath, "nabsl", "--help") + + for _, subcmd := range expectedSubcommands { + if !strings.Contains(output, subcmd) { + t.Errorf("Expected nabsl help to contain %q subcommand", subcmd) + } + } + }) +} diff --git a/internal/testutil/testutil.go b/internal/testutil/testutil.go new file mode 100644 index 00000000..f9d036f2 --- /dev/null +++ b/internal/testutil/testutil.go @@ -0,0 +1,156 @@ +/* +Copyright 2025 The OADP CLI 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 testutil provides shared testing utilities for the OADP CLI +package testutil + +import ( + "bytes" + "context" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + "time" +) + +const ( + // TestTimeout is the default timeout for test operations + TestTimeout = 30 * time.Second +) + +// GetProjectRoot returns the root directory of the project +func GetProjectRoot(t *testing.T) string { + t.Helper() + + // Get the directory of the current file + _, filename, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("Failed to get caller information") + } + + // Navigate up to find the project root (where go.mod is) + dir := filepath.Dir(filename) + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir + } + + parent := filepath.Dir(dir) + if parent == dir { + t.Fatal("Could not find project root (go.mod not found)") + } + dir = parent + } +} + +// BuildCLIBinary builds the CLI binary for testing and returns the path +func BuildCLIBinary(t *testing.T) string { + t.Helper() + + projectRoot := GetProjectRoot(t) + + // Create temp directory for the binary + tempDir := t.TempDir() + binaryPath := filepath.Join(tempDir, "oadp-test") + + t.Logf("Building CLI binary: %s", binaryPath) + t.Logf("Project root: %s", projectRoot) + + // Build the binary + ctx, cancel := context.WithTimeout(context.Background(), TestTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, "go", "build", "-o", binaryPath, ".") + cmd.Dir = projectRoot + output, err := cmd.CombinedOutput() + + if err != nil { + t.Fatalf("Failed to build CLI binary: %v\nOutput: %s", err, string(output)) + } + + return binaryPath +} + +// RunCommand runs a command with the given binary and arguments +func RunCommand(t *testing.T, binaryPath string, args ...string) (string, error) { + t.Helper() + + ctx, cancel := context.WithTimeout(context.Background(), TestTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, binaryPath) + cmd.Args = append(cmd.Args, args...) + + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + err := cmd.Run() + + // Log the command and output for debugging + t.Logf("Command: %s %s", binaryPath, strings.Join(args, " ")) + if stdout.Len() > 0 { + t.Logf("Stdout: %s", stdout.String()) + } + if stderr.Len() > 0 { + t.Logf("Stderr: %s", stderr.String()) + } + + return stdout.String(), err +} + +// TestHelpCommand tests that a command's help output contains expected strings +func TestHelpCommand(t *testing.T, binaryPath string, args []string, expectContains []string) { + t.Helper() + + output, _ := RunCommand(t, binaryPath, args...) + + // Help commands might exit with non-zero, which is normal + t.Logf("Command: %s %s", binaryPath, strings.Join(args, " ")) + t.Logf("Output:\n%s", output) + + // Check that all expected strings are present + for _, expected := range expectContains { + if !strings.Contains(output, expected) { + t.Errorf("Expected output to contain %q, but it didn't.\nFull output:\n%s", expected, output) + } + } +} + +// SetupTempHome creates a temporary home directory for testing client config +func SetupTempHome(t *testing.T) (string, func()) { + t.Helper() + + tempHome := t.TempDir() + configDir := filepath.Join(tempHome, ".config", "velero") + if err := os.MkdirAll(configDir, 0755); err != nil { + t.Fatalf("Failed to create config directory: %v", err) + } + + // Save original HOME + originalHome := os.Getenv("HOME") + os.Setenv("HOME", tempHome) + + // Return cleanup function + cleanup := func() { + os.Setenv("HOME", originalHome) + } + + return tempHome, cleanup +} diff --git a/tests/README.md b/tests/README.md deleted file mode 100644 index 156a63f7..00000000 --- a/tests/README.md +++ /dev/null @@ -1,143 +0,0 @@ -# OADP CLI Tests - -This directory contains organized integration tests for the OADP CLI. - -## Test Structure - -### Core Test Files - -- **`help_test.go`** - 🏆 **Help command tests** (baseline functionality) - - Tests all `--help` and `-h` commands across all paths - - Verifies expected help text appears - - Core functionality that must always work - -- **`build_test.go`** - Binary building and execution tests - - Tests that binary can be built successfully - - Smoke tests for basic command execution - - Version and basic functionality tests - -### Supporting Files - -- **`common.go`** - Shared test utilities and helper functions -- **`main_test.go`** - Test setup and teardown -- **`go.mod`** - Module configuration - -## Running Tests - -### Run All Tests -```bash -# Standard Go command - runs all tests in project -go test ./... - -# Run just the CLI tests -go test -v -timeout 60s ./tests - -# From tests directory -cd tests && go test -v -timeout 60s -``` - -### Run Specific Test Categories -```bash -# From project root -go test -v ./tests -run TestCLIHelp # Help command tests -go test -v ./tests -run TestCLIBinary # Build and smoke tests - -# From tests directory -go test -v -run TestCLIHelp # Help command tests -go test -v -run TestCLIBinary # Build and smoke tests -``` - -### Quick Test Run -```bash -# Standard Go pattern - finds all tests -go test ./... - -# Just CLI tests -go test ./tests - -# With output -go test -v ./tests -``` - -## Test Categories - -### 🏆 Help Tests (`help_test.go`) -These are the **core functionality tests** - verify all help commands work: -- `TestCLIHelpCommands` - All help commands work across all paths -- `TestCLIHelpFlags` - Both `--help` and `-h` work consistently - -### 🔧 Build Tests (`build_test.go`) -Tests that verify the binary itself works: -- `TestCLIBinaryBuild` - Binary builds and executes -- `TestCLIBinaryVersion` - Version command works -- `TestCLIBinarySmoke` - Basic commands don't crash - -## Test Configuration - -- **Build timeout**: 30 seconds -- **Test timeout**: 10 seconds per test -- **Binary name**: `oadp-test` (temporary) -- **Tests local code**: Whatever is currently on disk (including uncommitted changes) - -## Adding New Tests - -### Adding to Existing Categories -Add new test cases to the appropriate file: - -```go -// In help_test.go for new help commands -{ - name: "new command help", - args: []string{"new", "command", "--help"}, - expectContains: []string{ - "Description of new command", - }, -}, -``` - -### Creating New Test Categories -1. Create a new `*_test.go` file -2. Import the `tests` package -3. Use helper functions from `common.go` -4. Follow existing patterns - -Example: -```go -package tests - -import "testing" - -func TestCLINewFeature(t *testing.T) { - binaryPath := buildCLIBinary(t) - defer cleanup(t, binaryPath) - - // Your test logic here -} -``` - -## Troubleshooting - -### Build Failures -- Check that all dependencies in `../go.mod` are available -- Verify you're running from the correct directory -- Check that parent directory has valid Go module - -### Test Failures -- Check expected strings match actual CLI output -- Use `-v` flag to see detailed test output -- Look at the full command output in logs - -### Common Commands -```bash -# Run with verbose output -go test -v ./tests - -# Run with race detection -go test -race ./tests - -# Run specific test function -go test -v ./tests -run TestCLIHelpCommands - -# Run tests with coverage -go test -cover ./tests -``` diff --git a/tests/build_test.go b/tests/build_test.go deleted file mode 100644 index 7e10e264..00000000 --- a/tests/build_test.go +++ /dev/null @@ -1,79 +0,0 @@ -package tests - -import ( - "context" - "os/exec" - "testing" -) - -// TestCLIBinaryBuild tests that the binary can be built successfully -func TestCLIBinaryBuild(t *testing.T) { - binaryPath := buildCLIBinary(t) - defer cleanup(t, binaryPath) - - // Test that the binary is executable - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - defer cancel() - - cmd := exec.CommandContext(ctx, binaryPath, "--help") - err := cmd.Run() - - // Help command might exit with non-zero, but should not fail to execute - if err != nil { - if exitErr, ok := err.(*exec.ExitError); ok { - // Exit code != 0 is often normal for help commands - t.Logf("Binary executed but exited with code: %d", exitErr.ExitCode()) - } else { - t.Fatalf("Failed to execute binary: %v", err) - } - } -} - -// TestCLIBinaryVersion tests that we can build and get version info -func TestCLIBinaryVersion(t *testing.T) { - binaryPath := buildCLIBinary(t) - defer cleanup(t, binaryPath) - - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - defer cancel() - - cmd := exec.CommandContext(ctx, binaryPath, "version", "--client-only") - output, err := cmd.Output() - - // Version command should work - if err != nil { - t.Logf("Version command failed: %v", err) - // Some version commands might fail without proper setup, but we can still check they run - } - - t.Logf("Version output: %s", string(output)) -} - -// TestCLIBinarySmoke performs basic smoke tests -func TestCLIBinarySmoke(t *testing.T) { - binaryPath := buildCLIBinary(t) - defer cleanup(t, binaryPath) - - // Smoke tests - just verify commands don't crash - smokeCommands := [][]string{ - {"--help"}, - {"-h"}, - {"backup", "--help"}, - {"restore", "--help"}, - {"nonadmin", "--help"}, - {"version", "--help"}, - } - - for _, cmd := range smokeCommands { - t.Run("smoke_"+cmd[0], func(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - defer cancel() - - execCmd := exec.CommandContext(ctx, binaryPath) - execCmd.Args = append(execCmd.Args, cmd...) - - // We don't care about exit code for smoke tests, just that it doesn't hang/crash - _ = execCmd.Run() - }) - } -} diff --git a/tests/common.go b/tests/common.go deleted file mode 100644 index 2c86b6fb..00000000 --- a/tests/common.go +++ /dev/null @@ -1,158 +0,0 @@ -/* -Copyright 2025 The OADP CLI 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 tests - -import ( - "bytes" - "context" - "os" - "os/exec" - "path/filepath" - "runtime" - "strings" - "testing" - "time" -) - -const ( - binaryName = "oadp-test" - buildTimeout = 30 * time.Second - testTimeout = 10 * time.Second -) - -// buildCLIBinary builds the CLI binary for testing -// NOTE: This builds from LOCAL code (current filesystem), not repository code -func buildCLIBinary(t *testing.T) string { - t.Helper() - - // Create temporary directory for the binary - tmpDir := t.TempDir() - - // Add .exe extension on Windows - binaryNameWithExt := binaryName - if runtime.GOOS == "windows" { - binaryNameWithExt += ".exe" - } - binaryPath := filepath.Join(tmpDir, binaryNameWithExt) - - // Build the binary from parent directory (project root) - // This uses whatever code is currently on disk (including uncommitted changes) - ctx, cancel := context.WithTimeout(context.Background(), buildTimeout) - defer cancel() - - projectRoot := getProjectRoot(t) - cmd := exec.CommandContext(ctx, "go", "build", "-o", binaryPath, ".") - cmd.Dir = projectRoot - - var stderr bytes.Buffer - cmd.Stderr = &stderr - - t.Logf("Building CLI binary: %s", binaryPath) - t.Logf("Project root: %s", projectRoot) - - if err := cmd.Run(); err != nil { - t.Fatalf("Failed to build CLI binary: %v\nStderr: %s", err, stderr.String()) - } - - // Verify the binary was created - if _, err := os.Stat(binaryPath); err != nil { - t.Fatalf("Binary not found after build: %v", err) - } - - return binaryPath -} - -// getProjectRoot returns the project root directory -func getProjectRoot(t *testing.T) string { - t.Helper() - - // Start from the current directory (tests folder) - dir, err := os.Getwd() - if err != nil { - t.Fatalf("Failed to get working directory: %v", err) - } - - // Look for go.mod in current dir and parent directories - for { - goModPath := filepath.Join(dir, "go.mod") - if _, err := os.Stat(goModPath); err == nil { - // Check if this is the main project go.mod (not the tests go.mod) - if filepath.Base(dir) != "tests" { - return dir - } - } - - parent := filepath.Dir(dir) - if parent == dir { - break - } - dir = parent - } - - t.Fatalf("Could not find project root (go.mod not found)") - return "" -} - -// testHelpCommand tests a help command and verifies expected content -func testHelpCommand(t *testing.T, binaryPath string, args []string, expectContains []string) { - t.Helper() - - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - defer cancel() - - cmd := exec.CommandContext(ctx, binaryPath, args...) - - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - - err := cmd.Run() - output := stdout.String() - - // Help commands typically exit with 0 - if err != nil { - t.Logf("Command failed (this might be expected for some help commands): %v", err) - t.Logf("Stderr: %s", stderr.String()) - // For help commands, we often get the help text in stderr too - if stderr.Len() > 0 { - output += stderr.String() - } - } - - t.Logf("Command: %s %s", binaryPath, strings.Join(args, " ")) - t.Logf("Output:\n%s", output) - - // Check that expected content is present - for _, expected := range expectContains { - if !strings.Contains(output, expected) { - t.Errorf("Expected output to contain %q, but it didn't.\nFull output:\n%s", expected, output) - } - } - - // Basic sanity check - help output should not be empty - if len(strings.TrimSpace(output)) == 0 { - t.Error("Help output was empty") - } -} - -// cleanup removes test artifacts -func cleanup(t *testing.T, binaryPath string) { - t.Helper() - if err := os.Remove(binaryPath); err != nil && !os.IsNotExist(err) { - t.Logf("Warning: Failed to cleanup binary %s: %v", binaryPath, err) - } -} diff --git a/tests/help_test.go b/tests/help_test.go deleted file mode 100644 index d31ba631..00000000 --- a/tests/help_test.go +++ /dev/null @@ -1,182 +0,0 @@ -package tests - -import "testing" - -// TestCLIHelpCommands tests all help commands - this is the baseline test suite -// These tests verify that all command paths have working help documentation -func TestCLIHelpCommands(t *testing.T) { - // Build the binary first - binaryPath := buildCLIBinary(t) - defer cleanup(t, binaryPath) - - // Define all command paths to test - testCases := []struct { - name string - args []string - expectContains []string - }{ - { - name: "root help", - args: []string{"--help"}, - expectContains: []string{ - "OADP CLI commands", - "Available Commands:", - "version", - "backup", - "restore", - "nonadmin", - }, - }, - { - name: "root help short", - args: []string{"-h"}, - expectContains: []string{ - "OADP CLI commands", - "Available Commands:", - }, - }, - { - name: "version help", - args: []string{"version", "--help"}, - expectContains: []string{ - "Print the velero version and associated image", - }, - }, - { - name: "backup help", - args: []string{"backup", "--help"}, - expectContains: []string{ - "Work with backups", - }, - }, - { - name: "restore help", - args: []string{"restore", "--help"}, - expectContains: []string{ - "Work with restores", - }, - }, - { - name: "nonadmin help", - args: []string{"nonadmin", "--help"}, - expectContains: []string{ - "Work with non-admin resources", - "Work with non-admin resources like backups", - "backup", - "bsl", - }, - }, - { - name: "nonadmin backup help", - args: []string{"nonadmin", "backup", "--help"}, - expectContains: []string{ - "Work with non-admin backups", - "create", - }, - }, - { - name: "nonadmin backup create help", - args: []string{"nonadmin", "backup", "create", "--help"}, - expectContains: []string{ - "Create a non-admin backup", - }, - }, - { - name: "nonadmin bsl help", - args: []string{"nonadmin", "bsl", "--help"}, - expectContains: []string{ - "Create and manage non-admin backup storage locations", - "create", - "request", - }, - }, - { - name: "nonadmin bsl create help", - args: []string{"nonadmin", "bsl", "create", "--help"}, - expectContains: []string{ - "Create a non-admin backup storage location", - "--provider", - "--bucket", - "--credential-name", - }, - }, - { - name: "nonadmin bsl request help", - args: []string{"nonadmin", "bsl", "request", "--help"}, - expectContains: []string{ - "View and manage approval requests", - "approve", - "deny", - "describe", - "get", - }, - }, - { - name: "nonadmin bsl request approve help", - args: []string{"nonadmin", "bsl", "request", "approve", "--help"}, - expectContains: []string{ - "Approve a pending backup storage location request", - "--reason", - }, - }, - { - name: "nonadmin bsl request deny help", - args: []string{"nonadmin", "bsl", "request", "deny", "--help"}, - expectContains: []string{ - "Deny a pending backup storage location request", - "--reason", - }, - }, - { - name: "nonadmin bsl request get help", - args: []string{"nonadmin", "bsl", "request", "get", "--help"}, - expectContains: []string{ - "Get non-admin backup storage location requests", - }, - }, - { - name: "nonadmin bsl request describe help", - args: []string{"nonadmin", "bsl", "request", "describe", "--help"}, - expectContains: []string{ - "Describe a non-admin backup storage location request", - }, - }, - } - - // Run tests for each command path - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - testHelpCommand(t, binaryPath, tc.args, tc.expectContains) - }) - } -} - -// TestCLIHelpFlags tests that both --help and -h work consistently -func TestCLIHelpFlags(t *testing.T) { - binaryPath := buildCLIBinary(t) - defer cleanup(t, binaryPath) - - // Test both flags produce similar output - commands := [][]string{ - {"--help"}, - {"-h"}, - {"nonadmin", "--help"}, - {"nonadmin", "-h"}, - {"backup", "--help"}, - {"backup", "-h"}, - {"nonadmin", "bsl", "--help"}, - {"nonadmin", "bsl", "-h"}, - {"nonadmin", "bsl", "request", "--help"}, - {"nonadmin", "bsl", "request", "-h"}, - {"nonadmin", "bsl", "request", "approve", "--help"}, - {"nonadmin", "bsl", "request", "approve", "-h"}, - {"nonadmin", "bsl", "request", "deny", "--help"}, - {"nonadmin", "bsl", "request", "deny", "-h"}, - } - - for _, cmd := range commands { - t.Run("help_flags_"+cmd[len(cmd)-1], func(t *testing.T) { - testHelpCommand(t, binaryPath, cmd, []string{"Usage:"}) - }) - } -} diff --git a/tests/main_test.go b/tests/main_test.go deleted file mode 100644 index 1e2d2aac..00000000 --- a/tests/main_test.go +++ /dev/null @@ -1,50 +0,0 @@ -package tests - -import ( - "os" - "path/filepath" - "testing" -) - -// TestMain sets up the test environment -func TestMain(m *testing.M) { - // Ensure we can find the project root - if err := findProjectRoot(); err != nil { - panic("Could not find project root: " + err.Error()) - } - - // Run tests - code := m.Run() - - // Clean up any global test artifacts here if needed - - os.Exit(code) -} - -// findProjectRoot ensures we can locate the project root from the tests directory -func findProjectRoot() error { - // Get current working directory - dir, err := os.Getwd() - if err != nil { - return err - } - - // Look for go.mod in current dir and parent directories - for { - goModPath := filepath.Join(dir, "go.mod") - if _, err := os.Stat(goModPath); err == nil { - // Check if this is the main project go.mod (not the tests go.mod) - if filepath.Base(dir) != "tests" { - return nil // Found main project go.mod - } - } - - parent := filepath.Dir(dir) - if parent == dir { - break - } - dir = parent - } - - return os.ErrNotExist -} From 3c5a8d3d4302cb381c1b1a16f049c3112ef32243 Mon Sep 17 00:00:00 2001 From: Joseph Date: Wed, 6 Aug 2025 14:58:44 -0400 Subject: [PATCH 4/9] Enhance integration tests and improve error handling - Added runtime checks for binary creation in `integration_test.go` to support Windows compatibility. - Improved error handling in the Makefile installation test by logging non-fatal errors during cleanup. - Updated the binary path in `testutil.go` to accommodate Windows executable naming conventions. - Removed unnecessary code in `bsl_test.go` to streamline test logic. --- cmd/non-admin/bsl/bsl_test.go | 1 - cmd/non-admin/nonadmin.go | 1 - integration_test.go | 21 ++++++++++++++++----- internal/testutil/testutil.go | 6 +++++- 4 files changed, 21 insertions(+), 8 deletions(-) diff --git a/cmd/non-admin/bsl/bsl_test.go b/cmd/non-admin/bsl/bsl_test.go index a6ce111e..d3df06db 100644 --- a/cmd/non-admin/bsl/bsl_test.go +++ b/cmd/non-admin/bsl/bsl_test.go @@ -83,7 +83,6 @@ func TestBSLNoLongerHasRequestCommands(t *testing.T) { continue } if inAvailableCommands && strings.Contains(line, "Flags:") { - inAvailableCommands = false break } if inAvailableCommands && strings.Contains(strings.TrimSpace(line), "request") { diff --git a/cmd/non-admin/nonadmin.go b/cmd/non-admin/nonadmin.go index 72d84b3f..38dc65ea 100644 --- a/cmd/non-admin/nonadmin.go +++ b/cmd/non-admin/nonadmin.go @@ -40,4 +40,3 @@ func NewNonAdminCommand(f client.Factory) *cobra.Command { return c } - diff --git a/integration_test.go b/integration_test.go index 64c1cd4b..25e9a6e2 100644 --- a/integration_test.go +++ b/integration_test.go @@ -19,6 +19,7 @@ package main import ( "os" "os/exec" + "runtime" "strings" "testing" @@ -46,7 +47,11 @@ func TestMakefileInstallation(t *testing.T) { if err != nil { t.Fatalf("Failed to get current directory: %v", err) } - defer os.Chdir(originalDir) + defer func() { + if err := os.Chdir(originalDir); err != nil { + t.Logf("Failed to restore original directory: %v", err) + } + }() err = os.Chdir(projectRoot) if err != nil { @@ -76,7 +81,9 @@ func TestMakefileInstallation(t *testing.T) { t.Run("make build works", func(t *testing.T) { // Clean first - exec.Command("make", "clean").Run() + if err := exec.Command("make", "clean").Run(); err != nil { + t.Logf("Failed to clean (non-fatal): %v", err) + } cmd := exec.Command("make", "build") output, err := cmd.CombinedOutput() @@ -85,12 +92,16 @@ func TestMakefileInstallation(t *testing.T) { } // Check binary was created - if _, err := os.Stat("kubectl-oadp"); os.IsNotExist(err) { - t.Errorf("Binary kubectl-oadp was not created") + binaryName := "kubectl-oadp" + if runtime.GOOS == "windows" { + binaryName += ".exe" + } + if _, err := os.Stat(binaryName); os.IsNotExist(err) { + t.Errorf("Binary %s was not created", binaryName) } // Cleanup - os.Remove("kubectl-oadp") + os.Remove(binaryName) }) } diff --git a/internal/testutil/testutil.go b/internal/testutil/testutil.go index f9d036f2..f27f2be4 100644 --- a/internal/testutil/testutil.go +++ b/internal/testutil/testutil.go @@ -67,7 +67,11 @@ func BuildCLIBinary(t *testing.T) string { // Create temp directory for the binary tempDir := t.TempDir() - binaryPath := filepath.Join(tempDir, "oadp-test") + binaryName := "oadp-test" + if runtime.GOOS == "windows" { + binaryName += ".exe" + } + binaryPath := filepath.Join(tempDir, binaryName) t.Logf("Building CLI binary: %s", binaryPath) t.Logf("Project root: %s", projectRoot) From 291b8635ad0b30a96ae8655a4f95c241a35d4303 Mon Sep 17 00:00:00 2001 From: Joseph Date: Wed, 6 Aug 2025 15:01:00 -0400 Subject: [PATCH 5/9] Mod tidy --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 8820c3d9..8120d7e4 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ toolchain go1.24.3 require ( github.com/migtools/oadp-non-admin v0.0.0-20250505165924-a9be4321819c + github.com/pkg/errors v0.9.1 github.com/spf13/cobra v1.9.1 github.com/spf13/pflag v1.0.6 github.com/vmware-tanzu/velero v1.14.0 @@ -69,7 +70,6 @@ require ( github.com/modern-go/reflect2 v1.0.2 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/openshift/oadp-operator v1.0.2-0.20250425163444-a21288a0f20b // indirect - github.com/pkg/errors v0.9.1 // indirect github.com/prometheus/client_golang v1.20.5 // indirect github.com/prometheus/client_model v0.6.1 // indirect github.com/prometheus/common v0.62.0 // indirect From 230fcd96fa448833972f899ebff101372c5397f3 Mon Sep 17 00:00:00 2001 From: Joseph Date: Wed, 6 Aug 2025 15:40:29 -0400 Subject: [PATCH 6/9] Windows exe --- Makefile | 47 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/Makefile b/Makefile index 29b9a5c1..fb2a5e51 100644 --- a/Makefile +++ b/Makefile @@ -68,21 +68,38 @@ help: ## Show this help message .PHONY: build build: ## Build the kubectl plugin binary (use PLATFORM=os/arch for cross-compilation) @if [ -n "$(PLATFORM)" ]; then \ + if [ "$(GOOS)" = "windows" ]; then \ + binary_suffix=".exe"; \ + else \ + binary_suffix=""; \ + fi; \ echo "Building $(BINARY_NAME) for $(PLATFORM)..."; \ - GOOS=$(GOOS) GOARCH=$(GOARCH) go build -o $(BINARY_NAME)-$(GOOS)-$(GOARCH) .; \ - echo "✅ Built $(BINARY_NAME)-$(GOOS)-$(GOARCH) successfully!"; \ + GOOS=$(GOOS) GOARCH=$(GOARCH) go build -o $(BINARY_NAME)-$(GOOS)-$(GOARCH)$$binary_suffix .; \ + echo "✅ Built $(BINARY_NAME)-$(GOOS)-$(GOARCH)$$binary_suffix successfully!"; \ else \ - echo "Building $(BINARY_NAME) for current platform ($$(go env GOOS)/$$(go env GOARCH))..."; \ - go build -o $(BINARY_NAME) .; \ - echo "✅ Built $(BINARY_NAME) successfully!"; \ + GOOS=$$(go env GOOS); \ + if [ "$$GOOS" = "windows" ]; then \ + binary_name="$(BINARY_NAME).exe"; \ + else \ + binary_name="$(BINARY_NAME)"; \ + fi; \ + echo "Building $$binary_name for current platform ($$GOOS/$$(go env GOARCH))..."; \ + go build -o $$binary_name .; \ + echo "✅ Built $$binary_name successfully!"; \ fi # Installation targets .PHONY: install install: build ## Build and install the kubectl plugin to ~/.local/bin (no sudo required) - @echo "Installing $(BINARY_NAME) to $(INSTALL_PATH)..." - @mkdir -p $(INSTALL_PATH) - cp $(BINARY_NAME) $(INSTALL_PATH)/ + @GOOS=$$(go env GOOS); \ + if [ "$$GOOS" = "windows" ]; then \ + binary_name="$(BINARY_NAME).exe"; \ + else \ + binary_name="$(BINARY_NAME)"; \ + fi; \ + echo "Installing $$binary_name to $(INSTALL_PATH)..."; \ + mkdir -p $(INSTALL_PATH); \ + cp $$binary_name $(INSTALL_PATH)/ @echo "✅ Installed to $(INSTALL_PATH)" @echo "" @PATH_UPDATED=false; \ @@ -132,9 +149,15 @@ install: build ## Build and install the kubectl plugin to ~/.local/bin (no sudo fi; \ echo ""; \ fi; \ - echo "Setting Velero namespace to: $$NAMESPACE"; \ - $(INSTALL_PATH)/$(BINARY_NAME) client config set namespace=$$NAMESPACE 2>/dev/null || true; \ - echo "✅ Client config initialized"; \ + echo "Setting Velero namespace to: $$NAMESPACE"; \ + GOOS=$$(go env GOOS); \ + if [ "$$GOOS" = "windows" ]; then \ + binary_name="$(BINARY_NAME).exe"; \ + else \ + binary_name="$(BINARY_NAME)"; \ + fi; \ + $(INSTALL_PATH)/$$binary_name client config set namespace=$$NAMESPACE 2>/dev/null || true; \ + echo "✅ Client config initialized"; \ echo ""; \ echo "📋 Next steps:"; \ echo " 1. Test admin commands: kubectl oadp backup get"; \ @@ -239,7 +262,7 @@ test-integration: ## Run integration tests only .PHONY: clean clean: ## Remove built binaries @echo "Cleaning up..." - @rm -f $(BINARY_NAME) $(BINARY_NAME)-linux-* $(BINARY_NAME)-darwin-* $(BINARY_NAME)-windows-* + @rm -f $(BINARY_NAME) $(BINARY_NAME).exe $(BINARY_NAME)-linux-* $(BINARY_NAME)-darwin-* $(BINARY_NAME)-windows-* @rm -f *.tar.gz *.sha256 @rm -f oadp-*.yaml oadp-*.yaml.tmp @echo "✅ Cleanup complete!" From f15b0405bfcc3bad99f988859ad3436485ea8283 Mon Sep 17 00:00:00 2001 From: Joseph Date: Thu, 7 Aug 2025 12:12:37 -0400 Subject: [PATCH 7/9] Enhance installation process with automatic namespace detection - Updated the Makefile to include intelligent detection for OADP deployment namespaces during installation. - Improved help text in the Makefile and README to clarify installation options and the detection process. - Added detailed documentation in README and TESTING.md outlining the automatic detection steps and installation modes. - Ensured fallback mechanisms for namespace detection to enhance user experience. --- Makefile | 54 ++++++++++++++++++++++++++++++++++++++++++------------ README.md | 33 +++++++++++++++++++-------------- TESTING.md | 28 +++++++++++++++++++++++++++- 3 files changed, 88 insertions(+), 27 deletions(-) diff --git a/Makefile b/Makefile index fb2a5e51..7528e12f 100644 --- a/Makefile +++ b/Makefile @@ -33,9 +33,9 @@ help: ## Show this help message @awk 'BEGIN {FS = ":.*?## "} /^[a-zA-Z_-]+:.*?## / {printf " \033[36m%-15s\033[0m %s\n", $$1, $$2}' $(MAKEFILE_LIST) @echo "" @echo "Installation options:" - @echo " \033[36mmake install\033[0m # Install with interactive namespace prompt" - @echo " \033[36mmake install ASSUME_DEFAULT=true\033[0m # Install with default namespace (no prompt)" - @echo " \033[36mmake install VELERO_NAMESPACE=velero\033[0m # Install with custom namespace (no prompt)" + @echo " \033[36mmake install\033[0m # Install with auto-detection & interactive prompt" + @echo " \033[36mmake install ASSUME_DEFAULT=true\033[0m # Install with default namespace (no detection/prompt)" + @echo " \033[36mmake install VELERO_NAMESPACE=velero\033[0m # Install with custom namespace (no detection/prompt)" @echo " \033[36mmake install-user\033[0m # Same as install (legacy alias)" @echo " \033[36mmake install-bin\033[0m # Install to ~/bin (alternative, no sudo)" @echo " \033[36mmake install-system\033[0m # Install to /usr/local/bin (requires sudo)" @@ -135,21 +135,51 @@ install: build ## Build and install the kubectl plugin to ~/.local/bin (no sudo echo "🔄 Restart terminal or run: source ~/.zshrc"; \ fi; \ echo ""; \ - echo "📋 Configuration:"; \ + echo "📋 Configuration:"; \ NAMESPACE=$(VELERO_NAMESPACE); \ + DETECTED=false; \ if [[ "$(ASSUME_DEFAULT)" != "true" && "$(VELERO_NAMESPACE)" == "openshift-adp" ]]; then \ echo ""; \ - echo "🤔 Which namespace should admin commands use for Velero resources?"; \ - echo " (Common options: openshift-adp, velero, oadp)"; \ - echo ""; \ - printf "Enter namespace [default: $(VELERO_NAMESPACE)]: "; \ - read -r user_input; \ - if [[ -n "$$user_input" ]]; then \ - NAMESPACE=$$user_input; \ + echo "🔍 Detecting OADP deployment in cluster..."; \ + DETECTED_NS=$$(kubectl get deployment openshift-adp-controller-manager --all-namespaces -o jsonpath='{.items[0].metadata.namespace}' 2>/dev/null | head -1); \ + if [[ -n "$$DETECTED_NS" ]]; then \ + echo "✅ Found OADP controller in namespace: $$DETECTED_NS"; \ + NAMESPACE=$$DETECTED_NS; \ + DETECTED=true; \ + else \ + echo " Could not find openshift-adp-controller-manager deployment"; \ + echo "🔍 Looking for DataProtectionApplication (DPA) resources..."; \ + DETECTED_NS=$$(kubectl get dataprotectionapplication --all-namespaces -o jsonpath='{.items[0].metadata.namespace}' 2>/dev/null | head -1); \ + if [[ -n "$$DETECTED_NS" ]]; then \ + echo "✅ Found DPA resource in namespace: $$DETECTED_NS"; \ + NAMESPACE=$$DETECTED_NS; \ + DETECTED=true; \ + else \ + echo " Could not find DataProtectionApplication resources"; \ + echo "🔍 Looking for Velero deployment as fallback..."; \ + DETECTED_NS=$$(kubectl get deployment velero --all-namespaces -o jsonpath='{.items[0].metadata.namespace}' 2>/dev/null | head -1); \ + if [[ -n "$$DETECTED_NS" ]]; then \ + echo "✅ Found Velero deployment in namespace: $$DETECTED_NS"; \ + NAMESPACE=$$DETECTED_NS; \ + DETECTED=true; \ + else \ + echo "⚠️ Could not detect OADP or Velero deployment in cluster"; \ + fi; \ + fi; \ + fi; \ + if [[ "$$DETECTED" == "false" ]]; then \ + echo "🤔 Which namespace should admin commands use for Velero resources?"; \ + echo " (Common options: openshift-adp, velero, oadp)"; \ + echo ""; \ + printf "Enter namespace [default: $(VELERO_NAMESPACE)]: "; \ + read -r user_input; \ + if [[ -n "$$user_input" ]]; then \ + NAMESPACE=$$user_input; \ + fi; \ fi; \ echo ""; \ fi; \ - echo "Setting Velero namespace to: $$NAMESPACE"; \ + echo "Setting Velero namespace to: $$NAMESPACE"; \ GOOS=$$(go env GOOS); \ if [ "$$GOOS" = "windows" ]; then \ binary_name="$(BINARY_NAME).exe"; \ diff --git a/README.md b/README.md index 5e76999c..60b96493 100644 --- a/README.md +++ b/README.md @@ -30,23 +30,10 @@ kubectl oadp ## Installation -### Using Krew (Available soon!) - -```sh -# Install Krew if you haven't already -kubectl krew install krew - -# Install the OADP plugin -kubectl krew install oadp - -# Verify installation -kubectl oadp --help -``` - ### Manual Build and Install ```sh -# Recommended: Rootless install (no sudo required) +# Recommended: Smart install with auto-detection (no sudo required) make install # After install, refresh your terminal: @@ -60,8 +47,26 @@ kubectl oadp --help make install-system ``` +The `make install` command automatically detects your OADP deployment namespace by looking for: +1. **OADP Controller** (`openshift-adp-controller-manager` deployment) +2. **DPA Resources** (`DataProtectionApplication` custom resources) +3. **Velero Deployment** (fallback for vanilla Velero installations) + +If no OADP resources are detected, you'll be prompted to specify the namespace manually. + +**Installation Options:** +```sh +make install # Smart detection + interactive prompt +make install ASSUME_DEFAULT=true # Use default namespace (no detection) +make install VELERO_NAMESPACE=custom # Use specific namespace (no detection) +``` + **💡 Important:** After installation, you may need to refresh your terminal or run `source ~/.zshrc` (or `~/.bashrc`) for the `kubectl oadp` command to work. +You can set the velero namespace afterwards using the oadp client command + + + ## Usage Guide ### Non-Admin Backup Operations diff --git a/TESTING.md b/TESTING.md index c1b9452a..7e070c98 100644 --- a/TESTING.md +++ b/TESTING.md @@ -159,4 +159,30 @@ func TestNewCommand(t *testing.T) { 5. **Mock external dependencies** when possible 6. **Use descriptive test names** that explain what's being tested -This testing architecture ensures comprehensive coverage while maintaining clarity and ease of maintenance. \ No newline at end of file +This testing architecture ensures comprehensive coverage while maintaining clarity and ease of maintenance. + +## Installation Features + +The `make install` command includes intelligent namespace detection that automatically discovers where OADP is deployed in your cluster: + +### Automatic Detection Process + +1. **OADP Controller Detection**: Looks for `openshift-adp-controller-manager` deployment +2. **DPA Resource Detection**: Searches for `DataProtectionApplication` custom resources +3. **Velero Fallback**: Falls back to looking for `velero` deployment +4. **Interactive Prompt**: If no resources found, prompts for manual input + +### Installation Modes + +```bash +# Smart detection + interactive prompt (default) +make install + +# Skip detection, use default namespace +make install ASSUME_DEFAULT=true + +# Skip detection, use custom namespace +make install VELERO_NAMESPACE=my-oadp-namespace +``` + +This intelligent detection eliminates the guesswork of finding the correct OADP namespace in your cluster. \ No newline at end of file From b56ee2e31d45e3012feb454553971c814a2656b5 Mon Sep 17 00:00:00 2001 From: Joseph Date: Fri, 8 Aug 2025 12:44:40 -0400 Subject: [PATCH 8/9] Move shared code --- README.md | 1 + TESTING.md | 2 +- cmd/{nabsl => nabsl-request}/approve.go | 35 +-- cmd/{nabsl => nabsl-request}/describe.go | 4 +- cmd/{nabsl => nabsl-request}/get.go | 8 +- cmd/{nabsl => nabsl-request}/nabsl.go | 24 +- cmd/{nabsl => nabsl-request}/nabsl_test.go | 58 ++--- cmd/{nabsl => nabsl-request}/reject.go | 35 +-- cmd/non-admin/backup/backup_test.go | 261 +++++++++++++++++++++ cmd/non-admin/bsl/bsl_test.go | 2 +- cmd/root.go | 6 +- cmd/root_test.go | 2 +- cmd/shared/factories.go | 85 +++++++ cmd/shared/nabsl_requests.go | 62 +++++ cmd/shared/test_helpers.go | 83 +++++++ integration_test.go | 8 +- 16 files changed, 555 insertions(+), 121 deletions(-) rename cmd/{nabsl => nabsl-request}/approve.go (74%) rename cmd/{nabsl => nabsl-request}/describe.go (97%) rename cmd/{nabsl => nabsl-request}/get.go (96%) rename cmd/{nabsl => nabsl-request}/nabsl.go (63%) rename cmd/{nabsl => nabsl-request}/nabsl_test.go (67%) rename cmd/{nabsl => nabsl-request}/reject.go (73%) create mode 100644 cmd/non-admin/backup/backup_test.go create mode 100644 cmd/shared/factories.go create mode 100644 cmd/shared/nabsl_requests.go create mode 100644 cmd/shared/test_helpers.go diff --git a/README.md b/README.md index 60b96493..78f86c17 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,7 @@ kubectl oadp ├── backup # Velero cluster-wide backups (admin) ├── restore # Velero cluster-wide restores (admin) ├── version # Version information +├── nabsl-request # Manage NonAdminBackupStorageLocation approval requests └── nonadmin (na) # Namespace-scoped operations (non-admin) └── backup ├── create diff --git a/TESTING.md b/TESTING.md index 7e070c98..d061f0f2 100644 --- a/TESTING.md +++ b/TESTING.md @@ -69,7 +69,7 @@ make test-integration ### Specific Package ```bash # Test specific command -go test ./cmd/nabsl -v +go test ./cmd/nabsl-request -v # Test with coverage go test ./cmd/... -cover diff --git a/cmd/nabsl/approve.go b/cmd/nabsl-request/approve.go similarity index 74% rename from cmd/nabsl/approve.go rename to cmd/nabsl-request/approve.go index 3fda708c..a4dccb21 100644 --- a/cmd/nabsl/approve.go +++ b/cmd/nabsl-request/approve.go @@ -40,10 +40,10 @@ func NewApproveCommand(f client.Factory) *cobra.Command { Long: "Approve a pending backup storage location request to allow the controller to create the corresponding BackupStorageLocation", Args: cobra.ExactArgs(1), Example: ` # Approve a request by NABSL name (admin access required) - kubectl oadp nabsl approve user-test-bsl + kubectl oadp nabsl-request approve user-test-bsl # Approve a request by UUID with reason - kubectl oadp nabsl approve nacuser01-user-test-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 --reason "Approved for production use"`, + kubectl oadp nabsl-request approve nacuser01-user-test-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 --reason "Approved for production use"`, Run: func(c *cobra.Command, args []string) { cmd.CheckError(o.Complete(args, f)) cmd.CheckError(o.Validate(c, args, f)) @@ -94,7 +94,7 @@ func (o *ApproveOptions) Run(c *cobra.Command, f client.Factory) error { adminNS := f.Namespace() // Find the request either by UUID or by looking up NABSL name - requestName, err := o.findRequestName(adminNS) + requestName, err := shared.FindNABSLRequestByNameOrUUID(context.Background(), o.client, o.RequestName, adminNS) if err != nil { return err } @@ -141,32 +141,3 @@ func (o *ApproveOptions) Run(c *cobra.Command, f client.Factory) error { return nil } -func (o *ApproveOptions) findRequestName(adminNS string) (string, error) { - // First check if o.RequestName is already a UUID by trying to get it directly - var testRequest nacv1alpha1.NonAdminBackupStorageLocationRequest - err := o.client.Get(context.Background(), kbclient.ObjectKey{ - Name: o.RequestName, - Namespace: adminNS, - }, &testRequest) - if err == nil { - // Found it directly, it's a UUID - return o.RequestName, nil - } - - // Not found directly, so o.RequestName might be a NABSL name - // We need to search through all requests to find one with matching source NABSL name - var requestList nacv1alpha1.NonAdminBackupStorageLocationRequestList - err = o.client.List(context.Background(), &requestList, kbclient.InNamespace(adminNS)) - if err != nil { - return "", fmt.Errorf("failed to list requests: %w", err) - } - - for _, request := range requestList.Items { - if request.Status.SourceNonAdminBSL != nil && - request.Status.SourceNonAdminBSL.Name == o.RequestName { - return request.Name, nil - } - } - - return "", fmt.Errorf("request for NABSL %q not found", o.RequestName) -} diff --git a/cmd/nabsl/describe.go b/cmd/nabsl-request/describe.go similarity index 97% rename from cmd/nabsl/describe.go rename to cmd/nabsl-request/describe.go index ddc25da4..2caa3520 100644 --- a/cmd/nabsl/describe.go +++ b/cmd/nabsl-request/describe.go @@ -44,10 +44,10 @@ func NewDescribeCommand(f client.Factory) *cobra.Command { cmd.CheckError(o.Run(c, f)) }, Example: ` # Describe a request by NABSL name - kubectl oadp nabsl describe my-bsl-request + kubectl oadp nabsl-request describe my-bsl-request # Describe a request by UUID - kubectl oadp nabsl describe nacuser01-my-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8`, + kubectl oadp nabsl-request describe nacuser01-my-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8`, } return c diff --git a/cmd/nabsl/get.go b/cmd/nabsl-request/get.go similarity index 96% rename from cmd/nabsl/get.go rename to cmd/nabsl-request/get.go index 6a800658..00729edd 100644 --- a/cmd/nabsl/get.go +++ b/cmd/nabsl-request/get.go @@ -47,16 +47,16 @@ func NewGetCommand(f client.Factory) *cobra.Command { cmd.CheckError(o.Run(c, f)) }, Example: ` # Get all backup storage location requests (admin access required) - kubectl oadp nabsl get + kubectl oadp nabsl-request get # Get a specific request by NABSL name - kubectl oadp nabsl get my-bsl-request + kubectl oadp nabsl-request get my-bsl-request # Get a specific request by UUID - kubectl oadp nabsl get nacuser01-my-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 + kubectl oadp nabsl-request get nacuser01-my-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 # Get output in YAML format - kubectl oadp nabsl get my-bsl-request -o yaml`, + kubectl oadp nabsl-request get my-bsl-request -o yaml`, } o.BindFlags(c.Flags()) diff --git a/cmd/nabsl/nabsl.go b/cmd/nabsl-request/nabsl.go similarity index 63% rename from cmd/nabsl/nabsl.go rename to cmd/nabsl-request/nabsl.go index 87788576..e4003f16 100644 --- a/cmd/nabsl/nabsl.go +++ b/cmd/nabsl-request/nabsl.go @@ -21,28 +21,28 @@ import ( "github.com/vmware-tanzu/velero/pkg/client" ) -// NewNABSLCommand creates the "nabsl" command for managing non-admin backup storage location requests -func NewNABSLCommand(f client.Factory) *cobra.Command { +// NewNABSLRequestCommand creates the "nabsl-request" command for managing non-admin backup storage location requests +func NewNABSLRequestCommand(f client.Factory) *cobra.Command { c := &cobra.Command{ - Use: "nabsl", - Short: "Manage non-admin backup storage location requests", + Use: "nabsl-request", + Short: "Manage non-admin backup storage location approval requests", Long: `Manage approval requests for non-admin backup storage locations. Non-admin backup storage locations (NABSL) require admin approval before they can be used. When users create NABSLs, approval requests are automatically generated for admin review. Use these commands to view, approve, or reject pending NABSL requests.`, - Example: ` # List all pending NABSL requests - kubectl oadp nabsl get + Example: ` # List all pending NABSL approval requests + kubectl oadp nabsl-request get - # Describe a specific NABSL request - kubectl oadp nabsl describe my-storage-request + # Describe a specific NABSL approval request + kubectl oadp nabsl-request describe my-storage-request - # Approve a NABSL request - kubectl oadp nabsl approve my-storage-request + # Approve a NABSL approval request + kubectl oadp nabsl-request approve my-storage-request - # Reject a NABSL request - kubectl oadp nabsl reject my-storage-request`, + # Reject a NABSL approval request + kubectl oadp nabsl-request reject my-storage-request`, } c.AddCommand( diff --git a/cmd/nabsl/nabsl_test.go b/cmd/nabsl-request/nabsl_test.go similarity index 67% rename from cmd/nabsl/nabsl_test.go rename to cmd/nabsl-request/nabsl_test.go index 96bc4e57..54f21dba 100644 --- a/cmd/nabsl/nabsl_test.go +++ b/cmd/nabsl-request/nabsl_test.go @@ -32,8 +32,8 @@ func TestNABSLCommands(t *testing.T) { expectContains []string }{ { - name: "nabsl help", - args: []string{"nabsl", "--help"}, + name: "nabsl-request help", + args: []string{"nabsl-request", "--help"}, expectContains: []string{ "Manage approval requests for non-admin backup storage locations", "approve", @@ -43,31 +43,31 @@ func TestNABSLCommands(t *testing.T) { }, }, { - name: "nabsl approve help", - args: []string{"nabsl", "approve", "--help"}, + name: "nabsl-request approve help", + args: []string{"nabsl-request", "approve", "--help"}, expectContains: []string{ "Approve a pending backup storage location request", "--reason", }, }, { - name: "nabsl reject help", - args: []string{"nabsl", "reject", "--help"}, + name: "nabsl-request reject help", + args: []string{"nabsl-request", "reject", "--help"}, expectContains: []string{ "Reject a pending backup storage location request", "--reason", }, }, { - name: "nabsl get help", - args: []string{"nabsl", "get", "--help"}, + name: "nabsl-request get help", + args: []string{"nabsl-request", "get", "--help"}, expectContains: []string{ "Get non-admin backup storage location requests", }, }, { - name: "nabsl describe help", - args: []string{"nabsl", "describe", "--help"}, + name: "nabsl-request describe help", + args: []string{"nabsl-request", "describe", "--help"}, expectContains: []string{ "Describe a non-admin backup storage location request", }, @@ -81,21 +81,21 @@ func TestNABSLCommands(t *testing.T) { } } -// TestNABSLHelpFlags tests that both --help and -h work for nabsl commands +// TestNABSLHelpFlags tests that both --help and -h work for nabsl-request commands func TestNABSLHelpFlags(t *testing.T) { binaryPath := testutil.BuildCLIBinary(t) commands := [][]string{ - {"nabsl", "--help"}, - {"nabsl", "-h"}, - {"nabsl", "approve", "--help"}, - {"nabsl", "approve", "-h"}, - {"nabsl", "reject", "--help"}, - {"nabsl", "reject", "-h"}, - {"nabsl", "get", "--help"}, - {"nabsl", "get", "-h"}, - {"nabsl", "describe", "--help"}, - {"nabsl", "describe", "-h"}, + {"nabsl-request", "--help"}, + {"nabsl-request", "-h"}, + {"nabsl-request", "approve", "--help"}, + {"nabsl-request", "approve", "-h"}, + {"nabsl-request", "reject", "--help"}, + {"nabsl-request", "reject", "-h"}, + {"nabsl-request", "get", "--help"}, + {"nabsl-request", "get", "-h"}, + {"nabsl-request", "describe", "--help"}, + {"nabsl-request", "describe", "-h"}, } for _, cmd := range commands { @@ -105,33 +105,33 @@ func TestNABSLHelpFlags(t *testing.T) { } } -// TestNABSLClientConfigIntegration tests that NABSL commands respect client config +// TestNABSLClientConfigIntegration tests that NABSL request commands respect client config func TestNABSLClientConfigIntegration(t *testing.T) { binaryPath := testutil.BuildCLIBinary(t) _, cleanup := testutil.SetupTempHome(t) defer cleanup() - t.Run("nabsl commands work with client config", func(t *testing.T) { + t.Run("nabsl-request commands work with client config", func(t *testing.T) { // Set a known namespace _, err := testutil.RunCommand(t, binaryPath, "client", "config", "set", "namespace=admin-namespace") if err != nil { t.Fatalf("Failed to set client config: %v", err) } - // Test that nabsl commands can be invoked (they should respect the namespace) + // Test that nabsl-request commands can be invoked (they should respect the namespace) // We test help commands since they don't require actual K8s resources commands := [][]string{ - {"nabsl", "get", "--help"}, - {"nabsl", "approve", "--help"}, - {"nabsl", "reject", "--help"}, - {"nabsl", "describe", "--help"}, + {"nabsl-request", "get", "--help"}, + {"nabsl-request", "approve", "--help"}, + {"nabsl-request", "reject", "--help"}, + {"nabsl-request", "describe", "--help"}, } for _, cmd := range commands { t.Run("config_test_"+cmd[1], func(t *testing.T) { output, err := testutil.RunCommand(t, binaryPath, cmd...) if err != nil { - t.Fatalf("NABSL command should work with client config: %v", err) + t.Fatalf("NABSL request command should work with client config: %v", err) } if output == "" { t.Errorf("Expected help output for %v", cmd) diff --git a/cmd/nabsl/reject.go b/cmd/nabsl-request/reject.go similarity index 73% rename from cmd/nabsl/reject.go rename to cmd/nabsl-request/reject.go index 1d904fb3..6020cc11 100644 --- a/cmd/nabsl/reject.go +++ b/cmd/nabsl-request/reject.go @@ -40,10 +40,10 @@ func NewRejectCommand(f client.Factory) *cobra.Command { Long: "Reject a pending backup storage location request to deny the user's request for a backup storage location", Args: cobra.ExactArgs(1), Example: ` # Deny a request by NABSL name (admin access required) - kubectl oadp nabsl reject user-test-bsl --reason "Invalid configuration" + kubectl oadp nabsl-request reject user-test-bsl --reason "Invalid configuration" # Deny a request by UUID with detailed reason - kubectl oadp nabsl reject nacuser01-user-test-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 --reason "Bucket does not exist in specified region"`, + kubectl oadp nabsl-request reject nacuser01-user-test-bsl-96dfa8b7-3f6f-4c8d-a168-8527b00fbed8 --reason "Bucket does not exist in specified region"`, Run: func(c *cobra.Command, args []string) { cmd.CheckError(o.Complete(args, f)) cmd.CheckError(o.Validate(c, args, f)) @@ -94,7 +94,7 @@ func (o *RejectOptions) Run(c *cobra.Command, f client.Factory) error { adminNS := f.Namespace() // Find the request either by UUID or by looking up NABSL name - requestName, err := o.findRequestName(adminNS) + requestName, err := shared.FindNABSLRequestByNameOrUUID(context.Background(), o.client, o.RequestName, adminNS) if err != nil { return err } @@ -143,32 +143,3 @@ func (o *RejectOptions) Run(c *cobra.Command, f client.Factory) error { return nil } -func (o *RejectOptions) findRequestName(adminNS string) (string, error) { - // First check if o.RequestName is already a UUID by trying to get it directly - var testRequest nacv1alpha1.NonAdminBackupStorageLocationRequest - err := o.client.Get(context.Background(), kbclient.ObjectKey{ - Name: o.RequestName, - Namespace: adminNS, - }, &testRequest) - if err == nil { - // Found it directly, it's a UUID - return o.RequestName, nil - } - - // Not found directly, so o.RequestName might be a NABSL name - // We need to search through all requests to find one with matching source NABSL name - var requestList nacv1alpha1.NonAdminBackupStorageLocationRequestList - err = o.client.List(context.Background(), &requestList, kbclient.InNamespace(adminNS)) - if err != nil { - return "", fmt.Errorf("failed to list requests: %w", err) - } - - for _, request := range requestList.Items { - if request.Status.SourceNonAdminBSL != nil && - request.Status.SourceNonAdminBSL.Name == o.RequestName { - return request.Name, nil - } - } - - return "", fmt.Errorf("request for NABSL %q not found", o.RequestName) -} diff --git a/cmd/non-admin/backup/backup_test.go b/cmd/non-admin/backup/backup_test.go new file mode 100644 index 00000000..4379761e --- /dev/null +++ b/cmd/non-admin/backup/backup_test.go @@ -0,0 +1,261 @@ +/* +Copyright 2025 The OADP CLI 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 backup + +import ( + "testing" + + "github.com/migtools/oadp-cli/internal/testutil" +) + +// TestNonAdminBackupCommands tests the non-admin backup command functionality +func TestNonAdminBackupCommands(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + tests := []struct { + name string + args []string + expectContains []string + }{ + { + name: "nonadmin backup help", + args: []string{"nonadmin", "backup", "--help"}, + expectContains: []string{ + "Work with non-admin backups", + "create", + "describe", + "delete", + "get", + "logs", + }, + }, + { + name: "nonadmin backup create help", + args: []string{"nonadmin", "backup", "create", "--help"}, + expectContains: []string{ + "Create a non-admin backup", + "--storage-location", + "--include-resources", + "--exclude-resources", + "--wait", + "--force", + "--assume-yes", + }, + }, + { + name: "nonadmin backup describe help", + args: []string{"nonadmin", "backup", "describe", "--help"}, + expectContains: []string{ + "Describe a non-admin backup", + }, + }, + { + name: "nonadmin backup delete help", + args: []string{"nonadmin", "backup", "delete", "--help"}, + expectContains: []string{ + "Delete one or more non-admin backups", + "--confirm", + }, + }, + { + name: "nonadmin backup get help", + args: []string{"nonadmin", "backup", "get", "--help"}, + expectContains: []string{ + "Get one or more non-admin backups", + }, + }, + { + name: "nonadmin backup logs help", + args: []string{"nonadmin", "backup", "logs", "--help"}, + expectContains: []string{ + "Show logs for a non-admin backup", + }, + }, + { + name: "na backup shorthand help", + args: []string{"na", "backup", "--help"}, + expectContains: []string{ + "Work with non-admin backups", + "create", + "describe", + "delete", + "get", + "logs", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testutil.TestHelpCommand(t, binaryPath, tt.args, tt.expectContains) + }) + } +} + +// TestNonAdminBackupHelpFlags tests that both --help and -h work for backup commands +func TestNonAdminBackupHelpFlags(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + commands := [][]string{ + {"nonadmin", "backup", "--help"}, + {"nonadmin", "backup", "-h"}, + {"nonadmin", "backup", "create", "--help"}, + {"nonadmin", "backup", "create", "-h"}, + {"nonadmin", "backup", "describe", "--help"}, + {"nonadmin", "backup", "describe", "-h"}, + {"nonadmin", "backup", "delete", "--help"}, + {"nonadmin", "backup", "delete", "-h"}, + {"nonadmin", "backup", "get", "--help"}, + {"nonadmin", "backup", "get", "-h"}, + {"nonadmin", "backup", "logs", "--help"}, + {"nonadmin", "backup", "logs", "-h"}, + {"na", "backup", "--help"}, + {"na", "backup", "-h"}, + } + + for _, cmd := range commands { + t.Run("help_flags_"+cmd[len(cmd)-1], func(t *testing.T) { + testutil.TestHelpCommand(t, binaryPath, cmd, []string{"Usage:"}) + }) + } +} + +// TestNonAdminBackupCreateFlags tests create command specific flags +func TestNonAdminBackupCreateFlags(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + t.Run("create command has all expected flags", func(t *testing.T) { + expectedFlags := []string{ + "--storage-location", + "--include-resources", + "--exclude-resources", + "--labels", + "--annotations", + "--wait", + "--force", + "--assume-yes", + "--snapshot-volumes", + "--ttl", + "--selector", + "--or-selector", + } + + testutil.TestHelpCommand(t, binaryPath, + []string{"nonadmin", "backup", "create", "--help"}, + expectedFlags) + }) +} + +// TestNonAdminBackupExamples tests that help text contains proper examples +func TestNonAdminBackupExamples(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + t.Run("create examples use correct command format", func(t *testing.T) { + expectedExamples := []string{ + "kubectl oadp nonadmin backup create", + "--storage-location", + "--include-resources", + "--exclude-resources", + "--wait", + } + + testutil.TestHelpCommand(t, binaryPath, + []string{"nonadmin", "backup", "create", "--help"}, + expectedExamples) + }) + + t.Run("main backup help shows subcommands", func(t *testing.T) { + expectedSubcommands := []string{ + "create", + "delete", + "describe", + "get", + "logs", + } + + testutil.TestHelpCommand(t, binaryPath, + []string{"nonadmin", "backup", "--help"}, + expectedSubcommands) + }) +} + +// TestNonAdminBackupClientConfigIntegration tests that backup commands respect client config +func TestNonAdminBackupClientConfigIntegration(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + _, cleanup := testutil.SetupTempHome(t) + defer cleanup() + + t.Run("backup commands work with client config", func(t *testing.T) { + // Set a known namespace + _, err := testutil.RunCommand(t, binaryPath, "client", "config", "set", "namespace=user-namespace") + if err != nil { + t.Fatalf("Failed to set client config: %v", err) + } + + // Test that backup commands can be invoked (they should respect the namespace) + // We test help commands since they don't require actual K8s resources + commands := [][]string{ + {"nonadmin", "backup", "get", "--help"}, + {"nonadmin", "backup", "create", "--help"}, + {"nonadmin", "backup", "describe", "--help"}, + {"nonadmin", "backup", "delete", "--help"}, + {"nonadmin", "backup", "logs", "--help"}, + {"na", "backup", "get", "--help"}, + } + + for _, cmd := range commands { + t.Run("config_test_"+cmd[len(cmd)-2], func(t *testing.T) { + output, err := testutil.RunCommand(t, binaryPath, cmd...) + if err != nil { + t.Fatalf("Non-admin backup command should work with client config: %v", err) + } + if output == "" { + t.Errorf("Expected help output for %v", cmd) + } + }) + } + }) +} + +// TestNonAdminBackupCommandStructure tests the overall command structure +func TestNonAdminBackupCommandStructure(t *testing.T) { + binaryPath := testutil.BuildCLIBinary(t) + + t.Run("backup commands available under nonadmin", func(t *testing.T) { + _, err := testutil.RunCommand(t, binaryPath, "nonadmin", "--help") + if err != nil { + t.Fatalf("nonadmin command should exist: %v", err) + } + + expectedCommands := []string{"backup"} + for _, cmd := range expectedCommands { + testutil.TestHelpCommand(t, binaryPath, []string{"nonadmin", "--help"}, []string{cmd}) + } + }) + + t.Run("backup commands available under na shorthand", func(t *testing.T) { + _, err := testutil.RunCommand(t, binaryPath, "na", "--help") + if err != nil { + t.Fatalf("na command should exist: %v", err) + } + + expectedCommands := []string{"backup"} + for _, cmd := range expectedCommands { + testutil.TestHelpCommand(t, binaryPath, []string{"na", "--help"}, []string{cmd}) + } + }) +} \ No newline at end of file diff --git a/cmd/non-admin/bsl/bsl_test.go b/cmd/non-admin/bsl/bsl_test.go index d3df06db..b0ac1894 100644 --- a/cmd/non-admin/bsl/bsl_test.go +++ b/cmd/non-admin/bsl/bsl_test.go @@ -59,7 +59,7 @@ func TestBSLCommands(t *testing.T) { } } -// TestBSLNoLongerHasRequestCommands verifies that request commands were moved to nabsl +// TestBSLNoLongerHasRequestCommands verifies that request commands were moved to nabsl-request func TestBSLNoLongerHasRequestCommands(t *testing.T) { binaryPath := testutil.BuildCLIBinary(t) diff --git a/cmd/root.go b/cmd/root.go index 447b1e01..d955b660 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -22,7 +22,7 @@ import ( "path/filepath" "strings" - "github.com/migtools/oadp-cli/cmd/nabsl" + "github.com/migtools/oadp-cli/cmd/nabsl-request" nonadmin "github.com/migtools/oadp-cli/cmd/non-admin" "github.com/spf13/cobra" "github.com/vmware-tanzu/velero/pkg/cmd/cli/backup" @@ -84,8 +84,8 @@ func NewVeleroRootCommand() *cobra.Command { rootCmd.AddCommand(restoreCmd) rootCmd.AddCommand(clientCmd) - // Admin NABSL commands - use Velero factory (admin namespace) - rootCmd.AddCommand(nabsl.NewNABSLCommand(veleroFactory)) + // Admin NABSL request commands - use Velero factory (admin namespace) + rootCmd.AddCommand(nabsl.NewNABSLRequestCommand(veleroFactory)) // Custom subcommands - use NonAdmin factory rootCmd.AddCommand(nonadmin.NewNonAdminCommand(nonAdminFactory)) diff --git a/cmd/root_test.go b/cmd/root_test.go index 96f5d073..d388039f 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -40,7 +40,7 @@ func TestRootCommand(t *testing.T) { "version", "backup", "restore", - "nabsl", + "nabsl-request", "nonadmin", }, }, diff --git a/cmd/shared/factories.go b/cmd/shared/factories.go new file mode 100644 index 00000000..785d128a --- /dev/null +++ b/cmd/shared/factories.go @@ -0,0 +1,85 @@ +/* +Copyright 2025 The OADP CLI 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 shared + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + + "github.com/vmware-tanzu/velero/pkg/client" +) + +// ClientConfig represents the structure of the Velero client configuration file +type ClientConfig struct { + Namespace string `json:"namespace"` +} + +// CreateVeleroFactory creates a client factory for Velero operations (admin-scoped) +// that uses the client configuration to determine the namespace. +// Priority order: +// 1. Velero client config (~/.config/velero/config.json) +// 2. Kubeconfig context namespace +// 3. Velero default (usually "velero") +func CreateVeleroFactory() client.Factory { + cfg := client.VeleroConfig{} + + // Try to read client config to get configured namespace + if clientConfig, err := ReadVeleroClientConfig(); err == nil { + if clientConfig.Namespace != "" { + cfg[client.ConfigKeyNamespace] = clientConfig.Namespace + } + } + + return client.NewFactory("oadp-velero-cli", cfg) +} + +// CreateNonAdminFactory creates a client factory for NonAdminBackup operations +// that uses the current kubeconfig context namespace instead of hardcoded openshift-adp +func CreateNonAdminFactory() client.Factory { + // Don't set a default namespace, let it use the kubeconfig context + cfg := client.VeleroConfig{} + return client.NewFactory("oadp-velero-cli", cfg) +} + +// ReadVeleroClientConfig reads the Velero client configuration from ~/.config/velero/config.json +func ReadVeleroClientConfig() (*ClientConfig, error) { + homeDir, err := os.UserHomeDir() + if err != nil { + return nil, fmt.Errorf("failed to get user home directory: %w", err) + } + + configPath := filepath.Join(homeDir, ".config", "velero", "config.json") + + // Check if config file exists + if _, err := os.Stat(configPath); os.IsNotExist(err) { + return &ClientConfig{}, nil // Return empty config if file doesn't exist + } + + data, err := os.ReadFile(configPath) + if err != nil { + return nil, fmt.Errorf("failed to read client config: %w", err) + } + + var config ClientConfig + if err := json.Unmarshal(data, &config); err != nil { + return nil, fmt.Errorf("failed to parse client config: %w", err) + } + + return &config, nil +} \ No newline at end of file diff --git a/cmd/shared/nabsl_requests.go b/cmd/shared/nabsl_requests.go new file mode 100644 index 00000000..4d7defea --- /dev/null +++ b/cmd/shared/nabsl_requests.go @@ -0,0 +1,62 @@ +/* +Copyright 2025 The OADP CLI 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 shared + +import ( + "context" + "fmt" + + kbclient "sigs.k8s.io/controller-runtime/pkg/client" + + nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" +) + +// FindNABSLRequestByNameOrUUID finds a NonAdminBackupStorageLocationRequest by either: +// 1. Direct UUID lookup (if nameOrUUID is the actual request UUID) +// 2. NABSL name lookup (searches through all requests to find one with matching source NABSL name) +// +// This handles the common pattern where users can specify either the NABSL-friendly name +// or the system-generated UUID for approval/rejection operations. +func FindNABSLRequestByNameOrUUID(ctx context.Context, client kbclient.WithWatch, nameOrUUID string, adminNamespace string) (string, error) { + // First check if nameOrUUID is already a UUID by trying to get it directly + var testRequest nacv1alpha1.NonAdminBackupStorageLocationRequest + err := client.Get(ctx, kbclient.ObjectKey{ + Name: nameOrUUID, + Namespace: adminNamespace, + }, &testRequest) + if err == nil { + // Found it directly, it's a UUID + return nameOrUUID, nil + } + + // Not found directly, so nameOrUUID might be a NABSL name + // We need to search through all requests to find one with matching source NABSL name + var requestList nacv1alpha1.NonAdminBackupStorageLocationRequestList + err = client.List(ctx, &requestList, kbclient.InNamespace(adminNamespace)) + if err != nil { + return "", fmt.Errorf("failed to list requests: %w", err) + } + + for _, request := range requestList.Items { + if request.Status.SourceNonAdminBSL != nil && + request.Status.SourceNonAdminBSL.Name == nameOrUUID { + return request.Name, nil + } + } + + return "", fmt.Errorf("request for NABSL %q not found", nameOrUUID) +} \ No newline at end of file diff --git a/cmd/shared/test_helpers.go b/cmd/shared/test_helpers.go new file mode 100644 index 00000000..006891ae --- /dev/null +++ b/cmd/shared/test_helpers.go @@ -0,0 +1,83 @@ +/* +Copyright 2025 The OADP CLI 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 shared + +import ( + "testing" + + "github.com/migtools/oadp-cli/internal/testutil" +) + +// TestClientConfigIntegrationPattern provides a reusable pattern for testing that commands +// respect client configuration (like namespace settings). This avoids duplicating the +// setup/teardown and common testing pattern across multiple test files. +// +// testNamespace: The namespace to configure for testing +// commands: List of command sequences to test (each should work with the configured namespace) +// validateOutput: Optional function to perform additional validation on command output +func TestClientConfigIntegrationPattern(t *testing.T, testNamespace string, commands [][]string, validateOutput func(t *testing.T, cmd []string, output string)) { + binaryPath := testutil.BuildCLIBinary(t) + _, cleanup := testutil.SetupTempHome(t) + defer cleanup() + + // Set the test namespace via client config + _, err := testutil.RunCommand(t, binaryPath, "client", "config", "set", "namespace="+testNamespace) + if err != nil { + t.Fatalf("Failed to set client config namespace: %v", err) + } + + // Test each command sequence + for _, cmd := range commands { + t.Run("config_test_"+cmd[len(cmd)-2], func(t *testing.T) { + output, err := testutil.RunCommand(t, binaryPath, cmd...) + if err != nil { + t.Fatalf("Command should work with client config: %v\nCommand: %v", err, cmd) + } + if output == "" { + t.Errorf("Expected help output for command: %v", cmd) + } + + // Run additional validation if provided + if validateOutput != nil { + validateOutput(t, cmd, output) + } + }) + } +} + +// ClientConfigTestCommands is a helper type for organizing command test data +type ClientConfigTestCommands struct { + Name string + Commands [][]string + TestSetup func(t *testing.T, binaryPath string) // Optional additional setup + Namespace string // Namespace to configure + ValidateFunc func(t *testing.T, cmd []string, output string) // Optional output validation +} + +// RunClientConfigTests runs client config integration tests for multiple command groups +func RunClientConfigTests(t *testing.T, testGroups []ClientConfigTestCommands) { + for _, group := range testGroups { + t.Run(group.Name, func(t *testing.T) { + namespace := group.Namespace + if namespace == "" { + namespace = "test-namespace" // Default test namespace + } + + TestClientConfigIntegrationPattern(t, namespace, group.Commands, group.ValidateFunc) + }) + } +} \ No newline at end of file diff --git a/integration_test.go b/integration_test.go index 25e9a6e2..093f21d8 100644 --- a/integration_test.go +++ b/integration_test.go @@ -135,7 +135,7 @@ func TestCommandArchitecture(t *testing.T) { binaryPath := testutil.BuildCLIBinary(t) t.Run("all major commands exist", func(t *testing.T) { - majorCommands := []string{"backup", "restore", "nabsl", "nonadmin", "client", "version"} + majorCommands := []string{"backup", "restore", "nabsl-request", "nonadmin", "client", "version"} output, _ := testutil.RunCommand(t, binaryPath, "--help") @@ -146,14 +146,14 @@ func TestCommandArchitecture(t *testing.T) { } }) - t.Run("nabsl command has correct subcommands", func(t *testing.T) { + t.Run("nabsl-request command has correct subcommands", func(t *testing.T) { expectedSubcommands := []string{"approve", "reject", "describe", "get"} - output, _ := testutil.RunCommand(t, binaryPath, "nabsl", "--help") + output, _ := testutil.RunCommand(t, binaryPath, "nabsl-request", "--help") for _, subcmd := range expectedSubcommands { if !strings.Contains(output, subcmd) { - t.Errorf("Expected nabsl help to contain %q subcommand", subcmd) + t.Errorf("Expected nabsl-request help to contain %q subcommand", subcmd) } } }) From ab71a247de851366e45c45ffe83359b4bb0c30b8 Mon Sep 17 00:00:00 2001 From: Joseph Date: Tue, 12 Aug 2025 07:51:47 -0700 Subject: [PATCH 9/9] Go fmt --- cmd/nabsl-request/approve.go | 1 - cmd/nabsl-request/reject.go | 1 - cmd/non-admin/backup/backup_test.go | 10 +++++----- cmd/shared/factories.go | 4 ++-- cmd/shared/nabsl_requests.go | 2 +- cmd/shared/test_helpers.go | 6 +++--- 6 files changed, 11 insertions(+), 13 deletions(-) diff --git a/cmd/nabsl-request/approve.go b/cmd/nabsl-request/approve.go index a4dccb21..970c296f 100644 --- a/cmd/nabsl-request/approve.go +++ b/cmd/nabsl-request/approve.go @@ -140,4 +140,3 @@ func (o *ApproveOptions) Run(c *cobra.Command, f client.Factory) error { return nil } - diff --git a/cmd/nabsl-request/reject.go b/cmd/nabsl-request/reject.go index 6020cc11..2f9803a7 100644 --- a/cmd/nabsl-request/reject.go +++ b/cmd/nabsl-request/reject.go @@ -142,4 +142,3 @@ func (o *RejectOptions) Run(c *cobra.Command, f client.Factory) error { return nil } - diff --git a/cmd/non-admin/backup/backup_test.go b/cmd/non-admin/backup/backup_test.go index 4379761e..91eb1fd9 100644 --- a/cmd/non-admin/backup/backup_test.go +++ b/cmd/non-admin/backup/backup_test.go @@ -144,7 +144,7 @@ func TestNonAdminBackupCreateFlags(t *testing.T) { "--include-resources", "--exclude-resources", "--labels", - "--annotations", + "--annotations", "--wait", "--force", "--assume-yes", @@ -154,8 +154,8 @@ func TestNonAdminBackupCreateFlags(t *testing.T) { "--or-selector", } - testutil.TestHelpCommand(t, binaryPath, - []string{"nonadmin", "backup", "create", "--help"}, + testutil.TestHelpCommand(t, binaryPath, + []string{"nonadmin", "backup", "create", "--help"}, expectedFlags) }) } @@ -181,7 +181,7 @@ func TestNonAdminBackupExamples(t *testing.T) { t.Run("main backup help shows subcommands", func(t *testing.T) { expectedSubcommands := []string{ "create", - "delete", + "delete", "describe", "get", "logs", @@ -258,4 +258,4 @@ func TestNonAdminBackupCommandStructure(t *testing.T) { testutil.TestHelpCommand(t, binaryPath, []string{"na", "--help"}, []string{cmd}) } }) -} \ No newline at end of file +} diff --git a/cmd/shared/factories.go b/cmd/shared/factories.go index 785d128a..99f1baeb 100644 --- a/cmd/shared/factories.go +++ b/cmd/shared/factories.go @@ -65,7 +65,7 @@ func ReadVeleroClientConfig() (*ClientConfig, error) { } configPath := filepath.Join(homeDir, ".config", "velero", "config.json") - + // Check if config file exists if _, err := os.Stat(configPath); os.IsNotExist(err) { return &ClientConfig{}, nil // Return empty config if file doesn't exist @@ -82,4 +82,4 @@ func ReadVeleroClientConfig() (*ClientConfig, error) { } return &config, nil -} \ No newline at end of file +} diff --git a/cmd/shared/nabsl_requests.go b/cmd/shared/nabsl_requests.go index 4d7defea..7d334344 100644 --- a/cmd/shared/nabsl_requests.go +++ b/cmd/shared/nabsl_requests.go @@ -59,4 +59,4 @@ func FindNABSLRequestByNameOrUUID(ctx context.Context, client kbclient.WithWatch } return "", fmt.Errorf("request for NABSL %q not found", nameOrUUID) -} \ No newline at end of file +} diff --git a/cmd/shared/test_helpers.go b/cmd/shared/test_helpers.go index 006891ae..8fb9c172 100644 --- a/cmd/shared/test_helpers.go +++ b/cmd/shared/test_helpers.go @@ -63,8 +63,8 @@ func TestClientConfigIntegrationPattern(t *testing.T, testNamespace string, comm type ClientConfigTestCommands struct { Name string Commands [][]string - TestSetup func(t *testing.T, binaryPath string) // Optional additional setup - Namespace string // Namespace to configure + TestSetup func(t *testing.T, binaryPath string) // Optional additional setup + Namespace string // Namespace to configure ValidateFunc func(t *testing.T, cmd []string, output string) // Optional output validation } @@ -80,4 +80,4 @@ func RunClientConfigTests(t *testing.T, testGroups []ClientConfigTestCommands) { TestClientConfigIntegrationPattern(t, namespace, group.Commands, group.ValidateFunc) }) } -} \ No newline at end of file +}