diff --git a/manifests/0000_70_dns-operator_00-cluster-role.yaml b/manifests/0000_70_dns-operator_00-cluster-role.yaml index afd4c18005..a20f0724ed 100644 --- a/manifests/0000_70_dns-operator_00-cluster-role.yaml +++ b/manifests/0000_70_dns-operator_00-cluster-role.yaml @@ -48,6 +48,7 @@ rules: resources: - endpointslices verbs: + - get - list - watch @@ -91,6 +92,7 @@ rules: - rbac.authorization.k8s.io resources: - clusterroles + - roles verbs: - update diff --git a/manifests/0000_90_dns-operator_00_prometheusrole.yaml b/manifests/0000_90_dns-operator_00_prometheusrole.yaml index a7c6458c7a..b38a9393c9 100644 --- a/manifests/0000_90_dns-operator_00_prometheusrole.yaml +++ b/manifests/0000_90_dns-operator_00_prometheusrole.yaml @@ -18,3 +18,11 @@ rules: - get - list - watch +- apiGroups: + - discovery.k8s.io + resources: + - endpointslices + verbs: + - get + - list + - watch diff --git a/manifests/0000_90_dns-operator_02_servicemonitor.yaml b/manifests/0000_90_dns-operator_02_servicemonitor.yaml index fa763344db..70bd344d69 100644 --- a/manifests/0000_90_dns-operator_02_servicemonitor.yaml +++ b/manifests/0000_90_dns-operator_02_servicemonitor.yaml @@ -20,3 +20,4 @@ spec: selector: matchLabels: name: dns-operator + serviceDiscoveryRole: EndpointSlice diff --git a/pkg/manifests/assets/dns/metrics/role.yaml b/pkg/manifests/assets/dns/metrics/role.yaml index b73a5b5d81..8bce88e5c6 100644 --- a/pkg/manifests/assets/dns/metrics/role.yaml +++ b/pkg/manifests/assets/dns/metrics/role.yaml @@ -15,3 +15,11 @@ rules: - get - list - watch +- apiGroups: + - discovery.k8s.io + resources: + - endpointslices + verbs: + - get + - list + - watch diff --git a/pkg/operator/controller/controller.go b/pkg/operator/controller/controller.go index 58ba5eefa7..b04c722b26 100644 --- a/pkg/operator/controller/controller.go +++ b/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() diff --git a/pkg/operator/controller/controller_metrics_role.go b/pkg/operator/controller/controller_metrics_role.go new file mode 100644 index 0000000000..2c06a57589 --- /dev/null +++ b/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 +} diff --git a/pkg/operator/controller/controller_metrics_role_test.go b/pkg/operator/controller/controller_metrics_role_test.go new file mode 100644 index 0000000000..f144f2ffba --- /dev/null +++ b/pkg/operator/controller/controller_metrics_role_test.go @@ -0,0 +1,62 @@ +package controller + +import ( + "testing" + + "github.com/openshift/cluster-dns-operator/pkg/manifests" + rbacv1 "k8s.io/api/rbac/v1" +) + +func TestDNSMetricsRoleChanged(t *testing.T) { + testCases := []struct { + description string + mutate func(*rbacv1.Role) + expect bool + }{ + { + description: "if nothing changes", + mutate: func(_ *rbacv1.Role) {}, + expect: false, + }, + { + description: "if a rule is added", + mutate: func(role *rbacv1.Role) { + role.Rules = append(role.Rules, rbacv1.PolicyRule{ + APIGroups: []string{"example.io"}, + Resources: []string{"foos"}, + Verbs: []string{"get"}, + }) + }, + expect: true, + }, + { + description: "if a rule is removed", + mutate: func(role *rbacv1.Role) { + role.Rules = role.Rules[1:] + }, + expect: true, + }, + { + description: "if an annotation is added", + mutate: func(role *rbacv1.Role) { + role.Annotations = map[string]string{ + "test": "test", + } + }, + expect: false, + }, + } + + for _, tc := range testCases { + original := manifests.MetricsRole() + mutated := original.DeepCopy() + tc.mutate(mutated) + if changed, updated := dnsMetricsRoleChanged(original, mutated); changed != tc.expect { + t.Errorf("%s, expect dnsMetricsRoleChanged to be %t, got %t", tc.description, tc.expect, changed) + } else if changed { + if changedAgain, _ := dnsMetricsRoleChanged(mutated, updated); changedAgain { + t.Errorf("%s, dnsMetricsRoleChanged does not behave as a fixed point function", tc.description) + } + } + } +} diff --git a/pkg/operator/controller/controller_service_monitor.go b/pkg/operator/controller/controller_service_monitor.go index b154fbe574..f16789cbb5 100644 --- a/pkg/operator/controller/controller_service_monitor.go +++ b/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", diff --git a/pkg/operator/controller/controller_service_monitor_test.go b/pkg/operator/controller/controller_service_monitor_test.go index 85e94f976c..0d2c516595 100644 --- a/pkg/operator/controller/controller_service_monitor_test.go +++ b/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) {