fix: NonAdminBackup cluster validation (#120)

Signed-off-by: Mateus Oliveira <[email protected]>
migtools · Dec 6, 2024 · 973af37 · 973af37
1 parent ca7e290
commit 973af37
Show file tree

Hide file tree

Showing 6 changed files with 66 additions and 32 deletions.
diff --git a/api/v1alpha1/nonadminbackup_types.go b/api/v1alpha1/nonadminbackup_types.go
@@ -39,12 +39,7 @@ const (
 // NonAdminBackupSpec defines the desired state of NonAdminBackup
 type NonAdminBackupSpec struct {
 	// BackupSpec defines the specification for a Velero backup.
-	BackupSpec *velerov1.BackupSpec `json:"backupSpec,omitempty"`
-
-	// NonAdminBackup log level (use debug for the most logging, leave unset for default)
-	// +optional
-	// +kubebuilder:validation:Enum=trace;debug;info;warning;error;fatal;panic
-	LogLevel string `json:"logLevel,omitempty"`
+	BackupSpec *velerov1.BackupSpec `json:"backupSpec"`
 
 	// DeleteBackup removes the NonAdminBackup and its associated VeleroBackup from the cluster,
 	// as well as the corresponding object storage

diff --git a/config/crd/bases/oadp.openshift.io_nonadminbackups.yaml b/config/crd/bases/oadp.openshift.io_nonadminbackups.yaml
@@ -525,18 +525,8 @@ spec:
                   ForceDeleteBackup removes the NonAdminBackup and its associated VeleroBackup from the cluster,
                   regardless of whether deletion from object storage succeeds or fails
                 type: boolean
-              logLevel:
-                description: NonAdminBackup log level (use debug for the most logging,
-                  leave unset for default)
-                enum:
-                - trace
-                - debug
-                - info
-                - warning
-                - error
-                - fatal
-                - panic
-                type: string
+            required:
+            - backupSpec
             type: object
           status:
             description: NonAdminBackupStatus defines the observed state of NonAdminBackup

diff --git a/internal/common/function/function.go b/internal/common/function/function.go
@@ -80,11 +80,6 @@ func containsOnlyNamespace(namespaces []string, namespace string) bool {
 
 // ValidateBackupSpec return nil, if NonAdminBackup is valid; error otherwise
 func ValidateBackupSpec(nonAdminBackup *nacv1alpha1.NonAdminBackup, enforcedBackupSpec *velerov1.BackupSpec) error {
-	// this should be Kubernetes API validation
-	if nonAdminBackup.Spec.BackupSpec == nil {
-		return fmt.Errorf("BackupSpec is not defined")
-	}
-
 	if nonAdminBackup.Spec.BackupSpec.IncludedNamespaces != nil {
 		if !containsOnlyNamespace(nonAdminBackup.Spec.BackupSpec.IncludedNamespaces, nonAdminBackup.Namespace) {
 			return fmt.Errorf("NonAdminBackup spec.backupSpec.includedNamespaces can not contain namespaces other than: %s", nonAdminBackup.Namespace)

diff --git a/internal/common/function/function_test.go b/internal/common/function/function_test.go
@@ -91,11 +91,6 @@ func TestValidateBackupSpec(t *testing.T) {
 		name       string
 		errMessage string
 	}{
-		{
-			name:       "nil spec",
-			spec:       nil,
-			errMessage: "BackupSpec is not defined",
-		},
 		{
 			name: "namespace different than NonAdminBackup namespace",
 			spec: &velerov1.BackupSpec{

diff --git a/internal/controller/nonadminbackup_controller_test.go b/internal/controller/nonadminbackup_controller_test.go
@@ -41,6 +41,10 @@ import (
 	"github.com/migtools/oadp-non-admin/internal/common/function"
 )
 
+type nonAdminBackupClusterValidationScenario struct {
+	spec nacv1alpha1.NonAdminBackupSpec
+}
+
 type nonAdminBackupSingleReconcileScenario struct {
 	resultError                   error
 	nonAdminBackupPriorStatus     *nacv1alpha1.NonAdminBackupStatus
@@ -160,6 +164,50 @@ func deleteTestNamespaces(ctx context.Context, nonAdminNamespaceName string, oad
 	return k8sClient.Delete(ctx, nonAdminNamespace)
 }
 
+var _ = ginkgo.Describe("Test NonAdminBackup in cluster validation", func() {
+	var (
+		ctx                     context.Context
+		nonAdminBackupName      string
+		nonAdminBackupNamespace string
+		counter                 int
+	)
+
+	ginkgo.BeforeEach(func() {
+		ctx = context.Background()
+		counter++
+		nonAdminBackupName = fmt.Sprintf("non-admin-backup-object-%v", counter)
+		nonAdminBackupNamespace = fmt.Sprintf("test-non-admin-backup-cluster-validation-%v", counter)
+
+		nonAdminNamespace := &corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: nonAdminBackupNamespace,
+			},
+		}
+		gomega.Expect(k8sClient.Create(ctx, nonAdminNamespace)).To(gomega.Succeed())
+	})
+
+	ginkgo.AfterEach(func() {
+		nonAdminNamespace := &corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: nonAdminBackupNamespace,
+			},
+		}
+		gomega.Expect(k8sClient.Delete(ctx, nonAdminNamespace)).To(gomega.Succeed())
+	})
+
+	ginkgo.DescribeTable("Validation is false",
+		func(scenario nonAdminBackupClusterValidationScenario) {
+			nonAdminBackup := buildTestNonAdminBackup(nonAdminBackupNamespace, nonAdminBackupName, scenario.spec)
+			err := k8sClient.Create(ctx, nonAdminBackup)
+			gomega.Expect(err).To(gomega.HaveOccurred())
+			gomega.Expect(err.Error()).To(gomega.ContainSubstring("Required value"))
+		},
+		ginkgo.Entry("Should NOT create NonAdminBackup without spec.backupSpec", nonAdminBackupClusterValidationScenario{
+			spec: nacv1alpha1.NonAdminBackupSpec{},
+		}),
+	)
+})
+
 var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile function", func() {
 	var (
 		ctx                     = context.Background()
@@ -322,22 +370,30 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
 			// gomega.Expect(err).To(gomega.Not(gomega.HaveOccurred()))
 			// gomega.Expect(currentResourceVersion - priorResourceVersion).To(gomega.Equal(1))
 		},
-		ginkgo.Entry("When triggered by NonAdminBackup Create event without BackupSpec, should update NonAdminBackup phase to BackingOff and exit with terminal error", nonAdminBackupSingleReconcileScenario{
+		ginkgo.Entry("When triggered by NonAdminBackup Create event with invalid BackupSpec, should update NonAdminBackup phase to BackingOff and exit with terminal error", nonAdminBackupSingleReconcileScenario{
+			nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{
+				BackupSpec: &velerov1.BackupSpec{
+					IncludedNamespaces: []string{"wrong", "wrong-again"},
+				},
+			},
 			nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{
 				Phase: nacv1alpha1.NonAdminPhaseBackingOff,
 				Conditions: []metav1.Condition{
 					{
 						Type:    string(constant.NonAdminConditionAccepted),
 						Status:  metav1.ConditionFalse,
 						Reason:  "InvalidBackupSpec",
-						Message: "BackupSpec is not defined",
+						Message: "NonAdminBackup spec.backupSpec.includedNamespaces can not contain namespaces other than: ",
 					},
 				},
 			},
-			resultError: reconcile.TerminalError(fmt.Errorf("BackupSpec is not defined")),
+			resultError: reconcile.TerminalError(fmt.Errorf("NonAdminBackup spec.backupSpec.includedNamespaces can not contain namespaces other than: ")),
 		}),
 		ginkgo.Entry("When triggered by NonAdminBackup deleteNonAdmin spec field when BackupSpec is invalid, should delete NonAdminBackup without error", nonAdminBackupSingleReconcileScenario{
 			nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{
+				BackupSpec: &velerov1.BackupSpec{
+					IncludedNamespaces: []string{"wrong", "wrong-again"},
+				},
 				DeleteBackup: true,
 			},
 			nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{
@@ -358,6 +414,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
 		ginkgo.Entry("When triggered by NonAdminBackup deleteNonAdmin spec field with Finalizer set, should not delete NonAdminBackup as it's waiting for finalizer to be removed", nonAdminBackupSingleReconcileScenario{
 			addFinalizer: true,
 			nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{
+				BackupSpec:   &velerov1.BackupSpec{},
 				DeleteBackup: true,
 			},
 			nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{
@@ -467,6 +524,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
 		}),
 		ginkgo.Entry("When triggered by NonAdminBackup deleteNonAdmin spec field with Finalizer unset, should delete NonAdminBackup", nonAdminBackupSingleReconcileScenario{
 			nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{
+				BackupSpec:   &velerov1.BackupSpec{},
 				DeleteBackup: true,
 			},
 			nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{
@@ -495,6 +553,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
 			addFinalizer:            true,
 			addNabDeletionTimestamp: false,
 			nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{
+				BackupSpec:        &velerov1.BackupSpec{},
 				ForceDeleteBackup: true,
 			},
 			nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{

diff --git a/internal/controller/nonadminrestore_controller_test.go b/internal/controller/nonadminrestore_controller_test.go
@@ -178,7 +178,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminRestore Controller"
 
 			gomega.Expect(createTestNamespaces(ctx, nonAdminRestoreNamespace, oadpNamespace)).To(gomega.Succeed())
 
-			nonAdminBackup := buildTestNonAdminBackup(nonAdminRestoreNamespace, scenario.spec.RestoreSpec.BackupName, nacv1alpha1.NonAdminBackupSpec{})
+			nonAdminBackup := buildTestNonAdminBackup(nonAdminRestoreNamespace, scenario.spec.RestoreSpec.BackupName, nacv1alpha1.NonAdminBackupSpec{BackupSpec: &velerov1.BackupSpec{}})
 			gomega.Expect(k8sClient.Create(ctx, nonAdminBackup)).To(gomega.Succeed())
 			nonAdminBackup.Status = scenario.backupStatus
 			gomega.Expect(k8sClient.Status().Update(ctx, nonAdminBackup)).To(gomega.Succeed())