fix: Update NAB status phase and condition together

internal/common/constant/constant.go

@@ -23,6 +23,7 @@ package constant
 // of the specific Object, such as Backup/Restore.
 const (
 	OadpLabel                    = "openshift.io/oadp" // TODO import?
+	OadpLabelValue               = TrueString
 	ManagedByLabel               = "app.kubernetes.io/managed-by"
 	ManagedByLabelValue          = "oadp-nac-controller" // TODO why not use same project name as in PROJECT file?
 	NabOriginNameAnnotation      = "openshift.io/oadp-nab-origin-name"
@@ -38,8 +39,8 @@ const (
 // EmptyString defines a constant for the empty string
 const EmptyString = ""
 
-// MaxKubernetesNameLength represents maximum length of the name in k8s
-const MaxKubernetesNameLength = 253
+// TrueString defines a constant for the True string
+const TrueString = "True"
 
 // VeleroBackupNamePrefix represents the prefix for the object name generated
 // by the NonAdminController

internal/common/function/function.go

@@ -23,8 +23,10 @@ import (
 	"encoding/hex"
 	"fmt"
 
+	"github.com/go-logr/logr"
 	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
@@ -38,7 +40,7 @@ const requiredAnnotationError = "backup does not have the required annotation '%
 // If error occurs, a map with only the default Non Admin labels is returned
 func AddNonAdminLabels(labels map[string]string) map[string]string {
 	defaultLabels := map[string]string{
-		constant.OadpLabel:      "True",
+		constant.OadpLabel:      constant.OadpLabelValue,
 		constant.ManagedByLabel: constant.ManagedByLabelValue,
 	}
 
@@ -125,9 +127,9 @@ func GenerateVeleroBackupName(namespace, nabName string) string {
 	veleroBackupName := fmt.Sprintf("%s-%s-%s", constant.VeleroBackupNamePrefix, namespace, nameHash)
 
 	// Ensure the name is within the character limit
-	if len(veleroBackupName) > constant.MaxKubernetesNameLength {
+	if len(veleroBackupName) > validation.DNS1123SubdomainMaxLength {
 		// Truncate the namespace if necessary
-		maxNamespaceLength := constant.MaxKubernetesNameLength - len(nameHash) - prefixLength
+		maxNamespaceLength := validation.DNS1123SubdomainMaxLength - len(nameHash) - prefixLength
 		if len(namespace) > maxNamespaceLength {
 			namespace = namespace[:maxNamespaceLength]
 		}
@@ -137,11 +139,46 @@ func GenerateVeleroBackupName(namespace, nabName string) string {
 	return veleroBackupName
 }
 
-// CheckVeleroBackupLabels return true if Velero Backup object has required Non Admin labels, false otherwise
-func CheckVeleroBackupLabels(labels map[string]string) bool {
-	// TODO also need to check for constant.OadpLabel label?
-	value, exists := labels[constant.ManagedByLabel]
-	return exists && value == constant.ManagedByLabelValue
+// 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) {
+		return false
+	}
+	if !checkLabelValue(labels, constant.ManagedByLabel, constant.ManagedByLabelValue) {
+		return false
+	}
+
+	annotations := obj.GetAnnotations()
+	if !checkAnnotationValueIsValid(annotations, constant.NabOriginNamespaceAnnotation) {
+		return false
+	}
+	if !checkAnnotationValueIsValid(annotations, constant.NabOriginNameAnnotation) {
+		return false
+	}
+	// TODO what is a valid uuid?
+	if !checkAnnotationValueIsValid(annotations, constant.NabOriginUUIDAnnotation) {
+		return false
+	}
+
+	return true
+}
+
+func checkLabelValue(labels map[string]string, key string, value string) bool {
+	got, exists := labels[key]
+	if !exists {
+		return false
+	}
+	return got == value
+}
+
+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
 }
 
 // TODO not used
@@ -217,3 +254,9 @@ func mergeMaps[T comparable](maps ...map[T]T) (map[T]T, error) {
 	}
 	return merge, nil
 }
+
+// GetLogger return a logger from input ctx, with additional key/value pairs being
+// input key and input obj name and namespace
+func GetLogger(ctx context.Context, obj client.Object, key string) logr.Logger {
+	return log.FromContext(ctx).WithValues(key, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()})
+}

internal/common/function/function_test.go

@@ -20,13 +20,15 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"strings"
 	"testing"
 
 	"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/types"
+	"k8s.io/apimachinery/pkg/util/validation"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -38,6 +40,12 @@ import (
 
 var _ = ginkgo.Describe("PLACEHOLDER", func() {})
 
+const (
+	testNonAdminBackupNamespace = "non-admin-backup-namespace"
+	testNonAdminBackupName      = "non-admin-backup-name"
+	testNonAdminBackupUUID      = "12345678-1234-1234-1234-123456789abc"
+)
+
 func TestMergeMaps(t *testing.T) {
 	const (
 		d     = "d"
@@ -294,18 +302,18 @@ func TestGetNonAdminBackupFromVeleroBackup(t *testing.T) {
 			Namespace: "test-namespace",
 			Name:      "test-backup",
 			Annotations: map[string]string{
-				constant.NabOriginNamespaceAnnotation: "non-admin-backup-namespace",
-				constant.NabOriginNameAnnotation:      "non-admin-backup-name",
-				constant.NabOriginUUIDAnnotation:      "12345678-1234-1234-1234-123456789abc",
+				constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace,
+				constant.NabOriginNameAnnotation:      testNonAdminBackupName,
+				constant.NabOriginUUIDAnnotation:      testNonAdminBackupUUID,
 			},
 		},
 	}
 
 	nonAdminBackup := &nacv1alpha1.NonAdminBackup{
 		ObjectMeta: metav1.ObjectMeta{
-			Namespace: "non-admin-backup-namespace",
-			Name:      "non-admin-backup-name",
-			UID:       types.UID("12345678-1234-1234-1234-123456789abc"),
+			Namespace: testNonAdminBackupNamespace,
+			Name:      testNonAdminBackupName,
+			UID:       types.UID(testNonAdminBackupUUID),
 		},
 	}
 
@@ -317,32 +325,110 @@ func TestGetNonAdminBackupFromVeleroBackup(t *testing.T) {
 	assert.Equal(t, nonAdminBackup, result, "Returned NonAdminBackup should match expected NonAdminBackup")
 }
 
-func TestCheckVeleroBackupLabels(t *testing.T) {
-	// Backup has the required label
-	backupWithLabel := &velerov1.Backup{
-		ObjectMeta: metav1.ObjectMeta{
-			Labels: map[string]string{
-				constant.ManagedByLabel: constant.ManagedByLabelValue,
+func TestCheckVeleroBackupMetadata(t *testing.T) {
+	tests := []struct {
+		backup   *velerov1.Backup
+		name     string
+		expected bool
+	}{
+		{
+			name:     "Velero Backup without required non admin labels and annotations",
+			backup:   &velerov1.Backup{},
+			expected: false,
+		},
+		{
+			name: "Velero Backup without required non admin annotations",
+			backup: &velerov1.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						constant.OadpLabel:      constant.OadpLabelValue,
+						constant.ManagedByLabel: constant.ManagedByLabelValue,
+					},
+				},
 			},
+			expected: false,
 		},
-	}
-	assert.True(t, CheckVeleroBackupLabels(backupWithLabel.GetLabels()), "Expected backup to have required label")
-
-	// Backup does not have the required label
-	backupWithoutLabel := &velerov1.Backup{
-		ObjectMeta: metav1.ObjectMeta{
-			Labels: map[string]string{},
+		{
+			name: "Velero Backup with wrong required non admin label",
+			backup: &velerov1.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						constant.OadpLabel:      constant.OadpLabelValue,
+						constant.ManagedByLabel: "foo",
+					},
+				},
+			},
+			expected: false,
 		},
-	}
-	assert.False(t, CheckVeleroBackupLabels(backupWithoutLabel.GetLabels()), "Expected backup to not have required label")
-
-	// Backup has the required label with incorrect value
-	backupWithIncorrectValue := &velerov1.Backup{
-		ObjectMeta: metav1.ObjectMeta{
-			Labels: map[string]string{
-				constant.ManagedByLabel: "incorrect-value",
+		{
+			name: "Velero Backup without required non admin labels",
+			backup: &velerov1.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace,
+						constant.NabOriginNameAnnotation:      testNonAdminBackupName,
+						constant.NabOriginUUIDAnnotation:      testNonAdminBackupUUID,
+					},
+				},
+			},
+			expected: false,
+		},
+		{
+			name: "Velero Backup with wrong required non admin annotation [empty]",
+			backup: &velerov1.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						constant.OadpLabel:      constant.OadpLabelValue,
+						constant.ManagedByLabel: constant.ManagedByLabelValue,
+					},
+					Annotations: map[string]string{
+						constant.NabOriginNamespaceAnnotation: constant.EmptyString,
+						constant.NabOriginNameAnnotation:      testNonAdminBackupName,
+						constant.NabOriginUUIDAnnotation:      testNonAdminBackupUUID,
+					},
+				},
+			},
+			expected: false,
+		},
+		{
+			name: "Velero Backup with wrong required non admin annotation [long]",
+			backup: &velerov1.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						constant.OadpLabel:      constant.OadpLabelValue,
+						constant.ManagedByLabel: constant.ManagedByLabelValue,
+					},
+					Annotations: map[string]string{
+						constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace,
+						constant.NabOriginNameAnnotation:      strings.Repeat("nn", validation.DNS1123SubdomainMaxLength),
+						constant.NabOriginUUIDAnnotation:      testNonAdminBackupUUID,
+					},
+				},
 			},
+			expected: false,
 		},
+		{
+			name: "Velero Backup with required non admin labels and annotations",
+			backup: &velerov1.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						constant.OadpLabel:      constant.OadpLabelValue,
+						constant.ManagedByLabel: constant.ManagedByLabelValue,
+					},
+					Annotations: map[string]string{
+						constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace,
+						constant.NabOriginNameAnnotation:      testNonAdminBackupName,
+						constant.NabOriginUUIDAnnotation:      testNonAdminBackupUUID,
+					},
+				},
+			},
+			expected: true,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			result := CheckVeleroBackupMetadata(test.backup)
+			assert.Equal(t, test.expected, result)
+		})
 	}
-	assert.False(t, CheckVeleroBackupLabels(backupWithIncorrectValue.GetLabels()), "Expected backup to not have required label")
 }