make sure ServiceMonitors and Roles are changed and updated for DNS pods as well

Author: machine424

manifests/0000_70_dns-operator_00-cluster-role.yaml

@@ -91,6 +91,7 @@ rules:
   - rbac.authorization.k8s.io
   resources:
   - clusterroles
+  - roles
   verbs:
   - update
 

pkg/operator/controller/controller.go

@@ -464,15 +464,8 @@ func (r *reconciler) ensureMetricsIntegration(dns *operatorv1.DNS, svc *corev1.S
 		logrus.Infof("created dns metrics cluster role binding %s", crb.Name)
 	}
 
-	mr := manifests.MetricsRole()
-	if err := r.client.Get(context.TODO(), types.NamespacedName{Namespace: mr.Namespace, Name: mr.Name}, mr); err != nil {
-		if !errors.IsNotFound(err) {
-			return fmt.Errorf("failed to get dns metrics role %s/%s: %v", mr.Namespace, mr.Name, err)
-		}
-		if err := r.client.Create(context.TODO(), mr); err != nil {
-			return fmt.Errorf("failed to create dns metrics role %s/%s: %v", mr.Namespace, mr.Name, err)
-		}
-		logrus.Infof("created dns metrics role %s/%s", mr.Namespace, mr.Name)
+	if _, _, err := r.ensureDNSMetricsRole(); err != nil {
+		return fmt.Errorf("failed to ensure dns metrics role for %s: %v", dns.Name, err)
 	}
 
 	mrb := manifests.MetricsRoleBinding()

pkg/operator/controller/controller_metrics_role.go

@@ -0,0 +1,79 @@
+package controller
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/openshift/cluster-dns-operator/pkg/manifests"
+
+	"github.com/sirupsen/logrus"
+
+	rbacv1 "k8s.io/api/rbac/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/types"
+)
+
+func (r *reconciler) ensureDNSMetricsRole() (bool, *rbacv1.Role, error) {
+	desired := manifests.MetricsRole()
+
+	have, current, err := r.currentDNSMetricsRole()
+	if err != nil {
+		return false, nil, err
+	}
+
+	switch {
+	case !have:
+		if err := r.client.Create(context.TODO(), desired); err != nil {
+			return false, nil, fmt.Errorf("failed to create dns metrics role %s/%s: %v", desired.GetNamespace(), desired.GetName(), err)
+		}
+		logrus.Infof("created dns metrics role %s/%s", desired.GetNamespace(), desired.GetName())
+		return r.currentDNSMetricsRole()
+	case have:
+		if updated, err := r.updateDNSMetricsRole(current, desired); err != nil {
+			return true, current, err
+		} else if updated {
+			return r.currentDNSMetricsRole()
+		}
+	}
+	return true, current, nil
+}
+
+func (r *reconciler) currentDNSMetricsRole() (bool, *rbacv1.Role, error) {
+	desired := manifests.MetricsRole()
+	current := &rbacv1.Role{}
+	if err := r.client.Get(context.TODO(), types.NamespacedName{Namespace: desired.GetNamespace(), Name: desired.GetName()}, current); err != nil {
+		if errors.IsNotFound(err) {
+			return false, nil, nil
+		}
+		return false, nil, err
+	}
+	return true, current, nil
+}
+
+func (r *reconciler) updateDNSMetricsRole(current, desired *rbacv1.Role) (bool, error) {
+	changed, updated := dnsMetricsRoleChanged(current, desired)
+	if !changed {
+		return false, nil
+	}
+
+	// Diff before updating because the client may mutate the object.
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
+	if err := r.client.Update(context.TODO(), updated); err != nil {
+		return false, fmt.Errorf("failed to update dns metrics role %s/%s: %v", updated.GetNamespace(), updated.GetName(), err)
+	}
+	logrus.Infof("updated dns metrics role %s/%s: %v", updated.GetNamespace(), updated.GetName(), diff)
+	return true, nil
+}
+
+func dnsMetricsRoleChanged(current, desired *rbacv1.Role) (bool, *rbacv1.Role) {
+	if cmp.Equal(current.Rules, desired.Rules, cmpopts.EquateEmpty()) {
+		return false, nil
+	}
+
+	updated := current.DeepCopy()
+	updated.Rules = desired.Rules
+
+	return true, updated
+}

pkg/operator/controller/controller_metrics_role_test.go

@@ -0,0 +1,26 @@
+package controller
+
+import (
+	"testing"
+
+	"github.com/openshift/cluster-dns-operator/pkg/manifests"
+	rbacv1 "k8s.io/api/rbac/v1"
+)
+
+func TestDNSggMetricsRoleChanged(t *testing.T) {
+	role1 := manifests.MetricsRole()
+	role2 := manifests.MetricsRole()
+	if changed, _ := dnsMetricsRoleChanged(role1, role2); changed {
+		t.Fatal("expected changed to be false for two roles with identical rules")
+	}
+	role2.Rules = append(role2.Rules, rbacv1.PolicyRule{
+		APIGroups: []string{"example.io"},
+		Resources: []string{"foos"},
+		Verbs:     []string{"get"},
+	})
+	if changed, updated := dnsMetricsRoleChanged(role1, role2); !changed {
+		t.Fatal("expected changed to be true after adding a rule")
+	} else if changedAgain, _ := dnsMetricsRoleChanged(role2, updated); changedAgain {
+		t.Fatal("dnsMetricsRoleChanged does not behave as a fixed-point function")
+	}
+}

pkg/operator/controller/controller_service_monitor.go

@@ -57,7 +57,8 @@ func desiredServiceMonitor(dns *operatorv1.DNS, svc *corev1.Service, daemonsetRe
 						"openshift-dns",
 					},
 				},
-				"selector": map[string]interface{}{},
+				"selector":             map[string]interface{}{},
+				"serviceDiscoveryRole": "EndpointSlice",
 				"endpoints": []interface{}{
 					map[string]interface{}{
 						"bearerTokenFile": "/var/run/secrets/kubernetes.io/serviceaccount/token",

pkg/operator/controller/controller_service_monitor_test.go

@@ -31,6 +31,14 @@ func TestDNSServiceMonitorChanged(t *testing.T) {
 			},
 			expect: true,
 		},
+		{
+			description: "if serviceDiscoveryRole changes",
+			mutate: func(serviceMonitor *unstructured.Unstructured) {
+				spec := serviceMonitor.Object["spec"].(map[string]interface{})
+				spec["serviceDiscoveryRole"] = "EndpointSlice"
+			},
+			expect: true,
+		},
 		{
 			description: "if labels change",
 			mutate: func(serviceMonitor *unstructured.Unstructured) {