Use the controller's namespace for ServiceMonitor definitions

Instead of defaulting to the `openshift-monitoring` namespace for
OpenShift deployments, the ServiceMonitor is now defined in the
controller namespace by default. When the namespace is created by the
addon, the `openshift.io/cluster-monitoring: "true"` label is set on it
to enable automatic scraping from the OpenShift platform Prometheus.

Relates:
https://issues.redhat.com/browse/ACM-7633

Signed-off-by: mprahl <[email protected]>

Author: mprahl

pkg/addon/configpolicy/agent_addon.go

@@ -103,9 +103,6 @@ func getValues(cluster *clusterv1.ManagedCluster,
 
 	// Enable Prometheus metrics by default on OpenShift
 	userValues.Prometheus["enabled"] = userValues.KubernetesDistribution == "OpenShift"
-	if userValues.KubernetesDistribution == "OpenShift" {
-		userValues.Prometheus["serviceMonitor"] = map[string]interface{}{"namespace": "openshift-monitoring"}
-	}
 
 	annotations := addon.GetAnnotations()
 

pkg/addon/configpolicy/manifests/managedclusterchart/templates/install-namespace.yaml

@@ -8,6 +8,9 @@ metadata:
   labels:
     addon.open-cluster-management.io/namespace: "true"
     addon.open-cluster-management.io/hosted-manifest-location: hosting
+    {{- if and .Values.prometheus.enabled (eq .Values.kubernetesDistribution "OpenShift") }}
+    openshift.io/cluster-monitoring: "true"
+    {{- end }}
   {{- if eq (.Release.Namespace | trimPrefix "klusterlet-") .Values.clusterName }}
   annotations:
     "addon.open-cluster-management.io/deletion-orphan": ""

pkg/addon/configpolicy/manifests/managedclusterchart/templates/service_monitor.yaml

@@ -4,7 +4,7 @@
 apiVersion: monitoring.coreos.com/v1
 kind: ServiceMonitor
 metadata:
-  name: ocm-{{ include "controller.fullname" . }}-{{ .Release.Namespace }}-metrics
+  name: ocm-{{ include "controller.fullname" . }}-metrics
   namespace: {{ .Values.prometheus.serviceMonitor.namespace | default .Release.Namespace }}
   labels:
     app: {{ include "controller.fullname" . }}

pkg/addon/configpolicy/manifests/managedclusterchart/values.yaml

@@ -44,7 +44,7 @@ prometheus:
   # This will be automatically enabled if it's an OpenShift cluster.
   enabled: false
   serviceMonitor:
-    # This will be automatically set to openshift-monitoring if it's an OpenShift cluster.
+    # This will be automatically set to the controller's namespace.
     namespace: null
 
 global: 

pkg/addon/policyframework/agent_addon.go

@@ -133,9 +133,6 @@ func getValues(cluster *clusterv1.ManagedCluster,
 
 	// Enable Prometheus metrics by default on OpenShift
 	userValues.Prometheus["enabled"] = userValues.KubernetesDistribution == "OpenShift"
-	if userValues.KubernetesDistribution == "OpenShift" {
-		userValues.Prometheus["serviceMonitor"] = map[string]interface{}{"namespace": "openshift-monitoring"}
-	}
 
 	if val, ok := annotations[prometheusEnabledAnnotation]; ok {
 		valBool, err := strconv.ParseBool(val)

pkg/addon/policyframework/manifests/managedclusterchart/templates/install-namespace.yaml

@@ -8,6 +8,9 @@ metadata:
   labels:
     addon.open-cluster-management.io/namespace: "true"
     addon.open-cluster-management.io/hosted-manifest-location: hosting
+    {{- if and .Values.prometheus.enabled (eq .Values.kubernetesDistribution "OpenShift") }}
+    openshift.io/cluster-monitoring: "true"
+    {{- end }}
   {{- if eq (.Release.Namespace | trimPrefix "klusterlet-") .Values.clusterName }}
   annotations:
     "addon.open-cluster-management.io/deletion-orphan": ""

pkg/addon/policyframework/manifests/managedclusterchart/templates/service_monitor.yaml

@@ -4,7 +4,7 @@
 apiVersion: monitoring.coreos.com/v1
 kind: ServiceMonitor
 metadata:
-  name: ocm-{{ include "controller.fullname" . }}-{{ .Release.Namespace }}-metrics
+  name: ocm-{{ include "controller.fullname" . }}-metrics
   namespace: {{ .Values.prometheus.serviceMonitor.namespace | default .Release.Namespace }}
   labels:
     app: {{ include "controller.fullname" . }}

pkg/addon/policyframework/manifests/managedclusterchart/values.yaml

@@ -44,7 +44,7 @@ prometheus:
   # This will be automatically enabled if it's an OpenShift cluster.
   enabled: false
   serviceMonitor:
-    # This will be automatically set to openshift-monitoring if it's an OpenShift cluster.
+    # This will be automatically set to the controller's namespace.
     namespace: null
 
 global: 

test/e2e/case2_config_deployment_test.go

@@ -16,14 +16,15 @@ import (
 )
 
 const (
-	case2ManagedClusterAddOnName  string = "config-policy-controller"
-	case2ManagedClusterAddOnCR    string = "../resources/config_policy_addon_cr.yaml"
-	case2ClusterManagementAddOnCR string = "../resources/config_policy_clustermanagementaddon.yaml"
-	case2DeploymentName           string = "config-policy-controller"
-	case2PodSelector              string = "app=config-policy-controller"
-	case2OpenShiftClusterClaim    string = "../resources/openshift_cluster_claim.yaml"
-	policyCrdName                 string = "policies.policy.open-cluster-management.io"
-	deletionOrphanAnnotationKey   string = "addon.open-cluster-management.io/deletion-orphan"
+	case2ManagedClusterAddOnName       string = "config-policy-controller"
+	case2ManagedClusterAddOnCR         string = "../resources/config_policy_addon_cr.yaml"
+	case2ManagedClusterCustomNsAddOnCR string = "../resources/config_policy_addon_custom_ns.yaml"
+	case2ClusterManagementAddOnCR      string = "../resources/config_policy_clustermanagementaddon.yaml"
+	case2DeploymentName                string = "config-policy-controller"
+	case2PodSelector                   string = "app=config-policy-controller"
+	case2OpenShiftClusterClaim         string = "../resources/openshift_cluster_claim.yaml"
+	policyCrdName                      string = "policies.policy.open-cluster-management.io"
+	deletionOrphanAnnotationKey        string = "addon.open-cluster-management.io/deletion-orphan"
 )
 
 func verifyConfigPolicyDeployment(
@@ -369,7 +370,7 @@ var _ = Describe("Test config-policy-controller deployment", func() {
 			By(logPrefix + "verifying that the metrics ServiceMonitor exists")
 			Eventually(func(g Gomega) {
 				sm, err := cluster.clusterClient.Resource(gvrServiceMonitor).Namespace(addonNamespace).Get(
-					context.TODO(), "ocm-config-policy-controller-"+addonNamespace+"-metrics", metav1.GetOptions{},
+					context.TODO(), "ocm-config-policy-controller-metrics", metav1.GetOptions{},
 				)
 				g.Expect(err).ToNot(HaveOccurred())
 
@@ -405,18 +406,12 @@ var _ = Describe("Test config-policy-controller deployment", func() {
 	It("should create a config-policy-controller deployment with metrics monitoring on OpenShift clusters", func() {
 		Expect(managedClusterList).ToNot(BeEmpty())
 		hubClient := managedClusterList[0].clusterClient
+		// A custom namespace is used to see if the openshift.io/cluster-monitoring label is set.
+		addonNamespace := "e2e-test-config-policy-controller"
 
 		for i, cluster := range managedClusterList {
 			logPrefix := cluster.clusterType + " " + cluster.clusterName + ": "
 
-			By(logPrefix + "creating the openshift-monitoring namespace")
-			Kubectl(
-				"create",
-				"namespace",
-				"openshift-monitoring",
-				fmt.Sprintf("--kubeconfig=%s%d.kubeconfig", kubeconfigFilename, i+1),
-			)
-
 			By(logPrefix + "setting the product.open-cluster-management.io ClusterClaim to OpenShift")
 			Kubectl(
 				"apply",
@@ -451,7 +446,7 @@ var _ = Describe("Test config-policy-controller deployment", func() {
 			// The status doesn't need to be checked on the deployment because the deployment requires a cert that
 			// is auto-generated by OpenShift, which won't be present.
 			By(logPrefix + "deploying the default config-policy-controller managedclusteraddon")
-			Kubectl("apply", "-n", cluster.clusterName, "-f", case2ManagedClusterAddOnCR)
+			Kubectl("apply", "-n", cluster.clusterName, "-f", case2ManagedClusterCustomNsAddOnCR)
 			deploy := GetWithTimeout(
 				cluster.clusterClient, gvrDeployment, case2DeploymentName, addonNamespace, true, 30,
 			)
@@ -464,8 +459,8 @@ var _ = Describe("Test config-policy-controller deployment", func() {
 
 			By(logPrefix + "verifying that the metrics ServiceMonitor exists")
 			Eventually(func(g Gomega) {
-				sm, err := cluster.clusterClient.Resource(gvrServiceMonitor).Namespace("openshift-monitoring").Get(
-					context.TODO(), "ocm-config-policy-controller-"+addonNamespace+"-metrics", metav1.GetOptions{},
+				sm, err := cluster.clusterClient.Resource(gvrServiceMonitor).Namespace(addonNamespace).Get(
+					context.TODO(), "ocm-config-policy-controller-metrics", metav1.GetOptions{},
 				)
 				g.Expect(err).ToNot(HaveOccurred())
 
@@ -488,6 +483,16 @@ var _ = Describe("Test config-policy-controller deployment", func() {
 				g.Expect(port["targetPort"].(int64)).To(Equal(int64(8443)))
 			}, 120, 3).Should(Succeed())
 
+			By(logPrefix + "verifying that the addon namespace has the openshift.io/cluster-monitoring label set")
+			Eventually(func(g Gomega) {
+				ns, err := cluster.clusterClient.Resource(gvrNamespace).Get(
+					context.TODO(), addonNamespace, metav1.GetOptions{},
+				)
+				g.Expect(err).ToNot(HaveOccurred())
+
+				g.Expect(ns.GetLabels()["openshift.io/cluster-monitoring"]).To(Equal("true"))
+			}, 30, 3).Should(Succeed())
+
 			By(logPrefix + "cleaning up")
 			Kubectl(
 				"delete",
@@ -503,14 +508,6 @@ var _ = Describe("Test config-policy-controller deployment", func() {
 			)
 			Expect(deploy).To(BeNil())
 
-			Kubectl(
-				"delete",
-				"namespace",
-				"openshift-monitoring",
-				fmt.Sprintf("--kubeconfig=%s%d.kubeconfig", kubeconfigFilename, i+1),
-				"--timeout=15s",
-			)
-
 			By(logPrefix + "waiting for the ClusterClaim to not be in the ManagedCluster status")
 			Eventually(func(g Gomega) {
 				managedCluster, err := hubClient.Resource(gvrManagedCluster).Get(

test/resources/config_policy_addon_custom_ns.yaml

@@ -0,0 +1,6 @@
+apiVersion: addon.open-cluster-management.io/v1alpha1
+kind: ManagedClusterAddOn
+metadata:
+  name: config-policy-controller
+spec:
+  installNamespace: e2e-test-config-policy-controller