From 8bc854d6bde5ab4154de739b57533ea11fb3b108 Mon Sep 17 00:00:00 2001 From: Mateus Oliveira Date: Wed, 13 Nov 2024 10:19:16 -0300 Subject: [PATCH] fixup! fix: NAC install validation validate first Signed-off-by: Mateus Oliveira --- controllers/nonadmin_controller.go | 16 ----- controllers/nonadmin_controller_test.go | 77 +++-------------------- controllers/validator.go | 12 ++++ controllers/validator_test.go | 82 +++++++++++++++++++++++-- 4 files changed, 96 insertions(+), 91 deletions(-) diff --git a/controllers/nonadmin_controller.go b/controllers/nonadmin_controller.go index f82a16c61f..d4c9449ca8 100644 --- a/controllers/nonadmin_controller.go +++ b/controllers/nonadmin_controller.go @@ -10,7 +10,6 @@ import ( corev1 "k8s.io/api/core/v1" k8serror "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -85,21 +84,6 @@ func (r *DPAReconciler) ReconcileNonAdminController(log logr.Logger) (bool, erro return true, nil } - selector, err := fields.ParseSelector(fmt.Sprintf("metadata.namespace!=%s", r.NamespacedName.Namespace)) - if err != nil { - return false, err - } - dpaList := &oadpv1alpha1.DataProtectionApplicationList{} - err = r.ClusterWideClient.List(r.Context, dpaList, &client.ListOptions{FieldSelector: selector}) - if err != nil { - return false, err - } - for _, dpa := range dpaList.Items { - if (&DPAReconciler{dpa: &dpa}).checkNonAdminEnabled() { - return false, fmt.Errorf("only a single instance of Non-Admin Controller can be installed across the entire cluster. Non-Admin controller is also configured to be installed in %s namespace", dpa.Namespace) - } - } - operation, err := controllerutil.CreateOrUpdate( r.Context, r.Client, diff --git a/controllers/nonadmin_controller_test.go b/controllers/nonadmin_controller_test.go index 72343d53a8..2f6da58936 100644 --- a/controllers/nonadmin_controller_test.go +++ b/controllers/nonadmin_controller_test.go @@ -21,14 +21,12 @@ import ( const defaultNonAdminImage = "quay.io/konveyor/oadp-non-admin:latest" type ReconcileNonAdminControllerScenario struct { - namespace string - dpa string - errMessage string - eventWords []string - nonAdminEnabled bool - deployment *appsv1.Deployment - anotherNamespace string - anotherNonAdminEnabled bool + namespace string + dpa string + errMessage string + eventWords []string + nonAdminEnabled bool + deployment *appsv1.Deployment } func createTestDeployment(namespace string) *appsv1.Deployment { @@ -101,31 +99,6 @@ func runReconcileNonAdminControllerTest( gomega.Expect(k8sClient.Create(ctx, scenario.deployment)).To(gomega.Succeed()) } - if len(scenario.anotherNamespace) > 0 { - namespace := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: scenario.anotherNamespace, - }, - } - gomega.Expect(k8sClient.Create(ctx, namespace)).To(gomega.Succeed()) - dpa := &oadpv1alpha1.DataProtectionApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: scenario.dpa, - Namespace: scenario.anotherNamespace, - }, - Spec: oadpv1alpha1.DataProtectionApplicationSpec{ - Configuration: &oadpv1alpha1.ApplicationConfig{}, - NonAdmin: &oadpv1alpha1.NonAdmin{ - Enable: ptr.To(scenario.anotherNonAdminEnabled), - }, - UnsupportedOverrides: map[oadpv1alpha1.UnsupportedImageKey]string{ - oadpv1alpha1.TechPreviewAck: "true", - }, - }, - } - gomega.Expect(k8sClient.Create(ctx, dpa)).To(gomega.Succeed()) - } - os.Setenv("RELATED_IMAGE_NON_ADMIN_CONTROLLER", envVarValue) event := record.NewFakeRecorder(5) r := &DPAReconciler{ @@ -136,9 +109,8 @@ func runReconcileNonAdminControllerTest( Name: scenario.dpa, Namespace: scenario.namespace, }, - EventRecorder: event, - dpa: dpa, - ClusterWideClient: k8sClient, + EventRecorder: event, + dpa: dpa, } result, err := r.ReconcileNonAdminController(logr.Discard()) @@ -200,23 +172,6 @@ var _ = ginkgo.Describe("Test ReconcileNonAdminController function", func() { }, } gomega.Expect(k8sClient.Delete(ctx, namespace)).To(gomega.Succeed()) - - if len(currentTestScenario.anotherNamespace) > 0 { - dpa := &oadpv1alpha1.DataProtectionApplication{ - ObjectMeta: metav1.ObjectMeta{ - Name: currentTestScenario.dpa, - Namespace: currentTestScenario.anotherNamespace, - }, - } - gomega.Expect(k8sClient.Delete(ctx, dpa)).To(gomega.Succeed()) - - namespace := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: currentTestScenario.anotherNamespace, - }, - } - gomega.Expect(k8sClient.Delete(ctx, namespace)).To(gomega.Succeed()) - } }) ginkgo.DescribeTable("Reconcile is true", @@ -248,14 +203,6 @@ var _ = ginkgo.Describe("Test ReconcileNonAdminController function", func() { dpa: "test-4-dpa", nonAdminEnabled: false, }), - ginkgo.Entry("Should create non admin deployment with DPAs in other namespaces", ReconcileNonAdminControllerScenario{ - namespace: "test-5", - dpa: "test-5-dpa", - eventWords: []string{"Normal", "NonAdminDeploymentReconciled", "created"}, - nonAdminEnabled: true, - anotherNamespace: "test-6", - anotherNonAdminEnabled: false, - }), ) ginkgo.DescribeTable("Reconcile is false", @@ -290,14 +237,6 @@ var _ = ginkgo.Describe("Test ReconcileNonAdminController function", func() { }, }, }), - ginkgo.Entry("Should error because non admin is enabled in another namespace", ReconcileNonAdminControllerScenario{ - namespace: "test-error-2", - dpa: "test-error-2-dpa", - errMessage: "only a single instance of Non-Admin Controller can be installed across the entire cluster. Non-Admin controller is also configured to be installed in test-error-3 namespace", - nonAdminEnabled: true, - anotherNamespace: "test-error-3", - anotherNonAdminEnabled: true, - }), ) }) diff --git a/controllers/validator.go b/controllers/validator.go index ac7ddb3f46..e4e34267b3 100644 --- a/controllers/validator.go +++ b/controllers/validator.go @@ -84,6 +84,18 @@ func (r *DPAReconciler) ValidateDataProtectionCR(log logr.Logger) (bool, error) if !(r.dpa.Spec.UnsupportedOverrides[oadpv1alpha1.TechPreviewAck] == TrueVal) { return false, errors.New("in order to enable/disable the non-admin feature please set dpa.spec.unsupportedOverrides[tech-preview-ack]: 'true'") } + + dpaList := &oadpv1alpha1.DataProtectionApplicationList{} + err = r.ClusterWideClient.List(r.Context, dpaList) + if err != nil { + return false, err + } + for _, dpa := range dpaList.Items { + if dpa.Namespace != r.NamespacedName.Namespace && (&DPAReconciler{dpa: &dpa}).checkNonAdminEnabled() { + return false, fmt.Errorf("only a single instance of Non-Admin Controller can be installed across the entire cluster. Non-Admin controller is also configured to be installed in %s namespace", dpa.Namespace) + } + } + } return true, nil diff --git a/controllers/validator_test.go b/controllers/validator_test.go index caceab5c88..b16d59a1df 100644 --- a/controllers/validator_test.go +++ b/controllers/validator_test.go @@ -1389,7 +1389,7 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { messageErr: "Secret name specified in BackupLocation cannot be empty", }, { - name: "given invalid DPA CR tech-preview-ack not set as true but non-admin is enabled error case", + name: "[invalid] DPA CR: spec.nonAdmin.enable true, spec.unsupportedOverrides.tech-preview-ack not true string", dpa: &oadpv1alpha1.DataProtectionApplication{ ObjectMeta: metav1.ObjectMeta{ Name: "test-DPA-CR", @@ -1413,10 +1413,79 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { BackupImages: pointer.Bool(false), }, }, - objects: []client.Object{}, wantErr: true, messageErr: "in order to enable/disable the non-admin feature please set dpa.spec.unsupportedOverrides[tech-preview-ack]: 'true'", }, + { + name: "[valid] DPA CR: spec.nonAdmin.enable true, spec.unsupportedOverrides.tech-preview-ack true string", + dpa: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-DPA-CR", + Namespace: "test-ns", + }, + Spec: oadpv1alpha1.DataProtectionApplicationSpec{ + NonAdmin: &oadpv1alpha1.NonAdmin{ + Enable: pointer.Bool(true), + }, + UnsupportedOverrides: map[oadpv1alpha1.UnsupportedImageKey]string{ + oadpv1alpha1.TechPreviewAck: "true", + }, + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + DefaultPlugins: []oadpv1alpha1.DefaultPlugin{ + oadpv1alpha1.DefaultPluginAWS, + }, + NoDefaultBackupLocation: true, + }, + }, + BackupImages: pointer.Bool(false), + }, + }, + }, + { + name: "[invalid] DPA CR: multiple NACs in cluster", + dpa: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-DPA-CR", + Namespace: "test-ns", + }, + Spec: oadpv1alpha1.DataProtectionApplicationSpec{ + NonAdmin: &oadpv1alpha1.NonAdmin{ + Enable: pointer.Bool(true), + }, + UnsupportedOverrides: map[oadpv1alpha1.UnsupportedImageKey]string{ + oadpv1alpha1.TechPreviewAck: "true", + }, + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + DefaultPlugins: []oadpv1alpha1.DefaultPlugin{ + oadpv1alpha1.DefaultPluginAWS, + }, + NoDefaultBackupLocation: true, + }, + }, + BackupImages: pointer.Bool(false), + }, + }, + objects: []client.Object{ + &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "another-DPA-CR", + Namespace: "test-another-ns", + }, + Spec: oadpv1alpha1.DataProtectionApplicationSpec{ + NonAdmin: &oadpv1alpha1.NonAdmin{ + Enable: pointer.Bool(true), + }, + UnsupportedOverrides: map[oadpv1alpha1.UnsupportedImageKey]string{ + oadpv1alpha1.TechPreviewAck: "true", + }, + }, + }, + }, + wantErr: true, + messageErr: "only a single instance of Non-Admin Controller can be installed across the entire cluster. Non-Admin controller is also configured to be installed in test-another-ns namespace", + }, { name: "given invalid DPA CR aws and legacy-aws plugins both specified", dpa: &oadpv1alpha1.DataProtectionApplication{ @@ -1449,10 +1518,11 @@ func TestDPAReconciler_ValidateDataProtectionCR(t *testing.T) { t.Errorf("error in creating fake client, likely programmer error") } r := &DPAReconciler{ - Client: fakeClient, - Scheme: fakeClient.Scheme(), - Log: logr.Discard(), - Context: newContextForTest(tt.name), + Client: fakeClient, + ClusterWideClient: fakeClient, + Scheme: fakeClient.Scheme(), + Log: logr.Discard(), + Context: newContextForTest(tt.name), NamespacedName: types.NamespacedName{ Namespace: tt.dpa.Namespace, Name: tt.dpa.Name,