From df13e8d7ff29a703a5d6193570172a47cac04095 Mon Sep 17 00:00:00 2001 From: bartoszmajsak Date: Wed, 6 Dec 2023 15:55:35 +0100 Subject: [PATCH] fix: handles ns with recently removed annotation --- controllers/predicates.go | 45 ++++++++++++++++++ controllers/predicates_unit_test.go | 64 ++++++++++++++++++++++++++ controllers/project_mesh_controller.go | 16 ++----- controllers/unit_test.go | 23 --------- 4 files changed, 112 insertions(+), 36 deletions(-) create mode 100644 controllers/predicates.go create mode 100644 controllers/predicates_unit_test.go diff --git a/controllers/predicates.go b/controllers/predicates.go new file mode 100644 index 0000000..fa74138 --- /dev/null +++ b/controllers/predicates.go @@ -0,0 +1,45 @@ +package controllers + +import ( + "regexp" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" +) + +func MeshAwareNamespaces() predicate.Funcs { + filter := func(object client.Object) bool { + return !IsReservedNamespace(object.GetName()) && object.GetAnnotations()[AnnotationServiceMesh] != "" + } + + return predicate.Funcs{ + CreateFunc: func(createEvent event.CreateEvent) bool { + return filter(createEvent.Object) + }, + UpdateFunc: func(updateEvent event.UpdateEvent) bool { + if annotationRemoved(updateEvent, AnnotationServiceMesh) { + // annotation has been just removed, handle it in reconcile + return true + } + + return filter(updateEvent.ObjectNew) + }, + DeleteFunc: func(deleteEvent event.DeleteEvent) bool { + return filter(deleteEvent.Object) + }, + GenericFunc: func(genericEvent event.GenericEvent) bool { + return filter(genericEvent.Object) + }, + } +} + +func annotationRemoved(e event.UpdateEvent, annotation string) bool { + return e.ObjectOld.GetAnnotations()[annotation] != "" && e.ObjectNew.GetAnnotations()[annotation] == "" +} + +var reservedNamespaceRegex = regexp.MustCompile(`^(openshift|istio-system)$|^(kube|openshift)-.*$`) + +func IsReservedNamespace(namepace string) bool { + return reservedNamespaceRegex.MatchString(namepace) +} diff --git a/controllers/predicates_unit_test.go b/controllers/predicates_unit_test.go new file mode 100644 index 0000000..3843ff5 --- /dev/null +++ b/controllers/predicates_unit_test.go @@ -0,0 +1,64 @@ +package controllers_test + +import ( + "github.com/opendatahub-io/odh-project-controller/controllers" + "github.com/opendatahub-io/odh-project-controller/test/labels" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/event" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Controller predicates functions", Label(labels.Unit), func() { + + When("Checking namespace", func() { + + It("should trigger update event when annotation has been removed", func() { + // given + meshAwareNamespaces := controllers.MeshAwareNamespaces() + + // when + namespace := corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ns-with-annotation-removed", + }, + } + namespaceWithAnnotation := corev1.Namespace{} + namespace.DeepCopyInto(&namespaceWithAnnotation) + namespaceWithAnnotation.SetAnnotations(map[string]string{ + controllers.AnnotationServiceMesh: "true", + }) + + annotationRemovedEvent := event.UpdateEvent{ + ObjectOld: &namespaceWithAnnotation, + ObjectNew: &namespace, + } + + // then + Expect(meshAwareNamespaces.UpdateFunc(annotationRemovedEvent)).To(BeTrue()) + }) + + DescribeTable("it should not process reserved namespaces", + func(ns string, expected bool) { + Expect(controllers.IsReservedNamespace(ns)).To(Equal(expected)) + }, + Entry("kube-system is reserved namespace", "kube-system", true), + Entry("openshift-build is reserved namespace", "openshift-build", true), + Entry("kube-public is reserved namespace", "kube-public", true), + Entry("openshift-infra is reserved namespace", "openshift-infra", true), + Entry("kube-node-lease is reserved namespace", "kube-node-lease", true), + Entry("openshift is reserved namespace", "openshift", true), + Entry("istio-system is reserved namespace", "istio-system", true), + Entry("openshift-authentication is reserved namespace", "openshift-authentication", true), + Entry("openshift-apiserver is reserved namespace", "openshift-apiserver", true), + Entry("mynamespace is not reserved namespace", "mynamespace", false), + Entry("openshiftmynamespace is not reserved namespace", "openshiftmynamespace", false), + Entry("kubemynamespace is not reserved namespace", "kubemynamespace", false), + Entry("istio-system-openshift is not reserved namespace", "istio-system-openshift ", false), + ) + + }) + +}) diff --git a/controllers/project_mesh_controller.go b/controllers/project_mesh_controller.go index a40db76..eaff72a 100644 --- a/controllers/project_mesh_controller.go +++ b/controllers/project_mesh_controller.go @@ -2,7 +2,6 @@ package controllers import ( "context" - "regexp" "github.com/go-logr/logr" "github.com/pkg/errors" @@ -14,7 +13,6 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/predicate" ) // OpenshiftServiceMeshReconciler holds the controller configuration. @@ -51,6 +49,8 @@ func (r *OpenshiftServiceMeshReconciler) Reconcile(ctx context.Context, req ctrl } if serviceMeshIsNotEnabled(namespace.ObjectMeta) { + //nolint:godox //reason https://github.com/maistra/odh-project-controller/issues/65 + // TODO handle clean-up here return ctrl.Result{}, nil } @@ -65,17 +65,7 @@ func (r *OpenshiftServiceMeshReconciler) Reconcile(ctx context.Context, req ctrl func (r *OpenshiftServiceMeshReconciler) SetupWithManager(mgr ctrl.Manager) error { //nolint:wrapcheck //reason there is no point in wrapping it return ctrl.NewControllerManagedBy(mgr). - For(&v1.Namespace{}, builder.WithPredicates(predicate.NewPredicateFuncs(meshAwareNamespaces))). + For(&v1.Namespace{}, builder.WithPredicates(MeshAwareNamespaces())). Owns(&maistrav1.ServiceMeshMember{}). Complete(r) } - -func meshAwareNamespaces(object client.Object) bool { - return !IsReservedNamespace(object.GetName()) && object.GetAnnotations()[AnnotationServiceMesh] != "" -} - -var reservedNamespaceRegex = regexp.MustCompile(`^(openshift|istio-system)$|^(kube|openshift)-.*$`) - -func IsReservedNamespace(namepace string) bool { - return reservedNamespaceRegex.MatchString(namepace) -} diff --git a/controllers/unit_test.go b/controllers/unit_test.go index 7e249e3..38abc1d 100644 --- a/controllers/unit_test.go +++ b/controllers/unit_test.go @@ -26,27 +26,4 @@ var _ = Describe("Controller helper functions", Label(labels.Unit), func() { }) - When("Checking namespace", func() { - - DescribeTable("it should not process reserved namespaces", - func(ns string, expected bool) { - Expect(controllers.IsReservedNamespace(ns)).To(Equal(expected)) - }, - Entry("kube-system is reserved namespace", "kube-system", true), - Entry("openshift-build is reserved namespace", "openshift-build", true), - Entry("kube-public is reserved namespace", "kube-public", true), - Entry("openshift-infra is reserved namespace", "openshift-infra", true), - Entry("kube-node-lease is reserved namespace", "kube-node-lease", true), - Entry("openshift is reserved namespace", "openshift", true), - Entry("istio-system is reserved namespace", "istio-system", true), - Entry("openshift-authentication is reserved namespace", "openshift-authentication", true), - Entry("openshift-apiserver is reserved namespace", "openshift-apiserver", true), - Entry("mynamespace is not reserved namespace", "mynamespace", false), - Entry("openshiftmynamespace is not reserved namespace", "openshiftmynamespace", false), - Entry("kubemynamespace is not reserved namespace", "kubemynamespace", false), - Entry("istio-system-openshift is not reserved namespace", "istio-system-openshift ", false), - ) - - }) - })