Fix for migtools#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 <[email protected]>

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

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

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,24 +135,33 @@ 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["`
 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/]

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

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
 	}