From c798308c27a2495360140c3e2c7fcd76c64c5764 Mon Sep 17 00:00:00 2001 From: Simone Rodigari <32323373+SRodi@users.noreply.github.com> Date: Tue, 24 Sep 2024 18:18:16 +0100 Subject: [PATCH] fix(bug): ignore headless svc to prevent error messages (#763) # Description * update service controller to ignore headless svc and re-trigger reconcile when encountering one * create test to validate UpdateRetinaSvc still throws an error if a retina svc has no IPv4 (this is to make sure existing validation post retinaSvc creation is not broken) ## Related Issue If this pull request is related to any issue, please mention it here. Additionally, make sure that the issue is assigned to you before submitting this pull request. fix #547 ## Checklist - [x] I have read the [contributing documentation](https://retina.sh/docs/contributing). - [x] I signed and signed-off the commits (`git commit -S -s ...`). See [this documentation](https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification) on signing commits. - [x] I have correctly attributed the author(s) of the code. - [x] I have tested the changes locally. - [x] I have followed the project's style guidelines. - [x] I have updated the documentation, if necessary. - [x] I have added tests, if applicable. ## Screenshots (if applicable) or Testing Completed Please add any relevant screenshots or GIFs to showcase the changes made. ## Additional Notes Add any additional notes or context about the pull request here. --- Please refer to the [CONTRIBUTING.md](../CONTRIBUTING.md) file for more information on how to contribute to this project. Signed-off-by: Simone Rodigari --- pkg/controllers/daemon/service/controller.go | 11 +- .../daemon/service/controller_test.go | 133 ++++++++++++++++++ 2 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 pkg/controllers/daemon/service/controller_test.go diff --git a/pkg/controllers/daemon/service/controller.go b/pkg/controllers/daemon/service/controller.go index 297e3c60ec..7c8ef71689 100644 --- a/pkg/controllers/daemon/service/controller.go +++ b/pkg/controllers/daemon/service/controller.go @@ -14,6 +14,7 @@ import ( errors "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/predicate" retinaCommon "github.com/microsoft/retina/pkg/common" "github.com/microsoft/retina/pkg/controllers/cache" @@ -76,7 +77,6 @@ func (r *ServiceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct } ips := retinaCommon.IPAddresses{} ips.IPv4 = net.ParseIP(service.Spec.ClusterIP) - net.ParseIP(service.Spec.ClusterIP) var lbIP net.IP if service.Status.LoadBalancer.Ingress != nil && len(service.Status.LoadBalancer.Ingress) > 0 { lbIP = net.ParseIP(service.Status.LoadBalancer.Ingress[0].IP) @@ -93,7 +93,16 @@ func (r *ServiceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct // SetupWithManager sets up the controller with the Manager. func (r *ServiceReconciler) SetupWithManager(mgr ctrl.Manager) error { r.l.Info("Setting up Service controller") + // Define a predicate to filter out Services with ClusterIP == "None" + pred := predicate.NewPredicateFuncs(func(object client.Object) bool { + service, ok := object.(*corev1.Service) + if !ok { + return false + } + return service.Spec.ClusterIP != "None" + }) return ctrl.NewControllerManagedBy(mgr). For(&corev1.Service{}). + WithEventFilter(pred). Complete(r) } diff --git a/pkg/controllers/daemon/service/controller_test.go b/pkg/controllers/daemon/service/controller_test.go new file mode 100644 index 0000000000..5f25a90120 --- /dev/null +++ b/pkg/controllers/daemon/service/controller_test.go @@ -0,0 +1,133 @@ +package service + +import ( + "testing" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" +) + +func TestServiceFilter(t *testing.T) { + pred := predicate.NewPredicateFuncs(func(object client.Object) bool { + service, ok := object.(*corev1.Service) + if !ok { + return false + } + return service.Spec.ClusterIP != "None" + }) + + tests := []struct { + name string + event interface{} + expect bool + }{ + { + name: "CreateEvent with ClusterIP None", + event: event.CreateEvent{ + Object: &corev1.Service{ + Spec: corev1.ServiceSpec{ + ClusterIP: "None", + }, + }, + }, + expect: false, + }, + { + name: "CreateEvent with ClusterIP not None", + event: event.CreateEvent{ + Object: &corev1.Service{ + Spec: corev1.ServiceSpec{ + ClusterIP: "10.0.0.1", + }, + }, + }, + expect: true, + }, + { + name: "UpdateEvent with ClusterIP None", + event: event.UpdateEvent{ + ObjectNew: &corev1.Service{ + Spec: corev1.ServiceSpec{ + ClusterIP: "None", + }, + }, + }, + expect: false, + }, + { + name: "UpdateEvent with ClusterIP not None", + event: event.UpdateEvent{ + ObjectNew: &corev1.Service{ + Spec: corev1.ServiceSpec{ + ClusterIP: "10.0.0.1", + }, + }, + }, + expect: true, + }, + { + name: "DeleteEvent with ClusterIP None", + event: event.DeleteEvent{ + Object: &corev1.Service{ + Spec: corev1.ServiceSpec{ + ClusterIP: "None", + }, + }, + }, + expect: false, + }, + { + name: "DeleteEvent with ClusterIP not None", + event: event.DeleteEvent{ + Object: &corev1.Service{ + Spec: corev1.ServiceSpec{ + ClusterIP: "10.0.0.1", + }, + }, + }, + expect: true, + }, + { + name: "GenericEvent with ClusterIP None", + event: event.GenericEvent{ + Object: &corev1.Service{ + Spec: corev1.ServiceSpec{ + ClusterIP: "None", + }, + }, + }, + expect: false, + }, + { + name: "GenericEvent with ClusterIP not None", + event: event.GenericEvent{ + Object: &corev1.Service{ + Spec: corev1.ServiceSpec{ + ClusterIP: "10.0.0.1", + }, + }, + }, + expect: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var result bool + switch e := tt.event.(type) { + case event.CreateEvent: + result = pred.Create(e) + case event.UpdateEvent: + result = pred.Update(e) + case event.DeleteEvent: + result = pred.Delete(e) + case event.GenericEvent: + result = pred.Generic(e) + } + assert.Equal(t, tt.expect, result) + }) + } +}