From dc7b9df5d6541c2032a60b71b5c5702ccce080f3 Mon Sep 17 00:00:00 2001 From: Mateus Oliveira Date: Thu, 3 Oct 2024 09:37:52 -0300 Subject: [PATCH] fix: Reconcile duplication Signed-off-by: Mateus Oliveira --- internal/common/function/function.go | 157 ++------- internal/common/function/function_test.go | 297 ++++-------------- .../controller/nonadminbackup_controller.go | 125 +++----- .../nonadminbackup_controller_test.go | 4 +- 4 files changed, 131 insertions(+), 452 deletions(-) diff --git a/internal/common/function/function.go b/internal/common/function/function.go index 583210b..663e1a6 100644 --- a/internal/common/function/function.go +++ b/internal/common/function/function.go @@ -24,7 +24,7 @@ import ( "fmt" "github.com/go-logr/logr" - velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation" "sigs.k8s.io/controller-runtime/pkg/client" @@ -34,44 +34,21 @@ import ( "github.com/migtools/oadp-non-admin/internal/common/constant" ) -const requiredAnnotationError = "backup does not have the required annotation '%s'" - -// AddNonAdminLabels return a map with both the object labels and with the default Non Admin labels. -// If error occurs, a map with only the default Non Admin labels is returned -func AddNonAdminLabels(labels map[string]string) map[string]string { - defaultLabels := map[string]string{ +// AddNonAdminLabels return the required Non Admin labels +func AddNonAdminLabels() map[string]string { + return map[string]string{ constant.OadpLabel: constant.OadpLabelValue, constant.ManagedByLabel: constant.ManagedByLabelValue, } - - mergedLabels, err := mergeMaps(defaultLabels, labels) - if err != nil { - // TODO logger - _, _ = fmt.Println("Error merging labels:", err) - // TODO break? - return defaultLabels - } - return mergedLabels } -// AddNonAdminBackupAnnotations return a map with both the object annotations and with the default NonAdminBackup annotations. -// If error occurs, a map with only the default NonAdminBackup annotations is returned -func AddNonAdminBackupAnnotations(ownerNamespace string, ownerName string, ownerUUID string, existingAnnotations map[string]string) map[string]string { - // TODO could not receive object meta and get info from there? - defaultAnnotations := map[string]string{ - constant.NabOriginNamespaceAnnotation: ownerNamespace, - constant.NabOriginNameAnnotation: ownerName, - constant.NabOriginUUIDAnnotation: ownerUUID, - } - - mergedAnnotations, err := mergeMaps(defaultAnnotations, existingAnnotations) - if err != nil { - // TODO logger - _, _ = fmt.Println("Error merging annotations:", err) - // TODO break? - return defaultAnnotations +// AddNonAdminBackupAnnotations return the required Non Admin annotations +func AddNonAdminBackupAnnotations(objectMeta metav1.ObjectMeta) map[string]string { + return map[string]string{ + constant.NabOriginNamespaceAnnotation: objectMeta.Namespace, + constant.NabOriginNameAnnotation: objectMeta.Name, + constant.NabOriginUUIDAnnotation: string(objectMeta.UID), } - return mergedAnnotations } // containsOnlyNamespace checks if the given namespaces slice contains only the specified namespace @@ -84,27 +61,20 @@ func containsOnlyNamespace(namespaces []string, namespace string) bool { return true } -// GetBackupSpecFromNonAdminBackup return BackupSpec object from NonAdminBackup spec, if no error occurs -func GetBackupSpecFromNonAdminBackup(nonAdminBackup *nacv1alpha1.NonAdminBackup) (*velerov1.BackupSpec, error) { - // TODO https://github.com/migtools/oadp-non-admin/issues/60 +// ValidateBackupSpec return nil, if NonAdminBackup is valid; error otherwise +func ValidateBackupSpec(nonAdminBackup *nacv1alpha1.NonAdminBackup) error { // this should be Kubernetes API validation if nonAdminBackup.Spec.BackupSpec == nil { - return nil, fmt.Errorf("BackupSpec is not defined") + return fmt.Errorf("BackupSpec is not defined") } - veleroBackupSpec := nonAdminBackup.Spec.BackupSpec.DeepCopy() - - // TODO: Additional validations, before continuing - - if veleroBackupSpec.IncludedNamespaces == nil { - veleroBackupSpec.IncludedNamespaces = []string{nonAdminBackup.Namespace} - } else { - if !containsOnlyNamespace(veleroBackupSpec.IncludedNamespaces, nonAdminBackup.Namespace) { - return nil, fmt.Errorf("spec.backupSpec.IncludedNamespaces can not contain namespaces other than: %s", nonAdminBackup.Namespace) + if nonAdminBackup.Spec.BackupSpec.IncludedNamespaces != nil { + if !containsOnlyNamespace(nonAdminBackup.Spec.BackupSpec.IncludedNamespaces, nonAdminBackup.Namespace) { + return fmt.Errorf("spec.backupSpec.IncludedNamespaces can not contain namespaces other than: %s", nonAdminBackup.Namespace) } } - return veleroBackupSpec, nil + return nil } // GenerateVeleroBackupName generates a Velero backup name based on the provided namespace and NonAdminBackup name. @@ -112,31 +82,24 @@ func GetBackupSpecFromNonAdminBackup(nonAdminBackup *nacv1alpha1.NonAdminBackup) // 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 - const hashLength = 14 - prefixLength := len(constant.VeleroBackupNamePrefix) + len("--") // Account for two "-" - hasher := sha256.New() _, err := hasher.Write([]byte(nabName)) if err != nil { return "" } + const hashLength = 14 nameHash := hex.EncodeToString(hasher.Sum(nil))[:hashLength] // Take first 14 chars - // Generate the Velero backup name created from NAB - veleroBackupName := fmt.Sprintf("%s-%s-%s", constant.VeleroBackupNamePrefix, namespace, nameHash) - + usedLength := hashLength + len(constant.VeleroBackupNamePrefix) + len("--") + maxNamespaceLength := validation.DNS1123SubdomainMaxLength - usedLength // Ensure the name is within the character limit - if len(veleroBackupName) > validation.DNS1123SubdomainMaxLength { + if len(namespace) > maxNamespaceLength { // Truncate the namespace if necessary - maxNamespaceLength := validation.DNS1123SubdomainMaxLength - len(nameHash) - prefixLength - if len(namespace) > maxNamespaceLength { - namespace = namespace[:maxNamespaceLength] - } - veleroBackupName = fmt.Sprintf("%s-%s-%s", constant.VeleroBackupNamePrefix, namespace, nameHash) + return fmt.Sprintf("%s-%s-%s", constant.VeleroBackupNamePrefix, namespace[:maxNamespaceLength], nameHash) } - return veleroBackupName + return fmt.Sprintf("%s-%s-%s", constant.VeleroBackupNamePrefix, namespace, nameHash) } // CheckVeleroBackupMetadata return true if Velero Backup object has required Non Admin labels and annotations, false otherwise @@ -181,80 +144,6 @@ func checkAnnotationValueIsValid(annotations map[string]string, key string) bool return length > 0 && length < validation.DNS1123SubdomainMaxLength } -// TODO not used - -// GetNonAdminBackupFromVeleroBackup return referenced NonAdminBackup object from Velero Backup object, if no error occurs -func GetNonAdminBackupFromVeleroBackup(ctx context.Context, clientInstance client.Client, backup *velerov1.Backup) (*nacv1alpha1.NonAdminBackup, error) { - // Check if the backup has the required annotations to identify the associated NonAdminBackup object - logger := log.FromContext(ctx) - - annotations := backup.GetAnnotations() - - annotationsStr := fmt.Sprintf("%v", annotations) - logger.V(1).Info("Velero Backup Annotations", "annotations", annotationsStr) - - if annotations == nil { - return nil, fmt.Errorf("backup has no annotations") - } - - nabOriginNamespace, ok := annotations[constant.NabOriginNamespaceAnnotation] - if !ok { - return nil, fmt.Errorf(requiredAnnotationError, constant.NabOriginNamespaceAnnotation) - } - - nabOriginName, ok := annotations[constant.NabOriginNameAnnotation] - if !ok { - return nil, fmt.Errorf(requiredAnnotationError, constant.NabOriginNameAnnotation) - } - - nonAdminBackupKey := types.NamespacedName{ - Namespace: nabOriginNamespace, - Name: nabOriginName, - } - - nonAdminBackup := &nacv1alpha1.NonAdminBackup{} - err := clientInstance.Get(ctx, nonAdminBackupKey, nonAdminBackup) - if err != nil { - return nil, fmt.Errorf("failed to fetch NonAdminBackup object: %v", err) - } - - nabOriginUUID, ok := annotations[constant.NabOriginUUIDAnnotation] - if !ok { - return nil, fmt.Errorf(requiredAnnotationError, constant.NabOriginUUIDAnnotation) - } - // Ensure UID matches - if nonAdminBackup.ObjectMeta.UID != types.UID(nabOriginUUID) { - return nil, fmt.Errorf("UID from annotation does not match UID of fetched NonAdminBackup object") - } - - return nonAdminBackup, nil -} - -// TODO import? Similar to as pkg/common/common.go:AppendUniqueKeyTOfTMaps from github.com/openshift/oadp-operator - -// Return map, of the same type as the input maps, that contains all keys/values from all input maps. -// Key/value pairs that are identical in different input maps, are added only once to return map. -// If a key exists in more than one input map, with a different value, an error is returned -func mergeMaps[T comparable](maps ...map[T]T) (map[T]T, error) { - merge := make(map[T]T) - for _, m := range maps { - if m == nil { - continue - } - for k, v := range m { - existingValue, found := merge[k] - if found { - if existingValue != v { - return nil, fmt.Errorf("conflicting key %v: has both value %v and value %v in input maps", k, v, existingValue) - } - continue - } - merge[k] = v - } - } - return merge, nil -} - // GetLogger return a logger from input ctx, with additional key/value pairs being // input key and input obj name and namespace func GetLogger(ctx context.Context, obj client.Object, key string) logr.Logger { diff --git a/internal/common/function/function_test.go b/internal/common/function/function_test.go index ce02091..dd08417 100644 --- a/internal/common/function/function_test.go +++ b/internal/common/function/function_test.go @@ -17,9 +17,7 @@ limitations under the License. package function import ( - "context" "fmt" - "reflect" "strings" "testing" @@ -29,10 +27,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation" - clientgoscheme "k8s.io/client-go/kubernetes/scheme" - ctrl "sigs.k8s.io/controller-runtime" - "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" @@ -46,221 +40,88 @@ const ( testNonAdminBackupUUID = "12345678-1234-1234-1234-123456789abc" ) -func TestMergeMaps(t *testing.T) { - const ( - d = "d" - delta = "delta" - ) - tests := []struct { - name string - want map[string]string - args []map[string]string - wantErr bool - }{ - { - name: "append unique labels together", - args: []map[string]string{ - {"a": "alpha"}, - {"b": "beta"}, - }, - want: map[string]string{ - "a": "alpha", - "b": "beta", - }, - }, - { - name: "append unique labels together, with valid duplicates", - args: []map[string]string{ - {"c": "gamma"}, - {d: delta}, - {d: delta}, - }, - want: map[string]string{ - "c": "gamma", - d: delta, - }, - }, - { - name: "append unique labels together - nil sandwich", - args: []map[string]string{ - {"x": "chi"}, - nil, - {"y": "psi"}, - }, - want: map[string]string{ - "x": "chi", - "y": "psi", - }, - }, - { - name: "should error when append duplicate label keys with different value together", - args: []map[string]string{ - {"key": "value-1"}, - {"key": "value-2"}, - }, - want: nil, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := mergeMaps(tt.args...) - if (err != nil) != tt.wantErr { - t.Errorf("mergeMaps() error = %v, wantErr %v", err, tt.wantErr) - return - } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("mergeMaps() = %v, want %v", got, tt.want) - } - }) - } -} - func TestAddNonAdminLabels(t *testing.T) { - // Additional labels provided - additionalLabels := map[string]string{ - "additionalLabel1": "value1", - "additionalLabel2": "value2", - } - - expectedLabels := map[string]string{ - constant.OadpLabel: "True", + expected := map[string]string{ + constant.OadpLabel: constant.OadpLabelValue, constant.ManagedByLabel: constant.ManagedByLabelValue, - "additionalLabel1": "value1", - "additionalLabel2": "value2", } - mergedLabels := AddNonAdminLabels(additionalLabels) - assert.Equal(t, expectedLabels, mergedLabels, "Merged labels should match expected labels") + result := AddNonAdminLabels() + assert.Equal(t, expected, result) } func TestAddNonAdminBackupAnnotations(t *testing.T) { - // Merging annotations without conflicts - existingAnnotations := map[string]string{ - "existingKey1": "existingValue1", - "existingKey2": "existingValue2", - } - - ownerName := "testOwner" - ownerNamespace := "testNamespace" - ownerUUID := "f2c4d2c3-58d3-46ec-bf03-5940f567f7f8" - - expectedAnnotations := map[string]string{ - constant.NabOriginNamespaceAnnotation: ownerNamespace, - constant.NabOriginNameAnnotation: ownerName, - constant.NabOriginUUIDAnnotation: ownerUUID, - "existingKey1": "existingValue1", - "existingKey2": "existingValue2", - } - - mergedAnnotations := AddNonAdminBackupAnnotations(ownerNamespace, ownerName, ownerUUID, existingAnnotations) - assert.Equal(t, expectedAnnotations, mergedAnnotations, "Merged annotations should match expected annotations") - - // Merging annotations with conflicts - existingAnnotationsWithConflict := map[string]string{ - constant.NabOriginNameAnnotation: "conflictingValue", - } - - expectedAnnotationsWithConflict := map[string]string{ - constant.NabOriginNameAnnotation: ownerName, - constant.NabOriginNamespaceAnnotation: ownerNamespace, - constant.NabOriginUUIDAnnotation: ownerUUID, - } - - mergedAnnotationsWithConflict := AddNonAdminBackupAnnotations(ownerNamespace, ownerName, ownerUUID, existingAnnotationsWithConflict) - assert.Equal(t, expectedAnnotationsWithConflict, mergedAnnotationsWithConflict, "Merged annotations should match expected annotations with conflict") -} - -func TestGetBackupSpecFromNonAdminBackup(t *testing.T) { - // Test case: BackupSpec is nil nonAdminBackup := &nacv1alpha1.NonAdminBackup{ - Spec: nacv1alpha1.NonAdminBackupSpec{ - BackupSpec: nil, - }, - } - backupSpec, err := GetBackupSpecFromNonAdminBackup(nonAdminBackup) - assert.Error(t, err) - assert.Nil(t, backupSpec) - assert.Equal(t, "BackupSpec is not defined", err.Error()) - - // Test case: NonAdminBackup with valid BackupSpec - backupSpecInput := &velerov1.BackupSpec{ - IncludedNamespaces: []string{"namespace1"}, - StorageLocation: "s3://bucket-name/path/to/backup", - VolumeSnapshotLocations: []string{"volume-snapshot-location"}, - } - - nonAdminBackup = &nacv1alpha1.NonAdminBackup{ ObjectMeta: metav1.ObjectMeta{ - Namespace: "namespace1", // Set the namespace of NonAdminBackup - }, - Spec: nacv1alpha1.NonAdminBackupSpec{ - BackupSpec: backupSpecInput, + Namespace: testNonAdminBackupNamespace, + Name: testNonAdminBackupName, + UID: types.UID(testNonAdminBackupUUID), }, } - backupSpec, err = GetBackupSpecFromNonAdminBackup(nonAdminBackup) - - assert.NoError(t, err) - assert.NotNil(t, backupSpec) - assert.Equal(t, []string{"namespace1"}, backupSpec.IncludedNamespaces) // Check that only the namespace from NonAdminBackup is included - assert.Equal(t, backupSpecInput.ExcludedNamespaces, backupSpec.ExcludedNamespaces) - assert.Equal(t, backupSpecInput.StorageLocation, backupSpec.StorageLocation) - assert.Equal(t, backupSpecInput.VolumeSnapshotLocations, backupSpec.VolumeSnapshotLocations) - backupSpecInput = &velerov1.BackupSpec{ - IncludedNamespaces: []string{"namespace2", "namespace3"}, + expected := map[string]string{ + constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, + constant.NabOriginNameAnnotation: testNonAdminBackupName, + constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, } - nonAdminBackup = &nacv1alpha1.NonAdminBackup{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "namespace2", // Set the namespace of NonAdminBackup - }, - Spec: nacv1alpha1.NonAdminBackupSpec{ - BackupSpec: backupSpecInput, - }, - } - backupSpec, err = GetBackupSpecFromNonAdminBackup(nonAdminBackup) - - assert.Error(t, err) - assert.Nil(t, backupSpec) - assert.Equal(t, "spec.backupSpec.IncludedNamespaces can not contain namespaces other than: namespace2", err.Error()) - - backupSpecInput = &velerov1.BackupSpec{ - IncludedNamespaces: []string{"namespace3"}, - } + result := AddNonAdminBackupAnnotations(nonAdminBackup.ObjectMeta) + assert.Equal(t, expected, result) +} - nonAdminBackup = &nacv1alpha1.NonAdminBackup{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "namespace4", // Set the namespace of NonAdminBackup +func TestValidateBackupSpec(t *testing.T) { + tests := []struct { + spec *velerov1.BackupSpec + name string + errMessage string + }{ + { + name: "nil spec", + spec: nil, + errMessage: "BackupSpec is not defined", + }, + { + name: "namespace different than NonAdminBackup namespace", + spec: &velerov1.BackupSpec{ + IncludedNamespaces: []string{"namespace1", "namespace2", "namespace3"}, + }, + errMessage: "spec.backupSpec.IncludedNamespaces can not contain namespaces other than: non-admin-backup-namespace", }, - Spec: nacv1alpha1.NonAdminBackupSpec{ - BackupSpec: backupSpecInput, + { + name: "valid spec", + spec: &velerov1.BackupSpec{ + IncludedNamespaces: []string{testNonAdminBackupNamespace}, + }, }, } - backupSpec, err = GetBackupSpecFromNonAdminBackup(nonAdminBackup) - - assert.Error(t, err) - assert.Nil(t, backupSpec) - assert.Equal(t, "spec.backupSpec.IncludedNamespaces can not contain namespaces other than: namespace4", err.Error()) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + nonAdminBackup := &nacv1alpha1.NonAdminBackup{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNonAdminBackupNamespace, + }, + Spec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: test.spec, + }, + } + err := ValidateBackupSpec(nonAdminBackup) + if len(test.errMessage) == 0 { + assert.NoError(t, err) + } else { + assert.Error(t, err) + assert.Equal(t, test.errMessage, err.Error()) + } + }) + } } func TestGenerateVeleroBackupName(t *testing.T) { - longString := "" - const longStringSize = 240 - for i := 0; i < longStringSize; i++ { - longString += "m" - } - - truncatedString := "" - // constant.MaxKubernetesNameLength = 253 + // Max Kubernetes name length := 253 + // hashLength := 14 // deduct -3: constant.VeleroBackupNamePrefix = "nab" - // deduct -2: there are two additional separators "-" in the name - // 253 - len(nameHash) - 3 - 2 + // deduct -2: two "-" in the name // 253 - 14 - 3 - 2 = 234 const truncatedStringSize = 234 - for i := 0; i < truncatedStringSize; i++ { - truncatedString += "m" - } testCases := []struct { namespace string @@ -273,9 +134,9 @@ func TestGenerateVeleroBackupName(t *testing.T) { expected: "nab-example-namespace-1cdadb729eaac1", }, { - namespace: longString, + namespace: strings.Repeat("m", validation.DNS1123SubdomainMaxLength), name: "example-name", - expected: fmt.Sprintf("nab-%s-1cdadb729eaac1", truncatedString), + expected: fmt.Sprintf("nab-%s-1cdadb729eaac1", strings.Repeat("m", truncatedStringSize)), }, } @@ -287,44 +148,6 @@ func TestGenerateVeleroBackupName(t *testing.T) { } } -func TestGetNonAdminBackupFromVeleroBackup(t *testing.T) { - log := zap.New(zap.UseDevMode(true)) - ctx := context.Background() - ctx = ctrl.LoggerInto(ctx, log) - - // Register NonAdminBackup type with the scheme - if err := nacv1alpha1.AddToScheme(clientgoscheme.Scheme); err != nil { - t.Fatalf("Failed to register NonAdminBackup type: %v", err) - } - - backup := &velerov1.Backup{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test-namespace", - Name: "test-backup", - Annotations: map[string]string{ - constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, - constant.NabOriginNameAnnotation: testNonAdminBackupName, - constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, - }, - }, - } - - nonAdminBackup := &nacv1alpha1.NonAdminBackup{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: testNonAdminBackupNamespace, - Name: testNonAdminBackupName, - UID: types.UID(testNonAdminBackupUUID), - }, - } - - client := fake.NewClientBuilder().WithObjects(nonAdminBackup).Build() - - result, err := GetNonAdminBackupFromVeleroBackup(ctx, client, backup) - assert.NoError(t, err, "GetNonAdminFromBackup should not return an error") - assert.NotNil(t, result, "Returned NonAdminBackup should not be nil") - assert.Equal(t, nonAdminBackup, result, "Returned NonAdminBackup should match expected NonAdminBackup") -} - func TestCheckVeleroBackupMetadata(t *testing.T) { tests := []struct { backup *velerov1.Backup diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index b64a7cd..8d93f4d 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -48,6 +48,8 @@ type NonAdminBackupReconciler struct { OADPNamespace string } +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" @@ -68,8 +70,8 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque logger.V(1).Info("NonAdminBackup Reconcile start") // Get the NonAdminBackup object - nab := nacv1alpha1.NonAdminBackup{} - err := r.Get(ctx, req.NamespacedName, &nab) + nab := &nacv1alpha1.NonAdminBackup{} + err := r.Get(ctx, req.NamespacedName, nab) if err != nil { if apierrors.IsNotFound(err) { logger.V(1).Info(err.Error()) @@ -79,86 +81,67 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, err } - reconcileExit, reconcileRequeue, reconcileErr := r.Init(ctx, logger, &nab) - if reconcileRequeue { - return ctrl.Result{Requeue: true}, reconcileErr - } else if reconcileExit && reconcileErr != nil { - return ctrl.Result{}, reconcileErr - } else if reconcileExit { - return ctrl.Result{}, nil - } - - reconcileExit, reconcileRequeue, reconcileErr = r.ValidateSpec(ctx, logger, &nab) - if reconcileRequeue { - return ctrl.Result{Requeue: true}, reconcileErr - } else if reconcileExit && reconcileErr != nil { - return ctrl.Result{}, reconcileErr - } else if reconcileExit { - return ctrl.Result{}, nil + reconcileSteps := []reconcileStepFunction{ + r.init, + r.validateSpec, + r.syncVeleroBackupWithNonAdminBackup, } - - reconcileExit, reconcileRequeue, reconcileErr = r.SyncVeleroBackupWithNonAdminBackup(ctx, logger, &nab) - if reconcileRequeue { - return ctrl.Result{Requeue: true}, reconcileErr - } else if reconcileExit && reconcileErr != nil { - return ctrl.Result{}, reconcileErr - } else if reconcileExit { - return ctrl.Result{}, nil + for _, step := range reconcileSteps { + requeue, err := step(ctx, logger, nab) + if err != nil { + return ctrl.Result{}, err + } else if requeue { + return ctrl.Result{Requeue: true}, nil + } } - logger.V(1).Info("NonAdminBackup Reconcile exit") return ctrl.Result{}, nil } -// Init initializes the Status.Phase from the NonAdminBackup. +// init initializes the Status.Phase from the NonAdminBackup. // // Parameters: // // ctx: Context for the request. -// logrLogger: Logger instance for logging messages. +// logger: Logger instance for logging messages. // nab: Pointer to the NonAdminBackup object. // // The function checks if the Phase of the NonAdminBackup object is empty. // If it is empty, it sets the Phase to "New". // It then returns boolean values indicating whether the reconciliation loop should requeue or exit // and error value whether the status was updated successfully. -func (r *NonAdminBackupReconciler) Init(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - logger := logrLogger - +func (r *NonAdminBackupReconciler) init(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { if nab.Status.Phase == constant.EmptyString { updated := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseNew) if updated { if err := r.Status().Update(ctx, nab); err != nil { logger.Error(err, statusUpdateError) - return true, false, err + return false, err } logger.V(1).Info(phaseUpdateRequeue) - return false, true, nil + return true, nil } } logger.V(1).Info("NonAdminBackup Phase already initialized") - return false, false, nil + return false, nil } -// ValidateSpec validates the Spec from the NonAdminBackup. +// validateSpec validates the Spec from the NonAdminBackup. // // Parameters: // // ctx: Context for the request. -// logrLogger: Logger instance for logging messages. +// logger: Logger instance for logging messages. // nab: Pointer to the NonAdminBackup object. // // The function validates the Spec from the NonAdminBackup object. // If the BackupSpec is invalid, the function sets the NonAdminBackup phase to "BackingOff". // If the BackupSpec is invalid, the function sets the NonAdminBackup condition Accepted to "False". // If the BackupSpec is valid, the function sets the NonAdminBackup condition Accepted to "True". -func (r *NonAdminBackupReconciler) ValidateSpec(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { - logger := logrLogger - - // Main Validation point for the VeleroBackup included in NonAdminBackup spec - _, err := function.GetBackupSpecFromNonAdminBackup(nab) +func (r *NonAdminBackupReconciler) validateSpec(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { + err := function.ValidateBackupSpec(nab) if err != nil { updatedPhase := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseBackingOff) updatedCondition := meta.SetStatusCondition(&nab.Status.Conditions, @@ -172,12 +155,12 @@ func (r *NonAdminBackupReconciler) ValidateSpec(ctx context.Context, logrLogger if updatedPhase || updatedCondition { if updateErr := r.Status().Update(ctx, nab); updateErr != nil { logger.Error(updateErr, statusUpdateError) - return true, false, updateErr + return false, updateErr } } logger.Error(err, "NonAdminBackup Spec is not valid") - return true, false, reconcile.TerminalError(err) + return false, reconcile.TerminalError(err) } updated := meta.SetStatusCondition(&nab.Status.Conditions, @@ -191,18 +174,18 @@ func (r *NonAdminBackupReconciler) ValidateSpec(ctx context.Context, logrLogger if updated { if err := r.Status().Update(ctx, nab); err != nil { logger.Error(err, statusUpdateError) - return true, false, err + return false, err } logger.V(1).Info(conditionUpdateRequeue) - return false, true, nil + return true, nil } logger.V(1).Info("NonAdminBackup Spec already validated") - return false, false, nil + return false, nil } -// SyncVeleroBackupWithNonAdminBackup ensures the VeleroBackup associated with the given NonAdminBackup resource +// syncVeleroBackupWithNonAdminBackup 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. @@ -210,14 +193,12 @@ func (r *NonAdminBackupReconciler) ValidateSpec(ctx context.Context, logrLogger // Parameters: // // ctx: Context for the request. -// logrLogger: Logger instance for logging messages. +// logger: Logger instance for logging messages. // nab: Pointer to the NonAdminBackup object. -func (r *NonAdminBackupReconciler) SyncVeleroBackupWithNonAdminBackup(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) { //nolint:unparam // will address in https://github.com/migtools/oadp-non-admin/issues/62 - logger := logrLogger - +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 true, false, errors.New("unable to generate Velero Backup name") + return false, errors.New("unable to generate Velero Backup name") } veleroBackup := velerov1.Backup{} @@ -226,42 +207,28 @@ func (r *NonAdminBackupReconciler) SyncVeleroBackupWithNonAdminBackup(ctx contex if err != nil { if !apierrors.IsNotFound(err) { veleroBackupLogger.Error(err, "Unable to fetch VeleroBackup") - return true, false, err + return false, err } // Create VeleroBackup veleroBackupLogger.Info("VeleroBackup not found") - // We don't validate error here. - // This was already validated in the ValidateVeleroBackupSpec - backupSpec, errBackup := function.GetBackupSpecFromNonAdminBackup(nab) - if errBackup != nil { - // Should never happen as it was already checked - return true, false, errBackup - } + backupSpec := nab.Spec.BackupSpec.DeepCopy() + backupSpec.IncludedNamespaces = []string{nab.Namespace} veleroBackup = velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ - Name: veleroBackupName, - Namespace: r.OADPNamespace, + Name: veleroBackupName, + Namespace: r.OADPNamespace, + Labels: function.AddNonAdminLabels(), + Annotations: function.AddNonAdminBackupAnnotations(nab.ObjectMeta), }, Spec: *backupSpec, } - // Ensure labels are set for the Backup object - existingLabels := veleroBackup.Labels - naManagedLabels := function.AddNonAdminLabels(existingLabels) - veleroBackup.Labels = naManagedLabels - - // Ensure annotations are set for the Backup object - existingAnnotations := veleroBackup.Annotations - ownerUUID := string(nab.ObjectMeta.UID) - nabManagedAnnotations := function.AddNonAdminBackupAnnotations(nab.Namespace, nab.Name, ownerUUID, existingAnnotations) - veleroBackup.Annotations = nabManagedAnnotations - err = r.Create(ctx, &veleroBackup) if err != nil { veleroBackupLogger.Error(err, "Failed to create VeleroBackup") - return true, false, err + return false, err } veleroBackupLogger.Info("VeleroBackup successfully created") } @@ -279,11 +246,11 @@ func (r *NonAdminBackupReconciler) SyncVeleroBackupWithNonAdminBackup(ctx contex if updatedPhase || updatedCondition || updatedReference { if err := r.Status().Update(ctx, nab); err != nil { logger.Error(err, statusUpdateError) - return true, false, err + return false, err } logger.V(1).Info("NonAdminBackup - Exit after Status Update") - return false, false, nil + return false, nil } // Ensure that the NonAdminBackup's NonAdminBackupStatus is in sync @@ -294,13 +261,13 @@ func (r *NonAdminBackupReconciler) SyncVeleroBackupWithNonAdminBackup(ctx contex if updated { if err := r.Status().Update(ctx, nab); err != nil { veleroBackupLogger.Error(err, "Failed to update NonAdminBackup Status after VeleroBackup reconciliation") - return true, false, err + return false, err } logger.V(1).Info("NonAdminBackup Status updated successfully") } - return false, false, nil + return false, nil } // SetupWithManager sets up the controller with the Manager. diff --git a/internal/controller/nonadminbackup_controller_test.go b/internal/controller/nonadminbackup_controller_test.go index fe06d6b..1a81f7f 100644 --- a/internal/controller/nonadminbackup_controller_test.go +++ b/internal/controller/nonadminbackup_controller_test.go @@ -213,8 +213,8 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func ObjectMeta: metav1.ObjectMeta{ Name: function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName), Namespace: oadpNamespaceName, - Labels: function.AddNonAdminLabels(nil), - Annotations: function.AddNonAdminBackupAnnotations(nonAdminNamespaceName, testNonAdminBackupName, "", nil), + Labels: function.AddNonAdminLabels(), + Annotations: function.AddNonAdminBackupAnnotations(nonAdminBackup.ObjectMeta), }, Spec: velerov1.BackupSpec{ IncludedNamespaces: []string{nonAdminNamespaceName},