From 13a282ecb785bcc332e5c785481b34b9e14965af Mon Sep 17 00:00:00 2001 From: Dean Coakley Date: Thu, 25 Feb 2021 14:29:27 +0000 Subject: [PATCH] Add support for updating ExtraLabels --- .../nginxingresscontroller/service.go | 15 +- .../nginxingresscontroller/service_test.go | 260 +++++++++++++++++- 2 files changed, 271 insertions(+), 4 deletions(-) diff --git a/pkg/controller/nginxingresscontroller/service.go b/pkg/controller/nginxingresscontroller/service.go index 01ac41b4..e003f902 100644 --- a/pkg/controller/nginxingresscontroller/service.go +++ b/pkg/controller/nginxingresscontroller/service.go @@ -1,6 +1,8 @@ package nginxingresscontroller import ( + "reflect" + k8sv1alpha1 "github.com/nginxinc/nginx-ingress-operator/pkg/apis/k8s/v1alpha1" corev1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -47,10 +49,21 @@ func serviceForNginxIngressController(instance *k8sv1alpha1.NginxIngressControll } func hasServiceChanged(svc *corev1.Service, instance *k8sv1alpha1.NginxIngressController) bool { - return svc.Spec.Type != corev1.ServiceType(instance.Spec.ServiceType) + if svc.Spec.Type != corev1.ServiceType(instance.Spec.ServiceType) { + return true + } + if instance.Spec.Service != nil && !reflect.DeepEqual(svc.Labels, instance.Spec.Service.ExtraLabels) { + return true + } + return svc.Labels != nil && instance.Spec.Service == nil } func updateService(svc *corev1.Service, instance *k8sv1alpha1.NginxIngressController) *corev1.Service { svc.Spec.Type = corev1.ServiceType(instance.Spec.ServiceType) + if instance.Spec.Service != nil { + svc.Labels = instance.Spec.Service.ExtraLabels + } else { + svc.Labels = nil + } return svc } diff --git a/pkg/controller/nginxingresscontroller/service_test.go b/pkg/controller/nginxingresscontroller/service_test.go index 9590483a..b9f0bc44 100644 --- a/pkg/controller/nginxingresscontroller/service_test.go +++ b/pkg/controller/nginxingresscontroller/service_test.go @@ -13,7 +13,6 @@ import ( func TestServiceForNginxIngressController(t *testing.T) { name := "my-service" namespace := "my-nginx-ingress" - serviceType := "LoadBalancer" extraLabels := map[string]string{"app": "my-nginx-ingress"} instance := &k8sv1alpha1.NginxIngressController{ @@ -23,7 +22,7 @@ func TestServiceForNginxIngressController(t *testing.T) { Labels: extraLabels, }, Spec: k8sv1alpha1.NginxIngressControllerSpec{ - ServiceType: serviceType, + ServiceType: string(corev1.ServiceTypeLoadBalancer), Service: &k8sv1alpha1.Service{ ExtraLabels: extraLabels, }, @@ -57,7 +56,7 @@ func TestServiceForNginxIngressController(t *testing.T) { }, }, Selector: map[string]string{"app": instance.Name}, - Type: corev1.ServiceType(serviceType), + Type: corev1.ServiceTypeLoadBalancer, }, } @@ -66,3 +65,258 @@ func TestServiceForNginxIngressController(t *testing.T) { t.Errorf("serviceForNginxIngressController() mismatch (-want +got):\n%s", diff) } } + +func TestHasServiceChanged(t *testing.T) { + name := "my-service" + namespace := "my-nginx-ingress" + extraLabels := map[string]string{"app": "my-nginx-ingress"} + + tests := []struct { + svc *corev1.Service + instance *k8sv1alpha1.NginxIngressController + expected bool + msg string + }{ + { + &corev1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + }, + &k8sv1alpha1.NginxIngressController{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: k8sv1alpha1.NginxIngressControllerSpec{ + ServiceType: string(corev1.ServiceTypeLoadBalancer), + }, + }, + false, + "no changes", + }, + { + &corev1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + }, + &k8sv1alpha1.NginxIngressController{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: k8sv1alpha1.NginxIngressControllerSpec{ + ServiceType: "NodePort", + }, + }, + true, + "different service type", + }, + { + &corev1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: extraLabels, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + }, + &k8sv1alpha1.NginxIngressController{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: k8sv1alpha1.NginxIngressControllerSpec{ + ServiceType: string(corev1.ServiceTypeLoadBalancer), + Service: &k8sv1alpha1.Service{ + ExtraLabels: map[string]string{"new": "label"}, + }, + }, + }, + true, + "different label", + }, + { + &corev1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: extraLabels, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + }, + &k8sv1alpha1.NginxIngressController{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: k8sv1alpha1.NginxIngressControllerSpec{ + ServiceType: string(corev1.ServiceTypeLoadBalancer), + Service: &k8sv1alpha1.Service{ + ExtraLabels: nil, + }, + }, + }, + true, + "remove label", + }, + { + &corev1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: extraLabels, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + }, + &k8sv1alpha1.NginxIngressController{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: k8sv1alpha1.NginxIngressControllerSpec{ + ServiceType: string(corev1.ServiceTypeLoadBalancer), + }, + }, + true, + "remove service parameters", + }, + } + for _, test := range tests { + result := hasServiceChanged(test.svc, test.instance) + if result != test.expected { + t.Errorf("hasServiceChanged() = %v, want %v for the case of %v", result, test.expected, test.msg) + } + } +} + +func TestUpdateService(t *testing.T) { + name := "my-service" + namespace := "my-nginx-ingress" + extraLabels := map[string]string{"app": "my-nginx-ingress"} + + tests := []struct { + svc *corev1.Service + instance *k8sv1alpha1.NginxIngressController + expected *corev1.Service + msg string + }{ + { + &corev1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + }, + &k8sv1alpha1.NginxIngressController{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: k8sv1alpha1.NginxIngressControllerSpec{ + ServiceType: string(corev1.ServiceTypeNodePort), + }, + }, + &corev1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + }, + }, + "override service type", + }, + { + &corev1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: map[string]string{"my": "labelToBeOverridden"}, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + }, + &k8sv1alpha1.NginxIngressController{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: k8sv1alpha1.NginxIngressControllerSpec{ + ServiceType: string(corev1.ServiceTypeLoadBalancer), + Service: &k8sv1alpha1.Service{ + ExtraLabels: extraLabels, + }, + }, + }, + &corev1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: extraLabels, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + }, + "override labels", + }, + { + &corev1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: map[string]string{"my": "labelToBeRemoved"}, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + }, + &k8sv1alpha1.NginxIngressController{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: k8sv1alpha1.NginxIngressControllerSpec{ + ServiceType: string(corev1.ServiceTypeLoadBalancer), + }, + }, + &corev1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + }, + }, + "remove labels", + }, + } + for _, test := range tests { + result := updateService(test.svc, test.instance) + if diff := cmp.Diff(test.expected, result); diff != "" { + t.Errorf("updateService() mismatch for the case of %v (-want +got):\n%s", test.msg, diff) + } + } +}