From 14b8ef33b4bba2171d797d78ef6994840034866b Mon Sep 17 00:00:00 2001 From: Varsha B Date: Tue, 24 Dec 2024 00:01:12 +0530 Subject: [PATCH 1/3] fix PSS label reset issue Signed-off-by: Varsha B --- controllers/gitopsservice_controller.go | 32 ++++++++++++++++---- controllers/gitopsservice_controller_test.go | 3 +- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/controllers/gitopsservice_controller.go b/controllers/gitopsservice_controller.go index e6822e2bb..53077cf9c 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -50,6 +50,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -106,6 +107,11 @@ func (r *ReconcileGitopsService) SetupWithManager(mgr ctrl.Manager) error { Owns(&appsv1.Deployment{}, builder.WithPredicates(pred)). Owns(&corev1.Service{}, builder.WithPredicates(pred)). Owns(&routev1.Route{}, builder.WithPredicates(pred)). + Watches(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{ + Name: "openshift-gitops", + }}, + handler.EnqueueRequestsFromMapFunc(namespaceMapper), + ). Complete(r) } @@ -219,7 +225,7 @@ func (r *ReconcileGitopsService) Reconcile(ctx context.Context, request reconcil // Create namespace if it doesn't already exist namespaceRef := newRestrictedNamespace(namespace) - err = r.Client.Get(ctx, types.NamespacedName{Name: namespace}, &corev1.Namespace{}) + err = r.Client.Get(ctx, types.NamespacedName{Name: namespace}, namespaceRef) if err != nil { if errors.IsNotFound(err) { reqLogger.Info("Creating a new Namespace", "Name", namespace) @@ -231,8 +237,8 @@ func (r *ReconcileGitopsService) Reconcile(ctx context.Context, request reconcil return reconcile.Result{}, err } } else { - needUpdate, updateNameSpace := ensurePodSecurityLabels(namespaceRef) - if needUpdate { + needsUpdate, updateNameSpace := ensurePodSecurityLabels(namespaceRef) + if needsUpdate { err = r.Client.Update(context.TODO(), updateNameSpace) if err != nil { return reconcile.Result{}, err @@ -345,7 +351,7 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli // 4.6 Cluster: Backend in openshift-pipelines-app-delivery namespace and argocd in openshift-gitops namespace // 4.7 Cluster: Both backend and argocd instance in openshift-gitops namespace argocdNS := newRestrictedNamespace(defaultArgoCDInstance.Namespace) - err = r.Client.Get(context.TODO(), types.NamespacedName{Name: argocdNS.Name}, &corev1.Namespace{}) + err = r.Client.Get(context.TODO(), types.NamespacedName{Name: argocdNS.Name}, argocdNS) if err != nil { if errors.IsNotFound(err) { reqLogger.Info("Creating a new Namespace", "Name", argocdNS.Name) @@ -378,8 +384,8 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli } } - needUpdate, updateNameSpace := ensurePodSecurityLabels(argocdNS) - if needUpdate { + needsUpdate, updateNameSpace := ensurePodSecurityLabels(argocdNS) + if needsUpdate { err = r.Client.Update(context.TODO(), updateNameSpace) if err != nil { return reconcile.Result{}, err @@ -959,3 +965,17 @@ func ensurePodSecurityLabels(namespace *corev1.Namespace) (bool, *corev1.Namespa } return changed, namespace } + +func namespaceMapper(ctx context.Context, o client.Object) []reconcile.Request { + // var result = []reconcile.Request{} + namespacedName := client.ObjectKey{ + Name: o.GetName(), + } + // result := []reconcile.Request{ + // {NamespacedName: namespacedName}, + // } + // return result + return []reconcile.Request{ + {NamespacedName: namespacedName}, + } +} diff --git a/controllers/gitopsservice_controller_test.go b/controllers/gitopsservice_controller_test.go index 9b5d2478c..7578ae6c8 100644 --- a/controllers/gitopsservice_controller_test.go +++ b/controllers/gitopsservice_controller_test.go @@ -542,7 +542,8 @@ func TestReconcile_VerifyResourceQuotaDeletionForUpgrade(t *testing.T) { // Create namespace object for default ArgoCD instance and set resource quota to it. defaultArgoNS := &corev1.Namespace{ ObjectMeta: v1.ObjectMeta{ - Name: serviceNamespace, + Name: serviceNamespace, + Namespace: serviceNamespace, }, } fakeClient.Create(context.TODO(), defaultArgoNS) From b000341baaed423b12890a4b1846fae4414927ee Mon Sep 17 00:00:00 2001 From: Varsha B Date: Mon, 13 Jan 2025 16:43:53 +0530 Subject: [PATCH 2/3] addressed review comments Signed-off-by: Varsha B --- controllers/gitopsservice_controller.go | 14 ++- controllers/gitopsservice_controller_test.go | 104 +++++++++++++++++++ 2 files changed, 110 insertions(+), 8 deletions(-) diff --git a/controllers/gitopsservice_controller.go b/controllers/gitopsservice_controller.go index 53077cf9c..2e999bdab 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -107,10 +107,12 @@ func (r *ReconcileGitopsService) SetupWithManager(mgr ctrl.Manager) error { Owns(&appsv1.Deployment{}, builder.WithPredicates(pred)). Owns(&corev1.Service{}, builder.WithPredicates(pred)). Owns(&routev1.Route{}, builder.WithPredicates(pred)). - Watches(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{ - Name: "openshift-gitops", - }}, - handler.EnqueueRequestsFromMapFunc(namespaceMapper), + Watches( + &corev1.Namespace{}, + &handler.EnqueueRequestForObject{}, + builder.WithPredicates(predicate.NewPredicateFuncs(func(obj client.Object) bool { + return obj.GetName() == "openshift-gitops" + })), ). Complete(r) } @@ -971,10 +973,6 @@ func namespaceMapper(ctx context.Context, o client.Object) []reconcile.Request { namespacedName := client.ObjectKey{ Name: o.GetName(), } - // result := []reconcile.Request{ - // {NamespacedName: namespacedName}, - // } - // return result return []reconcile.Request{ {NamespacedName: namespacedName}, } diff --git a/controllers/gitopsservice_controller_test.go b/controllers/gitopsservice_controller_test.go index 7578ae6c8..63894460e 100644 --- a/controllers/gitopsservice_controller_test.go +++ b/controllers/gitopsservice_controller_test.go @@ -633,6 +633,110 @@ func TestReconcile_InfrastructureNode(t *testing.T) { } +func TestReconcile_PSSLabels(t *testing.T) { + logf.SetLogger(argocd.ZapLogger(true)) + s := scheme.Scheme + addKnownTypesToScheme(s) + + testCases := []struct { + name string + namespace string + labels map[string]string + }{ + { + name: "modified valid PSS labels for openshift-gitops ns", + namespace: "openshift-gitops", + labels: map[string]string{ + "pod-security.kubernetes.io/enforce": "privileged", + "pod-security.kubernetes.io/enforce-version": "v1.30", + "pod-security.kubernetes.io/audit": "privileged", + "pod-security.kubernetes.io/audit-version": "v1.29", + "pod-security.kubernetes.io/warn": "privileged", + "pod-security.kubernetes.io/warn-version": "v1.29", + }, + }, + { + name: "modified invalid and empty PSS labels for openshift-gitops ns", + namespace: "openshift-gitops", + labels: map[string]string{ + "pod-security.kubernetes.io/enforce": "invalid", + "pod-security.kubernetes.io/enforce-version": "invalid", + "pod-security.kubernetes.io/warn": "invalid", + "pod-security.kubernetes.io/warn-version": "invalid", + }, + }, + } + + expected_labels := map[string]string{ + "pod-security.kubernetes.io/enforce": "restricted", + "pod-security.kubernetes.io/enforce-version": "v1.29", + "pod-security.kubernetes.io/audit": "restricted", + "pod-security.kubernetes.io/audit-version": "latest", + "pod-security.kubernetes.io/warn": "restricted", + "pod-security.kubernetes.io/warn-version": "latest", + } + + fakeClient := fake.NewFakeClient(util.NewClusterVersion("4.7.1"), newGitopsService()) + reconciler := newReconcileGitOpsService(fakeClient, s) + + _, err := reconciler.Reconcile(context.TODO(), newRequest("test", "test")) + assertNoError(t, err) + + // Create a user defined namespace + testNS := newRestrictedNamespace("test") + err = fakeClient.Create(context.TODO(), testNS) + assertNoError(t, err) + + // Create an ArgoCD instance in the user defined namespace + testArgoCD := &argoapp.ArgoCD{ + ObjectMeta: v1.ObjectMeta{ + Name: "test", + Namespace: "test", + }, + Spec: argoapp.ArgoCDSpec{}, + } + err = fakeClient.Create(context.TODO(), testArgoCD) + assertNoError(t, err) + + _, err = reconciler.Reconcile(context.TODO(), newRequest("test", "test")) + assertNoError(t, err) + + // Check if PSS labels are addded to the user defined ns + reconciled_ns := &corev1.Namespace{} + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: "test"}, + reconciled_ns) + assertNoError(t, err) + + for label, _ := range reconciled_ns.ObjectMeta.Labels { + _, found := expected_labels[label] + // Fail if label is found + assert.Check(t, found != true) + } + + for _, tc := range testCases { + existing_ns := &corev1.Namespace{} + assert.NilError(t, fakeClient.Get(context.TODO(), types.NamespacedName{Name: tc.namespace}, existing_ns), err) + + // Assign new values, confirm the assignment and update the PSS labels + existing_ns.ObjectMeta.Labels = tc.labels + fakeClient.Update(context.TODO(), existing_ns) + assert.NilError(t, fakeClient.Get(context.TODO(), types.NamespacedName{Name: tc.namespace}, existing_ns), err) + assert.DeepEqual(t, existing_ns.ObjectMeta.Labels, tc.labels) + + _, err := reconciler.Reconcile(context.TODO(), newRequest("test", "test")) + assertNoError(t, err) + + assert.NilError(t, fakeClient.Get(context.TODO(), types.NamespacedName{Name: tc.namespace}, reconciled_ns), err) + + for key, value := range expected_labels { + label, found := reconciled_ns.ObjectMeta.Labels[key] + // Fail if label is not found, comapre the values with the expected values if found + assert.Check(t, found) + assert.Equal(t, label, value) + } + } +} + func addKnownTypesToScheme(scheme *runtime.Scheme) { scheme.AddKnownTypes(configv1.GroupVersion, &configv1.ClusterVersion{}) scheme.AddKnownTypes(pipelinesv1alpha1.GroupVersion, &pipelinesv1alpha1.GitopsService{}) From c269db4339000cf760eaa2f91b81895302ceec73 Mon Sep 17 00:00:00 2001 From: Varsha B Date: Thu, 16 Jan 2025 11:26:45 +0530 Subject: [PATCH 3/3] addressed review comment Signed-off-by: Varsha B --- controllers/gitopsservice_controller.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/controllers/gitopsservice_controller.go b/controllers/gitopsservice_controller.go index 2e999bdab..1e6f9c569 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -967,13 +967,3 @@ func ensurePodSecurityLabels(namespace *corev1.Namespace) (bool, *corev1.Namespa } return changed, namespace } - -func namespaceMapper(ctx context.Context, o client.Object) []reconcile.Request { - // var result = []reconcile.Request{} - namespacedName := client.ObjectKey{ - Name: o.GetName(), - } - return []reconcile.Request{ - {NamespacedName: namespacedName}, - } -}