Apply changes to service resource

Add support for applying changes to the service resource. Roll back 8ae4b02
since service topology feature gate isn't yet enabled, and add TODOs to
re-introduce support when the gate is finally enabled.

assets/dns/service.yaml

@@ -17,6 +17,7 @@ spec:
     port: 9153
     targetPort: metrics
     protocol: TCP
-  topologyKeys:
-    - "kubernetes.io/hostname"
-    - "*"
+  # TODO: Uncomment when service topology feature gate is enabled.
+  #topologyKeys:
+  #  - "kubernetes.io/hostname"
+  #  - "*"

pkg/operator/controller/controller_dns_service.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/cluster-dns-operator/pkg/manifests"
 
@@ -21,16 +23,20 @@ func (r *reconciler) ensureDNSService(dns *operatorv1.DNS, clusterIP string, dae
 	if err != nil {
 		return nil, err
 	}
-	if current != nil {
-		return current, nil
-	}
-
 	desired := desiredDNSService(dns, clusterIP, daemonsetRef)
-	if err := r.client.Create(context.TODO(), desired); err != nil {
-		return nil, fmt.Errorf("failed to create dns service: %v", err)
+
+	switch {
+	case desired != nil && current == nil:
+		if err := r.client.Create(context.TODO(), desired); err != nil {
+			return nil, fmt.Errorf("failed to create dns service: %v", err)
+		}
+		logrus.Infof("created dns service: %s/%s", desired.Namespace, desired.Name)
+	case desired != nil && current != nil:
+		if err := r.updateDNSService(current, desired); err != nil {
+			return nil, err
+		}
 	}
-	logrus.Infof("created dns service: %s/%s", desired.Namespace, desired.Name)
-	return desired, nil
+	return r.currentDNSService(dns)
 }
 
 func (r *reconciler) currentDNSService(dns *operatorv1.DNS) (*corev1.Service, error) {
@@ -64,3 +70,39 @@ func desiredDNSService(dns *operatorv1.DNS, clusterIP string, daemonsetRef metav
 	}
 	return s
 }
+
+func (r *reconciler) updateDNSService(current, desired *corev1.Service) error {
+	changed, updated := serviceChanged(current, desired)
+	if !changed {
+		return nil
+	}
+
+	if err := r.client.Update(context.TODO(), updated); err != nil {
+		return fmt.Errorf("failed to update dns service %s/%s: %v", updated.Namespace, updated.Name, err)
+	}
+	logrus.Infof("updated dns service: %s/%s", updated.Namespace, updated.Name)
+	return nil
+}
+
+func serviceChanged(current, expected *corev1.Service) (bool, *corev1.Service) {
+	serviceCmpOpts := []cmp.Option{
+		// Ignore fields that the API, other controllers, or user may
+		// have modified.
+		//
+		// TODO: Remove TopologyKeys when the service topology feature gate is enabled.
+		cmpopts.IgnoreFields(corev1.ServiceSpec{}, "ClusterIP", "TopologyKeys"),
+		cmpopts.EquateEmpty(),
+	}
+	if cmp.Equal(current.Spec, expected.Spec, serviceCmpOpts...) {
+		return false, nil
+	}
+
+	updated := current.DeepCopy()
+	updated.Spec = expected.Spec
+
+	// Preserve fields that the API, other controllers, or user may have
+	// modified.
+	updated.Spec.ClusterIP = current.Spec.ClusterIP
+
+	return true, updated
+}

pkg/operator/controller/controller_dns_service_test.go

@@ -0,0 +1,89 @@
+package controller
+
+import (
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestDNSServiceChanged(t *testing.T) {
+	testCases := []struct {
+		description string
+		mutate      func(*corev1.Service)
+		expect      bool
+	}{
+		{
+			description: "if nothing changes",
+			mutate:      func(_ *corev1.Service) {},
+			expect:      false,
+		},
+		{
+			description: "if .uid changes",
+			mutate: func(service *corev1.Service) {
+				service.UID = "2"
+			},
+			expect: false,
+		},
+		{
+			description: "if .spec.topologyKey changes",
+			mutate: func(service *corev1.Service) {
+				service.Spec.TopologyKeys = []string{"foo"}
+			},
+			// TODO: Change to true when the service topology feature gate is enabled.
+			expect: false,
+		},
+		{
+			description: "if .spec.selector changes",
+			mutate: func(service *corev1.Service) {
+				service.Spec.Selector = map[string]string{"foo": "bar"}
+			},
+			expect: true,
+		},
+		{
+			description: "if .spec.type changes",
+			mutate: func(service *corev1.Service) {
+				service.Spec.Type = corev1.ServiceTypeNodePort
+			},
+			expect: true,
+		},
+		{
+			description: "if .spec.sessionAffinity changes",
+			mutate: func(service *corev1.Service) {
+				service.Spec.SessionAffinity = corev1.ServiceAffinityClientIP
+			},
+			expect: true,
+		},
+		{
+			description: "if .spec.publishNotReadyAddresses changes",
+			mutate: func(service *corev1.Service) {
+				service.Spec.PublishNotReadyAddresses = true
+			},
+			expect: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		original := corev1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "dns-original",
+				Namespace: "openshift-dns",
+				UID:       "1",
+			},
+			Spec: corev1.ServiceSpec{
+				Type:            corev1.ServiceTypeClusterIP,
+				SessionAffinity: corev1.ServiceAffinityNone,
+			},
+		}
+		mutated := original.DeepCopy()
+		tc.mutate(mutated)
+		if changed, updated := serviceChanged(&original, mutated); changed != tc.expect {
+			t.Errorf("%s, expect serviceChanged to be %t, got %t", tc.description, tc.expect, changed)
+		} else if changed {
+			if changedAgain, _ := serviceChanged(mutated, updated); changedAgain {
+				t.Errorf("%s, serviceChanged does not behave as a fixed point function", tc.description)
+			}
+		}
+	}
+}