Skip to content

Commit

Permalink
fixup! fix: NAC install validation
Browse files Browse the repository at this point in the history 
validate first

Signed-off-by: Mateus Oliveira <[email protected]>
  • Loading branch information
@mateusoliveira43
mateusoliveira43 committed Nov 13, 2024
1 parent 80cb353 commit 6a598cc
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 91 deletions.
16 changes: 0 additions & 16 deletions controllers/nonadmin_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down
77 changes: 8 additions & 69 deletions controllers/nonadmin_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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{
Expand All @@ -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())

Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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,
}),
)
})

Expand Down
12 changes: 12 additions & 0 deletions controllers/validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
82 changes: 76 additions & 6 deletions controllers/validator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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{
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit 6a598cc

Please sign in to comment.