Merge pull request #2595 from danwinship/crd-metrics

MON-4282: Multi-tenant support for KSM's CRS

Author: openshift-merge-bot[bot]

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Note: This CHANGELOG is only for the monitoring team to track all monitoring related changes. Please see OpenShift release notes for official changes.
 
+## 4.20
+
+- [#2595](https://github.com/openshift/cluster-monitoring-operator/pull/2595) Multi-tenant support for KSM's CRS feature-set downstream.
+
 ## 4.18
 
 - [#2503](https://github.com/openshift/cluster-monitoring-operator/issues/2503) Expose `scrapeInterval` setting for UWM Prometheus.

assets/kube-state-metrics/cluster-role.yaml

@@ -129,17 +129,17 @@ rules:
   - list
   - watch
 - apiGroups:
-  - autoscaling.k8s.io
+  - apiextensions.k8s.io
   resources:
-  - verticalpodautoscalers
+  - customresourcedefinitions
   verbs:
+  - get
   - list
   - watch
 - apiGroups:
-  - apiextensions.k8s.io
+  - autoscaling.k8s.io
   resources:
-  - customresourcedefinitions
+  - verticalpodautoscalers
   verbs:
-  - get
   - list
   - watch

assets/kube-state-metrics/deployment.yaml

@@ -36,6 +36,7 @@ spec:
         - --port=8081
         - --telemetry-host=127.0.0.1
         - --telemetry-port=8082
+        - --custom-resource-state-config-file=/etc/kube-state-metrics/custom-resource-state-configmap.yaml
         - |
           --metric-denylist=
           ^kube_secret_labels$,

jsonnet/components/kube-state-metrics.libsonnet

@@ -42,18 +42,18 @@ function(params)
 
     clusterRole+: {
       rules+: [
-        {
-          apiGroups: ['autoscaling.k8s.io'],
-          resources: ['verticalpodautoscalers'],
-          verbs: ['list', 'watch'],
-        },
         // CRD read permissions are required for kube-state-metrics to support the CRS feature-set.
         // Refer: https://github.com/kubernetes/kube-state-metrics/pull/1851/files#diff-916e6863e1245c673b4e5965c98dc27bafbd72650fdb38ce65ea73ee6304e027R45-R47
         {
           apiGroups: ['apiextensions.k8s.io'],
           resources: ['customresourcedefinitions'],
           verbs: ['get', 'list', 'watch'],
         },
+        {
+          apiGroups: ['autoscaling.k8s.io'],
+          resources: ['verticalpodautoscalers'],
+          verbs: ['list', 'watch'],
+        },
       ],
     },
 
@@ -173,8 +173,8 @@ function(params)
     kubeRbacProxySecret: generateSecret.staticAuthSecret(cfg.namespace, cfg.commonLabels, 'kube-state-metrics-kube-rbac-proxy-config'),
 
     // This removes the upstream addon-resizer and all resource requests and
-    // limits. Additionally configures the kube-rbac-proxies to use the serving
-    // cert configured on the `Service` above.
+    // limits. Additionally, it configures the kube-rbac-proxies to use the serving
+    // cert configured on the `Service` above, and enables CRD metrics.
     //
     // The upstream kube-state-metrics Dockerfile defines a `VOLUME` directive
     // in `/tmp`. Although this is unused it will take some time for it to get
@@ -237,6 +237,7 @@ function(params)
                   else
                     c {
                       args+: [
+                        '--custom-resource-state-config-file=/etc/kube-state-metrics/custom-resource-state-configmap.yaml',
                         |||
                           --metric-denylist=
                           ^kube_secret_labels$,
@@ -259,7 +260,6 @@ function(params)
                           name: tmpVolumeName,
                           readOnly: false,
                         },
-                        // The custom resource state configmap is always mounted in the kube-state-metrics container and only when the VPA CRD is installed, CMO will add `--custom-resource-state-config-file` to the container arguments list.
                         {
                           mountPath: '/etc/kube-state-metrics',
                           name: crsVolumeName,

manifests/0000_50_cluster-monitoring-operator_02-role.yaml

@@ -360,18 +360,18 @@ rules:
   - list
   - watch
 - apiGroups:
-  - autoscaling.k8s.io
+  - apiextensions.k8s.io
   resources:
-  - verticalpodautoscalers
+  - customresourcedefinitions
   verbs:
+  - get
   - list
   - watch
 - apiGroups:
-  - apiextensions.k8s.io
+  - autoscaling.k8s.io
   resources:
-  - customresourcedefinitions
+  - verticalpodautoscalers
   verbs:
-  - get
   - list
   - watch
 - apiGroups:

pkg/client/client.go

@@ -65,14 +65,12 @@ import (
 	"k8s.io/klog/v2"
 	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
 	aggregatorclient "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset"
-	"k8s.io/utils/ptr"
 )
 
 const (
-	deleteTimeout                        = 10 * time.Minute
-	metadataPrefix                       = "monitoring.openshift.io/"
-	clusterConsole                       = "cluster"
-	VerticalPodAutoscalerCRDMetadataName = "verticalpodautoscalers.autoscaling.k8s.io"
+	deleteTimeout  = 10 * time.Minute
+	metadataPrefix = "monitoring.openshift.io/"
+	clusterConsole = "cluster"
 )
 
 type Client struct {
@@ -276,10 +274,6 @@ func (c *Client) KubernetesInterface() kubernetes.Interface {
 	return c.kclient
 }
 
-func (c *Client) ApiExtensionsInterface() apiextensionsclient.Interface {
-	return c.eclient
-}
-
 func (c *Client) EventRecorder() events.Recorder {
 	return c.eventRecorder
 }
@@ -423,21 +417,6 @@ func (c *Client) ClusterOperatorListWatch(ctx context.Context, name string) *cac
 	}
 }
 
-func (c *Client) VerticalPodAutoscalerCRDListWatch(ctx context.Context) *cache.ListWatch {
-	return &cache.ListWatch{
-		ListFunc: func(options metav1.ListOptions) (runtime.Object, error) {
-			return c.eclient.ApiextensionsV1().CustomResourceDefinitions().List(ctx, metav1.ListOptions{
-				FieldSelector: fields.OneTermEqualSelector("metadata.name", VerticalPodAutoscalerCRDMetadataName).String(),
-			})
-		},
-		WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) {
-			return c.eclient.ApiextensionsV1().CustomResourceDefinitions().Watch(ctx, metav1.ListOptions{
-				FieldSelector: fields.OneTermEqualSelector("metadata.name", VerticalPodAutoscalerCRDMetadataName).String(),
-			})
-		},
-	}
-}
-
 func (c *Client) HasRouteCapability(ctx context.Context) (bool, error) {
 	_, err := c.oscclient.ConfigV1().ClusterOperators().Get(ctx, "ingress", metav1.GetOptions{})
 	if apierrors.IsNotFound(err) {
@@ -1850,29 +1829,6 @@ func (c *Client) RegisterConsolePlugin(ctx context.Context, name string) error {
 	return nil
 }
 
-// VPACustomResourceDefinitionPresent checks if VerticalPodAutoscaler CRD is present in the cluster.
-func (c *Client) VPACustomResourceDefinitionPresent(ctx context.Context, lastKnownVPACustomResourceDefinitionPresent *bool) (*bool, error) {
-	_, err := c.ApiExtensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(ctx, VerticalPodAutoscalerCRDMetadataName, metav1.GetOptions{})
-	if err != nil {
-		// VPA CRD is absent.
-		if apierrors.IsNotFound(err) {
-			return ptr.To(false), nil
-		}
-
-		// See if we have an idea of the state of the CRD's presence before the transient error occurred.
-		// If we do, resort to that since we do not want this to cause unnecessary reconciles.
-		if lastKnownVPACustomResourceDefinitionPresent != nil {
-			return lastKnownVPACustomResourceDefinitionPresent, nil
-		}
-
-		// If we don't, throw.
-		return nil, fmt.Errorf("failed to get %s CRD: %w", VerticalPodAutoscalerCRDMetadataName, err)
-	}
-
-	// VPA CRD is present.
-	return ptr.To(true), nil
-}
-
 // mergeMetadata merges labels and annotations from `existing` map into `required` one where `required` has precedence
 // over `existing` keys and values. Additionally, function performs filtering of labels and annotations from `exiting` map
 // where keys starting from string defined in `metadataPrefix` are deleted. This prevents issues with preserving stale

pkg/manifests/manifests.go

@@ -316,9 +316,6 @@ var (
 	KubeRbacProxyTLSCipherSuitesFlag                     = "--tls-cipher-suites="
 	KubeRbacProxyMinTLSVersionFlag                       = "--tls-min-version="
 
-	kubeStateMetricsCustomResourceStateConfigFileFlag = "--custom-resource-state-config-file="
-	kubeStateMetricsCustomResourceStateConfigFile     = "/etc/kube-state-metrics/custom-resource-state-configmap.yaml"
-
 	AuthProxyExternalURLFlag  = "-external-url="
 	AuthProxyCookieDomainFlag = "-cookie-domain="
 	AuthProxyRedirectURLFlag  = "-redirect-url="
@@ -748,8 +745,7 @@ func (f *Factory) KubeStateMetricsMinimalServiceMonitor() (*monv1.ServiceMonitor
 	return f.NewServiceMonitor(f.assets.MustNewAssetSlice(KubeStateMetricsMinimalServiceMonitor))
 }
 
-func (f *Factory) KubeStateMetricsDeployment(enableCRSMetrics bool) (*appsv1.Deployment, error) {
-	flagCRSConfigFile := kubeStateMetricsCustomResourceStateConfigFileFlag + kubeStateMetricsCustomResourceStateConfigFile
+func (f *Factory) KubeStateMetricsDeployment() (*appsv1.Deployment, error) {
 	d, err := f.NewDeployment(f.assets.MustNewAssetSlice(KubeStateMetricsDeployment))
 	if err != nil {
 		return nil, err
@@ -764,9 +760,6 @@ func (f *Factory) KubeStateMetricsDeployment(enableCRSMetrics bool) (*appsv1.Dep
 			if f.config.ClusterMonitoringConfiguration.KubeStateMetricsConfig.Resources != nil {
 				d.Spec.Template.Spec.Containers[i].Resources = *f.config.ClusterMonitoringConfiguration.KubeStateMetricsConfig.Resources
 			}
-			if enableCRSMetrics {
-				d.Spec.Template.Spec.Containers[i].Args = append(container.Args, flagCRSConfigFile)
-			}
 		}
 	}
 

pkg/manifests/manifests_test.go

@@ -272,7 +272,7 @@ func TestUnconfiguredManifests(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	_, err = f.KubeStateMetricsDeployment(false)
+	_, err = f.KubeStateMetricsDeployment()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -3694,14 +3694,13 @@ func TestKubeStateMetrics(t *testing.T) {
 
 	f := NewFactory("openshift-monitoring", "openshift-user-workload-monitoring", c, defaultInfrastructureReader(), &fakeProxyReader{}, NewAssets(assetsPath), &APIServerConfig{}, &configv1.Console{})
 
-	d, err := f.KubeStateMetricsDeployment(true)
+	d, err := f.KubeStateMetricsDeployment()
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	kubeRbacProxyTLSCipherSuitesArg := ""
 	kubeRbacProxyMinTLSVersionArg := ""
-	gotKubeStateMetricsCustomResourceStateConfigFile := ""
 	for _, container := range d.Spec.Template.Spec.Containers {
 		switch container.Name {
 		case "kube-state-metrics":
@@ -3712,7 +3711,6 @@ func TestKubeStateMetrics(t *testing.T) {
 				t.Fatal("kube-state-metrics resources incorrectly configured")
 			}
 
-			gotKubeStateMetricsCustomResourceStateConfigFile = getContainerArgValue(d.Spec.Template.Spec.Containers, kubeStateMetricsCustomResourceStateConfigFileFlag, container.Name)
 		case "kube-rbac-proxy-self", "kube-rbac-proxy-main":
 			if container.Image != "docker.io/openshift/origin-kube-rbac-proxy:latest" {
 				t.Fatalf("%s image incorrectly configured", container.Name)
@@ -3745,11 +3743,7 @@ func TestKubeStateMetrics(t *testing.T) {
 		t.Fatal("kube-state-metrics topology spread constraints WhenUnsatisfiable not configured correctly")
 	}
 
-	if gotKubeStateMetricsCustomResourceStateConfigFile != kubeStateMetricsCustomResourceStateConfigFileFlag+kubeStateMetricsCustomResourceStateConfigFile {
-		t.Fatal("expected kube-state-metrics to enable custom-resource-state metrics")
-	}
-
-	d2, err := f.KubeStateMetricsDeployment(true)
+	d2, err := f.KubeStateMetricsDeployment()
 	if err != nil {
 		t.Fatal(err)
 	}

pkg/operator/operator.go

@@ -33,7 +33,6 @@ import (
 	"github.com/openshift/library-go/pkg/operator/events"
 	certapiv1 "k8s.io/api/certificates/v1"
 	v1 "k8s.io/api/core/v1"
-	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sruntime "k8s.io/apimachinery/pkg/runtime"
@@ -205,27 +204,6 @@ type Operator struct {
 
 	ruleController    *alert.RuleController
 	relabelController *alert.RelabelConfigController
-
-	// lastKnownVPACustomResourceDefinitionPresent is a boolean pointer that
-	// remembers the presence of the VPA CRD in the cluster between
-	// kube-state-metrics task reconciliations. It is used to determine
-	// whether to enable kube-state-metrics custom-resource-state-based
-	// metrics for VPA CRs, even in cases where the Kube API may emit a
-	// transient error (errors excluding `IsNotFound`) on the VPA CRD `GET`
-	// requests (to determine its presence).
-	// * `true` indicates that the VPA CRD is already present in the
-	// cluster, and the custom-resource-state-based metrics can be safely
-	// enabled.
-	// * `false` indicates that the VPA CRD is not present in the cluster,
-	// and enabling the custom-resource-state-based metrics will cause
-	// kube-state-metrics to error (affecting `KubeStateMetricsListErrors`).
-	// * `nil` indicates that the presence of the VPA CRD is unknown, and
-	// the operator will attempt to determine the presence of the VPA CRD in
-	// the current reconciliation cycle, and remember its state in the next
-	// one. In the case where the VPA CRD is added or removed between
-	// reconciliations, the variable "forgets" it, and is set to `nil`,
-	// triggering a check on the next cycle.
-	lastKnownVPACustomResourceDefinitionPresent *bool
 }
 
 func New(
@@ -446,20 +424,6 @@ func New(
 	}
 	o.informers = append(o.informers, informer)
 
-	informer = cache.NewSharedIndexInformer(
-		o.client.VerticalPodAutoscalerCRDListWatch(ctx),
-		&apiextv1.CustomResourceDefinition{}, resyncPeriod, cache.Indexers{},
-	)
-	// Only trigger reconciliation on the addition or removal of VPA CRDs.
-	_, err = informer.AddEventHandler(cache.ResourceEventHandlerFuncs{
-		AddFunc:    o.handleEvent,
-		DeleteFunc: o.handleEvent,
-	})
-	if err != nil {
-		return nil, err
-	}
-	o.informers = append(o.informers, informer)
-
 	kubeInformersOperatorNS := informers.NewSharedInformerFactoryWithOptions(
 		c.KubernetesInterface(),
 		resyncPeriod,
@@ -667,10 +631,7 @@ func (o *Operator) handleEvent(obj interface{}) {
 		*configv1.APIServer,
 		*configv1.Console,
 		*configv1.ClusterOperator,
-		*configv1.ClusterVersion,
-		// Currently, the CRDs that trigger reconciliation are:
-		// * verticalpodautoscalers.autoscaling.k8s.io
-		*apiextv1.CustomResourceDefinition:
+		*configv1.ClusterVersion:
 		// Log GroupKind and Name of the obj
 		rtObj := obj.(k8sruntime.Object)
 		gk := rtObj.GetObjectKind().GroupVersionKind().GroupKind()
@@ -819,18 +780,6 @@ func (o *Operator) sync(ctx context.Context, key string) error {
 		klog.Warningf("Fail to load ConsoleConfig, AlertManager's externalURL may be outdated")
 	}
 
-	// Enable kube-state-metrics' custom-resource-state-based metrics if VPA CRD is installed within the cluster.
-	o.lastKnownVPACustomResourceDefinitionPresent, err = o.client.VPACustomResourceDefinitionPresent(ctx, o.lastKnownVPACustomResourceDefinitionPresent)
-	if err != nil {
-		// Throw on all transient errors.
-		return fmt.Errorf("unable to guess the desired state for kube-state-metrics' custom-resource-state metrics enablement: %w", err)
-	} else {
-		// If we didn't get an error, we can safely assume that the CRD is deterministically either present or absent.
-		if *o.lastKnownVPACustomResourceDefinitionPresent {
-			klog.Infof("%s CRD found, enabling kube-state-metrics' custom-resource-state-based metrics", client.VerticalPodAutoscalerCRDMetadataName)
-		}
-	}
-
 	factory := manifests.NewFactory(
 		o.namespace,
 		o.namespaceUserWorkload,
@@ -859,7 +808,7 @@ func (o *Operator) sync(ctx context.Context, key string) error {
 				newTaskSpec("Prometheus", tasks.NewPrometheusTask(o.client, factory, config)),
 				newTaskSpec("Alertmanager", tasks.NewAlertmanagerTask(o.client, factory, config)),
 				newTaskSpec("NodeExporter", tasks.NewNodeExporterTask(o.client, factory)),
-				newTaskSpec("KubeStateMetrics", tasks.NewKubeStateMetricsTask(o.client, factory, *o.lastKnownVPACustomResourceDefinitionPresent)),
+				newTaskSpec("KubeStateMetrics", tasks.NewKubeStateMetricsTask(o.client, factory)),
 				newTaskSpec("OpenshiftStateMetrics", tasks.NewOpenShiftStateMetricsTask(o.client, factory)),
 				newTaskSpec("MetricsServer", tasks.NewMetricsServerTask(ctx, o.namespace, o.client, factory, config)),
 				newTaskSpec("TelemeterClient", tasks.NewTelemeterClientTask(o.client, factory, config)),