Merge pull request #3008 from vmware/rr/impersonator-service-labels

impersonator config controller ignores Service labels added by others

internal/controller/impersonatorconfig/impersonator_config.go

@@ -1,4 +1,4 @@
-// Copyright 2021-2025 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2026 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package impersonatorconfig
@@ -11,8 +11,9 @@ import (
 	"encoding/json"
 	"encoding/pem"
 	"fmt"
+	"maps"
 	"net"
-	"sort"
+	"slices"
 	"strings"
 	"time"
 
@@ -56,6 +57,7 @@ const (
 	caKeyKey                     = "ca.key"
 	appLabelKey                  = "app"
 	annotationKeysKey            = "credentialissuer.pinniped.dev/annotation-keys"
+	labelKeysKey                 = "credentialissuer.pinniped.dev/label-keys"
 )
 
 type impersonatorConfigController struct {
@@ -626,6 +628,62 @@ func (c *impersonatorConfigController) ensureClusterIPServiceIsStopped(ctx conte
 	return utilerrors.FilterOut(err, apierrors.IsNotFound)
 }
 
+// recordDesiredKeysAsBookkeepingAnnotation adds a bookkeeping annotation to the given service that records which
+// keys were explicitly desired by this controller. This allows future runs to detect which keys have been removed
+// from the desired state (so they can be cleaned up) vs. which keys belong to other actors (which should
+// be left alone). The bookkeepingKey is the annotation key under which the JSON-encoded list of desired keys will
+// be stored. If the desiredMap is empty, no bookkeeping entry is added.
+func recordDesiredKeysAsBookkeepingAnnotation(service *corev1.Service, desiredMap map[string]string, bookkeepingKey string) error {
+	desiredKeys := slices.Sorted(maps.Keys(desiredMap))
+
+	if len(desiredKeys) > 0 {
+		keysJSONArray, err := json.Marshal(desiredKeys)
+		if err != nil {
+			return err // This shouldn't really happen. We should always be able to marshal an array of strings.
+		}
+
+		// Save the desired keys to a bookkeeping annotation on the service.
+		if service.Annotations == nil {
+			service.Annotations = map[string]string{}
+		}
+		service.Annotations[bookkeepingKey] = string(keysJSONArray)
+	}
+
+	return nil
+}
+
+func mergeMap(existingMap map[string]string, desiredMap map[string]string, existingBookkeepingSource map[string]string, bookkeepingKey string) map[string]string {
+	resultMap := maps.Clone(existingMap)
+	if resultMap == nil {
+		resultMap = map[string]string{}
+	}
+
+	// Merge desired into existing, with desired overwriting when there are conflicts.
+	for k, v := range desiredMap {
+		resultMap[k] = v
+	}
+
+	// Check if the existing Service contains a record of previous keys that were added by this controller.
+	// Note that in an upgrade, older versions of Pinniped might have created the Service without this bookkeeping entry.
+	oldDesiredKeysJSON, foundOldDesiredKeysJSON := existingBookkeepingSource[bookkeepingKey]
+	oldDesiredKeys := []string{}
+	if foundOldDesiredKeysJSON {
+		_ = json.Unmarshal([]byte(oldDesiredKeysJSON), &oldDesiredKeys)
+		// In the unlikely event that we cannot parse the value of our bookkeeping entry, just act like it
+		// wasn't present and update it to the new value that it should have based on the current desired state.
+	}
+
+	// Check if any keys which were previously managed by this controller are now gone from the desired state,
+	// which means that those now-missing keys should get deleted.
+	for _, oldKey := range oldDesiredKeys {
+		if _, existsInDesired := desiredMap[oldKey]; !existsInDesired {
+			delete(resultMap, oldKey)
+		}
+	}
+
+	return resultMap
+}
+
 func (c *impersonatorConfigController) createOrUpdateService(ctx context.Context, desiredService *corev1.Service) error {
 	log := c.log.WithValues("serviceType", desiredService.Spec.Type, "service", klog.KObj(desiredService))
 
@@ -634,19 +692,15 @@ func (c *impersonatorConfigController) createOrUpdateService(ctx context.Context
 	// to be able to detect that the missing key means that we should remove the key. This is needed to
 	// differentiate it from a key that was added by another actor, which we should not remove.
 	// But don't bother recording the requested annotations if there were no annotations requested.
-	desiredAnnotationKeys := make([]string, 0, len(desiredService.Annotations))
-	for k := range desiredService.Annotations {
-		desiredAnnotationKeys = append(desiredAnnotationKeys, k)
+	err := recordDesiredKeysAsBookkeepingAnnotation(desiredService, desiredService.Annotations, annotationKeysKey)
+	if err != nil {
+		return err
 	}
-	if len(desiredAnnotationKeys) > 0 {
-		// Sort them since they come out of the map in no particular order.
-		sort.Strings(desiredAnnotationKeys)
-		keysJSONArray, err := json.Marshal(desiredAnnotationKeys)
-		if err != nil {
-			return err // This shouldn't really happen. We should always be able to marshal an array of strings.
-		}
-		// Save the desired annotations to a bookkeeping annotation.
-		desiredService.Annotations[annotationKeysKey] = string(keysJSONArray)
+
+	// Similarly, prepare to remember which label keys were added by this controller.
+	err = recordDesiredKeysAsBookkeepingAnnotation(desiredService, desiredService.Labels, labelKeysKey)
+	if err != nil {
+		return err
 	}
 
 	// Get the Service from the informer, and create it if it does not already exist.
@@ -662,49 +716,35 @@ func (c *impersonatorConfigController) createOrUpdateService(ctx context.Context
 
 	// The Service already exists, so update only the specific fields that are meaningfully part of our desired state.
 	updatedService := existingService.DeepCopy()
-	updatedService.Labels = desiredService.Labels
 	updatedService.Spec.LoadBalancerIP = desiredService.Spec.LoadBalancerIP
 	updatedService.Spec.Type = desiredService.Spec.Type
 	updatedService.Spec.Selector = desiredService.Spec.Selector
 
 	// Do not simply overwrite the existing annotations with the desired annotations. Instead, merge-overwrite.
 	// Another actor in the system, like a human user or a non-Pinniped controller, might have updated the
-	// existing Service's annotations. If they did, then we do not want to overwrite those keys expect for
+	// existing Service's annotations. If they did, then we do not want to overwrite those keys except for
 	// the specific keys that are from the CredentialIssuer's spec, because if we overwrite keys belonging
 	// to another controller then we could end up infinitely flapping back and forth with the other controller,
 	// both updating that annotation on the Service.
-	if updatedService.Annotations == nil {
-		updatedService.Annotations = map[string]string{}
-	}
-	for k, v := range desiredService.Annotations {
-		updatedService.Annotations[k] = v
-	}
-
-	// Check if the the existing Service contains a record of previous annotations that were added by this controller.
-	// Note that in an upgrade, older versions of Pinniped might have created the Service without this bookkeeping annotation.
-	oldDesiredAnnotationKeysJSON, foundOldDesiredAnnotationKeysJSON := existingService.Annotations[annotationKeysKey]
-	oldDesiredAnnotationKeys := []string{}
-	if foundOldDesiredAnnotationKeysJSON {
-		_ = json.Unmarshal([]byte(oldDesiredAnnotationKeysJSON), &oldDesiredAnnotationKeys)
-		// In the unlikely event that we cannot parse the value of our bookkeeping annotation, just act like it
-		// wasn't present and update it to the new value that it should have based on the current desired state.
-	}
-
-	// Check if any annotations which were previously in the CredentialIssuer spec are now gone from the spec,
-	// which means that those now-missing annotations should get deleted.
-	for _, oldKey := range oldDesiredAnnotationKeys {
-		if _, existsInDesired := desiredService.Annotations[oldKey]; !existsInDesired {
-			delete(updatedService.Annotations, oldKey)
-		}
-	}
+	updatedService.Annotations = mergeMap(existingService.Annotations, desiredService.Annotations, existingService.Annotations, annotationKeysKey)
 
 	// If no annotations were requested, then remove the special bookkeeping annotation which might be
 	// leftover from a previous update. During the next update, non-existence will be taken to mean
 	// that no annotations were previously requested by the CredentialIssuer spec.
-	if len(desiredAnnotationKeys) == 0 {
+	if len(desiredService.Annotations) == 0 || mapHasExactlyOneKey(desiredService.Annotations, labelKeysKey) {
 		delete(updatedService.Annotations, annotationKeysKey)
 	}
 
+	// Same merge strategy as above, but for labels this time.
+	updatedService.Labels = mergeMap(existingService.Labels, desiredService.Labels, existingService.Annotations, labelKeysKey)
+
+	// If no labels were requested, then remove the special bookkeeping annotation which might be
+	// leftover from a previous update. During the next update, non-existence will be taken to mean
+	// that no labels were previously requested by this controller.
+	if len(desiredService.Labels) == 0 {
+		delete(updatedService.Annotations, labelKeysKey)
+	}
+
 	// If our updates didn't change anything, we're done.
 	if equality.Semantic.DeepEqual(existingService, updatedService) {
 		return nil
@@ -1222,3 +1262,11 @@ func validateCredentialIssuerSpec(spec *conciergeconfigv1alpha1.ImpersonationPro
 
 	return nil
 }
+
+func mapHasExactlyOneKey(m map[string]string, key string) bool {
+	if len(m) != 1 {
+		return false
+	}
+	_, ok := m[key]
+	return ok
+}

internal/controller/impersonatorconfig/impersonator_config_test.go

@@ -1,4 +1,4 @@
-// Copyright 2021-2025 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2026 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package impersonatorconfig
@@ -12,6 +12,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"maps"
 	"net"
 	"net/http"
 	"reflect"
@@ -684,9 +685,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 		var newLoadBalancerService = func(resourceName string, status corev1.ServiceStatus) *corev1.Service {
 			return &corev1.Service{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      resourceName,
-					Namespace: installedInNamespace,
-					Labels:    labels,
+					Name:        resourceName,
+					Namespace:   installedInNamespace,
+					Labels:      labels,
+					Annotations: map[string]string{labelKeysKey: `["app","other-key"]`},
 				},
 				Spec: corev1.ServiceSpec{
 					Type: corev1.ServiceTypeLoadBalancer,
@@ -706,9 +708,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 		var newClusterIPService = func(resourceName string, status corev1.ServiceStatus, spec corev1.ServiceSpec) *corev1.Service {
 			return &corev1.Service{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      resourceName,
-					Namespace: installedInNamespace,
-					Labels:    labels,
+					Name:        resourceName,
+					Namespace:   installedInNamespace,
+					Labels:      labels,
+					Annotations: map[string]string{labelKeysKey: `["app","other-key"]`},
 				},
 				Spec:   spec,
 				Status: status,
@@ -1020,7 +1023,6 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			r.Equal(installedInNamespace, updatedLoadBalancerService.Namespace)
 			r.Equal(corev1.ServiceTypeLoadBalancer, updatedLoadBalancerService.Spec.Type)
 			r.Equal("app-name", updatedLoadBalancerService.Spec.Selector["app"])
-			r.Equal(labels, updatedLoadBalancerService.Labels)
 			return updatedLoadBalancerService
 		}
 
@@ -1045,7 +1047,6 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			r.Equal(installedInNamespace, updatedLoadBalancerService.Namespace)
 			r.Equal(corev1.ServiceTypeClusterIP, updatedLoadBalancerService.Spec.Type)
 			r.Equal("app-name", updatedLoadBalancerService.Spec.Selector["app"])
-			r.Equal(labels, updatedLoadBalancerService.Labels)
 			return updatedLoadBalancerService
 		}
 
@@ -1078,12 +1079,12 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			r.NoError(err, "key does not match cert")
 			// Decode and parse the cert to check some of its fields.
 			block, _ := pem.Decode(createdCertPEM)
-			require.NotNil(t, block)
+			r.NotNil(block)
 			caCert, err := x509.ParseCertificate(block.Bytes)
-			require.NoError(t, err)
-			require.Equal(t, "Pinniped Impersonation Proxy Serving CA", caCert.Subject.CommonName)
-			require.WithinDuration(t, time.Now().Add(-5*time.Minute), caCert.NotBefore, 10*time.Second)
-			require.WithinDuration(t, time.Now().Add(100*time.Hour*24*365), caCert.NotAfter, 10*time.Second)
+			r.NoError(err)
+			r.Equal("Pinniped Impersonation Proxy Serving CA", caCert.Subject.CommonName)
+			r.WithinDuration(time.Now().Add(-5*time.Minute), caCert.NotBefore, 10*time.Second)
+			r.WithinDuration(time.Now().Add(100*time.Hour*24*365), caCert.NotAfter, 10*time.Second)
 			return createdCertPEM
 		}
 
@@ -1985,9 +1986,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					r.Len(kubeAPIClient.Actions(), 3)
 					requireNodesListed(kubeAPIClient.Actions()[0])
 					lbService := requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
-					require.Equal(t, lbService.Annotations, map[string]string{
+					r.Equal(lbService.Annotations, map[string]string{
 						"some-annotation-key":                           "some-annotation-value",
 						"credentialissuer.pinniped.dev/annotation-keys": `["some-annotation-key"]`,
+						"credentialissuer.pinniped.dev/label-keys":      `["app","other-key"]`,
 					})
 					requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 					requireTLSServerIsRunningWithoutCerts()
@@ -2180,7 +2182,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					r.Len(kubeAPIClient.Actions(), 4)
 					requireNodesListed(kubeAPIClient.Actions()[0])
 					lbService := requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
-					require.Equal(t, lbService.Spec.LoadBalancerIP, localhostIP)
+					r.Equal(lbService.Spec.LoadBalancerIP, localhostIP)
 					ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 					requireTLSSecretWasCreated(kubeAPIClient.Actions()[3], ca)
 					// Check that the server is running and that TLS certs that are being served are are for fakeHostnameWithPort.
@@ -2755,7 +2757,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			})
 		})
 
-		when("requesting a load balancer via CredentialIssuer, then updating the annotations", func() {
+		when("requesting a load balancer via CredentialIssuer, then updating the annotations and labels", func() {
 			it.Before(func() {
 				addSecretToTrackers(mTLSClientCertCASecret, kubeInformerClient)
 				addCredentialIssuerToTrackers(conciergeconfigv1alpha1.CredentialIssuer{
@@ -2781,7 +2783,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.Len(kubeAPIClient.Actions(), 4)
 				requireNodesListed(kubeAPIClient.Actions()[0])
 				lbService := requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
-				require.Equal(t, map[string]string(nil), lbService.Annotations) // there should be no annotations at first
+				r.Equal(map[string]string{"credentialissuer.pinniped.dev/label-keys": `["app","other-key"]`}, lbService.Annotations)
 				ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 				requireTLSSecretWasCreated(kubeAPIClient.Actions()[3], ca)
 				requireTLSServerIsRunning(ca, testServerAddr(), nil)
@@ -2789,11 +2791,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				requireMTLSClientCertProviderHasLoadedCerts(mTLSClientCertCACertPEM, mTLSClientCertCAPrivateKeyPEM)
 
 				// Simulate another actor in the system, like a human user or a non-Pinniped controller,
-				// updating the new Service's annotations. The map was nil, so we can overwrite the whole thing,
-				lbService.Annotations = map[string]string{
-					"annotation-from-unrelated-controller-key": "annotation-from-unrelated-controller-val",
-					"my-annotation-key":                        "my-annotation-from-unrelated-controller-val",
-				}
+				// updating the new Service's annotations and labels to add some more.
+				lbService.Annotations["annotation-from-unrelated-controller-key"] = "annotation-from-unrelated-controller-val"
+				lbService.Annotations["my-annotation-key"] = "my-annotation-from-unrelated-controller-val"
+				lbService.Labels["my-label-key"] = "my-label-from-unrelated-controller-val"
 
 				// Simulate the informer cache's background update from its watch.
 				addObjectToKubeInformerAndWait(lbService, kubeInformers.Core().V1().Services())
@@ -2819,13 +2820,17 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.NoError(runControllerSync())
 				r.Len(kubeAPIClient.Actions(), 5) // one more item to update the loadbalancer
 				lbService = requireLoadBalancerWasUpdated(kubeAPIClient.Actions()[4])
-				require.Equal(t, map[string]string{
+				wantLabels := maps.Clone(labels)
+				wantLabels["my-label-key"] = "my-label-from-unrelated-controller-val"
+				r.Equal(wantLabels, lbService.Labels)
+				r.Equal(map[string]string{
 					// Now the CredentialIssuer annotations should be merged on the load balancer.
 					// In the unlikely case where keys conflict, the CredentialIssuer value overwrites the other value.
 					// Otherwise the annotations from the other actor should not be modified.
 					"annotation-from-unrelated-controller-key":      "annotation-from-unrelated-controller-val",
 					"my-annotation-key":                             "my-annotation-val",
 					"credentialissuer.pinniped.dev/annotation-keys": `["my-annotation-key"]`,
+					"credentialissuer.pinniped.dev/label-keys":      `["app","other-key"]`,
 				}, lbService.Annotations)
 				requireTLSServerIsRunning(ca, testServerAddr(), nil)
 				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
@@ -2833,7 +2838,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			})
 		})
 
-		when("requesting a cluster ip via CredentialIssuer, then updating the annotations", func() {
+		when("requesting a cluster ip via CredentialIssuer, then updating the annotations and labels", func() {
 			it.Before(func() {
 				addSecretToTrackers(mTLSClientCertCASecret, kubeInformerClient)
 				addCredentialIssuerToTrackers(conciergeconfigv1alpha1.CredentialIssuer{
@@ -2859,7 +2864,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.Len(kubeAPIClient.Actions(), 4)
 				requireNodesListed(kubeAPIClient.Actions()[0])
 				clusterIPService := requireClusterIPWasCreated(kubeAPIClient.Actions()[1])
-				require.Equal(t, map[string]string(nil), clusterIPService.Annotations) // there should be no annotations at first
+				r.Equal(map[string]string{"credentialissuer.pinniped.dev/label-keys": `["app","other-key"]`}, clusterIPService.Annotations)
 				ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 				requireTLSSecretWasCreated(kubeAPIClient.Actions()[3], ca)
 				requireTLSServerIsRunning(ca, testServerAddr(), nil)
@@ -2867,11 +2872,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				requireMTLSClientCertProviderHasLoadedCerts(mTLSClientCertCACertPEM, mTLSClientCertCAPrivateKeyPEM)
 
 				// Simulate another actor in the system, like a human user or a non-Pinniped controller,
-				// updating the new Service's annotations.
-				clusterIPService.Annotations = map[string]string{
-					"annotation-from-unrelated-controller-key": "annotation-from-unrelated-controller-val",
-					"my-annotation-key":                        "my-annotation-from-unrelated-controller-val",
-				}
+				// updating the new Service's annotations and labels to add some more.
+				clusterIPService.Annotations["annotation-from-unrelated-controller-key"] = "annotation-from-unrelated-controller-val"
+				clusterIPService.Annotations["my-annotation-key"] = "my-annotation-from-unrelated-controller-val"
+				clusterIPService.Labels["my-label-key"] = "my-label-from-unrelated-controller-val"
 
 				// Simulate the informer cache's background update from its watch.
 				addObjectToKubeInformerAndWait(clusterIPService, kubeInformers.Core().V1().Services())
@@ -2897,13 +2901,17 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.NoError(runControllerSync())
 				r.Len(kubeAPIClient.Actions(), 5) // one more item to update the loadbalancer
 				clusterIPService = requireClusterIPWasUpdated(kubeAPIClient.Actions()[4])
-				require.Equal(t, map[string]string{
+				wantLabels := maps.Clone(labels)
+				wantLabels["my-label-key"] = "my-label-from-unrelated-controller-val"
+				r.Equal(wantLabels, clusterIPService.Labels)
+				r.Equal(map[string]string{
 					// Now the CredentialIssuer annotations should be merged on the load balancer.
 					// In the unlikely case where keys conflict, the CredentialIssuer value overwrites the other value.
 					// Otherwise the annotations from the other actor should not be modified.
 					"annotation-from-unrelated-controller-key":      "annotation-from-unrelated-controller-val",
 					"my-annotation-key":                             "my-annotation-val",
 					"credentialissuer.pinniped.dev/annotation-keys": `["my-annotation-key"]`,
+					"credentialissuer.pinniped.dev/label-keys":      `["app","other-key"]`,
 				}, clusterIPService.Annotations)
 				requireTLSServerIsRunning(ca, testServerAddr(), nil)
 				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
@@ -2942,11 +2950,12 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.Len(kubeAPIClient.Actions(), 4)
 				requireNodesListed(kubeAPIClient.Actions()[0])
 				lbService := requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
-				require.Equal(t, map[string]string{
+				r.Equal(map[string]string{
 					"my-initial-annotation1-key":                    "my-initial-annotation1-val",
 					"my-initial-annotation2-key":                    "my-initial-annotation2-val",
 					"my-initial-annotation3-key":                    "my-initial-annotation3-val",
 					"credentialissuer.pinniped.dev/annotation-keys": `["my-initial-annotation1-key","my-initial-annotation2-key","my-initial-annotation3-key"]`,
+					"credentialissuer.pinniped.dev/label-keys":      `["app","other-key"]`,
 				}, lbService.Annotations) // there should be some annotations at first
 				ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 				requireTLSSecretWasCreated(kubeAPIClient.Actions()[3], ca)
@@ -2984,7 +2993,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.NoError(runControllerSync())
 				r.Len(kubeAPIClient.Actions(), 5) // one more item to update the loadbalancer
 				lbService = requireLoadBalancerWasUpdated(kubeAPIClient.Actions()[4])
-				require.Equal(t, map[string]string{
+				r.Equal(labels, lbService.Labels)
+				r.Equal(map[string]string{
 					// Now the CredentialIssuer annotations should be merged on the load balancer.
 					// Since the user removed the "my-initial-annotation2-key" key from the CredentialIssuer spec,
 					// it should be removed from the Service.
@@ -2993,6 +3003,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					"my-initial-annotation1-key":                    "my-initial-annotation1-val",
 					"my-initial-annotation3-key":                    "my-initial-annotation3-val",
 					"credentialissuer.pinniped.dev/annotation-keys": `["my-initial-annotation1-key","my-initial-annotation3-key"]`,
+					"credentialissuer.pinniped.dev/label-keys":      `["app","other-key"]`,
 				}, lbService.Annotations)
 				requireTLSServerIsRunning(ca, testServerAddr(), nil)
 				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
@@ -3013,11 +3024,14 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.NoError(runControllerSync())
 				r.Len(kubeAPIClient.Actions(), 6) // one more item to update the loadbalancer
 				lbService = requireLoadBalancerWasUpdated(kubeAPIClient.Actions()[5])
-				require.Equal(t, map[string]string{
+				r.Equal(labels, lbService.Labels)
+				r.Equal(map[string]string{
 					// Since the user removed all annotations from the CredentialIssuer spec,
 					// they should all be removed from the Service, along with the special bookkeeping annotation too.
 					// The annotations from the other actor should not be modified.
 					"annotation-from-unrelated-controller-key": "annotation-from-unrelated-controller-val",
+					// The special bookkeeping annotation for labels should not be modified.
+					"credentialissuer.pinniped.dev/label-keys": `["app","other-key"]`,
 				}, lbService.Annotations)
 				requireTLSServerIsRunning(ca, testServerAddr(), nil)
 				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
@@ -3047,6 +3061,9 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				loadBalancerService.Annotations = map[string]string{
 					annotationKeysKey: `["this is not valid json`,
 				}
+				loadBalancerService.Annotations = map[string]string{
+					labelKeysKey: `["this is not valid json`,
+				}
 				addServiceToTrackers(loadBalancerService, kubeInformerClient, kubeAPIClient)
 			})
 
@@ -3058,9 +3075,11 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.Len(kubeAPIClient.Actions(), 4)
 				requireNodesListed(kubeAPIClient.Actions()[0])
 				lbService := requireLoadBalancerWasUpdated(kubeAPIClient.Actions()[1])
-				require.Equal(t, map[string]string{
+				r.Equal(labels, lbService.Labels)
+				r.Equal(map[string]string{
 					"some-annotation": "annotation-value",
 					"credentialissuer.pinniped.dev/annotation-keys": `["some-annotation"]`,
+					"credentialissuer.pinniped.dev/label-keys":      `["app","other-key"]`,
 				}, lbService.Annotations)
 				ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 				requireTLSSecretWasCreated(kubeAPIClient.Actions()[3], ca)
@@ -3096,8 +3115,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.Len(kubeAPIClient.Actions(), 4)
 				requireNodesListed(kubeAPIClient.Actions()[0])
 				lbService := requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
-				require.Equal(t, map[string]string(nil), lbService.Annotations) // there should be no annotations at first
-				require.Equal(t, "", lbService.Spec.LoadBalancerIP)
+				r.Equal(map[string]string{"credentialissuer.pinniped.dev/label-keys": `["app","other-key"]`}, lbService.Annotations)
+				r.Equal("", lbService.Spec.LoadBalancerIP)
 				ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 				requireTLSSecretWasCreated(kubeAPIClient.Actions()[3], ca)
 				requireTLSServerIsRunning(ca, testServerAddr(), nil)
@@ -3109,7 +3128,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[2], kubeInformers.Core().V1().Secrets())
 				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[3], kubeInformers.Core().V1().Secrets())
 
-				// Add annotations to the spec.
+				// Add IP to the spec.
 				loadBalancerIP := "1.2.3.4"
 				updateCredentialIssuerInInformerAndWait(credentialIssuerResourceName, conciergeconfigv1alpha1.CredentialIssuerSpec{
 					ImpersonationProxy: &conciergeconfigv1alpha1.ImpersonationProxySpec{
@@ -3125,7 +3144,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.NoError(runControllerSync())
 				r.Len(kubeAPIClient.Actions(), 5) // one more item to update the loadbalancer
 				lbService = requireLoadBalancerWasUpdated(kubeAPIClient.Actions()[4])
-				require.Equal(t, loadBalancerIP, lbService.Spec.LoadBalancerIP)
+				r.Equal(labels, lbService.Labels)
+				r.Equal(loadBalancerIP, lbService.Spec.LoadBalancerIP)
 				requireTLSServerIsRunning(ca, testServerAddr(), nil)
 				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
 				requireMTLSClientCertProviderHasLoadedCerts(mTLSClientCertCACertPEM, mTLSClientCertCAPrivateKeyPEM)

test/integration/concierge_impersonation_proxy_test.go

@@ -18,6 +18,7 @@ import (
 	"encoding/pem"
 	"fmt"
 	"io"
+	"maps"
 	"net"
 	"net/http"
 	"net/url"
@@ -1626,13 +1627,13 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 
 		// Use this string in all annotation keys added by this test, so the assertions can ignore annotation keys
 		// which might exist on the Service which are not related to this test.
-		recognizableAnnotationKeyString := "pinniped.dev"
+		recognizableKeyString := "pinniped.dev"
 
 		// Grab the state of the CredentialIssuer prior to this test, so we can restore things back afterwards.
 		previous, err := adminConciergeClient.ConfigV1alpha1().CredentialIssuers().Get(ctx, credentialIssuerName(env), metav1.GetOptions{})
 		require.NoError(t, err)
 
-		updateServiceAnnotations := func(annotations map[string]string) {
+		updateServiceAnnotationsAndLabels := func(annotations map[string]string, labels map[string]string) {
 			require.NoError(t, retry.RetryOnConflict(retry.DefaultRetry, func() error {
 				service, err := adminClient.CoreV1().Services(env.ConciergeNamespace).Get(ctx, impersonationProxyLoadBalancerName(env), metav1.GetOptions{})
 				if err != nil {
@@ -1642,21 +1643,28 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 				if updated.Annotations == nil {
 					updated.Annotations = map[string]string{}
 				}
+				if updated.Labels == nil {
+					updated.Labels = map[string]string{}
+				}
 				// Add/update each requested annotation, without overwriting others that are already there.
 				for k, v := range annotations {
 					updated.Annotations[k] = v
 				}
+				// Add/update each requested label, without overwriting others that are already there.
+				for k, v := range labels {
+					updated.Labels[k] = v
+				}
 				if equality.Semantic.DeepEqual(service, updated) {
 					return nil
 				}
 
-				t.Logf("updating Service with annotations: %v", annotations)
+				t.Logf("updating Service with annotations %v and labels %v", annotations, labels)
 				_, err = adminClient.CoreV1().Services(env.ConciergeNamespace).Update(ctx, updated, metav1.UpdateOptions{})
 				return err
 			}))
 		}
 
-		deleteServiceAnnotations := func(annotations map[string]string) {
+		deleteServiceAnnotationsAndLabels := func(annotations map[string]string, labels map[string]string) {
 			require.NoError(t, retry.RetryOnConflict(retry.DefaultRetry, func() error {
 				service, err := adminClient.CoreV1().Services(env.ConciergeNamespace).Get(ctx, impersonationProxyLoadBalancerName(env), metav1.GetOptions{})
 				if err != nil {
@@ -1668,11 +1676,16 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 						delete(updated.Annotations, k)
 					}
 				}
+				if updated.Labels != nil {
+					for k := range labels {
+						delete(updated.Labels, k)
+					}
+				}
 				if equality.Semantic.DeepEqual(service, updated) {
 					return nil
 				}
 
-				t.Logf("updating Service to remove annotations: %v", annotations)
+				t.Logf("updating Service to remove annotations %v and labels %v", annotations, labels)
 				_, err = adminClient.CoreV1().Services(env.ConciergeNamespace).Update(ctx, updated, metav1.UpdateOptions{})
 				return err
 			}))
@@ -1722,6 +1735,29 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			}, 1*time.Minute, 1*time.Second)
 		}
 
+		waitForServiceLabels := func(wantLabels map[string]string, labelKeyFilter string) {
+			testlib.RequireEventuallyWithoutError(t, func() (bool, error) {
+				service, err := adminClient.CoreV1().Services(env.ConciergeNamespace).Get(ctx, impersonationProxyLoadBalancerName(env), metav1.GetOptions{})
+				if err != nil {
+					return false, err
+				}
+				filteredActualLabels := map[string]string{}
+				for k, v := range service.Labels {
+					// We do want to pay attention to any label for which we intend to make an explicit assertion.
+					_, wantToMakeAssertionOnThisLabel := wantLabels[k]
+					// We do not want to pay attention to Service labels added by other controllers.
+					// These can come and go in time intervals outside of our control.
+					labelContainsFilterString := strings.Contains(k, labelKeyFilter)
+					if wantToMakeAssertionOnThisLabel || labelContainsFilterString {
+						filteredActualLabels[k] = v
+					}
+				}
+				t.Logf("found Service %s of type %s with actual labels %q; filtered by interesting keys results in %q; expected labels %q",
+					service.Name, service.Spec.Type, service.Labels, filteredActualLabels, wantLabels)
+				return equality.Semantic.DeepEqual(filteredActualLabels, wantLabels), nil
+			}, 1*time.Minute, 1*time.Second)
+		}
+
 		expectedAnnotations := func(credentialIssuerSpecAnnotations map[string]string, otherAnnotations map[string]string) map[string]string {
 			credentialIssuerSpecAnnotationKeys := make([]string, 0, len(credentialIssuerSpecAnnotations))
 			expectedAnnotations := map[string]string{}
@@ -1737,44 +1773,61 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			// Also expect the internal bookkeeping annotation to be present. It tracks the requested keys from the spec.
 			// Our controller sorts these keys to make the order in the annotation's value predictable.
 			sort.Strings(credentialIssuerSpecAnnotationKeys)
-			credentialIssuerSpecAnnotationKeysJSON, err := json.Marshal(credentialIssuerSpecAnnotationKeys)
+			if len(credentialIssuerSpecAnnotationKeys) > 0 {
+				credentialIssuerSpecAnnotationKeysJSON, err := json.Marshal(credentialIssuerSpecAnnotationKeys)
+				require.NoError(t, err)
+				// The name of this annotation key is decided by our controller.
+				expectedAnnotations["credentialissuer."+recognizableKeyString+"/annotation-keys"] = string(credentialIssuerSpecAnnotationKeysJSON)
+			}
+			// There should also be a bookkeeping annotation for labels. Our ytt templates always add the "app" label.
+			wantKeys := slices.AppendSeq([]string{"app"}, maps.Keys(env.ConciergeCustomLabels))
+			slices.Sort(wantKeys)
+			conciergeCustomLabelKeysJSON, err := json.Marshal(wantKeys)
 			require.NoError(t, err)
 			// The name of this annotation key is decided by our controller.
-			expectedAnnotations["credentialissuer."+recognizableAnnotationKeyString+"/annotation-keys"] = string(credentialIssuerSpecAnnotationKeysJSON)
+			expectedAnnotations["credentialissuer."+recognizableKeyString+"/label-keys"] = string(conciergeCustomLabelKeysJSON)
 			return expectedAnnotations
 		}
 
 		otherActorAnnotations := map[string]string{
-			recognizableAnnotationKeyString + "/test-other-actor-" + testlib.RandHex(t, 8): "test-other-actor-" + testlib.RandHex(t, 8),
+			recognizableKeyString + "/test-other-actor-" + testlib.RandHex(t, 8): "test-other-actor-" + testlib.RandHex(t, 8),
+		}
+		otherActorLabels := map[string]string{
+			recognizableKeyString + "/test-other-actor-" + testlib.RandHex(t, 8): "test-other-actor-" + testlib.RandHex(t, 8),
 		}
 
 		// Whatever happens, set the annotations back to the original value and expect the Service to be updated.
 		t.Cleanup(func() {
 			t.Log("reverting CredentialIssuer back to previous configuration")
-			deleteServiceAnnotations(otherActorAnnotations)
+			deleteServiceAnnotationsAndLabels(otherActorAnnotations, otherActorLabels)
 			applyCredentialIssuerAnnotations(previous.Spec.ImpersonationProxy.Service.DeepCopy().Annotations)
 			waitForServiceAnnotations(
 				expectedAnnotations(previous.Spec.ImpersonationProxy.Service.DeepCopy().Annotations, map[string]string{}),
-				recognizableAnnotationKeyString,
+				recognizableKeyString,
 			)
+			waitForServiceLabels(map[string]string{}, recognizableKeyString)
 		})
 
-		// Having another actor, like a human or a non-Pinniped controller, add unrelated annotations to the Service
-		// should not cause the Pinniped controllers to overwrite those annotations.
-		updateServiceAnnotations(otherActorAnnotations)
+		// Having another actor, like a human or a non-Pinniped controller, add unrelated annotations and labels
+		// to the Service should not cause the Pinniped controllers to overwrite those annotations.
+		updateServiceAnnotationsAndLabels(otherActorAnnotations, otherActorLabels)
 
 		// Set a new annotation in the CredentialIssuer spec.impersonationProxy.service.annotations field.
-		newAnnotationKey := recognizableAnnotationKeyString + "/test-" + testlib.RandHex(t, 8)
+		newAnnotationKey := recognizableKeyString + "/test-" + testlib.RandHex(t, 8)
 		newAnnotationValue := "test-" + testlib.RandHex(t, 8)
 		updatedAnnotations := previous.Spec.ImpersonationProxy.Service.DeepCopy().Annotations
+		if updatedAnnotations == nil {
+			updatedAnnotations = map[string]string{}
+		}
 		updatedAnnotations[newAnnotationKey] = newAnnotationValue
 		applyCredentialIssuerAnnotations(updatedAnnotations)
 
 		// Expect them to be applied to the Service.
 		waitForServiceAnnotations(
 			expectedAnnotations(updatedAnnotations, otherActorAnnotations),
-			recognizableAnnotationKeyString,
+			recognizableKeyString,
 		)
+		waitForServiceLabels(otherActorLabels, recognizableKeyString)
 	})
 
 	t.Run("running impersonation proxy with ClusterIP service", func(t *testing.T) {