diff --git a/pkg/controller/common/defaults/service.go b/pkg/controller/common/defaults/service.go index 1eb8c81681..638a4bf4cd 100644 --- a/pkg/controller/common/defaults/service.go +++ b/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 diff --git a/pkg/controller/common/defaults/service_test.go b/pkg/controller/common/defaults/service_test.go index 76fe82703c..a2ab340577 100644 --- a/pkg/controller/common/defaults/service_test.go +++ b/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}, + }, + }, + } +} diff --git a/pkg/controller/common/service_control.go b/pkg/controller/common/service_control.go index fc42c1409f..b1a5e39e34 100644 --- a/pkg/controller/common/service_control.go +++ b/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. diff --git a/pkg/controller/common/service_control_test.go b/pkg/controller/common/service_control_test.go index df7a835cce..2ee5e9c795 100644 --- a/pkg/controller/common/service_control_test.go +++ b/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) }) } } diff --git a/pkg/controller/kibana/services_test.go b/pkg/controller/kibana/services_test.go index 83d36ec818..6d2156e104 100644 --- a/pkg/controller/kibana/services_test.go +++ b/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 { diff --git a/pkg/utils/compare/objects.go b/pkg/utils/compare/objects.go new file mode 100644 index 0000000000..41dde1bfdd --- /dev/null +++ b/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) +} diff --git a/pkg/utils/maps/maps.go b/pkg/utils/maps/maps.go index 2bd303d1f4..c2f5df4f77 100644 --- a/pkg/utils/maps/maps.go +++ b/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 +} diff --git a/pkg/utils/maps/maps_test.go b/pkg/utils/maps/maps_test.go index e279509d0d..774bfbbad5 100644 --- a/pkg/utils/maps/maps_test.go +++ b/pkg/utils/maps/maps_test.go @@ -156,3 +156,76 @@ func TestMerge(t *testing.T) { }) } } + +func TestMergePreservingExistingKeys(t *testing.T) { + tests := []struct { + name string + dest map[string]string + src map[string]string + want map[string]string + }{ + { + name: "when dest is nil", + src: map[string]string{"x": "y"}, + want: map[string]string{"x": "y"}, + }, + { + name: "when src is nil", + dest: map[string]string{"x": "y"}, + want: map[string]string{"x": "y"}, + }, + { + name: "when both maps are nil", + }, + { + name: "when dest is empty", + dest: map[string]string{}, + src: map[string]string{"x": "y"}, + want: map[string]string{"x": "y"}, + }, + { + name: "when src is empty", + dest: map[string]string{"x": "y"}, + src: map[string]string{}, + want: map[string]string{"x": "y"}, + }, + { + name: "when both maps are empty", + dest: map[string]string{}, + src: map[string]string{}, + want: map[string]string{}, + }, + { + name: "when both maps contain the same items", + dest: map[string]string{"x": "y", "a": "b"}, + src: map[string]string{"x": "y", "a": "b"}, + want: map[string]string{"x": "y", "a": "b"}, + }, + { + name: "when keys are the same but value are different", + dest: map[string]string{"x": "p", "a": "q"}, + src: map[string]string{"x": "y", "a": "b"}, + want: map[string]string{"x": "p", "a": "q"}, + }, + + { + name: "when dest has fewer items than src", + dest: map[string]string{"x": "y"}, + src: map[string]string{"x": "z", "a": "b"}, + want: map[string]string{"x": "y", "a": "b"}, + }, + { + name: "when dest has more items than src", + dest: map[string]string{"x": "y", "a": "b"}, + src: map[string]string{"x": "z"}, + want: map[string]string{"x": "y", "a": "b"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + have := MergePreservingExistingKeys(tt.dest, tt.src) + require.Equal(t, tt.want, have) + }) + } +}