diff --git a/api/v1alpha1/nonadminbackup_types.go b/api/v1alpha1/nonadminbackup_types.go index 4e29310..6b9fa28 100644 --- a/api/v1alpha1/nonadminbackup_types.go +++ b/api/v1alpha1/nonadminbackup_types.go @@ -51,9 +51,9 @@ type VeleroBackup struct { // +optional Status *velerov1.BackupStatus `json:"status,omitempty"` - // name references the Velero backup by it's name. + // nameuuid references the Velero Backup object by it's label containing same NameUUID. // +optional - Name string `json:"name,omitempty"` + NameUUID string `json:"nameuuid,omitempty"` // namespace references the Namespace in which Velero backup exists. // +optional diff --git a/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml b/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml index 495d8a3..c1b4390 100644 --- a/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml +++ b/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml @@ -612,13 +612,14 @@ spec: description: VeleroBackup contains information of the related Velero backup object. properties: - name: - description: name references the Velero backup by it's name. - type: string namespace: description: namespace references the Namespace in which Velero backup exists. type: string + nameuuid: + description: nameuuid references the Velero Backup object by it's + label containing same NameUUID. + type: string status: description: status captures the current status of the Velero backup. diff --git a/docs/design/Non_Admin_Controller_design.md b/docs/design/Non_Admin_Controller_design.md index e157aa8..5c40c92 100644 --- a/docs/design/Non_Admin_Controller_design.md +++ b/docs/design/Non_Admin_Controller_design.md @@ -139,17 +139,17 @@ This design intends to enable non-admin users the ability to perform Backup and backupSpec: {} ``` - **NAB controller reconciles on this NAB CR:** The NonAdminBackup controller continuously reconciles the NonAdminBackup object's desired state with the actual state in the cluster. - - **NAB controller validates the NAB CR and then creates a corresponding Velero Backup CR:** When the NonAdminBackup controller detects a new or modified NonAdminBackup object, it creates or updates a corresponding Velero Backup object within the OADP Namespace using the information provided in the `backupSpec` field of the NonAdminBackup object. The resulting Backup object is named as `nab--`, where the `` is the NonAdminBackup namespace and the `` is computed from the original NonAdminBackup name. The resulting Backup object is labeled and annotated with the following additional metadata: + - **NAB controller validates the NAB CR and then creates a corresponding Velero Backup CR:** When the NonAdminBackup controller detects a new or modified NonAdminBackup object, it creates or updates a corresponding Velero Backup object within the OADP Namespace using the information provided in the `backupSpec` field of the NonAdminBackup object. The resulting Backup object is named as `--`, where the `` is the NonAdminBackup namespace, `` is the NonAdminBackup name and the `` is the generated UUID (version 4). If the resulting Backup object name is too long the name is adapted to 63 characters limit strpping first characters from name and then from namespace. The resulting Backup object is labeled and annotated with the following additional metadata: ```yaml metadata: annotations: openshift.io/oadp-nab-origin-name: openshift.io/oadp-nab-origin-namespace: - openshift.io/oadp-nab-origin-uuid: labels: app.kubernetes.io/managed-by: openshift.io/oadp: 'True' + openshift.io/oadp-nab-origin-nameuuid: ``` - **Velero runs Backup**: Velero executes the backup operation based on the configuration specified in the Velero Backup object. Velero updates the status of the Velero Backup object to reflect the outcome of the backup process. - **Reconcile loop updates NonAdminBackup object Status**: Upon detecting changes in the status of the Velero Backup object, the NonAdminBackup controller's reconciliation loop updates the Status field of the corresponding NonAdminBackup object with the updated status from the Velero Backup object. @@ -162,17 +162,17 @@ This design intends to enable non-admin users the ability to perform Backup and - **Backup Sync controller syncs the Non-admin Backup CRs:** The Backup-Sync controller ensures that the NS relevant backups are synced and Non-admin backup CRs exists for non-admin users to refer them for restore operations. - **Non-Admin user creates the Non-Admin restore CR:** The user creates NonAdminRestore custom resource object in the Namespace on which the restore will run within the Kubernetes cluster. The `NonAdminRestore` schema has the `restoreSpec`, which is the same as `Restore` CR from the `velero.io/v1` apiVersion. Note that the non-admin user will use non-admin backup name (and not Velero backup name) in non-admin restore spec. - **NAR controller reconciles on this NAR CR:** The NonAdminRestore controller continuously reconciles the NonAdminRestore object's desired state with the actual state in the cluster. -- **NAR controller validates the NAR CR and then creates a corresponding Velero Restore CR:** When the NonAdminRestore controller detects a new or modified NonAdminRestore object, it creates the corresponding Velero Restore object within the OADP Namespace using the information provided in the `restoreSpec` field of the NonAdminRestore object. The resulting Restore object is named as `nar--`, where the `` is the NonAdminRestore namespace and the `` is computed from the original NonAdminRestore name. The resulting Restore object is labeled and annotated with the following additional metadata: +- **NAR controller validates the NAR CR and then creates a corresponding Velero Restore CR:** When the NonAdminRestore controller detects a new or modified NonAdminRestore object, it creates the corresponding Velero Restore object within the OADP Namespace using the information provided in the `restoreSpec` field of the NonAdminRestore object. The resulting Restore object is named as `--`, where the `` is the NonAdminRestore namespace, `` is the NonAdminRestore name and the `` is the generated UUID (version 4). If the resulting Restore object name is too long the name is adapted to 63 characters limit strpping first characters from name and then from namespace. The resulting Restore object is labeled and annotated with the following additional metadata: ```yaml metadata: annotations: openshift.io/oadp-nar-origin-name: openshift.io/oadp-nar-origin-namespace: - openshift.io/oadp-nar-origin-uuid: labels: app.kubernetes.io/managed-by: openshift.io/oadp: 'True' + openshift.io/oadp-nar-origin-nameuuid: ``` - **Velero runs Restore**: Velero executes the restore operation based on the configuration specified in the Velero Restore object. Velero updates the status of the Velero Restore object to reflect the outcome of the restore process. - **Reconcile loop updates NonAdminRestore object Status**: Upon detecting changes in the status of the Velero Restore object, the NonAdminRestore controller's reconciliation loop updates the Status field of the corresponding NonAdminRestore object with the updated status from the Velero Restore object. diff --git a/docs/design/nab_and_nar_status_update.md b/docs/design/nab_and_nar_status_update.md index 4ea0927..a7b0d7d 100644 --- a/docs/design/nab_and_nar_status_update.md +++ b/docs/design/nab_and_nar_status_update.md @@ -56,8 +56,8 @@ Those are are the possible values for `NonAdminCondition`: NonAdminBackup/NonAdminRestore `status` contains reference to the related Velero Backup/Restore. -NonAdminBackup `status.veleroBackup` contains `name`, `namespace` and `status`. -- `status.veleroBackup.name` represents the name of the `VeleroBackup` object. +NonAdminBackup `status.veleroBackup` contains `nameuuid`, `namespace` and `status`. +- `status.veleroBackup.nameuuid` field stores generated unique UUID of the `VeleroBackup` object. The same UUID is also stored as the label value `openshift.io/oadp-nab-origin-nameuuid` within the created `VeleroBackup` object. - `status.veleroBackup.namespace` represents the namespace in which the `VeleroBackup` object was created. - `status.veleroBackup.status` field is a copy of the `VeleroBackup` object status. @@ -76,10 +76,10 @@ status: velero backup describe -n openshift-adp nab-nacproject-c3499c2729730a ``` -Similarly, NonAdminRestore `status.veleroRestore` contains `name`, `namespace` and `status`. -- `status.veleroRestore.name` represents the name of the `veleroRestore` object. +Similarly, NonAdminRestore `status.veleroRestore` contains `nameuuid`, `namespace` and `status`. +- `status.veleroRestore.nameuuid` field stores generated unique UUID of the `VeleroRestore` object. The same UUID is also stored as the label value `openshift.io/oadp-nar-origin-nameuuid` within the created `VeleroRestore` object. - `status.veleroRestore.namespace` represents the namespace in which the `veleroRestore` object was created. -- `status.veleroRestore.status` field is a copy of the `VeleroBackup` object status. +- `status.veleroRestore.status` field is a copy of the `VeleroRestore` object status. ## Example @@ -91,7 +91,7 @@ Object passed validation and Velero `Backup` object was created, but there was a ```yaml status: veleroBackup: - name: nab-nacproject-83fc04a2fd253d + nameuuid: nonadmin-test-86b8d92b-66b2-11e4-8a2d-42010af06f3f namespace: openshift-adp status: expiration: '2024-05-16T08:12:11Z' @@ -124,69 +124,107 @@ It is similar for a NonAdminRestore. ```mermaid %%{init: {'theme':'neutral'}}%% graph -event1{{predicate: Create event NAB}} == If event is accepted ==> -reconcileStart1[/Reconcile start\] ==> -phaseNew["`status.phase to **New**`"] -. Requeue: false, err .-> reconcileStart1 -phaseNew ==> +predicateUpdateNabEvent{{predicate: Accepted **update** event NAB}}; +predicateCreateNabEvent{{predicate: Accepted **create** event NAB}}; +predicateUpdateVBEvent{{predicate: Accepted **update** event Velero Backup}}; -reconcileExit1[\Requeue: true, nil/] -reconcileExit1 ==> question(is NonAdminBackup Spec valid?) == yes ==> reconcileStart2 -question == no ==> reconcileStartInvalid1 +requeueTrueNil[\"Requeue: true"/]; +requeueFalseNil[\"Requeue: false"/]; +requeueFalseErr[\"Requeue: false, error"/]; +requeueFalseTerminalErr[\"Requeue: false, Terminal error"/]; -reconcileStart2[/Reconcile start\] ==> +reconcileStartAcceptedPredicate[/Reconcile start\]; -conditionsAcceptedTrue["`status.conditions[Accepted] to **True**`"] -. Requeue: false, err .- reconcileStart2 -conditionsAcceptedTrue ==> +questionPhaseStatusSetToNew{"Is status.phase: **New** ?"}; +questionConditionAcceptedTrue{"Is status.conditions[Accepted]: **True** ?"}; +questionIsNabValid{"Is NonAdminBackup Spec valid ?"}; +questionPhaseStatusSetToBackingOff{"Is status.phase: **BackingOff** + and status.conditions[Accepted]: **False** ?"}; +questionPhaseStatusSetToCreated{"Is status.phase: **Created** + and status.conditions[BackupScheduled]: **True** ?"}; +questionStatusVeleroBackupUUID{"Is status.VeleroBackup.NameUUID existing and valid ?"}; +questionSuccess{"Success ?"}; +questionSuccessCreateVB{"Success ?"} +questionSuccessWithTerminalError{"Success ?"}; +questionSuccessWithNoRequeue{"Success ?"}; +questionSuccessGetVB{"Error ?"}; +questionGetSingleVB{"Is Single Velero Backup found ?"}; +questionNabStatusUpdateFromVB{"Does NAB Object status requires update ?"}; -reconcileExit2[\Requeue: true, nil/] ==> +createVBObject{{"Create New Velero Backup Object"}}; -reconcileStart3[/Reconcile start\] ==> -createVeleroBackup([Create Velero Backup]) -. Requeue: false, err .- reconcileStart3 -createVeleroBackup ==> +getUUIDMatchingVeleroBackup("Get Velero Backup with label openshift.io/oadp-nab-origin-nameuuid matching status.VeleroBackup.NameUUID"); -phaseCreated["` -status.phase: **Created** -status.conditions[Queued] to **True** -status.conditions.veleroBackup.name -status.conditions.veleroBackup.namespace -`"] -. Requeue: false, err .- reconcileStart3 -phaseCreated ==> +statusPhaseSetToNew["Set status.phase to: **New**"]; +statusPhaseStatusSetToBackingOff["Set status.phase: **BackingOff** + and status.conditions[Accepted]: **False** ?"]; +statusConditionSetAcceptedToTrue["Set status.conditions[Accepted] to **True**"]; +statusPhaseStatusSetToCreated["Set status.phase: **Created** + and status.conditions[BackupScheduled]: **True** ?"]; +statusSetVeleroBackupUUID["Set valid status.VeleroBackup.NameUUID"]; +statusNabStatusUpdateFromVB["Update NonAdminBackup status from Velero Backup"]; -reconcileExit4[\Requeue: false, nil/] +predicateCreateNabEvent --> reconcileStartAcceptedPredicate; +predicateUpdateNabEvent --> reconcileStartAcceptedPredicate; +predicateUpdateVBEvent --> reconcileStartAcceptedPredicate; -reconcileStartInvalid1[/Reconcile start\] ==> +reconcileStartAcceptedPredicate --> questionPhaseStatusSetToNew; +questionPhaseStatusSetToNew -- No --> statusPhaseSetToNew; +questionPhaseStatusSetToNew -- Yes --> questionIsNabValid; -phaseBackingOff["` -status.phase to **BackingOff** -status.conditions[Accepted] to **False** -`"] -. Requeue: false, err .- reconcileStartInvalid1 -phaseBackingOff ==> +statusPhaseSetToNew --> questionSuccess; +questionSuccess -- No --> requeueFalseErr; +questionSuccess -- Yes --> requeueTrueNil; -reconcileExitInvalid1[\Requeue: false, TerminalError/] +requeueTrueNil --> reconcileStartAcceptedPredicate; -reconcileExit4 ~~~ event2 -reconcileExitInvalid1 ~~~ event2 +questionIsNabValid -- No --> questionPhaseStatusSetToBackingOff; +questionIsNabValid -- Yes --> questionConditionAcceptedTrue; -event2{{predicate: Update event VeleroBackup}} == If event is accepted ==> -handler1{{handler: Handle VeleroBackup update event}} ==> reconcileStart5 +questionConditionAcceptedTrue -- No --> statusConditionSetAcceptedToTrue; +questionConditionAcceptedTrue -- Yes --> questionStatusVeleroBackupUUID; -reconcileStart5[/Reconcile start\] ==> conditionsVeleroBackupStatus +questionStatusVeleroBackupUUID -- No --> statusSetVeleroBackupUUID; +questionStatusVeleroBackupUUID -- Yes --> getUUIDMatchingVeleroBackup; -conditionsVeleroBackupStatus["`status.conditions.veleroBackup.status`"] -. Requeue: false, err .- reconcileStart5 -conditionsVeleroBackupStatus ==> +statusSetVeleroBackupUUID --> questionSuccess; -reconcileExit5[\Requeue: false, nil/] +statusConditionSetAcceptedToTrue --> questionSuccess; -reconcileExit5 ~~~ event3 -event3{{predicate: Delete event NAB}} == If event is accepted ==> +questionPhaseStatusSetToBackingOff -- No --> statusPhaseStatusSetToBackingOff; +questionPhaseStatusSetToBackingOff -- Yes --> requeueFalseTerminalErr; +statusPhaseStatusSetToBackingOff --> questionSuccessWithTerminalError; -reconcileDelete1[/Reconcile start\] ==> +questionSuccessWithTerminalError -- No --> requeueFalseErr; +questionSuccessWithTerminalError -- Yes --> requeueFalseTerminalErr; -reconcileExitDelete1[\Requeue: false, nil/] -. Requeue: false, err .-> reconcileDelete1 +getUUIDMatchingVeleroBackup --> questionSuccessGetVB; + +questionSuccessGetVB -- No --> questionGetSingleVB; +questionSuccessGetVB -- Yes --> requeueFalseErr; + +questionGetSingleVB -- No --> createVBObject; +questionGetSingleVB -- Yes --> questionPhaseStatusSetToCreated; +createVBObject --> questionSuccessCreateVB; + +questionSuccessCreateVB -- No --> requeueFalseErr; +questionSuccessCreateVB -- Yes --> questionPhaseStatusSetToCreated; + +questionPhaseStatusSetToCreated -- No --> statusPhaseStatusSetToCreated; +questionPhaseStatusSetToCreated -- Yes --> questionNabStatusUpdateFromVB; +statusPhaseStatusSetToCreated --> questionSuccessWithNoRequeue; + +questionSuccessWithNoRequeue -- No --> requeueFalseErr; +questionSuccessWithNoRequeue -- Yes --> requeueFalseNil; + +questionNabStatusUpdateFromVB -- No --> requeueFalseNil; + +questionNabStatusUpdateFromVB -- Yes --> statusNabStatusUpdateFromVB; + +statusNabStatusUpdateFromVB --> questionSuccessWithNoRequeue; -event4{{predicate: Update event NAB}} == If event is accepted ==> question ``` diff --git a/internal/common/constant/constant.go b/internal/common/constant/constant.go index c7a41a8..ed9dfc6 100644 --- a/internal/common/constant/constant.go +++ b/internal/common/constant/constant.go @@ -17,6 +17,8 @@ limitations under the License. // Package constant contains all common constants used in the project package constant +import "k8s.io/apimachinery/pkg/util/validation" + // Common labels for objects manipulated by the Non Admin Controller // Labels should be used to identify the NAC object // Annotations on the other hand should be used to define ownership @@ -28,7 +30,8 @@ const ( ManagedByLabelValue = "oadp-nac-controller" // TODO why not use same project name as in PROJECT file? NabOriginNameAnnotation = "openshift.io/oadp-nab-origin-name" NabOriginNamespaceAnnotation = "openshift.io/oadp-nab-origin-namespace" - NabOriginUUIDAnnotation = "openshift.io/oadp-nab-origin-uuid" + NabOriginNameUUIDLabel = "openshift.io/oadp-nab-origin-nameuuid" + NarOriginNameUUIDLabel = "openshift.io/oadp-nar-origin-nameuuid" ) // Common environment variables for the Non Admin Controller @@ -39,9 +42,12 @@ const ( // EmptyString defines a constant for the empty string const EmptyString = "" +// NameDelimiter defines character that is used to separate name parts +const NameDelimiter = "-" + // TrueString defines a constant for the True string const TrueString = "True" -// VeleroBackupNamePrefix represents the prefix for the object name generated -// by the NonAdminController -const VeleroBackupNamePrefix = "nab" +// MaximumNacObjectNameLength represents Generated Non Admin Object Name and +// must be below 63 characters, because it's used within object Label Value +const MaximumNacObjectNameLength = validation.DNS1123LabelMaxLength diff --git a/internal/common/function/function.go b/internal/common/function/function.go index 66a34c9..2823791 100644 --- a/internal/common/function/function.go +++ b/internal/common/function/function.go @@ -19,12 +19,13 @@ package function import ( "context" - "crypto/sha256" - "encoding/hex" "fmt" "github.com/go-logr/logr" + "github.com/google/uuid" + velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation" "sigs.k8s.io/controller-runtime/pkg/client" @@ -47,7 +48,6 @@ func GetNonAdminBackupAnnotations(objectMeta metav1.ObjectMeta) map[string]strin return map[string]string{ constant.NabOriginNamespaceAnnotation: objectMeta.Namespace, constant.NabOriginNameAnnotation: objectMeta.Name, - constant.NabOriginUUIDAnnotation: string(objectMeta.UID), } } @@ -77,66 +77,125 @@ func ValidateBackupSpec(nonAdminBackup *nacv1alpha1.NonAdminBackup) error { return nil } -// GenerateVeleroBackupName generates a Velero backup name based on the provided namespace and NonAdminBackup name. -// It calculates a hash of the NonAdminBackup name and combines it with the namespace and a prefix to create the Velero backup name. -// If the resulting name exceeds the maximum Kubernetes name length, it truncates the namespace to fit within the limit. -func GenerateVeleroBackupName(namespace, nabName string) string { - // Calculate a hash of the name - hasher := sha256.New() - _, err := hasher.Write([]byte(nabName)) - if err != nil { - return "" +// GenerateNacObjectNameWithUUID generates a unique name based on the provided namespace and object origin name. +// It includes a UUID suffix. If the name exceeds the maximum length, it truncates nacName first, then namespace. +func GenerateNacObjectNameWithUUID(namespace, nacName string) string { + // Generate UUID suffix + uuidSuffix := uuid.New().String() + + // Build the initial name based on the presence of namespace and nacName + nacObjectName := uuidSuffix + if len(nacName) > 0 { + nacObjectName = nacName + constant.NameDelimiter + nacObjectName + } + if len(namespace) > 0 { + nacObjectName = namespace + constant.NameDelimiter + nacObjectName + } + + if len(nacObjectName) > constant.MaximumNacObjectNameLength { + // Calculate remaining length after UUID + remainingLength := constant.MaximumNacObjectNameLength - len(uuidSuffix) + + delimeterLength := len(constant.NameDelimiter) + + // Subtract two delimiter lengths to avoid a corner case where the namespace + // and delimiters leave no space for any part of nabName + if len(namespace) > remainingLength-delimeterLength-delimeterLength { + namespace = namespace[:remainingLength-delimeterLength-delimeterLength] + nacObjectName = namespace + constant.NameDelimiter + uuidSuffix + } else { + remainingLength = remainingLength - len(namespace) - delimeterLength - delimeterLength + nacName = nacName[:remainingLength] + nacObjectName = uuidSuffix + if len(nacName) > 0 { + nacObjectName = nacName + constant.NameDelimiter + nacObjectName + } + if len(namespace) > 0 { + nacObjectName = namespace + constant.NameDelimiter + nacObjectName + } + } } - const hashLength = 14 - nameHash := hex.EncodeToString(hasher.Sum(nil))[:hashLength] // Take first 14 chars + return nacObjectName +} - usedLength := hashLength + len(constant.VeleroBackupNamePrefix) + len("--") - maxNamespaceLength := validation.DNS1123SubdomainMaxLength - usedLength - // Ensure the name is within the character limit - if len(namespace) > maxNamespaceLength { - // Truncate the namespace if necessary - return fmt.Sprintf("%s-%s-%s", constant.VeleroBackupNamePrefix, namespace[:maxNamespaceLength], nameHash) +// ListObjectsByLabel retrieves a list of Kubernetes objects in a specified namespace +// that match a given label key-value pair. +func ListObjectsByLabel(ctx context.Context, clientInstance client.Client, namespace string, labelKey string, labelValue string, objectList client.ObjectList) error { + // Validate input parameters + if namespace == constant.EmptyString || labelKey == constant.EmptyString || labelValue == constant.EmptyString { + return fmt.Errorf("invalid input: namespace, labelKey, and labelValue must not be empty") } - return fmt.Sprintf("%s-%s-%s", constant.VeleroBackupNamePrefix, namespace, nameHash) + labelSelector := labels.SelectorFromSet(labels.Set{labelKey: labelValue}) + + // Attempt to list objects with the specified label + if err := clientInstance.List(ctx, objectList, &client.ListOptions{ + LabelSelector: labelSelector, + Namespace: namespace, + }); err != nil { + return fmt.Errorf("failed to list objects in namespace '%s': %w", namespace, err) + } + + return nil +} + +// GetVeleroBackupByLabel retrieves a VeleroBackup object based on a specified label within a given namespace. +// It returns the VeleroBackup only when exactly one object is found, throws an error if multiple backups are found, +// or returns nil if no matches are found. +func GetVeleroBackupByLabel(ctx context.Context, clientInstance client.Client, namespace string, labelValue string) (*velerov1.Backup, error) { + veleroBackupList := &velerov1.BackupList{} + + // Call the generic ListLabeledObjectsInNamespace function + if err := ListObjectsByLabel(ctx, clientInstance, namespace, constant.NabOriginNameUUIDLabel, labelValue, veleroBackupList); err != nil { + return nil, err + } + + switch len(veleroBackupList.Items) { + case 0: + return nil, nil // No matching VeleroBackup found + case 1: + return &veleroBackupList.Items[0], nil // Found 1 matching VeleroBackup + default: + return nil, fmt.Errorf("multiple VeleroBackup objects found with label %s=%s in namespace '%s'", constant.NabOriginNameUUIDLabel, labelValue, namespace) + } } // CheckVeleroBackupMetadata return true if Velero Backup object has required Non Admin labels and annotations, false otherwise func CheckVeleroBackupMetadata(obj client.Object) bool { - labels := obj.GetLabels() - if !checkLabelValue(labels, constant.OadpLabel, constant.OadpLabelValue) { + objLabels := obj.GetLabels() + if !checkLabelValue(objLabels, constant.OadpLabel, constant.OadpLabelValue) { return false } - if !checkLabelValue(labels, constant.ManagedByLabel, constant.ManagedByLabelValue) { + if !checkLabelValue(objLabels, constant.ManagedByLabel, constant.ManagedByLabelValue) { return false } - annotations := obj.GetAnnotations() - if !checkAnnotationValueIsValid(annotations, constant.NabOriginNamespaceAnnotation) { + if !checkLabelAnnotationValueIsValid(objLabels, constant.NabOriginNameUUIDLabel) { return false } - if !checkAnnotationValueIsValid(annotations, constant.NabOriginNameAnnotation) { + + annotations := obj.GetAnnotations() + if !checkLabelAnnotationValueIsValid(annotations, constant.NabOriginNamespaceAnnotation) { return false } - // TODO what is a valid uuid? - if !checkAnnotationValueIsValid(annotations, constant.NabOriginUUIDAnnotation) { + if !checkLabelAnnotationValueIsValid(annotations, constant.NabOriginNameAnnotation) { return false } return true } -func checkLabelValue(labels map[string]string, key string, value string) bool { - got, exists := labels[key] +func checkLabelValue(objLabels map[string]string, key string, value string) bool { + got, exists := objLabels[key] if !exists { return false } return got == value } -func checkAnnotationValueIsValid(annotations map[string]string, key string) bool { - value, exists := annotations[key] +func checkLabelAnnotationValueIsValid(labelsOrAnnotations map[string]string, key string) bool { + value, exists := labelsOrAnnotations[key] if !exists { return false } diff --git a/internal/common/function/function_test.go b/internal/common/function/function_test.go index 034b963..30c1c31 100644 --- a/internal/common/function/function_test.go +++ b/internal/common/function/function_test.go @@ -17,16 +17,23 @@ limitations under the License. package function import ( - "fmt" + "context" + "errors" "strings" "testing" + "github.com/google/uuid" "github.com/onsi/ginkgo/v2" "github.com/stretchr/testify/assert" velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/log/zap" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" "github.com/migtools/oadp-non-admin/internal/common/constant" @@ -38,6 +45,7 @@ const ( testNonAdminBackupNamespace = "non-admin-backup-namespace" testNonAdminBackupName = "non-admin-backup-name" testNonAdminBackupUUID = "12345678-1234-1234-1234-123456789abc" + defaultStr = "default" ) func TestGetNonAdminLabels(t *testing.T) { @@ -62,7 +70,6 @@ func TestGetNonAdminBackupAnnotations(t *testing.T) { expected := map[string]string{ constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, constant.NabOriginNameAnnotation: testNonAdminBackupName, - constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, } result := GetNonAdminBackupAnnotations(nonAdminBackup.ObjectMeta) @@ -115,36 +122,184 @@ func TestValidateBackupSpec(t *testing.T) { } } -func TestGenerateVeleroBackupName(t *testing.T) { - // Max Kubernetes name length := 253 - // hashLength := 14 - // deduct -3: constant.VeleroBackupNamePrefix = "nab" - // deduct -2: two "-" in the name - // 253 - 14 - 3 - 2 = 234 - const truncatedStringSize = 234 - - testCases := []struct { - namespace string +func TestGenerateNacObjectNameWithUUID(t *testing.T) { + tests := []struct { name string - expected string + namespace string + nabName string }{ { - namespace: "example-namespace", - name: "example-name", - expected: "nab-example-namespace-1cdadb729eaac1", + name: "Valid names without truncation", + namespace: defaultStr, + nabName: "my-backup", + }, + { + name: "Truncate nabName due to length", + namespace: "some", + nabName: strings.Repeat("q", constant.MaximumNacObjectNameLength+10), // too long for DNS limit + }, + { + name: "Truncate very long namespace and very long name", + namespace: strings.Repeat("w", constant.MaximumNacObjectNameLength+10), + nabName: strings.Repeat("e", constant.MaximumNacObjectNameLength+10), + }, + { + name: "nabName empty", + namespace: "example", + nabName: constant.EmptyString, + }, + { + name: "namespace empty", + namespace: constant.EmptyString, + nabName: "my-backup", }, { - namespace: strings.Repeat("m", validation.DNS1123SubdomainMaxLength), - name: "example-name", - expected: fmt.Sprintf("nab-%s-1cdadb729eaac1", strings.Repeat("m", truncatedStringSize)), + name: "very long name and namespace empty", + namespace: constant.EmptyString, + nabName: strings.Repeat("r", constant.MaximumNacObjectNameLength+10), + }, + { + name: "very long namespace and name empty", + namespace: strings.Repeat("t", constant.MaximumNacObjectNameLength+10), + nabName: constant.EmptyString, + }, + { + name: "empty namespace and empty name", + namespace: constant.EmptyString, + nabName: constant.EmptyString, }, } - for _, tc := range testCases { - actual := GenerateVeleroBackupName(tc.namespace, tc.name) - if actual != tc.expected { - t.Errorf("Expected: %s, but got: %s", tc.expected, actual) - } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := GenerateNacObjectNameWithUUID(tt.namespace, tt.nabName) + + // Check length, don't use constant.MaximumNacObjectNameLength here + // so if constant is changed test needs to be changed as well + if len(result) > validation.DNS1123LabelMaxLength { + t.Errorf("Generated name is too long: %s", result) + } + + // Extract the last 36 characters, which should be the UUID + if len(result) < 36 { + t.Errorf("Generated name is too short to contain a valid UUID: %s", result) + } + uuidPart := result[len(result)-36:] // The UUID is always the last 36 characters + + // Attempt to parse the UUID part + if _, err := uuid.Parse(uuidPart); err != nil { + t.Errorf("Last part is not a valid UUID: %s", uuidPart) + } + + // Check if no double hyphens are present + if strings.Contains(result, "--") { + t.Errorf("Generated name contains double hyphens: %s", result) + } + }) + } +} + +func TestGetVeleroBackupByLabel(t *testing.T) { + log := zap.New(zap.UseDevMode(true)) + ctx := context.Background() + ctx = ctrl.LoggerInto(ctx, log) + scheme := runtime.NewScheme() + const testAppStr = "test-app" + + // Register VeleroBackup type with the scheme + if err := velerov1.AddToScheme(scheme); err != nil { + t.Fatalf("Failed to register VeleroBackup type: %v", err) + } + + tests := []struct { + name string + namespace string + labelValue string + expected *velerov1.Backup + expectedError error + mockBackups []velerov1.Backup + }{ + { + name: "Single VeleroBackup found", + namespace: defaultStr, + labelValue: testAppStr, + mockBackups: []velerov1.Backup{ + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: defaultStr, + Name: "backup1", + Labels: map[string]string{constant.NabOriginNameUUIDLabel: testAppStr}, + }, + }, + }, + expected: &velerov1.Backup{ObjectMeta: metav1.ObjectMeta{Namespace: defaultStr, Name: "backup1", Labels: map[string]string{constant.NabOriginNameUUIDLabel: testAppStr}}}, + expectedError: nil, + }, + { + name: "No VeleroBackups found", + namespace: defaultStr, + labelValue: testAppStr, + mockBackups: []velerov1.Backup{}, + expected: nil, + expectedError: nil, + }, + { + name: "Multiple VeleroBackups found", + namespace: defaultStr, + labelValue: testAppStr, + mockBackups: []velerov1.Backup{ + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: defaultStr, + Name: "backup2", + Labels: map[string]string{constant.NabOriginNameUUIDLabel: testAppStr}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: defaultStr, + Name: "backup3", + Labels: map[string]string{constant.NabOriginNameUUIDLabel: testAppStr}, + }, + }, + }, + expected: nil, + expectedError: errors.New("multiple VeleroBackup objects found with label openshift.io/oadp-nab-origin-nameuuid=test-app in namespace 'default'"), + }, + { + name: "Invalid input - empty namespace", + namespace: "", + labelValue: testAppStr, + mockBackups: []velerov1.Backup{}, + expected: nil, + expectedError: errors.New("invalid input: namespace, labelKey, and labelValue must not be empty"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var objects []client.Object + for _, backup := range tt.mockBackups { + backupCopy := backup // Create a copy to avoid memory aliasing + objects = append(objects, &backupCopy) + } + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + + result, err := GetVeleroBackupByLabel(ctx, client, tt.namespace, tt.labelValue) + + if tt.expectedError != nil { + assert.EqualError(t, err, tt.expectedError.Error()) + } else { + assert.NoError(t, err) + if tt.expected != nil && result != nil { + assert.Equal(t, tt.expected.Name, result.Name, "VeleroBackup Name should match") + assert.Equal(t, tt.expected.Namespace, result.Namespace, "VeleroBackup Namespace should match") + assert.Equal(t, tt.expected.Labels, result.Labels, "VeleroBackup Labels should match") + } else { + assert.Nil(t, result, "Expected result should be nil") + } + } + }) } } @@ -190,7 +345,6 @@ func TestCheckVeleroBackupMetadata(t *testing.T) { Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, constant.NabOriginNameAnnotation: testNonAdminBackupName, - constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, }, }, }, @@ -201,30 +355,64 @@ func TestCheckVeleroBackupMetadata(t *testing.T) { backup: &velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - constant.OadpLabel: constant.OadpLabelValue, - constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.NabOriginNameUUIDLabel: testNonAdminBackupUUID, }, Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: constant.EmptyString, constant.NabOriginNameAnnotation: testNonAdminBackupName, - constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, }, }, }, expected: false, }, { - name: "Velero Backup with wrong required non admin annotation [long]", + name: "Velero Backup with wrong required non admin label [long]", backup: &velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - constant.OadpLabel: constant.OadpLabelValue, - constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: strings.Repeat("ll", validation.DNS1123SubdomainMaxLength), + constant.NabOriginNameUUIDLabel: testNonAdminBackupUUID, + }, + Annotations: map[string]string{ + constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, + constant.NabOriginNameAnnotation: testNonAdminBackupName, + }, + }, + }, + expected: false, + }, + { + name: "Velero Backup with wrong required non admin name annotation [long]", + backup: &velerov1.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.NabOriginNameUUIDLabel: testNonAdminBackupUUID, }, Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, - constant.NabOriginNameAnnotation: strings.Repeat("nn", validation.DNS1123SubdomainMaxLength), - constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, + constant.NabOriginNameAnnotation: strings.Repeat("nm", validation.DNS1123SubdomainMaxLength), + }, + }, + }, + expected: false, + }, + { + name: "Velero Backup with wrong required non admin name annotation [long]", + backup: &velerov1.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.NabOriginNameUUIDLabel: testNonAdminBackupUUID, + }, + Annotations: map[string]string{ + constant.NabOriginNamespaceAnnotation: strings.Repeat("ns", validation.DNS1123SubdomainMaxLength), + constant.NabOriginNameAnnotation: testNonAdminBackupName, }, }, }, @@ -235,13 +423,13 @@ func TestCheckVeleroBackupMetadata(t *testing.T) { backup: &velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - constant.OadpLabel: constant.OadpLabelValue, - constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.NabOriginNameUUIDLabel: testNonAdminBackupUUID, }, Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, constant.NabOriginNameAnnotation: testNonAdminBackupName, - constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, }, }, }, diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index e1ca726..104cc06 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -51,9 +51,11 @@ type NonAdminBackupReconciler struct { type reconcileStepFunction func(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) const ( - phaseUpdateRequeue = "NonAdminBackup - Requeue after Phase Update" - conditionUpdateRequeue = "NonAdminBackup - Requeue after Condition Update" - statusUpdateError = "Failed to update NonAdminBackup Status" + phaseUpdateRequeue = "NonAdminBackup - Requeue after Phase Update" + conditionUpdateRequeue = "NonAdminBackup - Requeue after Condition Update" + veleroReferenceUpdateRequeue = "NonAdminBackup - Requeue after Status Update with UUID reference" + statusUpdateExit = "NonAdminBackup - Exit after Status Update" + statusUpdateError = "Failed to update NonAdminBackup Status" ) // +kubebuilder:rbac:groups=nac.oadp.openshift.io,resources=nonadminbackups,verbs=get;list;watch;create;update;patch;delete @@ -84,7 +86,8 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque reconcileSteps := []reconcileStepFunction{ r.init, r.validateSpec, - r.syncVeleroBackupWithNonAdminBackup, + r.setBackupUUIDInStatus, + r.createVeleroBackupAndSyncWithNonAdminBackup, } for _, step := range reconcileSteps { requeue, err := step(ctx, logger, nab) @@ -185,7 +188,34 @@ func (r *NonAdminBackupReconciler) validateSpec(ctx context.Context, logger logr return false, nil } -// syncVeleroBackupWithNonAdminBackup ensures the VeleroBackup associated with the given NonAdminBackup resource +// setBackupUUIDInStatus generates a UUID for VeleroBackup and stores it in the NonAdminBackup status. +// +// Parameters: +// +// ctx: Context for the request. +// logger: Logger instance for logging messages. +// nab: Pointer to the NonAdminBackup object. +// +// This function generates a UUID and stores it in the VeleroBackup status field of NonAdminBackup. +func (r *NonAdminBackupReconciler) setBackupUUIDInStatus(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { + if nab.Status.VeleroBackup == nil || nab.Status.VeleroBackup.NameUUID == constant.EmptyString { + veleroBackupNameUUID := function.GenerateNacObjectNameWithUUID(nab.Namespace, nab.Name) + nab.Status.VeleroBackup = &nacv1alpha1.VeleroBackup{ + NameUUID: veleroBackupNameUUID, + Namespace: r.OADPNamespace, + } + if err := r.Status().Update(ctx, nab); err != nil { + logger.Error(err, statusUpdateError) + return false, err + } + logger.V(1).Info(veleroReferenceUpdateRequeue) + return true, nil + } + logger.V(1).Info("NonAdminBackup already contains VeleroBackup UUID reference") + return false, nil +} + +// createVeleroBackupAndSyncWithNonAdminBackup ensures the VeleroBackup associated with the given NonAdminBackup resource // is created, if it does not exist. // The function also updates the status and conditions of the NonAdminBackup resource to reflect the state // of the VeleroBackup. @@ -195,29 +225,30 @@ func (r *NonAdminBackupReconciler) validateSpec(ctx context.Context, logger logr // ctx: Context for the request. // logger: Logger instance for logging messages. // nab: Pointer to the NonAdminBackup object. -func (r *NonAdminBackupReconciler) syncVeleroBackupWithNonAdminBackup(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { - veleroBackupName := function.GenerateVeleroBackupName(nab.Namespace, nab.Name) - if veleroBackupName == constant.EmptyString { - return false, errors.New("unable to generate Velero Backup name") +func (r *NonAdminBackupReconciler) createVeleroBackupAndSyncWithNonAdminBackup(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { + if nab.Status.VeleroBackup == nil || nab.Status.VeleroBackup.NameUUID == constant.EmptyString { + return false, errors.New("unable to get Velero Backup UUID from NonAdminBackup Status") } - veleroBackup := velerov1.Backup{} - veleroBackupLogger := logger.WithValues("VeleroBackup", types.NamespacedName{Name: veleroBackupName, Namespace: r.OADPNamespace}) - err := r.Get(ctx, client.ObjectKey{Namespace: r.OADPNamespace, Name: veleroBackupName}, &veleroBackup) + veleroBackupNameUUID := nab.Status.VeleroBackup.NameUUID + + veleroBackup, err := function.GetVeleroBackupByLabel(ctx, r.Client, r.OADPNamespace, veleroBackupNameUUID) + if err != nil { - if !apierrors.IsNotFound(err) { - veleroBackupLogger.Error(err, "Unable to fetch VeleroBackup") - return false, err - } - // Create VeleroBackup - veleroBackupLogger.Info("VeleroBackup not found") + // Case in which more then one VeleroBackup is found with the same label UUID + logger.Error(err, "Failed to find single VeleroBackup object", "UUID", veleroBackupNameUUID) + return false, err + } + + if veleroBackup == nil { + logger.Info("VeleroBackup with label not found, creating one", "UUID", veleroBackupNameUUID) backupSpec := nab.Spec.BackupSpec.DeepCopy() backupSpec.IncludedNamespaces = []string{nab.Namespace} - veleroBackup = velerov1.Backup{ + veleroBackup := velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ - Name: veleroBackupName, + Name: veleroBackupNameUUID, Namespace: r.OADPNamespace, Labels: function.GetNonAdminLabels(), Annotations: function.GetNonAdminBackupAnnotations(nab.ObjectMeta), @@ -225,15 +256,26 @@ func (r *NonAdminBackupReconciler) syncVeleroBackupWithNonAdminBackup(ctx contex Spec: *backupSpec, } + // Add NonAdminBackup's veleroBackupNameUUID as the label to the VeleroBackup object + // We don't add this as an argument of GetNonAdminLabels(), because there may be + // situations where NAC object do not require NabOriginUUIDLabel + veleroBackup.Labels[constant.NabOriginNameUUIDLabel] = veleroBackupNameUUID + err = r.Create(ctx, &veleroBackup) + if err != nil { - veleroBackupLogger.Error(err, "Failed to create VeleroBackup") + // We do not retry here as the veleroBackupNameUUID + // should be guaranteed to be unique + logger.Error(err, "Failed to create VeleroBackup") return false, err } - veleroBackupLogger.Info("VeleroBackup successfully created") + logger.Info("VeleroBackup successfully created") } + veleroBackupLogger := logger.WithValues("VeleroBackup", types.NamespacedName{Name: veleroBackupNameUUID, Namespace: r.OADPNamespace}) + updatedPhase := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseCreated) + updatedCondition := meta.SetStatusCondition(&nab.Status.Conditions, metav1.Condition{ Type: string(nacv1alpha1.NonAdminConditionQueued), @@ -242,22 +284,20 @@ func (r *NonAdminBackupReconciler) syncVeleroBackupWithNonAdminBackup(ctx contex Message: "Created Velero Backup object", }, ) - updatedReference := updateNonAdminBackupVeleroBackupReference(&nab.Status, &veleroBackup) - if updatedPhase || updatedCondition || updatedReference { + + if updatedPhase || updatedCondition { if err := r.Status().Update(ctx, nab); err != nil { logger.Error(err, statusUpdateError) return false, err } - - logger.V(1).Info("NonAdminBackup - Exit after Status Update") + logger.V(1).Info(statusUpdateExit) return false, nil } // Ensure that the NonAdminBackup's NonAdminBackupStatus is in sync // with the VeleroBackup. Any required updates to the NonAdminBackup // Status will be applied based on the current state of the VeleroBackup. - veleroBackupLogger.Info("VeleroBackup already exists, verifying if NonAdminBackup Status requires update") - updated := updateNonAdminBackupVeleroBackupStatus(&nab.Status, &veleroBackup) + updated := updateNonAdminBackupVeleroBackupStatus(&nab.Status, veleroBackup) if updated { if err := r.Status().Update(ctx, nab); err != nil { veleroBackupLogger.Error(err, "Failed to update NonAdminBackup Status after VeleroBackup reconciliation") @@ -301,32 +341,16 @@ func updateNonAdminPhase(phase *nacv1alpha1.NonAdminBackupPhase, newPhase nacv1a return true } -// updateNonAdminBackupVeleroBackupStatus sets the VeleroBackup reference fields in NonAdminBackup object status and returns true -// if the VeleroBackup fields are changed by this call. -func updateNonAdminBackupVeleroBackupReference(status *nacv1alpha1.NonAdminBackupStatus, veleroBackup *velerov1.Backup) bool { - if status.VeleroBackup == nil { - status.VeleroBackup = &nacv1alpha1.VeleroBackup{ - Name: veleroBackup.Name, - Namespace: veleroBackup.Namespace, - } - return true - } else if status.VeleroBackup.Name != veleroBackup.Name || status.VeleroBackup.Namespace != veleroBackup.Namespace { - status.VeleroBackup.Name = veleroBackup.Name - status.VeleroBackup.Namespace = veleroBackup.Namespace - return true - } - return false -} - // updateNonAdminBackupVeleroBackupStatus sets the VeleroBackup status field in NonAdminBackup object status and returns true // if the VeleroBackup fields are changed by this call. func updateNonAdminBackupVeleroBackupStatus(status *nacv1alpha1.NonAdminBackupStatus, veleroBackup *velerov1.Backup) bool { + if status == nil || veleroBackup == nil { + return false + } if status.VeleroBackup == nil { - status.VeleroBackup = &nacv1alpha1.VeleroBackup{ - Status: veleroBackup.Status.DeepCopy(), - } - return true - } else if !reflect.DeepEqual(status.VeleroBackup.Status, &veleroBackup.Status) { + status.VeleroBackup = &nacv1alpha1.VeleroBackup{} + } + if status.VeleroBackup.Status == nil || !reflect.DeepEqual(status.VeleroBackup.Status, veleroBackup.Status) { status.VeleroBackup.Status = veleroBackup.Status.DeepCopy() return true } diff --git a/internal/controller/nonadminbackup_controller_test.go b/internal/controller/nonadminbackup_controller_test.go index d72ece9..bd508b3 100644 --- a/internal/controller/nonadminbackup_controller_test.go +++ b/internal/controller/nonadminbackup_controller_test.go @@ -29,25 +29,24 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/reconcile" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" + "github.com/migtools/oadp-non-admin/internal/common/constant" "github.com/migtools/oadp-non-admin/internal/common/function" ) -const ( - testNonAdminBackupName = "test-non-admin-backup" - placeholder = "PLACEHOLDER" -) - type nonAdminBackupSingleReconcileScenario struct { - resultError error - priorStatus *nacv1alpha1.NonAdminBackupStatus - spec nacv1alpha1.NonAdminBackupSpec - ExpectedStatus nacv1alpha1.NonAdminBackupStatus - result reconcile.Result - createVeleroBackup bool + resultError error + nonAdminBackupPriorStatus *nacv1alpha1.NonAdminBackupStatus + nonAdminBackupSpec nacv1alpha1.NonAdminBackupSpec + nonAdminBackupExpectedStatus nacv1alpha1.NonAdminBackupStatus + result reconcile.Result + createVeleroBackup bool + uuidCreatedByReconcile bool + uuidFromTestCase bool } type nonAdminBackupFullReconcileScenario struct { @@ -55,37 +54,40 @@ type nonAdminBackupFullReconcileScenario struct { status nacv1alpha1.NonAdminBackupStatus } -func buildTestNonAdminBackup(namespace string, spec nacv1alpha1.NonAdminBackupSpec) *nacv1alpha1.NonAdminBackup { +func buildTestNonAdminBackup(nonAdminNamespace string, nonAdminName string, spec nacv1alpha1.NonAdminBackupSpec) *nacv1alpha1.NonAdminBackup { return &nacv1alpha1.NonAdminBackup{ ObjectMeta: metav1.ObjectMeta{ - Name: testNonAdminBackupName, - Namespace: namespace, + Name: nonAdminName, + Namespace: nonAdminNamespace, }, Spec: spec, } } -func checkTestNonAdminBackupStatus(nonAdminBackup *nacv1alpha1.NonAdminBackup, expectedStatus nacv1alpha1.NonAdminBackupStatus, nonAdminNamespaceName string, oadpNamespaceName string) error { +func checkTestNonAdminBackupStatus(nonAdminBackup *nacv1alpha1.NonAdminBackup, expectedStatus nacv1alpha1.NonAdminBackupStatus, oadpNamespaceName string) error { if nonAdminBackup.Status.Phase != expectedStatus.Phase { return fmt.Errorf("NonAdminBackup Status Phase %v is not equal to expected %v", nonAdminBackup.Status.Phase, expectedStatus.Phase) } - if expectedStatus.VeleroBackup != nil { - veleroBackupName := expectedStatus.VeleroBackup.Name - if expectedStatus.VeleroBackup.Name == placeholder { - veleroBackupName = function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName) - } - if nonAdminBackup.Status.VeleroBackup.Name != veleroBackupName { - return fmt.Errorf("NonAdminBackup Status VeleroBackupName %v is not equal to expected %v", nonAdminBackup.Status.VeleroBackup.Name, veleroBackupName) - } - veleroBackupNamespace := expectedStatus.VeleroBackup.Namespace - if expectedStatus.VeleroBackup.Namespace == placeholder { - veleroBackupNamespace = oadpNamespaceName - } - if nonAdminBackup.Status.VeleroBackup.Namespace != veleroBackupNamespace { - return fmt.Errorf("NonAdminBackup Status VeleroBackupNamespace %v is not equal to expected %v", nonAdminBackup.Status.VeleroBackup.Namespace, veleroBackupNamespace) + + if nonAdminBackup.Status.VeleroBackup != nil { + if nonAdminBackup.Status.VeleroBackup.NameUUID == "" { + return fmt.Errorf("NonAdminBackup Status VeleroBackupName %v is 0 length string", nonAdminBackup.Status.VeleroBackup.NameUUID) } - if !reflect.DeepEqual(nonAdminBackup.Status.VeleroBackup.Status, expectedStatus.VeleroBackup.Status) { - return fmt.Errorf("NonAdminBackup Status VeleroBackupStatus %v is not equal to expected %v", nonAdminBackup.Status.VeleroBackup.Status, expectedStatus.VeleroBackup.Status) + + if expectedStatus.VeleroBackup != nil { + // When there is no VeleroBackup expected Namespace provided, use one that should be result of reconcile loop + veleroBackupNamespace := expectedStatus.VeleroBackup.Namespace + if veleroBackupNamespace == "" { + veleroBackupNamespace = oadpNamespaceName + } + if nonAdminBackup.Status.VeleroBackup.Namespace != veleroBackupNamespace { + return fmt.Errorf("NonAdminBackup Status VeleroBackupNamespace %v is not equal to expected %v", nonAdminBackup.Status.VeleroBackup.Namespace, veleroBackupNamespace) + } + if expectedStatus.VeleroBackup.Status != nil { + if !reflect.DeepEqual(nonAdminBackup.Status.VeleroBackup.Status, expectedStatus.VeleroBackup.Status) { + return fmt.Errorf("NonAdminBackup Status VeleroBackupStatus %v is not equal to expected %v", nonAdminBackup.Status.VeleroBackup.Status, expectedStatus.VeleroBackup.Status) + } + } } } @@ -119,7 +121,6 @@ func createTestNamespaces(ctx context.Context, nonAdminNamespaceName string, oad if err != nil { return err } - oadpNamespace := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: oadpNamespaceName, @@ -127,7 +128,6 @@ func createTestNamespaces(ctx context.Context, nonAdminNamespaceName string, oad } return k8sClient.Create(ctx, oadpNamespace) } - func deleteTestNamespaces(ctx context.Context, nonAdminNamespaceName string, oadpNamespaceName string) error { oadpNamespace := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ @@ -138,7 +138,6 @@ func deleteTestNamespaces(ctx context.Context, nonAdminNamespaceName string, oad if err != nil { return err } - nonAdminNamespace := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: nonAdminNamespaceName, @@ -149,50 +148,47 @@ func deleteTestNamespaces(ctx context.Context, nonAdminNamespaceName string, oad var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile function", func() { var ( - ctx = context.Background() - nonAdminNamespaceName = "" - oadpNamespaceName = "" - counter = 0 - updateTestScenario = func() { - counter++ - nonAdminNamespaceName = fmt.Sprintf("test-nonadminbackup-reconcile-%v", counter) - oadpNamespaceName = nonAdminNamespaceName + "-oadp" - } + ctx = context.Background() + nonAdminObjectName string + nonAdminObjectNamespace string + oadpNamespace string + veleroBackupNameUUID string + counter = 0 ) - + ginkgo.BeforeEach(func() { + counter++ + nonAdminObjectName = fmt.Sprintf("nab-object-%v", counter) + nonAdminObjectNamespace = fmt.Sprintf("test-nab-reconcile-%v", counter) + oadpNamespace = nonAdminObjectNamespace + "-oadp" + veleroBackupNameUUID = function.GenerateNacObjectNameWithUUID(nonAdminObjectNamespace, nonAdminObjectName) + gomega.Expect(createTestNamespaces(ctx, nonAdminObjectNamespace, oadpNamespace)).To(gomega.Succeed()) + }) ginkgo.AfterEach(func() { nonAdminBackup := &nacv1alpha1.NonAdminBackup{} if k8sClient.Get( ctx, types.NamespacedName{ - Name: testNonAdminBackupName, - Namespace: nonAdminNamespaceName, + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, }, nonAdminBackup, ) == nil { gomega.Expect(k8sClient.Delete(ctx, nonAdminBackup)).To(gomega.Succeed()) } - - gomega.Expect(deleteTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) + gomega.Expect(deleteTestNamespaces(ctx, nonAdminObjectNamespace, oadpNamespace)).To(gomega.Succeed()) }) - ginkgo.DescribeTable("Reconcile triggered by NonAdminBackup Delete event", func(scenario nonAdminBackupSingleReconcileScenario) { - updateTestScenario() - - gomega.Expect(createTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) - result, err := (&NonAdminBackupReconciler{ Client: k8sClient, Scheme: testEnv.Scheme, }).Reconcile( context.Background(), reconcile.Request{NamespacedName: types.NamespacedName{ - Namespace: nonAdminNamespaceName, - Name: testNonAdminBackupName, + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, }}, ) - gomega.Expect(result).To(gomega.Equal(scenario.result)) gomega.Expect(err).To(gomega.Not(gomega.HaveOccurred())) }, @@ -200,56 +196,60 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func result: reconcile.Result{}, }), ) - ginkgo.DescribeTable("Reconcile triggered by NonAdminBackup Create/Update events and by Requeue", func(scenario nonAdminBackupSingleReconcileScenario) { - updateTestScenario() - - gomega.Expect(createTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) - - nonAdminBackup := buildTestNonAdminBackup(nonAdminNamespaceName, scenario.spec) - gomega.Expect(k8sClient.Create(ctx, nonAdminBackup)).To(gomega.Succeed()) + nonAdminBackup := buildTestNonAdminBackup(nonAdminObjectNamespace, nonAdminObjectName, scenario.nonAdminBackupSpec) + gomega.Expect(k8sClient.Create(ctx, nonAdminBackup.DeepCopy())).To(gomega.Succeed()) + nonAdminBackupAfterCreate := &nacv1alpha1.NonAdminBackup{} + gomega.Expect(k8sClient.Get( + ctx, + types.NamespacedName{ + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, + }, + nonAdminBackupAfterCreate, + )).To(gomega.Succeed()) + if scenario.nonAdminBackupPriorStatus != nil { + nonAdminBackupAfterCreate.Status = *scenario.nonAdminBackupPriorStatus + if scenario.uuidFromTestCase { + nonAdminBackupAfterCreate.Status.VeleroBackup = &nacv1alpha1.VeleroBackup{ + NameUUID: veleroBackupNameUUID, + Namespace: oadpNamespace, + } + } + gomega.Expect(k8sClient.Status().Update(ctx, nonAdminBackupAfterCreate)).To(gomega.Succeed()) + } if scenario.createVeleroBackup { veleroBackup := &velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ - Name: function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName), - Namespace: oadpNamespaceName, - Labels: function.GetNonAdminLabels(), + Name: nonAdminBackupAfterCreate.Status.VeleroBackup.NameUUID, + Namespace: oadpNamespace, + Labels: map[string]string{ + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.NabOriginNameUUIDLabel: nonAdminBackupAfterCreate.Status.VeleroBackup.NameUUID, + }, Annotations: function.GetNonAdminBackupAnnotations(nonAdminBackup.ObjectMeta), }, Spec: velerov1.BackupSpec{ - IncludedNamespaces: []string{nonAdminNamespaceName}, + IncludedNamespaces: []string{nonAdminObjectNamespace}, }, } gomega.Expect(k8sClient.Create(ctx, veleroBackup)).To(gomega.Succeed()) } - - if scenario.priorStatus != nil { - nonAdminBackup.Status = *scenario.priorStatus - if nonAdminBackup.Status.VeleroBackup != nil { - if nonAdminBackup.Status.VeleroBackup.Name == placeholder { - nonAdminBackup.Status.VeleroBackup.Name = function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName) - } - if nonAdminBackup.Status.VeleroBackup.Namespace == placeholder { - nonAdminBackup.Status.VeleroBackup.Namespace = oadpNamespaceName - } - } - gomega.Expect(k8sClient.Status().Update(ctx, nonAdminBackup)).To(gomega.Succeed()) - } // easy hack to test that only one update call happens per reconcile // priorResourceVersion, err := strconv.Atoi(nonAdminBackup.ResourceVersion) // gomega.Expect(err).To(gomega.Not(gomega.HaveOccurred())) - result, err := (&NonAdminBackupReconciler{ Client: k8sClient, Scheme: testEnv.Scheme, - OADPNamespace: oadpNamespaceName, + OADPNamespace: oadpNamespace, }).Reconcile( context.Background(), reconcile.Request{NamespacedName: types.NamespacedName{ - Namespace: nonAdminNamespaceName, - Name: testNonAdminBackupName, + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, }}, ) gomega.Expect(result).To(gomega.Equal(scenario.result)) @@ -259,37 +259,39 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func gomega.Expect(err).To(gomega.HaveOccurred()) gomega.Expect(err.Error()).To(gomega.ContainSubstring(scenario.resultError.Error())) } - + nonAdminBackupAfterReconcile := &nacv1alpha1.NonAdminBackup{} gomega.Expect(k8sClient.Get( ctx, types.NamespacedName{ - Name: testNonAdminBackupName, - Namespace: nonAdminNamespaceName, + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, }, - nonAdminBackup, + nonAdminBackupAfterReconcile, )).To(gomega.Succeed()) - - gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackup, scenario.ExpectedStatus, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) - + gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackupAfterReconcile, scenario.nonAdminBackupExpectedStatus, oadpNamespace)).To(gomega.Succeed()) + if scenario.uuidCreatedByReconcile { + gomega.Expect(nonAdminBackupAfterReconcile.Status.VeleroBackup.NameUUID).To(gomega.ContainSubstring(nonAdminObjectNamespace)) + gomega.Expect(nonAdminBackupAfterReconcile.Status.VeleroBackup.Namespace).To(gomega.Equal(oadpNamespace)) + } // easy hack to test that only one update call happens per reconcile // currentResourceVersion, err := strconv.Atoi(nonAdminBackup.ResourceVersion) // gomega.Expect(err).To(gomega.Not(gomega.HaveOccurred())) // gomega.Expect(currentResourceVersion - priorResourceVersion).To(gomega.Equal(1)) }, ginkgo.Entry("When triggered by NonAdminBackup Create event, should update NonAdminBackup phase to new and Requeue", nonAdminBackupSingleReconcileScenario{ - ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseNew, }, result: reconcile.Result{Requeue: true}, }), ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new), should update NonAdminBackup Condition to Accepted True and Requeue", nonAdminBackupSingleReconcileScenario{ - spec: nacv1alpha1.NonAdminBackupSpec{ + nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{}, }, - priorStatus: &nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseNew, }, - ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseNew, Conditions: []metav1.Condition{ { @@ -302,11 +304,41 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, result: reconcile.Result{Requeue: true}, }), - ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new; Conditions Accepted True), should update NonAdminBackup phase to created and Condition to Queued True and VeleroBackup reference and Exit", nonAdminBackupSingleReconcileScenario{ - spec: nacv1alpha1.NonAdminBackupSpec{ + ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new; Conditions Accepted True), should update NonAdminBackup Status generated UUID for VeleroBackup and Requeue", nonAdminBackupSingleReconcileScenario{ + nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: &velerov1.BackupSpec{}, + }, + nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseNew, + Conditions: []metav1.Condition{ + { + Type: string(nacv1alpha1.NonAdminConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + LastTransitionTime: metav1.NewTime(time.Now()), + }, + }, + }, + nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseNew, + Conditions: []metav1.Condition{ + { + Type: string(nacv1alpha1.NonAdminConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + }, + }, + }, + uuidCreatedByReconcile: true, + result: reconcile.Result{Requeue: true}, + }), + ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new; Conditions Accepted True; NonAdminBackup Status NameUUID set), should update NonAdminBackup phase to created and Condition to Queued True and Exit", nonAdminBackupSingleReconcileScenario{ + nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{}, }, - priorStatus: &nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseNew, Conditions: []metav1.Condition{ { @@ -318,12 +350,8 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, }, }, - ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseCreated, - VeleroBackup: &nacv1alpha1.VeleroBackup{ - Name: placeholder, - Namespace: placeholder, - }, Conditions: []metav1.Condition{ { Type: "Accepted", @@ -339,19 +367,17 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, }, }, - result: reconcile.Result{}, + uuidFromTestCase: true, + result: reconcile.Result{}, }), ginkgo.Entry("When triggered by VeleroBackup Update event, should update NonAdminBackup VeleroBackupStatus and Exit", nonAdminBackupSingleReconcileScenario{ createVeleroBackup: true, - spec: nacv1alpha1.NonAdminBackupSpec{ + nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{}, }, - priorStatus: &nacv1alpha1.NonAdminBackupStatus{ - Phase: nacv1alpha1.NonAdminBackupPhaseCreated, - VeleroBackup: &nacv1alpha1.VeleroBackup{ - Name: placeholder, - Namespace: placeholder, - }, + nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + VeleroBackup: &nacv1alpha1.VeleroBackup{}, Conditions: []metav1.Condition{ { Type: "Accepted", @@ -369,12 +395,10 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, }, }, - ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseCreated, VeleroBackup: &nacv1alpha1.VeleroBackup{ - Name: placeholder, - Namespace: placeholder, - Status: &velerov1.BackupStatus{}, + Status: &velerov1.BackupStatus{}, }, Conditions: []metav1.Condition{ { @@ -391,18 +415,19 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, }, }, - result: reconcile.Result{}, + uuidFromTestCase: true, + result: reconcile.Result{}, }), ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new) [invalid spec], should update NonAdminBackup phase to BackingOff and Condition to Accepted False and Exit with terminal error", nonAdminBackupSingleReconcileScenario{ - spec: nacv1alpha1.NonAdminBackupSpec{ + nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{ IncludedNamespaces: []string{"not-valid"}, }, }, - priorStatus: &nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseNew, }, - ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseBackingOff, Conditions: []metav1.Condition{ { @@ -414,27 +439,28 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, }, resultError: reconcile.TerminalError(fmt.Errorf("spec.backupSpec.IncludedNamespaces can not contain namespaces other than: ")), - }), - ) + })) }) var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", func() { var ( - ctx context.Context - cancel context.CancelFunc - nonAdminNamespaceName = "" - oadpNamespaceName = "" - counter = 0 - updateTestScenario = func() { - ctx, cancel = context.WithCancel(context.Background()) - counter++ - nonAdminNamespaceName = fmt.Sprintf("test-nonadminbackup-reconcile-full-%v", counter) - oadpNamespaceName = nonAdminNamespaceName + "-oadp" - } + ctx context.Context + cancel context.CancelFunc + nonAdminObjectName = "" + nonAdminObjectNamespace = "" + oadpNamespace = "" + counter = 0 ) + ginkgo.BeforeEach(func() { + counter++ + nonAdminObjectName = fmt.Sprintf("nab-object-%v", counter) + nonAdminObjectNamespace = fmt.Sprintf("test-nab-reconcile-full-%v", counter) + oadpNamespace = nonAdminObjectNamespace + "-oadp" + }) + ginkgo.AfterEach(func() { - gomega.Expect(deleteTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) + gomega.Expect(deleteTestNamespaces(ctx, nonAdminObjectNamespace, oadpNamespace)).To(gomega.Succeed()) cancel() // wait cancel @@ -443,9 +469,9 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", ginkgo.DescribeTable("Reconcile triggered by NonAdminBackup Create event", func(scenario nonAdminBackupFullReconcileScenario) { - updateTestScenario() + ctx, cancel = context.WithCancel(context.Background()) - gomega.Expect(createTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) + gomega.Expect(createTestNamespaces(ctx, nonAdminObjectNamespace, oadpNamespace)).To(gomega.Succeed()) k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{ Scheme: k8sClient.Scheme(), @@ -455,7 +481,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", err = (&NonAdminBackupReconciler{ Client: k8sManager.GetClient(), Scheme: k8sManager.GetScheme(), - OADPNamespace: oadpNamespaceName, + OADPNamespace: oadpNamespace, }).SetupWithManager(k8sManager) gomega.Expect(err).ToNot(gomega.HaveOccurred()) @@ -465,28 +491,44 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", gomega.Expect(err).ToNot(gomega.HaveOccurred(), "failed to run manager") }() // wait manager start - time.Sleep(1 * time.Second) + managerStartTimeout := 10 * time.Second + pollInterval := 100 * time.Millisecond + ctxTimeout, cancel := context.WithTimeout(ctx, managerStartTimeout) + defer cancel() + + err = wait.PollUntilContextTimeout(ctxTimeout, pollInterval, managerStartTimeout, true, func(ctx context.Context) (done bool, err error) { + select { + case <-ctx.Done(): + return false, ctx.Err() + default: + // Check if the manager has started by verifying if the client is initialized + return k8sManager.GetClient() != nil, nil + } + }) + // Check if the context timeout or another error occurred + gomega.Expect(err).ToNot(gomega.HaveOccurred(), "Manager failed to start within the timeout period") ginkgo.By("Waiting Reconcile of create event") - nonAdminBackup := buildTestNonAdminBackup(nonAdminNamespaceName, scenario.spec) - gomega.Expect(k8sClient.Create(ctx, nonAdminBackup)).To(gomega.Succeed()) + nonAdminBackup := buildTestNonAdminBackup(nonAdminObjectNamespace, nonAdminObjectName, scenario.spec) + gomega.Expect(k8sClient.Create(ctxTimeout, nonAdminBackup)).To(gomega.Succeed()) // wait NAB reconcile - time.Sleep(1 * time.Second) + time.Sleep(2 * time.Second) ginkgo.By("Fetching NonAdminBackup after Reconcile") gomega.Expect(k8sClient.Get( - ctx, + ctxTimeout, types.NamespacedName{ - Name: testNonAdminBackupName, - Namespace: nonAdminNamespaceName, + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, }, nonAdminBackup, )).To(gomega.Succeed()) ginkgo.By("Validating NonAdminBackup Status") - gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackup, scenario.status, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) - if scenario.status.VeleroBackup != nil && len(scenario.status.VeleroBackup.Name) > 0 { + gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackup, scenario.status, oadpNamespace)).To(gomega.Succeed()) + + if scenario.status.VeleroBackup != nil && len(nonAdminBackup.Status.VeleroBackup.NameUUID) > 0 { ginkgo.By("Checking if NonAdminBackup Spec was not changed") gomega.Expect(reflect.DeepEqual( nonAdminBackup.Spec, @@ -494,32 +536,41 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", )).To(gomega.BeTrue()) ginkgo.By("Simulating VeleroBackup update to finished state") + veleroBackup := &velerov1.Backup{} gomega.Expect(k8sClient.Get( - ctx, + ctxTimeout, types.NamespacedName{ - Name: function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName), - Namespace: oadpNamespaceName, + Name: nonAdminBackup.Status.VeleroBackup.NameUUID, + Namespace: oadpNamespace, }, veleroBackup, )).To(gomega.Succeed()) - veleroBackup.Status.Phase = velerov1.BackupPhaseCompleted - // TODO can not call .Status().Update() for veleroBackup object: backups.velero.io "name..." not found error - gomega.Expect(k8sClient.Update(ctx, veleroBackup)).To(gomega.Succeed()) + + veleroBackup.Status = velerov1.BackupStatus{ + Phase: velerov1.BackupPhaseCompleted, + } + + gomega.Expect(k8sClient.Update(ctxTimeout, veleroBackup)).To(gomega.Succeed()) + + time.Sleep(1 * time.Second) + ginkgo.By("VeleroBackup updated") + + // wait NAB reconcile gomega.Eventually(func() (bool, error) { err := k8sClient.Get( - ctx, + ctxTimeout, types.NamespacedName{ - Name: testNonAdminBackupName, - Namespace: nonAdminNamespaceName, + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, }, nonAdminBackup, ) if err != nil { return false, err } - if nonAdminBackup.Status.VeleroBackup.Status == nil { + if nonAdminBackup == nil || nonAdminBackup.Status.VeleroBackup == nil || nonAdminBackup.Status.VeleroBackup.Status == nil { return false, nil } return nonAdminBackup.Status.VeleroBackup.Status.Phase == velerov1.BackupPhaseCompleted, nil @@ -527,7 +578,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", } ginkgo.By("Waiting Reconcile of delete event") - gomega.Expect(k8sClient.Delete(ctx, nonAdminBackup)).To(gomega.Succeed()) + gomega.Expect(k8sClient.Delete(ctxTimeout, nonAdminBackup)).To(gomega.Succeed()) time.Sleep(1 * time.Second) }, ginkgo.Entry("Should update NonAdminBackup until VeleroBackup completes and then delete it", nonAdminBackupFullReconcileScenario{ @@ -537,8 +588,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", status: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseCreated, VeleroBackup: &nacv1alpha1.VeleroBackup{ - Name: placeholder, - Namespace: placeholder, + Namespace: oadpNamespace, Status: nil, }, Conditions: []metav1.Condition{