cmd/k8s-operator,k8s-operator: allow users to set custom labels for the optional ServiceMonitor (tailscale#14475)

* cmd/k8s-operator,k8s-operator: allow users to set custom labels for the optional ServiceMonitor

Updates tailscale#14381

Signed-off-by: Irbe Krumina <[email protected]>

Author: irbekrm

cmd/k8s-operator/connector_test.go

@@ -203,7 +203,7 @@ func TestConnectorWithProxyClass(t *testing.T) {
 	pc := &tsapi.ProxyClass{
 		ObjectMeta: metav1.ObjectMeta{Name: "custom-metadata"},
 		Spec: tsapi.ProxyClassSpec{StatefulSet: &tsapi.StatefulSet{
-			Labels:      map[string]string{"foo": "bar"},
+			Labels:      tsapi.Labels{"foo": "bar"},
 			Annotations: map[string]string{"bar.io/foo": "some-val"},
 			Pod:         &tsapi.Pod{Annotations: map[string]string{"foo.io/bar": "some-val"}}}},
 	}

cmd/k8s-operator/deploy/crds/tailscale.com_proxyclasses.yaml

@@ -99,6 +99,16 @@ spec:
                         enable:
                           description: If Enable is set to true, a Prometheus ServiceMonitor will be created. Enable can only be set to true if metrics are enabled.
                           type: boolean
+                        labels:
+                          description: |-
+                            Labels to add to the ServiceMonitor.
+                            Labels must be valid Kubernetes labels.
+                            https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set
+                          type: object
+                          additionalProperties:
+                            type: string
+                            maxLength: 63
+                            pattern: ^(([a-zA-Z0-9][-._a-zA-Z0-9]*)?[a-zA-Z0-9])?$
                   x-kubernetes-validations:
                     - rule: '!(has(self.serviceMonitor) && self.serviceMonitor.enable  && !self.enable)'
                       message: ServiceMonitor can only be enabled if metrics are enabled
@@ -133,6 +143,8 @@ spec:
                       type: object
                       additionalProperties:
                         type: string
+                        maxLength: 63
+                        pattern: ^(([a-zA-Z0-9][-._a-zA-Z0-9]*)?[a-zA-Z0-9])?$
                     pod:
                       description: Configuration for the proxy Pod.
                       type: object
@@ -1062,6 +1074,8 @@ spec:
                           type: object
                           additionalProperties:
                             type: string
+                            maxLength: 63
+                            pattern: ^(([a-zA-Z0-9][-._a-zA-Z0-9]*)?[a-zA-Z0-9])?$
                         nodeName:
                           description: |-
                             Proxy Pod's node name.

cmd/k8s-operator/deploy/manifests/operator.yaml

@@ -563,6 +563,16 @@ spec:
                                             enable:
                                                 description: If Enable is set to true, a Prometheus ServiceMonitor will be created. Enable can only be set to true if metrics are enabled.
                                                 type: boolean
+                                            labels:
+                                                additionalProperties:
+                                                    maxLength: 63
+                                                    pattern: ^(([a-zA-Z0-9][-._a-zA-Z0-9]*)?[a-zA-Z0-9])?$
+                                                    type: string
+                                                description: |-
+                                                    Labels to add to the ServiceMonitor.
+                                                    Labels must be valid Kubernetes labels.
+                                                    https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set
+                                                type: object
                                         required:
                                             - enable
                                         type: object
@@ -592,6 +602,8 @@ spec:
                                         type: object
                                     labels:
                                         additionalProperties:
+                                            maxLength: 63
+                                            pattern: ^(([a-zA-Z0-9][-._a-zA-Z0-9]*)?[a-zA-Z0-9])?$
                                             type: string
                                         description: |-
                                             Labels that will be added to the StatefulSet created for the proxy.
@@ -1522,6 +1534,8 @@ spec:
                                                 type: array
                                             labels:
                                                 additionalProperties:
+                                                    maxLength: 63
+                                                    pattern: ^(([a-zA-Z0-9][-._a-zA-Z0-9]*)?[a-zA-Z0-9])?$
                                                     type: string
                                                 description: |-
                                                     Labels that will be added to the proxy Pod.

cmd/k8s-operator/ingress_test.go

@@ -295,7 +295,7 @@ func TestTailscaleIngressWithProxyClass(t *testing.T) {
 	pc := &tsapi.ProxyClass{
 		ObjectMeta: metav1.ObjectMeta{Name: "custom-metadata"},
 		Spec: tsapi.ProxyClassSpec{StatefulSet: &tsapi.StatefulSet{
-			Labels:      map[string]string{"foo": "bar"},
+			Labels:      tsapi.Labels{"foo": "bar"},
 			Annotations: map[string]string{"bar.io/foo": "some-val"},
 			Pod:         &tsapi.Pod{Annotations: map[string]string{"foo.io/bar": "some-val"}}}},
 	}
@@ -424,45 +424,14 @@ func TestTailscaleIngressWithProxyClass(t *testing.T) {
 func TestTailscaleIngressWithServiceMonitor(t *testing.T) {
 	pc := &tsapi.ProxyClass{
 		ObjectMeta: metav1.ObjectMeta{Name: "metrics", Generation: 1},
-		Spec: tsapi.ProxyClassSpec{
-			Metrics: &tsapi.Metrics{
-				Enable:         true,
-				ServiceMonitor: &tsapi.ServiceMonitor{Enable: true},
-			},
-		},
+		Spec:       tsapi.ProxyClassSpec{},
 		Status: tsapi.ProxyClassStatus{
 			Conditions: []metav1.Condition{{
 				Status:             metav1.ConditionTrue,
 				Type:               string(tsapi.ProxyClassReady),
 				ObservedGeneration: 1,
 			}}},
 	}
-	crd := &apiextensionsv1.CustomResourceDefinition{ObjectMeta: metav1.ObjectMeta{Name: serviceMonitorCRD}}
-	tsIngressClass := &networkingv1.IngressClass{ObjectMeta: metav1.ObjectMeta{Name: "tailscale"}, Spec: networkingv1.IngressClassSpec{Controller: "tailscale.com/ts-ingress"}}
-	fc := fake.NewClientBuilder().
-		WithScheme(tsapi.GlobalScheme).
-		WithObjects(pc, tsIngressClass).
-		WithStatusSubresource(pc).
-		Build()
-	ft := &fakeTSClient{}
-	fakeTsnetServer := &fakeTSNetServer{certDomains: []string{"foo.com"}}
-	zl, err := zap.NewDevelopment()
-	if err != nil {
-		t.Fatal(err)
-	}
-	ingR := &IngressReconciler{
-		Client: fc,
-		ssr: &tailscaleSTSReconciler{
-			Client:            fc,
-			tsClient:          ft,
-			tsnetServer:       fakeTsnetServer,
-			defaultTags:       []string{"tag:k8s"},
-			operatorNamespace: "operator-ns",
-			proxyImage:        "tailscale/tailscale",
-		},
-		logger: zl.Sugar(),
-	}
-	// 1. Enable metrics- expect metrics Service to be created
 	ing := &networkingv1.Ingress{
 		TypeMeta: metav1.TypeMeta{Kind: "Ingress", APIVersion: "networking.k8s.io/v1"},
 		ObjectMeta: metav1.ObjectMeta{
@@ -491,8 +460,7 @@ func TestTailscaleIngressWithServiceMonitor(t *testing.T) {
 			},
 		},
 	}
-	mustCreate(t, fc, ing)
-	mustCreate(t, fc, &corev1.Service{
+	svc := &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "test",
 			Namespace: "default",
@@ -504,11 +472,38 @@ func TestTailscaleIngressWithServiceMonitor(t *testing.T) {
 				Name: "http"},
 			},
 		},
-	})
-
+	}
+	crd := &apiextensionsv1.CustomResourceDefinition{ObjectMeta: metav1.ObjectMeta{Name: serviceMonitorCRD}}
+	tsIngressClass := &networkingv1.IngressClass{ObjectMeta: metav1.ObjectMeta{Name: "tailscale"}, Spec: networkingv1.IngressClassSpec{Controller: "tailscale.com/ts-ingress"}}
+	fc := fake.NewClientBuilder().
+		WithScheme(tsapi.GlobalScheme).
+		WithObjects(pc, tsIngressClass, ing, svc).
+		WithStatusSubresource(pc).
+		Build()
+	ft := &fakeTSClient{}
+	fakeTsnetServer := &fakeTSNetServer{certDomains: []string{"foo.com"}}
+	zl, err := zap.NewDevelopment()
+	if err != nil {
+		t.Fatal(err)
+	}
+	ingR := &IngressReconciler{
+		Client: fc,
+		ssr: &tailscaleSTSReconciler{
+			Client:            fc,
+			tsClient:          ft,
+			tsnetServer:       fakeTsnetServer,
+			defaultTags:       []string{"tag:k8s"},
+			operatorNamespace: "operator-ns",
+			proxyImage:        "tailscale/tailscale",
+		},
+		logger: zl.Sugar(),
+	}
 	expectReconciled(t, ingR, "default", "test")
-
 	fullName, shortName := findGenName(t, fc, "default", "test", "ingress")
+	serveConfig := &ipn.ServeConfig{
+		TCP: map[uint16]*ipn.TCPPortHandler{443: {HTTPS: true}},
+		Web: map[ipn.HostPort]*ipn.WebServerConfig{"${TS_CERT_DOMAIN}:443": {Handlers: map[string]*ipn.HTTPHandler{"/": {Proxy: "http://1.2.3.4:8080/"}}}},
+	}
 	opts := configOpts{
 		stsName:            shortName,
 		secretName:         fullName,
@@ -517,27 +512,51 @@ func TestTailscaleIngressWithServiceMonitor(t *testing.T) {
 		parentType:         "ingress",
 		hostname:           "default-test",
 		app:                kubetypes.AppIngressResource,
-		enableMetrics:      true,
 		namespaced:         true,
 		proxyType:          proxyTypeIngressResource,
+		serveConfig:        serveConfig,
+		resourceVersion:    "1",
 	}
-	serveConfig := &ipn.ServeConfig{
-		TCP: map[uint16]*ipn.TCPPortHandler{443: {HTTPS: true}},
-		Web: map[ipn.HostPort]*ipn.WebServerConfig{"${TS_CERT_DOMAIN}:443": {Handlers: map[string]*ipn.HTTPHandler{"/": {Proxy: "http://1.2.3.4:8080/"}}}},
-	}
-	opts.serveConfig = serveConfig
 
-	expectEqual(t, fc, expectedSecret(t, fc, opts), nil)
-	expectEqual(t, fc, expectedHeadlessService(shortName, "ingress"), nil)
+	// 1. Enable metrics- expect metrics Service to be created
+	mustUpdate(t, fc, "", "metrics", func(proxyClass *tsapi.ProxyClass) {
+		proxyClass.Spec.Metrics = &tsapi.Metrics{Enable: true}
+	})
+	opts.enableMetrics = true
+
+	expectReconciled(t, ingR, "default", "test")
+
 	expectEqual(t, fc, expectedMetricsService(opts), nil)
-	expectEqual(t, fc, expectedSTSUserspace(t, fc, opts), removeHashAnnotation)
+
 	// 2. Enable ServiceMonitor - should not error when there is no ServiceMonitor CRD in cluster
 	mustUpdate(t, fc, "", "metrics", func(pc *tsapi.ProxyClass) {
-		pc.Spec.Metrics.ServiceMonitor = &tsapi.ServiceMonitor{Enable: true}
+		pc.Spec.Metrics.ServiceMonitor = &tsapi.ServiceMonitor{Enable: true, Labels: tsapi.Labels{"foo": "bar"}}
 	})
 	expectReconciled(t, ingR, "default", "test")
+	expectEqual(t, fc, expectedMetricsService(opts), nil)
+
 	// 3. Create ServiceMonitor CRD and reconcile- ServiceMonitor should get created
 	mustCreate(t, fc, crd)
 	expectReconciled(t, ingR, "default", "test")
+	opts.serviceMonitorLabels = tsapi.Labels{"foo": "bar"}
+	expectEqual(t, fc, expectedMetricsService(opts), nil)
+	expectEqualUnstructured(t, fc, expectedServiceMonitor(t, opts))
+
+	// 4. Update ServiceMonitor CRD and reconcile- ServiceMonitor should get updated
+	mustUpdate(t, fc, pc.Namespace, pc.Name, func(proxyClass *tsapi.ProxyClass) {
+		proxyClass.Spec.Metrics.ServiceMonitor.Labels = nil
+	})
+	expectReconciled(t, ingR, "default", "test")
+	opts.serviceMonitorLabels = nil
+	opts.resourceVersion = "2"
+	expectEqual(t, fc, expectedMetricsService(opts), nil)
 	expectEqualUnstructured(t, fc, expectedServiceMonitor(t, opts))
+
+	// 5. Disable metrics - metrics resources should get deleted.
+	mustUpdate(t, fc, pc.Namespace, pc.Name, func(proxyClass *tsapi.ProxyClass) {
+		proxyClass.Spec.Metrics = nil
+	})
+	expectReconciled(t, ingR, "default", "test")
+	expectMissing[corev1.Service](t, fc, "operator-ns", metricsResourceName(shortName))
+	// ServiceMonitor gets garbage collected when the Service is deleted - we cannot test that here.
 }

cmd/k8s-operator/metrics_resources.go

@@ -8,6 +8,7 @@ package main
 import (
 	"context"
 	"fmt"
+	"reflect"
 
 	"go.uber.org/zap"
 	corev1 "k8s.io/api/core/v1"
@@ -115,15 +116,15 @@ func reconcileMetricsResources(ctx context.Context, logger *zap.SugaredLogger, o
 		return maybeCleanupServiceMonitor(ctx, cl, opts.proxyStsName, opts.tsNamespace)
 	}
 
-	logger.Info("ensuring ServiceMonitor for metrics Service %s/%s", metricsSvc.Namespace, metricsSvc.Name)
-	svcMonitor, err := newServiceMonitor(metricsSvc)
+	logger.Infof("ensuring ServiceMonitor for metrics Service %s/%s", metricsSvc.Namespace, metricsSvc.Name)
+	svcMonitor, err := newServiceMonitor(metricsSvc, pc.Spec.Metrics.ServiceMonitor)
 	if err != nil {
 		return fmt.Errorf("error creating ServiceMonitor: %w", err)
 	}
-	// We don't use createOrUpdate here because that does not work with unstructured types. We also do not update
-	// the ServiceMonitor because it is not expected that any of its fields would change. Currently this is good
-	// enough, but in future we might want to add logic to create-or-update unstructured types.
-	err = cl.Get(ctx, client.ObjectKeyFromObject(metricsSvc), svcMonitor.DeepCopy())
+
+	// We don't use createOrUpdate here because that does not work with unstructured types.
+	existing := svcMonitor.DeepCopy()
+	err = cl.Get(ctx, client.ObjectKeyFromObject(metricsSvc), existing)
 	if apierrors.IsNotFound(err) {
 		if err := cl.Create(ctx, svcMonitor); err != nil {
 			return fmt.Errorf("error creating ServiceMonitor: %w", err)
@@ -133,6 +134,13 @@ func reconcileMetricsResources(ctx context.Context, logger *zap.SugaredLogger, o
 	if err != nil {
 		return fmt.Errorf("error getting ServiceMonitor: %w", err)
 	}
+	// Currently, we only update labels on the ServiceMonitor as those are the only values that can change.
+	if !reflect.DeepEqual(existing.GetLabels(), svcMonitor.GetLabels()) {
+		existing.SetLabels(svcMonitor.GetLabels())
+		if err := cl.Update(ctx, existing); err != nil {
+			return fmt.Errorf("error updating ServiceMonitor: %w", err)
+		}
+	}
 	return nil
 }
 
@@ -165,9 +173,13 @@ func maybeCleanupServiceMonitor(ctx context.Context, cl client.Client, stsName,
 // newServiceMonitor takes a metrics Service created for a proxy and constructs and returns a ServiceMonitor for that
 // proxy that can be applied to the kube API server.
 // The ServiceMonitor is returned as Unstructured type - this allows us to avoid importing prometheus-operator API server client/schema.
-func newServiceMonitor(metricsSvc *corev1.Service) (*unstructured.Unstructured, error) {
+func newServiceMonitor(metricsSvc *corev1.Service, spec *tsapi.ServiceMonitor) (*unstructured.Unstructured, error) {
 	sm := serviceMonitorTemplate(metricsSvc.Name, metricsSvc.Namespace)
 	sm.ObjectMeta.Labels = metricsSvc.Labels
+	if spec != nil && len(spec.Labels) > 0 {
+		sm.ObjectMeta.Labels = mergeMapKeys(sm.ObjectMeta.Labels, spec.Labels.Parse())
+	}
+
 	sm.ObjectMeta.OwnerReferences = []metav1.OwnerReference{*metav1.NewControllerRef(metricsSvc, corev1.SchemeGroupVersion.WithKind("Service"))}
 	sm.Spec = ServiceMonitorSpec{
 		Selector: metav1.LabelSelector{MatchLabels: metricsSvc.Labels},
@@ -270,3 +282,14 @@ type metricsOpts struct {
 func isNamespacedProxyType(typ string) bool {
 	return typ == proxyTypeIngressResource || typ == proxyTypeIngressService
 }
+
+func mergeMapKeys(a, b map[string]string) map[string]string {
+	m := make(map[string]string, len(a)+len(b))
+	for key, val := range b {
+		m[key] = val
+	}
+	for key, val := range a {
+		m[key] = val
+	}
+	return m
+}