Update service when labels and annotations are modified (#2210)

* Preserve service labels and annotations

* Add tests

pkg/controller/common/defaults/service.go

@@ -5,6 +5,7 @@
 package defaults
 
 import (
+	"github.com/elastic/cloud-on-k8s/pkg/utils/maps"
 	v1 "k8s.io/api/core/v1"
 )
 
@@ -15,16 +16,7 @@ func SetServiceDefaults(
 	defaultSelector map[string]string,
 	defaultPorts []v1.ServicePort,
 ) *v1.Service {
-	if svc.ObjectMeta.Labels == nil {
-		svc.ObjectMeta.Labels = defaultLabels
-	} else {
-		// add our labels, but don't overwrite user labels
-		for k, v := range defaultLabels {
-			if _, ok := svc.ObjectMeta.Labels[k]; !ok {
-				svc.ObjectMeta.Labels[k] = v
-			}
-		}
-	}
+	svc.Labels = maps.MergePreservingExistingKeys(svc.Labels, defaultLabels)
 
 	if svc.Spec.Selector == nil {
 		svc.Spec.Selector = defaultSelector

pkg/controller/common/defaults/service_test.go

@@ -7,100 +7,70 @@ package defaults
 import (
 	"testing"
 
-	"github.com/stretchr/testify/assert"
-	v1 "k8s.io/api/core/v1"
-	v12 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"github.com/elastic/cloud-on-k8s/pkg/utils/compare"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestSetServiceDefaults(t *testing.T) {
-	sampleSvc := v1.Service{
-		ObjectMeta: v12.ObjectMeta{
-			Labels: map[string]string{
-				"foo": "bar",
-			},
-		},
-		Spec: v1.ServiceSpec{
-			Selector: map[string]string{
-				"foo": "bar",
-			},
-			Ports: []v1.ServicePort{
-				{Name: "foo"},
-			},
-		},
-	}
-
-	sampleSvcWith := func(setter func(svc *v1.Service)) *v1.Service {
-		svc := sampleSvc.DeepCopy()
-		setter(svc)
-		return svc
-	}
-
-	type args struct {
-		svc             *v1.Service
+	testCases := []struct {
+		name            string
+		inSvc           func() *corev1.Service
 		defaultLabels   map[string]string
 		defaultSelector map[string]string
-		defaultPorts    []v1.ServicePort
-	}
-	tests := []struct {
-		name string
-		args args
-		want *v1.Service
+		defaultPorts    []corev1.ServicePort
+		wantSvc         func() *corev1.Service
 	}{
 		{
-			name: "with empty defaults",
-			args: args{
-				svc: sampleSvc.DeepCopy(),
+			name: "defaults are applied to empty service",
+			inSvc: func() *corev1.Service {
+				return &corev1.Service{
+					ObjectMeta: metav1.ObjectMeta{
+						Annotations: map[string]string{"bar": "baz"},
+					},
+				}
 			},
-			want: &sampleSvc,
+			defaultLabels:   map[string]string{"foo": "bar"},
+			defaultSelector: map[string]string{"foo": "bar"},
+			defaultPorts:    []corev1.ServicePort{{Name: "https", Port: 443}},
+			wantSvc:         mkService,
 		},
 		{
-			name: "should not overwrite, but add labels",
-			args: args{
-				svc: sampleSvc.DeepCopy(),
-				defaultLabels: map[string]string{
-					// this should be ignored
-					"foo": "foo",
-					// this should be added
-					"bar": "baz",
-				},
+			name:  "existing values take precedence over defaults",
+			inSvc: mkService,
+			defaultLabels: map[string]string{
+				"foo": "foo", // should be ignored
+				"bar": "baz", // should be added
 			},
-			want: sampleSvcWith(func(svc *v1.Service) {
+			defaultSelector: map[string]string{"foo": "foo", "bar": "bar"},     // should be completely ignored
+			defaultPorts:    []corev1.ServicePort{{Name: "https", Port: 8443}}, // should be completely ignored
+			wantSvc: func() *corev1.Service {
+				svc := mkService()
 				svc.Labels["bar"] = "baz"
-			}),
-		},
-		{
-			name: "should use default selector",
-			args: args{
-				svc:             &v1.Service{},
-				defaultSelector: map[string]string{"foo": "foo"},
-			},
-			want: &v1.Service{
-				Spec: v1.ServiceSpec{
-					Selector: map[string]string{"foo": "foo"},
-				},
-			},
-		},
-		{
-			name: "should use default ports",
-			args: args{
-				svc: &v1.Service{},
-				defaultPorts: []v1.ServicePort{
-					{Name: "bar"},
-				},
-			},
-			want: &v1.Service{
-				Spec: v1.ServiceSpec{
-					Ports: []v1.ServicePort{
-						{Name: "bar"},
-					},
-				},
+				return svc
 			},
 		},
 	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got := SetServiceDefaults(tt.args.svc, tt.args.defaultLabels, tt.args.defaultSelector, tt.args.defaultPorts)
-			assert.Equal(t, tt.want, got)
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			haveSvc := SetServiceDefaults(tc.inSvc(), tc.defaultLabels, tc.defaultSelector, tc.defaultPorts)
+			compare.JSONEqual(t, tc.wantSvc(), haveSvc)
 		})
 	}
 }
+
+func mkService() *corev1.Service {
+	return &corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Labels:      map[string]string{"foo": "bar"},
+			Annotations: map[string]string{"bar": "baz"},
+		},
+		Spec: corev1.ServiceSpec{
+			Selector: map[string]string{"foo": "bar"},
+			Ports: []corev1.ServicePort{
+				{Name: "https", Port: 443},
+			},
+		},
+	}
+}

pkg/controller/common/service_control.go

@@ -8,7 +8,9 @@ import (
 	"reflect"
 
 	"github.com/elastic/cloud-on-k8s/pkg/controller/common/reconciler"
+	"github.com/elastic/cloud-on-k8s/pkg/utils/compare"
 	"github.com/elastic/cloud-on-k8s/pkg/utils/k8s"
+	"github.com/elastic/cloud-on-k8s/pkg/utils/maps"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -36,7 +38,9 @@ func ReconcileService(
 			return needsUpdate(expected, reconciled)
 		},
 		UpdateReconciled: func() {
-			reconciled.Spec = expected.Spec // only update spec, keep the rest
+			reconciled.Annotations = expected.Annotations
+			reconciled.Labels = expected.Labels
+			reconciled.Spec = expected.Spec
 		},
 	})
 	return reconciled, err
@@ -77,7 +81,12 @@ func needsUpdate(expected *corev1.Service, reconciled *corev1.Service) bool {
 		expected.Spec.HealthCheckNodePort = reconciled.Spec.HealthCheckNodePort
 	}
 
-	return !reflect.DeepEqual(expected.Spec, reconciled.Spec)
+	expected.Annotations = maps.MergePreservingExistingKeys(expected.Annotations, reconciled.Annotations)
+	expected.Labels = maps.MergePreservingExistingKeys(expected.Labels, reconciled.Labels)
+
+	// if the specs, labels, or annotations differ, the object should be updated
+	return !(reflect.DeepEqual(expected.Spec, reconciled.Spec) &&
+		compare.LabelsAndAnnotationsAreEqual(expected.ObjectMeta, reconciled.ObjectMeta))
 }
 
 // hasNodePort returns for a given service type, if the service ports have a NodePort or not.

pkg/controller/common/service_control_test.go

@@ -5,13 +5,61 @@
 package common
 
 import (
-	"reflect"
 	"testing"
 
+	kbtype "github.com/elastic/cloud-on-k8s/pkg/apis/kibana/v1beta1"
+	"github.com/elastic/cloud-on-k8s/pkg/utils/compare"
+	"github.com/elastic/cloud-on-k8s/pkg/utils/k8s"
+	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 )
 
+func TestReconcileService(t *testing.T) {
+	owner := &kbtype.Kibana{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "owner-obj",
+			Namespace: "test",
+		},
+	}
+
+	existingSvc := mkService()
+	scheme := k8s.Scheme()
+	client := k8s.WrappedFakeClient(owner, existingSvc)
+
+	expectedSvc := mkService()
+	delete(expectedSvc.Labels, "lbl2")
+	delete(expectedSvc.Annotations, "ann2")
+	expectedSvc.Labels["lbl3"] = "lblval3"
+	expectedSvc.Annotations["ann3"] = "annval3"
+
+	wantSvc := mkService()
+	wantSvc.Labels["lbl3"] = "lblval3"
+	wantSvc.Annotations["ann3"] = "annval3"
+
+	haveSvc, err := ReconcileService(client, scheme, expectedSvc, owner)
+	require.NoError(t, err)
+	compare.JSONEqual(t, wantSvc, haveSvc)
+}
+
+func mkService() *corev1.Service {
+	return &corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "owner-svc",
+			Namespace:   "test",
+			Labels:      map[string]string{"lbl1": "lblval1", "lbl2": "lbl2val"},
+			Annotations: map[string]string{"ann1": "annval1", "ann2": "annval2"},
+		},
+		Spec: corev1.ServiceSpec{
+			Selector: map[string]string{"foo": "bar"},
+			Ports: []corev1.ServicePort{
+				{Name: "https", Port: 443},
+			},
+		},
+	}
+}
+
 func TestNeedsUpdate(t *testing.T) {
 	type args struct {
 		expected   corev1.Service
@@ -159,13 +207,50 @@ func TestNeedsUpdate(t *testing.T) {
 				Ports:                 []corev1.ServicePort{{Port: int32(9200), NodePort: int32(33433)}},
 			}},
 		},
+		{
+			name: "Annotations and labels are preserved",
+			args: args{
+				expected: corev1.Service{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:        "kibana-service",
+						Labels:      map[string]string{"label1": "label1val"},
+						Annotations: map[string]string{"annotation1": "annotation1val"},
+					},
+					Spec: corev1.ServiceSpec{
+						Type:      corev1.ServiceTypeClusterIP,
+						ClusterIP: "1.2.3.4",
+					},
+				},
+				reconciled: corev1.Service{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:        "kibana-service",
+						Labels:      map[string]string{"label1": "label1val", "label2": "label2val"},
+						Annotations: map[string]string{"annotation1": "annotation1val", "annotation2": "annotation2val"},
+					},
+					Spec: corev1.ServiceSpec{
+						Type:      corev1.ServiceTypeClusterIP,
+						ClusterIP: "1.2.3.4",
+					},
+				},
+			},
+			want: corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        "kibana-service",
+					Labels:      map[string]string{"label1": "label1val", "label2": "label2val"},
+					Annotations: map[string]string{"annotation1": "annotation1val", "annotation2": "annotation2val"},
+				},
+				Spec: corev1.ServiceSpec{
+					Type:      corev1.ServiceTypeClusterIP,
+					ClusterIP: "1.2.3.4",
+				},
+			},
+		},
 	}
+
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			_ = needsUpdate(&tt.args.expected, &tt.args.reconciled)
-			if !reflect.DeepEqual(tt.args.expected, tt.want) {
-				t.Errorf("needsUpdate(expected, reconcilied); expected is %v, want %v", tt.args.expected, tt.want)
-			}
+			compare.JSONEqual(t, tt.want, tt.args.expected)
 		})
 	}
 }

pkg/controller/kibana/services_test.go

@@ -68,6 +68,24 @@ func TestNewService(t *testing.T) {
 				return svc
 			},
 		},
+		{
+			name: "service template",
+			httpConf: commonv1beta1.HTTPConfig{
+				Service: commonv1beta1.ServiceTemplate{
+					ObjectMeta: metav1.ObjectMeta{
+						Labels:      map[string]string{"foo": "bar"},
+						Annotations: map[string]string{"bar": "baz"},
+					},
+				},
+			},
+			wantSvc: func() corev1.Service {
+				svc := mkService()
+				svc.Labels["foo"] = "bar"
+				svc.Annotations = map[string]string{"bar": "baz"}
+				svc.Spec.Ports[0].Name = "https"
+				return svc
+			},
+		},
 	}
 
 	for _, tc := range testCases {

pkg/utils/compare/objects.go

@@ -0,0 +1,16 @@
+// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+// or more contributor license agreements. Licensed under the Elastic License;
+// you may not use this file except in compliance with the Elastic License.
+
+package compare
+
+import (
+	"reflect"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// LabelsAndAnnotationsAreEqual compares just the labels and annotations for equality from two ObjectMeta instances.
+func LabelsAndAnnotationsAreEqual(a, b metav1.ObjectMeta) bool {
+	return reflect.DeepEqual(a.Labels, b.Labels) && reflect.DeepEqual(a.Annotations, b.Annotations)
+}

pkg/utils/maps/maps.go

@@ -34,3 +34,21 @@ func Merge(dest, src map[string]string) map[string]string {
 
 	return dest
 }
+
+// MergePreservingExistingKeys merges source into destination while skipping any keys that exist in the destination.
+func MergePreservingExistingKeys(dest, src map[string]string) map[string]string {
+	if dest == nil {
+		if src == nil {
+			return nil
+		}
+		dest = make(map[string]string, len(src))
+	}
+
+	for k, v := range src {
+		if _, exists := dest[k]; !exists {
+			dest[k] = v
+		}
+	}
+
+	return dest
+}