From 2cbd08585b0fe3293d0f914d7031d2561007dee9 Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Mon, 14 Oct 2024 19:49:02 +0200 Subject: [PATCH 1/5] Fix for #89 - Use custom UUID for NAB Object With this change a new UUID is generated to reference parent/child relationship between objects in the Non Admin Controller use cases. The first consumer of this UUID is a Velero Backup, created when the NonAdminBackup object is reconciled. The NonAdminBackup object generates the NAC UUID and stores it in its Status. This prevents users from modifying it. The UUID is later used to create the Velero Backup during reconciliation. While the NAC UUID is currently used as the Velero Backup name, this is not required, as the UUID is also stored as a Velero Backup label, which is used during the reconcile loop. Usage of NAC UUID as Velero Backup name is to easy it's creation. This PR also includes small changes to fix linting issues of the code, as well reworks the tests to properly take advantage of gingko BeforeEach function. Signed-off-by: Michal Pryc --- api/v1alpha1/nonadminbackup_types.go | 4 +- ...nac.oadp.openshift.io_nonadminbackups.yaml | 5 +- docs/design/nab_and_nar_status_update.md | 25 +- internal/common/constant/constant.go | 14 +- internal/common/function/function.go | 125 ++++-- internal/common/function/function_test.go | 222 +++++++++-- .../controller/nonadminbackup_controller.go | 124 +++--- .../nonadminbackup_controller_test.go | 366 ++++++++++-------- 8 files changed, 594 insertions(+), 291 deletions(-) diff --git a/api/v1alpha1/nonadminbackup_types.go b/api/v1alpha1/nonadminbackup_types.go index 4e29310..3b25f9c 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. + // nabnacuuid references the Non Admin Backup object by it's nacUUID. // +optional - Name string `json:"name,omitempty"` + NabNacUUID string `json:"nabnacuuid,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..5b3fccd 100644 --- a/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml +++ b/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml @@ -612,8 +612,9 @@ spec: description: VeleroBackup contains information of the related Velero backup object. properties: - name: - description: name references the Velero backup by it's name. + nabnacuuid: + description: nabnacuuid references the Non Admin Backup object + by it's nacUUID. type: string namespace: description: namespace references the Namespace in which Velero diff --git a/docs/design/nab_and_nar_status_update.md b/docs/design/nab_and_nar_status_update.md index 4ea0927..4af9cf9 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 `nabnacuuid`, `namespace` and `status`. +- `status.veleroBackup.nabnacuuid` field stores generated unique UUID of the `VeleroBackup` object. The same UUID is also stored as the label value `openshift.io/oadp-nab-origin-uuid` 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 `nabnacuuid`, `namespace` and `status`. +- `status.veleroRestore.nabnacuuid` field stores generated unique UUID of the `VeleroRestore` object. The same UUID is also stored as the label value `openshift.io/oadp-nar-origin-uuid` 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 + nabnacuuid: nonadmin-test-86b8d92b-66b2-11e4-8a2d-42010af06f3f namespace: openshift-adp status: expiration: '2024-05-16T08:12:11Z' @@ -135,16 +135,25 @@ reconcileExit1[\Requeue: true, nil/] reconcileExit1 ==> question(is NonAdminBackup Spec valid?) == yes ==> reconcileStart2 question == no ==> reconcileStartInvalid1 + + reconcileStart2[/Reconcile start\] ==> conditionsAcceptedTrue["`status.conditions[Accepted] to **True**`"] -. Requeue: false, err .- reconcileStart2 + conditionsAcceptedTrue ==> reconcileExit2[\Requeue: true, nil/] ==> reconcileStart3[/Reconcile start\] ==> -createVeleroBackup([Create Velero Backup]) -. Requeue: false, err .- reconcileStart3 +setVeleroBackupUUID([Set status.veleroBackup.nabNacUUID]) -. Requeue: false, err .- reconcileStart3 + +setVeleroBackupUUID ==> + +reconcileStart4[/Reconcile start\] ==> + +createVeleroBackup([Create Velero Backup]) -. Requeue: false, err .- reconcileStart4 createVeleroBackup ==> phaseCreated["` @@ -152,7 +161,7 @@ status.phase: **Created** status.conditions[Queued] to **True** status.conditions.veleroBackup.name status.conditions.veleroBackup.namespace -`"] -. Requeue: false, err .- reconcileStart3 +`"] -. Requeue: false, err .- reconcileStart4 phaseCreated ==> reconcileExit4[\Requeue: false, nil/] diff --git a/internal/common/constant/constant.go b/internal/common/constant/constant.go index c7a41a8..0aa1a0f 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" + NabOriginUUIDLabel = "openshift.io/oadp-nab-origin-uuid" + NarOriginUUIDLabel = "openshift.io/oadp-nar-origin-uuid" ) // 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..e5e0deb 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) +// GetObjectByLabel retrieves a list of Kubernetes objects of a specified type based on a label within a given namespace. +// It returns a slice of the specified object type and an error. +func GetObjectByLabel(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 GetObjectByLabel function + if err := GetObjectByLabel(ctx, clientInstance, namespace, constant.NabOriginUUIDLabel, 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.NabOriginUUIDLabel, 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 !checkAnnotationOrLabelsValueIsValid(objLabels, constant.NabOriginUUIDLabel) { return false } - if !checkAnnotationValueIsValid(annotations, constant.NabOriginNameAnnotation) { + + annotations := obj.GetAnnotations() + if !checkAnnotationOrLabelsValueIsValid(annotations, constant.NabOriginNamespaceAnnotation) { return false } - // TODO what is a valid uuid? - if !checkAnnotationValueIsValid(annotations, constant.NabOriginUUIDAnnotation) { + if !checkAnnotationOrLabelsValueIsValid(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 checkAnnotationOrLabelsValueIsValid(annotationsOrLabels map[string]string, key string) bool { + value, exists := annotationsOrLabels[key] if !exists { return false } diff --git a/internal/common/function/function_test.go b/internal/common/function/function_test.go index 034b963..5f6209b 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 + }{ + { + 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", + }, + { + 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 _, 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 }{ { - namespace: "example-namespace", - name: "example-name", - expected: "nab-example-namespace-1cdadb729eaac1", + name: "Single VeleroBackup found", + namespace: defaultStr, + labelValue: testAppStr, + mockBackups: []velerov1.Backup{ + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: defaultStr, + Name: "backup1", + Labels: map[string]string{constant.NabOriginUUIDLabel: testAppStr}, + }, + }, + }, + expected: &velerov1.Backup{ObjectMeta: metav1.ObjectMeta{Namespace: defaultStr, Name: "backup1", Labels: map[string]string{constant.NabOriginUUIDLabel: 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.NabOriginUUIDLabel: testAppStr}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: defaultStr, + Name: "backup3", + Labels: map[string]string{constant.NabOriginUUIDLabel: testAppStr}, + }, + }, + }, + expected: nil, + expectedError: errors.New("multiple VeleroBackup objects found with label openshift.io/oadp-nab-origin-uuid=test-app in namespace 'default'"), }, { - namespace: strings.Repeat("m", validation.DNS1123SubdomainMaxLength), - name: "example-name", - expected: fmt.Sprintf("nab-%s-1cdadb729eaac1", strings.Repeat("m", truncatedStringSize)), + 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 _, 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) { + 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,13 +355,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.NabOriginUUIDLabel: testNonAdminBackupUUID, }, Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: constant.EmptyString, constant.NabOriginNameAnnotation: testNonAdminBackupName, - constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, }, }, }, @@ -218,13 +372,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.NabOriginUUIDLabel: testNonAdminBackupUUID, }, Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, constant.NabOriginNameAnnotation: strings.Repeat("nn", validation.DNS1123SubdomainMaxLength), - constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, }, }, }, @@ -235,13 +389,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.NabOriginUUIDLabel: 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..9ab594e 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.NabNacUUID == constant.EmptyString { + nabNacUUID := function.GenerateNacObjectNameWithUUID(nab.Namespace, nab.Name) + nab.Status.VeleroBackup = &nacv1alpha1.VeleroBackup{ + NabNacUUID: nabNacUUID, + 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.NabNacUUID == 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) + veleroBackupUUID := nab.Status.VeleroBackup.NabNacUUID + + veleroBackup, err := function.GetVeleroBackupByLabel(ctx, r.Client, r.OADPNamespace, veleroBackupUUID) + 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", veleroBackupUUID) + return false, err + } + + if veleroBackup == nil { + logger.Info("VeleroBackup with label not found, creating one", "UUID", veleroBackupUUID) backupSpec := nab.Spec.BackupSpec.DeepCopy() backupSpec.IncludedNamespaces = []string{nab.Namespace} - veleroBackup = velerov1.Backup{ + veleroBackup := velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ - Name: veleroBackupName, + Name: veleroBackupUUID, 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 veleroBackupUUID 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.NabOriginUUIDLabel] = veleroBackupUUID + err = r.Create(ctx, &veleroBackup) + if err != nil { - veleroBackupLogger.Error(err, "Failed to create VeleroBackup") + // We do not retry here as the veleroBackupUUID + // 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: veleroBackupUUID, 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..6e16198 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.NabNacUUID == "" { + return fmt.Errorf("NonAdminBackup Status VeleroBackupName %v is 0 length string", nonAdminBackup.Status.VeleroBackup.NabNacUUID) } - 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 + veleroBackupUUID 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" + veleroBackupUUID = 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{ + NabNacUUID: veleroBackupUUID, + 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.NabNacUUID, + Namespace: oadpNamespace, + Labels: map[string]string{ + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.NabOriginUUIDLabel: nonAdminBackupAfterCreate.Status.VeleroBackup.NabNacUUID, + }, 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.NabNacUUID).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 NabNacUUID 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.NabNacUUID) > 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.NabNacUUID, + 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{ From 97dacc63f1f009c9b6422b02165438614d482a29 Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Tue, 15 Oct 2024 15:48:20 +0200 Subject: [PATCH 2/5] Update function to validate NAC Object Labels Improved function to properly validate NAC Object Labels Dropped length of Annotation Value validation, as it's not limited to 63 or 256 chars. Signed-off-by: Michal Pryc --- internal/common/function/function.go | 21 +++++++++++++++------ internal/common/function/function_test.go | 4 ++-- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/internal/common/function/function.go b/internal/common/function/function.go index e5e0deb..e95b667 100644 --- a/internal/common/function/function.go +++ b/internal/common/function/function.go @@ -171,15 +171,15 @@ func CheckVeleroBackupMetadata(obj client.Object) bool { return false } - if !checkAnnotationOrLabelsValueIsValid(objLabels, constant.NabOriginUUIDLabel) { + if !checkLabelValueIsValid(objLabels, constant.NabOriginUUIDLabel) { return false } annotations := obj.GetAnnotations() - if !checkAnnotationOrLabelsValueIsValid(annotations, constant.NabOriginNamespaceAnnotation) { + if !checkAnnotationValueIsValid(annotations, constant.NabOriginNamespaceAnnotation) { return false } - if !checkAnnotationOrLabelsValueIsValid(annotations, constant.NabOriginNameAnnotation) { + if !checkAnnotationValueIsValid(annotations, constant.NabOriginNameAnnotation) { return false } @@ -194,13 +194,22 @@ func checkLabelValue(objLabels map[string]string, key string, value string) bool return got == value } -func checkAnnotationOrLabelsValueIsValid(annotationsOrLabels map[string]string, key string) bool { - value, exists := annotationsOrLabels[key] +func checkAnnotationValueIsValid(annotations map[string]string, key string) bool { + value, exists := annotations[key] if !exists { return false } length := len(value) - return length > 0 && length < validation.DNS1123SubdomainMaxLength + return length > 0 +} + +func checkLabelValueIsValid(objLabels map[string]string, key string) bool { + value, exists := objLabels[key] + if !exists { + return false + } + length := len(value) + return length > 0 && length < validation.DNS1123LabelMaxLength } // GetLogger return a logger from input ctx, with additional key/value pairs being diff --git a/internal/common/function/function_test.go b/internal/common/function/function_test.go index 5f6209b..bb1d0c5 100644 --- a/internal/common/function/function_test.go +++ b/internal/common/function/function_test.go @@ -368,12 +368,12 @@ func TestCheckVeleroBackupMetadata(t *testing.T) { 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.ManagedByLabel: strings.Repeat("ll", validation.DNS1123SubdomainMaxLength), constant.NabOriginUUIDLabel: testNonAdminBackupUUID, }, Annotations: map[string]string{ From ba85bd565369577e0c0ffee81e276aa473a758d3 Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Wed, 16 Oct 2024 21:09:54 +0200 Subject: [PATCH 3/5] Update custom UUID for NAB Object to reflect Code Review Changes to address code review. Signed-off-by: Michal Pryc --- api/v1alpha1/nonadminbackup_types.go | 4 +-- ...nac.oadp.openshift.io_nonadminbackups.yaml | 8 +++--- docs/design/Non_Admin_Controller_design.md | 8 +++--- docs/design/nab_and_nar_status_update.md | 12 ++++---- internal/common/constant/constant.go | 4 +-- internal/common/function/function.go | 14 +++++----- internal/common/function/function_test.go | 28 +++++++++---------- .../controller/nonadminbackup_controller.go | 28 +++++++++---------- .../nonadminbackup_controller_test.go | 28 +++++++++---------- 9 files changed, 67 insertions(+), 67 deletions(-) diff --git a/api/v1alpha1/nonadminbackup_types.go b/api/v1alpha1/nonadminbackup_types.go index 3b25f9c..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"` - // nabnacuuid references the Non Admin Backup object by it's nacUUID. + // nameuuid references the Velero Backup object by it's label containing same NameUUID. // +optional - NabNacUUID string `json:"nabnacuuid,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 5b3fccd..c1b4390 100644 --- a/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml +++ b/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml @@ -612,14 +612,14 @@ spec: description: VeleroBackup contains information of the related Velero backup object. properties: - nabnacuuid: - description: nabnacuuid references the Non Admin Backup object - by it's nacUUID. - 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 4af9cf9..5ca5234 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 `nabnacuuid`, `namespace` and `status`. -- `status.veleroBackup.nabnacuuid` field stores generated unique UUID of the `VeleroBackup` object. The same UUID is also stored as the label value `openshift.io/oadp-nab-origin-uuid` within the created `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,8 +76,8 @@ status: velero backup describe -n openshift-adp nab-nacproject-c3499c2729730a ``` -Similarly, NonAdminRestore `status.veleroRestore` contains `nabnacuuid`, `namespace` and `status`. -- `status.veleroRestore.nabnacuuid` field stores generated unique UUID of the `VeleroRestore` object. The same UUID is also stored as the label value `openshift.io/oadp-nar-origin-uuid` within the created `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 `VeleroRestore` object status. @@ -91,7 +91,7 @@ Object passed validation and Velero `Backup` object was created, but there was a ```yaml status: veleroBackup: - nabnacuuid: nonadmin-test-86b8d92b-66b2-11e4-8a2d-42010af06f3f + nameuuid: nonadmin-test-86b8d92b-66b2-11e4-8a2d-42010af06f3f namespace: openshift-adp status: expiration: '2024-05-16T08:12:11Z' @@ -147,7 +147,7 @@ reconcileExit2[\Requeue: true, nil/] ==> reconcileStart3[/Reconcile start\] ==> -setVeleroBackupUUID([Set status.veleroBackup.nabNacUUID]) -. Requeue: false, err .- reconcileStart3 +setVeleroBackupUUID([Set status.veleroBackup.nameUUID]) -. Requeue: false, err .- reconcileStart3 setVeleroBackupUUID ==> diff --git a/internal/common/constant/constant.go b/internal/common/constant/constant.go index 0aa1a0f..ed9dfc6 100644 --- a/internal/common/constant/constant.go +++ b/internal/common/constant/constant.go @@ -30,8 +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" - NabOriginUUIDLabel = "openshift.io/oadp-nab-origin-uuid" - NarOriginUUIDLabel = "openshift.io/oadp-nar-origin-uuid" + NabOriginNameUUIDLabel = "openshift.io/oadp-nab-origin-nameuuid" + NarOriginNameUUIDLabel = "openshift.io/oadp-nar-origin-nameuuid" ) // Common environment variables for the Non Admin Controller diff --git a/internal/common/function/function.go b/internal/common/function/function.go index e95b667..1f742ac 100644 --- a/internal/common/function/function.go +++ b/internal/common/function/function.go @@ -119,9 +119,9 @@ func GenerateNacObjectNameWithUUID(namespace, nacName string) string { return nacObjectName } -// GetObjectByLabel retrieves a list of Kubernetes objects of a specified type based on a label within a given namespace. -// It returns a slice of the specified object type and an error. -func GetObjectByLabel(ctx context.Context, clientInstance client.Client, namespace string, labelKey string, labelValue string, objectList client.ObjectList) error { +// ListLabeledObjectsInNamespace retrieves a list of Kubernetes objects in a specified namespace +// that match a given label key-value pair. +func ListLabeledObjectsInNamespace(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") @@ -146,8 +146,8 @@ func GetObjectByLabel(ctx context.Context, clientInstance client.Client, namespa func GetVeleroBackupByLabel(ctx context.Context, clientInstance client.Client, namespace string, labelValue string) (*velerov1.Backup, error) { veleroBackupList := &velerov1.BackupList{} - // Call the generic GetObjectByLabel function - if err := GetObjectByLabel(ctx, clientInstance, namespace, constant.NabOriginUUIDLabel, labelValue, veleroBackupList); err != nil { + // Call the generic ListLabeledObjectsInNamespace function + if err := ListLabeledObjectsInNamespace(ctx, clientInstance, namespace, constant.NabOriginNameUUIDLabel, labelValue, veleroBackupList); err != nil { return nil, err } @@ -157,7 +157,7 @@ func GetVeleroBackupByLabel(ctx context.Context, clientInstance client.Client, n 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.NabOriginUUIDLabel, labelValue, namespace) + return nil, fmt.Errorf("multiple VeleroBackup objects found with label %s=%s in namespace '%s'", constant.NabOriginNameUUIDLabel, labelValue, namespace) } } @@ -171,7 +171,7 @@ func CheckVeleroBackupMetadata(obj client.Object) bool { return false } - if !checkLabelValueIsValid(objLabels, constant.NabOriginUUIDLabel) { + if !checkLabelValueIsValid(objLabels, constant.NabOriginNameUUIDLabel) { return false } diff --git a/internal/common/function/function_test.go b/internal/common/function/function_test.go index bb1d0c5..3b90ad4 100644 --- a/internal/common/function/function_test.go +++ b/internal/common/function/function_test.go @@ -228,11 +228,11 @@ func TestGetVeleroBackupByLabel(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Namespace: defaultStr, Name: "backup1", - Labels: map[string]string{constant.NabOriginUUIDLabel: testAppStr}, + Labels: map[string]string{constant.NabOriginNameUUIDLabel: testAppStr}, }, }, }, - expected: &velerov1.Backup{ObjectMeta: metav1.ObjectMeta{Namespace: defaultStr, Name: "backup1", Labels: map[string]string{constant.NabOriginUUIDLabel: testAppStr}}}, + expected: &velerov1.Backup{ObjectMeta: metav1.ObjectMeta{Namespace: defaultStr, Name: "backup1", Labels: map[string]string{constant.NabOriginNameUUIDLabel: testAppStr}}}, expectedError: nil, }, { @@ -252,19 +252,19 @@ func TestGetVeleroBackupByLabel(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Namespace: defaultStr, Name: "backup2", - Labels: map[string]string{constant.NabOriginUUIDLabel: testAppStr}, + Labels: map[string]string{constant.NabOriginNameUUIDLabel: testAppStr}, }, }, { ObjectMeta: metav1.ObjectMeta{ Namespace: defaultStr, Name: "backup3", - Labels: map[string]string{constant.NabOriginUUIDLabel: testAppStr}, + Labels: map[string]string{constant.NabOriginNameUUIDLabel: testAppStr}, }, }, }, expected: nil, - expectedError: errors.New("multiple VeleroBackup objects found with label openshift.io/oadp-nab-origin-uuid=test-app in namespace 'default'"), + 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", @@ -355,9 +355,9 @@ func TestCheckVeleroBackupMetadata(t *testing.T) { backup: &velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - constant.OadpLabel: constant.OadpLabelValue, - constant.ManagedByLabel: constant.ManagedByLabelValue, - constant.NabOriginUUIDLabel: testNonAdminBackupUUID, + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.NabOriginNameUUIDLabel: testNonAdminBackupUUID, }, Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: constant.EmptyString, @@ -372,9 +372,9 @@ func TestCheckVeleroBackupMetadata(t *testing.T) { backup: &velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - constant.OadpLabel: constant.OadpLabelValue, - constant.ManagedByLabel: strings.Repeat("ll", validation.DNS1123SubdomainMaxLength), - constant.NabOriginUUIDLabel: testNonAdminBackupUUID, + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: strings.Repeat("ll", validation.DNS1123SubdomainMaxLength), + constant.NabOriginNameUUIDLabel: testNonAdminBackupUUID, }, Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, @@ -389,9 +389,9 @@ func TestCheckVeleroBackupMetadata(t *testing.T) { backup: &velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - constant.OadpLabel: constant.OadpLabelValue, - constant.ManagedByLabel: constant.ManagedByLabelValue, - constant.NabOriginUUIDLabel: testNonAdminBackupUUID, + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.NabOriginNameUUIDLabel: testNonAdminBackupUUID, }, Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index 9ab594e..104cc06 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -198,11 +198,11 @@ func (r *NonAdminBackupReconciler) validateSpec(ctx context.Context, logger logr // // 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.NabNacUUID == constant.EmptyString { - nabNacUUID := function.GenerateNacObjectNameWithUUID(nab.Namespace, nab.Name) + if nab.Status.VeleroBackup == nil || nab.Status.VeleroBackup.NameUUID == constant.EmptyString { + veleroBackupNameUUID := function.GenerateNacObjectNameWithUUID(nab.Namespace, nab.Name) nab.Status.VeleroBackup = &nacv1alpha1.VeleroBackup{ - NabNacUUID: nabNacUUID, - Namespace: r.OADPNamespace, + NameUUID: veleroBackupNameUUID, + Namespace: r.OADPNamespace, } if err := r.Status().Update(ctx, nab); err != nil { logger.Error(err, statusUpdateError) @@ -226,29 +226,29 @@ func (r *NonAdminBackupReconciler) setBackupUUIDInStatus(ctx context.Context, lo // logger: Logger instance for logging messages. // nab: Pointer to the NonAdminBackup object. func (r *NonAdminBackupReconciler) createVeleroBackupAndSyncWithNonAdminBackup(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { - if nab.Status.VeleroBackup == nil || nab.Status.VeleroBackup.NabNacUUID == constant.EmptyString { + if nab.Status.VeleroBackup == nil || nab.Status.VeleroBackup.NameUUID == constant.EmptyString { return false, errors.New("unable to get Velero Backup UUID from NonAdminBackup Status") } - veleroBackupUUID := nab.Status.VeleroBackup.NabNacUUID + veleroBackupNameUUID := nab.Status.VeleroBackup.NameUUID - veleroBackup, err := function.GetVeleroBackupByLabel(ctx, r.Client, r.OADPNamespace, veleroBackupUUID) + veleroBackup, err := function.GetVeleroBackupByLabel(ctx, r.Client, r.OADPNamespace, veleroBackupNameUUID) if err != nil { // Case in which more then one VeleroBackup is found with the same label UUID - logger.Error(err, "Failed to find single VeleroBackup object", "UUID", veleroBackupUUID) + 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", veleroBackupUUID) + logger.Info("VeleroBackup with label not found, creating one", "UUID", veleroBackupNameUUID) backupSpec := nab.Spec.BackupSpec.DeepCopy() backupSpec.IncludedNamespaces = []string{nab.Namespace} veleroBackup := velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ - Name: veleroBackupUUID, + Name: veleroBackupNameUUID, Namespace: r.OADPNamespace, Labels: function.GetNonAdminLabels(), Annotations: function.GetNonAdminBackupAnnotations(nab.ObjectMeta), @@ -256,15 +256,15 @@ func (r *NonAdminBackupReconciler) createVeleroBackupAndSyncWithNonAdminBackup(c Spec: *backupSpec, } - // Add NonAdminBackup's veleroBackupUUID as the label to the VeleroBackup object + // 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.NabOriginUUIDLabel] = veleroBackupUUID + veleroBackup.Labels[constant.NabOriginNameUUIDLabel] = veleroBackupNameUUID err = r.Create(ctx, &veleroBackup) if err != nil { - // We do not retry here as the veleroBackupUUID + // We do not retry here as the veleroBackupNameUUID // should be guaranteed to be unique logger.Error(err, "Failed to create VeleroBackup") return false, err @@ -272,7 +272,7 @@ func (r *NonAdminBackupReconciler) createVeleroBackupAndSyncWithNonAdminBackup(c logger.Info("VeleroBackup successfully created") } - veleroBackupLogger := logger.WithValues("VeleroBackup", types.NamespacedName{Name: veleroBackupUUID, Namespace: r.OADPNamespace}) + veleroBackupLogger := logger.WithValues("VeleroBackup", types.NamespacedName{Name: veleroBackupNameUUID, Namespace: r.OADPNamespace}) updatedPhase := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseCreated) diff --git a/internal/controller/nonadminbackup_controller_test.go b/internal/controller/nonadminbackup_controller_test.go index 6e16198..bd508b3 100644 --- a/internal/controller/nonadminbackup_controller_test.go +++ b/internal/controller/nonadminbackup_controller_test.go @@ -70,8 +70,8 @@ func checkTestNonAdminBackupStatus(nonAdminBackup *nacv1alpha1.NonAdminBackup, e } if nonAdminBackup.Status.VeleroBackup != nil { - if nonAdminBackup.Status.VeleroBackup.NabNacUUID == "" { - return fmt.Errorf("NonAdminBackup Status VeleroBackupName %v is 0 length string", nonAdminBackup.Status.VeleroBackup.NabNacUUID) + if nonAdminBackup.Status.VeleroBackup.NameUUID == "" { + return fmt.Errorf("NonAdminBackup Status VeleroBackupName %v is 0 length string", nonAdminBackup.Status.VeleroBackup.NameUUID) } if expectedStatus.VeleroBackup != nil { @@ -152,7 +152,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func nonAdminObjectName string nonAdminObjectNamespace string oadpNamespace string - veleroBackupUUID string + veleroBackupNameUUID string counter = 0 ) ginkgo.BeforeEach(func() { @@ -160,7 +160,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func nonAdminObjectName = fmt.Sprintf("nab-object-%v", counter) nonAdminObjectNamespace = fmt.Sprintf("test-nab-reconcile-%v", counter) oadpNamespace = nonAdminObjectNamespace + "-oadp" - veleroBackupUUID = function.GenerateNacObjectNameWithUUID(nonAdminObjectNamespace, nonAdminObjectName) + veleroBackupNameUUID = function.GenerateNacObjectNameWithUUID(nonAdminObjectNamespace, nonAdminObjectName) gomega.Expect(createTestNamespaces(ctx, nonAdminObjectNamespace, oadpNamespace)).To(gomega.Succeed()) }) ginkgo.AfterEach(func() { @@ -214,8 +214,8 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func if scenario.uuidFromTestCase { nonAdminBackupAfterCreate.Status.VeleroBackup = &nacv1alpha1.VeleroBackup{ - NabNacUUID: veleroBackupUUID, - Namespace: oadpNamespace, + NameUUID: veleroBackupNameUUID, + Namespace: oadpNamespace, } } gomega.Expect(k8sClient.Status().Update(ctx, nonAdminBackupAfterCreate)).To(gomega.Succeed()) @@ -223,12 +223,12 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func if scenario.createVeleroBackup { veleroBackup := &velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ - Name: nonAdminBackupAfterCreate.Status.VeleroBackup.NabNacUUID, + Name: nonAdminBackupAfterCreate.Status.VeleroBackup.NameUUID, Namespace: oadpNamespace, Labels: map[string]string{ - constant.OadpLabel: constant.OadpLabelValue, - constant.ManagedByLabel: constant.ManagedByLabelValue, - constant.NabOriginUUIDLabel: nonAdminBackupAfterCreate.Status.VeleroBackup.NabNacUUID, + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.NabOriginNameUUIDLabel: nonAdminBackupAfterCreate.Status.VeleroBackup.NameUUID, }, Annotations: function.GetNonAdminBackupAnnotations(nonAdminBackup.ObjectMeta), }, @@ -270,7 +270,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func )).To(gomega.Succeed()) gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackupAfterReconcile, scenario.nonAdminBackupExpectedStatus, oadpNamespace)).To(gomega.Succeed()) if scenario.uuidCreatedByReconcile { - gomega.Expect(nonAdminBackupAfterReconcile.Status.VeleroBackup.NabNacUUID).To(gomega.ContainSubstring(nonAdminObjectNamespace)) + 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 @@ -334,7 +334,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func uuidCreatedByReconcile: true, result: reconcile.Result{Requeue: true}, }), - ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new; Conditions Accepted True; NonAdminBackup Status NabNacUUID set), should update NonAdminBackup phase to created and Condition to Queued True and Exit", nonAdminBackupSingleReconcileScenario{ + 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{}, }, @@ -528,7 +528,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackup, scenario.status, oadpNamespace)).To(gomega.Succeed()) - if scenario.status.VeleroBackup != nil && len(nonAdminBackup.Status.VeleroBackup.NabNacUUID) > 0 { + 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, @@ -541,7 +541,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", gomega.Expect(k8sClient.Get( ctxTimeout, types.NamespacedName{ - Name: nonAdminBackup.Status.VeleroBackup.NabNacUUID, + Name: nonAdminBackup.Status.VeleroBackup.NameUUID, Namespace: oadpNamespace, }, veleroBackup, From 3cb8cdd2989a9f2f9c3d0baedceb96f21ce39f65 Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Thu, 17 Oct 2024 10:11:26 +0200 Subject: [PATCH 4/5] Rename function to ListObjectsByLabel Renamed function which lists objects by label in a specific namespace. Signed-off-by: Michal Pryc --- internal/common/function/function.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/common/function/function.go b/internal/common/function/function.go index 1f742ac..4ea8d2f 100644 --- a/internal/common/function/function.go +++ b/internal/common/function/function.go @@ -119,9 +119,9 @@ func GenerateNacObjectNameWithUUID(namespace, nacName string) string { return nacObjectName } -// ListLabeledObjectsInNamespace retrieves a list of Kubernetes objects in a specified namespace +// ListObjectsByLabel retrieves a list of Kubernetes objects in a specified namespace // that match a given label key-value pair. -func ListLabeledObjectsInNamespace(ctx context.Context, clientInstance client.Client, namespace string, labelKey string, labelValue string, objectList client.ObjectList) error { +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") @@ -147,7 +147,7 @@ func GetVeleroBackupByLabel(ctx context.Context, clientInstance client.Client, n veleroBackupList := &velerov1.BackupList{} // Call the generic ListLabeledObjectsInNamespace function - if err := ListLabeledObjectsInNamespace(ctx, clientInstance, namespace, constant.NabOriginNameUUIDLabel, labelValue, veleroBackupList); err != nil { + if err := ListObjectsByLabel(ctx, clientInstance, namespace, constant.NabOriginNameUUIDLabel, labelValue, veleroBackupList); err != nil { return nil, err } From e06454e34b5da3604c5ba607628b554834f75612 Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Mon, 21 Oct 2024 22:11:59 +0200 Subject: [PATCH 5/5] NAB reconcile fix to include design diagram and annotation value check Change to include: - Reworked diagram to include one reconcile entry and multiple states - Labels and annotations uses common function to check for the name and namespace value length - Additional test to cover above scenario. Signed-off-by: Michal Pryc --- docs/design/nab_and_nar_status_update.md | 119 ++++++++++++++-------- internal/common/function/function.go | 21 ++-- internal/common/function/function_test.go | 36 ++++++- 3 files changed, 115 insertions(+), 61 deletions(-) diff --git a/docs/design/nab_and_nar_status_update.md b/docs/design/nab_and_nar_status_update.md index 5ca5234..a7b0d7d 100644 --- a/docs/design/nab_and_nar_status_update.md +++ b/docs/design/nab_and_nar_status_update.md @@ -124,78 +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"/]; +reconcileStartAcceptedPredicate[/Reconcile start\]; +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 ?"}; -reconcileStart2[/Reconcile start\] ==> +createVBObject{{"Create New Velero Backup Object"}}; -conditionsAcceptedTrue["`status.conditions[Accepted] to **True**`"] -. Requeue: false, err .- reconcileStart2 -conditionsAcceptedTrue ==> +getUUIDMatchingVeleroBackup("Get Velero Backup with label openshift.io/oadp-nab-origin-nameuuid matching status.VeleroBackup.NameUUID"); -reconcileExit2[\Requeue: true, nil/] ==> +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"]; -reconcileStart3[/Reconcile start\] ==> +predicateCreateNabEvent --> reconcileStartAcceptedPredicate; +predicateUpdateNabEvent --> reconcileStartAcceptedPredicate; +predicateUpdateVBEvent --> reconcileStartAcceptedPredicate; -setVeleroBackupUUID([Set status.veleroBackup.nameUUID]) -. Requeue: false, err .- reconcileStart3 +reconcileStartAcceptedPredicate --> questionPhaseStatusSetToNew; +questionPhaseStatusSetToNew -- No --> statusPhaseSetToNew; +questionPhaseStatusSetToNew -- Yes --> questionIsNabValid; -setVeleroBackupUUID ==> +statusPhaseSetToNew --> questionSuccess; +questionSuccess -- No --> requeueFalseErr; +questionSuccess -- Yes --> requeueTrueNil; -reconcileStart4[/Reconcile start\] ==> +requeueTrueNil --> reconcileStartAcceptedPredicate; -createVeleroBackup([Create Velero Backup]) -. Requeue: false, err .- reconcileStart4 -createVeleroBackup ==> +questionIsNabValid -- No --> questionPhaseStatusSetToBackingOff; +questionIsNabValid -- Yes --> questionConditionAcceptedTrue; -phaseCreated["` -status.phase: **Created** -status.conditions[Queued] to **True** -status.conditions.veleroBackup.name -status.conditions.veleroBackup.namespace -`"] -. Requeue: false, err .- reconcileStart4 -phaseCreated ==> +questionConditionAcceptedTrue -- No --> statusConditionSetAcceptedToTrue; +questionConditionAcceptedTrue -- Yes --> questionStatusVeleroBackupUUID; -reconcileExit4[\Requeue: false, nil/] +questionStatusVeleroBackupUUID -- No --> statusSetVeleroBackupUUID; +questionStatusVeleroBackupUUID -- Yes --> getUUIDMatchingVeleroBackup; -reconcileStartInvalid1[/Reconcile start\] ==> +statusSetVeleroBackupUUID --> questionSuccess; -phaseBackingOff["` -status.phase to **BackingOff** -status.conditions[Accepted] to **False** -`"] -. Requeue: false, err .- reconcileStartInvalid1 -phaseBackingOff ==> +statusConditionSetAcceptedToTrue --> questionSuccess; -reconcileExitInvalid1[\Requeue: false, TerminalError/] -reconcileExit4 ~~~ event2 -reconcileExitInvalid1 ~~~ event2 +questionPhaseStatusSetToBackingOff -- No --> statusPhaseStatusSetToBackingOff; +questionPhaseStatusSetToBackingOff -- Yes --> requeueFalseTerminalErr; +statusPhaseStatusSetToBackingOff --> questionSuccessWithTerminalError; -event2{{predicate: Update event VeleroBackup}} == If event is accepted ==> -handler1{{handler: Handle VeleroBackup update event}} ==> reconcileStart5 +questionSuccessWithTerminalError -- No --> requeueFalseErr; +questionSuccessWithTerminalError -- Yes --> requeueFalseTerminalErr; -reconcileStart5[/Reconcile start\] ==> conditionsVeleroBackupStatus +getUUIDMatchingVeleroBackup --> questionSuccessGetVB; -conditionsVeleroBackupStatus["`status.conditions.veleroBackup.status`"] -. Requeue: false, err .- reconcileStart5 -conditionsVeleroBackupStatus ==> +questionSuccessGetVB -- No --> questionGetSingleVB; +questionSuccessGetVB -- Yes --> requeueFalseErr; -reconcileExit5[\Requeue: false, nil/] +questionGetSingleVB -- No --> createVBObject; +questionGetSingleVB -- Yes --> questionPhaseStatusSetToCreated; +createVBObject --> questionSuccessCreateVB; -reconcileExit5 ~~~ event3 +questionSuccessCreateVB -- No --> requeueFalseErr; +questionSuccessCreateVB -- Yes --> questionPhaseStatusSetToCreated; -event3{{predicate: Delete event NAB}} == If event is accepted ==> +questionPhaseStatusSetToCreated -- No --> statusPhaseStatusSetToCreated; +questionPhaseStatusSetToCreated -- Yes --> questionNabStatusUpdateFromVB; +statusPhaseStatusSetToCreated --> questionSuccessWithNoRequeue; -reconcileDelete1[/Reconcile start\] ==> +questionSuccessWithNoRequeue -- No --> requeueFalseErr; +questionSuccessWithNoRequeue -- Yes --> requeueFalseNil; -reconcileExitDelete1[\Requeue: false, nil/] -. Requeue: false, err .-> reconcileDelete1 +questionNabStatusUpdateFromVB -- No --> requeueFalseNil; + +questionNabStatusUpdateFromVB -- Yes --> statusNabStatusUpdateFromVB; + +statusNabStatusUpdateFromVB --> questionSuccessWithNoRequeue; -event4{{predicate: Update event NAB}} == If event is accepted ==> question ``` diff --git a/internal/common/function/function.go b/internal/common/function/function.go index 4ea8d2f..2823791 100644 --- a/internal/common/function/function.go +++ b/internal/common/function/function.go @@ -171,15 +171,15 @@ func CheckVeleroBackupMetadata(obj client.Object) bool { return false } - if !checkLabelValueIsValid(objLabels, constant.NabOriginNameUUIDLabel) { + if !checkLabelAnnotationValueIsValid(objLabels, constant.NabOriginNameUUIDLabel) { return false } annotations := obj.GetAnnotations() - if !checkAnnotationValueIsValid(annotations, constant.NabOriginNamespaceAnnotation) { + if !checkLabelAnnotationValueIsValid(annotations, constant.NabOriginNamespaceAnnotation) { return false } - if !checkAnnotationValueIsValid(annotations, constant.NabOriginNameAnnotation) { + if !checkLabelAnnotationValueIsValid(annotations, constant.NabOriginNameAnnotation) { return false } @@ -194,22 +194,13 @@ func checkLabelValue(objLabels map[string]string, key string, value string) bool 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 } length := len(value) - return length > 0 -} - -func checkLabelValueIsValid(objLabels map[string]string, key string) bool { - value, exists := objLabels[key] - if !exists { - return false - } - length := len(value) - return length > 0 && length < validation.DNS1123LabelMaxLength + return length > 0 && length < validation.DNS1123SubdomainMaxLength } // GetLogger return a logger from input ctx, with additional key/value pairs being diff --git a/internal/common/function/function_test.go b/internal/common/function/function_test.go index 3b90ad4..30c1c31 100644 --- a/internal/common/function/function_test.go +++ b/internal/common/function/function_test.go @@ -378,7 +378,41 @@ func TestCheckVeleroBackupMetadata(t *testing.T) { }, Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, - constant.NabOriginNameAnnotation: strings.Repeat("nn", validation.DNS1123SubdomainMaxLength), + 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("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, }, }, },