From caaa30cb763325f738d0568e8f2bd867eef5b845 Mon Sep 17 00:00:00 2001 From: machine424 Date: Mon, 9 Feb 2026 11:12:05 +0100 Subject: [PATCH 1/3] chore: add permissions on endpointslice to Prometheus Role and use serviceDiscoveryRole: EndpointSlice in ServiceMonitors This PR migrates Prometheus service discovery from the deprecated Endpoints API to the EndpointSlices API, by: Setting serviceDiscoveryRole: EndpointSlice on ServiceMonitors. Granting Prometheus endpointslices permissions. We're taking a conservative approach by keeping the existing endpoints permissions alongside the new endpointslices ones. This provides a safety net in case any ServiceMonitors, whether deployed from this repo or from another source, still rely on the same Role and were missed during the migration. That said, since both resources provide essentially the same data, keeping both isn't meaningfully more permissive from a security standpoint. These changes target OpenShift 4.22+ and should not be backported to earlier releases. Due to the scope of changes across multiple repositories, these modifications were generated with Claude assistance. --- manifests/0000_90_dns-operator_00_prometheusrole.yaml | 8 ++++++++ manifests/0000_90_dns-operator_02_servicemonitor.yaml | 1 + pkg/manifests/assets/dns/metrics/role.yaml | 8 ++++++++ 3 files changed, 17 insertions(+) diff --git a/manifests/0000_90_dns-operator_00_prometheusrole.yaml b/manifests/0000_90_dns-operator_00_prometheusrole.yaml index a7c6458c7..b38a9393c 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 fa763344d..70bd344d6 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 b73a5b5d8..8bce88e5c 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 From 454182e37979015a5ff38877317a1a010daa5740 Mon Sep 17 00:00:00 2001 From: machine424 Date: Thu, 12 Mar 2026 08:44:33 +0100 Subject: [PATCH 2/3] make sure ServiceMonitors and Roles are changed and updated for DNS pods as well --- .../0000_70_dns-operator_00-cluster-role.yaml | 1 + pkg/operator/controller/controller.go | 11 +-- .../controller/controller_metrics_role.go | 79 +++++++++++++++++++ .../controller_metrics_role_test.go | 26 ++++++ .../controller/controller_service_monitor.go | 3 +- .../controller_service_monitor_test.go | 8 ++ 6 files changed, 118 insertions(+), 10 deletions(-) create mode 100644 pkg/operator/controller/controller_metrics_role.go create mode 100644 pkg/operator/controller/controller_metrics_role_test.go diff --git a/manifests/0000_70_dns-operator_00-cluster-role.yaml b/manifests/0000_70_dns-operator_00-cluster-role.yaml index afd4c1800..e7158aa5c 100644 --- a/manifests/0000_70_dns-operator_00-cluster-role.yaml +++ b/manifests/0000_70_dns-operator_00-cluster-role.yaml @@ -91,6 +91,7 @@ rules: - rbac.authorization.k8s.io resources: - clusterroles + - roles verbs: - update diff --git a/pkg/operator/controller/controller.go b/pkg/operator/controller/controller.go index 58ba5eefa..b04c722b2 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 000000000..2c06a5758 --- /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 000000000..2f4fa7ffc --- /dev/null +++ b/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") + } +} diff --git a/pkg/operator/controller/controller_service_monitor.go b/pkg/operator/controller/controller_service_monitor.go index b154fbe57..f16789cbb 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 85e94f976..0d2c51659 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) { From 5ad1159a3ce6fc079c0cb39eb9319028ce397254 Mon Sep 17 00:00:00 2001 From: Ayoub Mrini Date: Sat, 25 Jul 2026 20:01:08 +0200 Subject: [PATCH 3/3] more adjustments and fixes --- .../0000_70_dns-operator_00-cluster-role.yaml | 1 + .../controller_metrics_role_test.go | 64 +++++++++++++++---- 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/manifests/0000_70_dns-operator_00-cluster-role.yaml b/manifests/0000_70_dns-operator_00-cluster-role.yaml index e7158aa5c..a20f0724e 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 diff --git a/pkg/operator/controller/controller_metrics_role_test.go b/pkg/operator/controller/controller_metrics_role_test.go index 2f4fa7ffc..f144f2ffb 100644 --- a/pkg/operator/controller/controller_metrics_role_test.go +++ b/pkg/operator/controller/controller_metrics_role_test.go @@ -7,20 +7,56 @@ import ( 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") +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, + }, } - 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") + + 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) + } + } } }