Merge pull request #739 from vmware-tanzu/merge_impersonator_service_annotations

Carefully merge desired annotations into impersonation proxy Service

internal/controller/impersonatorconfig/impersonator_config.go

@@ -8,9 +8,11 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/base64"
+	"encoding/json"
 	"encoding/pem"
 	"fmt"
 	"net"
+	"sort"
 	"strings"
 	"time"
 
@@ -53,6 +55,7 @@ const (
 	caCrtKey                     = "ca.crt"
 	caKeyKey                     = "ca.key"
 	appLabelKey                  = "app"
+	annotationKeysKey            = "credentialissuer.pinniped.dev/annotation-keys"
 )
 
 type impersonatorConfigController struct {
@@ -521,34 +524,93 @@ func (c *impersonatorConfigController) ensureClusterIPServiceIsStopped(ctx conte
 	return utilerrors.FilterOut(err, k8serrors.IsNotFound)
 }
 
-func (c *impersonatorConfigController) createOrUpdateService(ctx context.Context, service *v1.Service) error {
+func (c *impersonatorConfigController) createOrUpdateService(ctx context.Context, desiredService *v1.Service) error {
-	log := c.infoLog.WithValues("serviceType", service.Spec.Type, "service", klog.KObj(service))
+	log := c.infoLog.WithValues("serviceType", desiredService.Spec.Type, "service", klog.KObj(desiredService))
-	existing, err := c.servicesInformer.Lister().Services(c.namespace).Get(service.Name)
+
+	// Prepare to remember which annotation keys were added from the CredentialIssuer spec, both for
+	// creates and for updates, in case someone removes a key from the spec in the future. We would like
+	// 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)
+	}
+	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)
+	}
+
+	// Get the Service from the informer, and create it if it does not already exist.
+	existingService, err := c.servicesInformer.Lister().Services(c.namespace).Get(desiredService.Name)
 	if k8serrors.IsNotFound(err) {
 		log.Info("creating service for impersonation proxy")
-		_, err := c.k8sClient.CoreV1().Services(c.namespace).Create(ctx, service, metav1.CreateOptions{})
+		_, err := c.k8sClient.CoreV1().Services(c.namespace).Create(ctx, desiredService, metav1.CreateOptions{})
 		return err
 	}
 	if err != nil {
 		return err
 	}
 
-	// Update only the specific fields that are meaningfully part of our desired state.
+	// The Service already exists, so update only the specific fields that are meaningfully part of our desired state.
-	updated := existing.DeepCopy()
+	updatedService := existingService.DeepCopy()
-	updated.ObjectMeta.Labels = service.ObjectMeta.Labels
+	updatedService.ObjectMeta.Labels = desiredService.ObjectMeta.Labels
-	updated.ObjectMeta.Annotations = service.ObjectMeta.Annotations
+	updatedService.Spec.LoadBalancerIP = desiredService.Spec.LoadBalancerIP
-	updated.Spec.LoadBalancerIP = service.Spec.LoadBalancerIP
+	updatedService.Spec.Type = desiredService.Spec.Type
-	updated.Spec.Type = service.Spec.Type
+	updatedService.Spec.Selector = desiredService.Spec.Selector
-	updated.Spec.Selector = service.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
+	// 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)
+		}
+	}
+
+	// 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 {
+		delete(updatedService.Annotations, annotationKeysKey)
+	}
 
 	// If our updates didn't change anything, we're done.
-	if equality.Semantic.DeepEqual(existing, updated) {
+	if equality.Semantic.DeepEqual(existingService, updatedService) {
 		return nil
 	}
 
 	// Otherwise apply the updates.
 	c.infoLog.Info("updating service for impersonation proxy")
-	_, err = c.k8sClient.CoreV1().Services(c.namespace).Update(ctx, updated, metav1.UpdateOptions{})
+	_, err = c.k8sClient.CoreV1().Services(c.namespace).Update(ctx, updatedService, metav1.UpdateOptions{})
 	return err
 }
 

internal/controller/impersonatorconfig/impersonator_config_test.go

@@ -785,6 +785,13 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			}
 		}
 
+		var addServiceToTrackers = func(service *corev1.Service, clients ...*kubernetesfake.Clientset) {
+			for _, client := range clients {
+				serviceCopy := service.DeepCopy()
+				r.NoError(client.Tracker().Add(serviceCopy))
+			}
+		}
+
 		var deleteServiceFromTracker = func(resourceName string, client *kubernetesfake.Clientset) {
 			r.NoError(client.Tracker().Delete(
 				schema.GroupVersionResource{Version: "v1", Resource: "services"},
@@ -1644,7 +1651,6 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			})
 
 			when("credentialissuer has service type loadbalancer and custom annotations", func() {
-				annotations := map[string]string{"some-annotation-key": "some-annotation-value"}
 				it.Before(func() {
 					addCredentialIssuerToTrackers(v1alpha1.CredentialIssuer{
 						ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerResourceName},
@@ -1653,7 +1659,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 								Mode: v1alpha1.ImpersonationProxyModeEnabled,
 								Service: v1alpha1.ImpersonationProxyServiceSpec{
 									Type:        v1alpha1.ImpersonationProxyServiceTypeLoadBalancer,
-									Annotations: annotations,
+									Annotations: map[string]string{"some-annotation-key": "some-annotation-value"},
 								},
 							},
 						},
@@ -1667,7 +1673,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, annotations)
+					require.Equal(t, lbService.Annotations, map[string]string{
+						"some-annotation-key":                           "some-annotation-value",
+						"credentialissuer.pinniped.dev/annotation-keys": `["some-annotation-key"]`,
+					})
 					requireCASecretWasCreated(kubeAPIClient.Actions()[2])
 					requireTLSServerIsRunningWithoutCerts()
 					requireCredentialIssuer(newPendingStrategyWaitingForLB())
@@ -2386,20 +2395,30 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
 				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 
+				// 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",
+				}
+
 				// Simulate the informer cache's background update from its watch.
-				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services())
+				addObjectToKubeInformerAndWait(lbService, kubeInformers.Core().V1().Services())
 				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[2], kubeInformers.Core().V1().Secrets())
 				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[3], kubeInformers.Core().V1().Secrets())
 
-				// Add annotations to the spec.
+				r.NoError(runControllerSync())
-				annotations := map[string]string{"my-annotation-key": "my-annotation-val"}
+				r.Len(kubeAPIClient.Actions(), 4) // no new actions because the controller decides there is nothing to update on the Service
+
+				// Add annotations to the CredentialIssuer spec.
+				credentialIssuerAnnotations := map[string]string{"my-annotation-key": "my-annotation-val"}
 				updateCredentialIssuerInInformerAndWait(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{
 					ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{
 						Mode:             v1alpha1.ImpersonationProxyModeEnabled,
 						ExternalEndpoint: localhostIP,
 						Service: v1alpha1.ImpersonationProxyServiceSpec{
 							Type:        v1alpha1.ImpersonationProxyServiceTypeLoadBalancer,
-							Annotations: annotations,
+							Annotations: credentialIssuerAnnotations,
 						},
 					},
 				}, pinnipedInformers.Config().V1alpha1().CredentialIssuers())
@@ -2407,7 +2426,14 @@ 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, annotations, lbService.Annotations) // now the annotations should exist on the load balancer
+				require.Equal(t, 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"]`,
+				}, lbService.Annotations)
 				requireTLSServerIsRunning(ca, testServerAddr(), nil)
 				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
 				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
@@ -2447,20 +2473,30 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
 				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
 
+				// 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",
+				}
+
 				// Simulate the informer cache's background update from its watch.
-				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], kubeInformers.Core().V1().Services())
+				addObjectToKubeInformerAndWait(clusterIPService, kubeInformers.Core().V1().Services())
 				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[2], kubeInformers.Core().V1().Secrets())
 				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[3], kubeInformers.Core().V1().Secrets())
 
-				// Add annotations to the spec.
+				r.NoError(runControllerSync())
-				annotations := map[string]string{"my-annotation-key": "my-annotation-val"}
+				r.Len(kubeAPIClient.Actions(), 4) // no new actions because the controller decides there is nothing to update on the Service
+
+				// Add annotations to the CredentialIssuer spec.
+				credentialIssuerAnnotations := map[string]string{"my-annotation-key": "my-annotation-val"}
 				updateCredentialIssuerInInformerAndWait(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{
 					ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{
 						Mode:             v1alpha1.ImpersonationProxyModeEnabled,
 						ExternalEndpoint: localhostIP,
 						Service: v1alpha1.ImpersonationProxyServiceSpec{
 							Type:        v1alpha1.ImpersonationProxyServiceTypeClusterIP,
-							Annotations: annotations,
+							Annotations: credentialIssuerAnnotations,
 						},
 					},
 				}, pinnipedInformers.Config().V1alpha1().CredentialIssuers())
@@ -2468,7 +2504,173 @@ 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, annotations, clusterIPService.Annotations) // now the annotations should exist on the load balancer
+				require.Equal(t, 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"]`,
+				}, clusterIPService.Annotations)
+				requireTLSServerIsRunning(ca, testServerAddr(), nil)
+				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
+			})
+		})
+
+		when("requesting a load balancer via CredentialIssuer with annotations, then updating the CredentialIssuer annotations to remove one", func() {
+			it.Before(func() {
+				addSecretToTrackers(signingCASecret, kubeInformerClient)
+				addCredentialIssuerToTrackers(v1alpha1.CredentialIssuer{
+					ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerResourceName},
+					Spec: v1alpha1.CredentialIssuerSpec{
+						ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{
+							Mode:             v1alpha1.ImpersonationProxyModeEnabled,
+							ExternalEndpoint: localhostIP,
+							Service: v1alpha1.ImpersonationProxyServiceSpec{
+								Type: v1alpha1.ImpersonationProxyServiceTypeLoadBalancer,
+								Annotations: 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",
+								},
+							},
+						},
+					},
+				}, pinnipedInformerClient, pinnipedAPIClient)
+				addNodeWithRoleToTracker("worker", kubeAPIClient)
+			})
+
+			it("creates the load balancer with annotations, then removes the removed annotation", func() {
+				startInformersAndController()
+
+				// Should have started in "enabled" mode with service type load balancer, so one is created.
+				r.NoError(runControllerSync())
+				r.Len(kubeAPIClient.Actions(), 4)
+				requireNodesListed(kubeAPIClient.Actions()[0])
+				lbService := requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
+				require.Equal(t, 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"]`,
+				}, lbService.Annotations) // there should be some annotations at first
+				ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2])
+				requireTLSSecretWasCreated(kubeAPIClient.Actions()[3], ca)
+				requireTLSServerIsRunning(ca, testServerAddr(), nil)
+				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
+
+				// Simulate another actor in the system, like a human user or a non-Pinniped controller,
+				// updating the new Service to add another annotation.
+				lbService.Annotations["annotation-from-unrelated-controller-key"] = "annotation-from-unrelated-controller-val"
+
+				// Simulate the informer cache's background update from its watch.
+				addObjectToKubeInformerAndWait(lbService, kubeInformers.Core().V1().Services())
+				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[2], kubeInformers.Core().V1().Secrets())
+				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[3], kubeInformers.Core().V1().Secrets())
+
+				r.NoError(runControllerSync())
+				r.Len(kubeAPIClient.Actions(), 4) // no new actions because the controller decides there is nothing to update on the Service
+
+				// Remove one of the annotations from the CredentialIssuer spec.
+				updateCredentialIssuerInInformerAndWait(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{
+					ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{
+						Mode:             v1alpha1.ImpersonationProxyModeEnabled,
+						ExternalEndpoint: localhostIP,
+						Service: v1alpha1.ImpersonationProxyServiceSpec{
+							Type: v1alpha1.ImpersonationProxyServiceTypeLoadBalancer,
+							Annotations: map[string]string{
+								"my-initial-annotation1-key": "my-initial-annotation1-val",
+								"my-initial-annotation3-key": "my-initial-annotation3-val",
+							},
+						},
+					},
+				}, pinnipedInformers.Config().V1alpha1().CredentialIssuers())
+
+				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{
+					// 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.
+					// The annotations from the other actor should not be modified.
+					"annotation-from-unrelated-controller-key":      "annotation-from-unrelated-controller-val",
+					"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"]`,
+				}, lbService.Annotations)
+				requireTLSServerIsRunning(ca, testServerAddr(), nil)
+				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
+
+				// Remove all the rest of the annotations from the CredentialIssuer spec so there are none remaining.
+				updateCredentialIssuerInInformerAndWait(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{
+					ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{
+						Mode:             v1alpha1.ImpersonationProxyModeEnabled,
+						ExternalEndpoint: localhostIP,
+						Service: v1alpha1.ImpersonationProxyServiceSpec{
+							Type:        v1alpha1.ImpersonationProxyServiceTypeLoadBalancer,
+							Annotations: map[string]string{},
+						},
+					},
+				}, pinnipedInformers.Config().V1alpha1().CredentialIssuers())
+
+				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{
+					// 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",
+				}, lbService.Annotations)
+				requireTLSServerIsRunning(ca, testServerAddr(), nil)
+				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
+			})
+		})
+
+		when("requesting a load balancer via CredentialIssuer, but there is already a load balancer with an invalid bookkeeping annotation value", func() {
+			it.Before(func() {
+				addSecretToTrackers(signingCASecret, kubeInformerClient)
+				addCredentialIssuerToTrackers(v1alpha1.CredentialIssuer{
+					ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerResourceName},
+					Spec: v1alpha1.CredentialIssuerSpec{
+						ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{
+							Mode:             v1alpha1.ImpersonationProxyModeEnabled,
+							ExternalEndpoint: localhostIP,
+							Service: v1alpha1.ImpersonationProxyServiceSpec{
+								Type:        v1alpha1.ImpersonationProxyServiceTypeLoadBalancer,
+								Annotations: map[string]string{"some-annotation": "annotation-value"},
+							},
+						},
+					},
+				}, pinnipedInformerClient, pinnipedAPIClient)
+				addNodeWithRoleToTracker("worker", kubeAPIClient)
+				// Add a Service with a messed up bookkeeping annotation.
+				loadBalancerService := newLoadBalancerService(loadBalancerServiceName, corev1.ServiceStatus{})
+				loadBalancerService.Annotations = map[string]string{
+					annotationKeysKey: `["this is not valid json`,
+				}
+				addServiceToTrackers(loadBalancerService, kubeInformerClient, kubeAPIClient)
+			})
+
+			it("just acts like the annotation wasn't present since that is better than becoming inoperable", func() {
+				startInformersAndController()
+
+				// Should have started in "enabled" mode with service type load balancer, so one is created.
+				r.NoError(runControllerSync())
+				r.Len(kubeAPIClient.Actions(), 4)
+				requireNodesListed(kubeAPIClient.Actions()[0])
+				lbService := requireLoadBalancerWasUpdated(kubeAPIClient.Actions()[1])
+				require.Equal(t, map[string]string{
+					"some-annotation": "annotation-value",
+					"credentialissuer.pinniped.dev/annotation-keys": `["some-annotation"]`,
+				}, lbService.Annotations)
+				ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2])
+				requireTLSSecretWasCreated(kubeAPIClient.Actions()[3], ca)
 				requireTLSServerIsRunning(ca, testServerAddr(), nil)
 				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
 				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)

test/integration/concierge_impersonation_proxy_test.go

@@ -22,6 +22,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"sort"
 	"strings"
 	"sync"
 	"testing"
@@ -1481,6 +1482,52 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		previous, err := adminConciergeClient.ConfigV1alpha1().CredentialIssuers().Get(ctx, credentialIssuerName(env), metav1.GetOptions{})
 		require.NoError(t, err)
 
+		updateServiceAnnotations := func(annotations 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 {
+					return err
+				}
+				updated := service.DeepCopy()
+				if updated.Annotations == nil {
+					updated.Annotations = map[string]string{}
+				}
+				// Add/update each requested annotation, without overwriting others that are already there.
+				for k, v := range annotations {
+					updated.Annotations[k] = v
+				}
+				if equality.Semantic.DeepEqual(service, updated) {
+					return nil
+				}
+
+				t.Logf("updating Service with annotations: %v", annotations)
+				_, err = adminClient.CoreV1().Services(env.ConciergeNamespace).Update(ctx, updated, metav1.UpdateOptions{})
+				return err
+			}))
+		}
+
+		deleteServiceAnnotations := func(annotations 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 {
+					return err
+				}
+				updated := service.DeepCopy()
+				if updated.Annotations != nil {
+					for k := range annotations {
+						delete(updated.Annotations, k)
+					}
+				}
+				if equality.Semantic.DeepEqual(service, updated) {
+					return nil
+				}
+
+				t.Logf("updating Service to remove annotations: %v", annotations)
+				_, err = adminClient.CoreV1().Services(env.ConciergeNamespace).Update(ctx, updated, metav1.UpdateOptions{})
+				return err
+			}))
+		}
+
 		applyCredentialIssuerAnnotations := func(annotations map[string]string) {
 			require.NoError(t, retry.RetryOnConflict(retry.DefaultRetry, func() error {
 				issuer, err := adminConciergeClient.ConfigV1alpha1().CredentialIssuers().Get(ctx, credentialIssuerName(env), metav1.GetOptions{})
@@ -1505,18 +1552,51 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 				if err != nil {
 					return false, err
 				}
-				t.Logf("found Service %s of type %s with annotations: %s", service.Name, service.Spec.Type, service.Annotations)
+				t.Logf("found Service %s of type %s with actual annotations %q; expected annotations %q",
+					service.Name, service.Spec.Type, service.Annotations, annotations)
 				return equality.Semantic.DeepEqual(service.Annotations, annotations), nil
 			}, 30*time.Second, 100*time.Millisecond)
 		}
 
+		expectedAnnotations := func(credentialIssuerSpecAnnotations map[string]string, otherAnnotations map[string]string) map[string]string {
+			credentialIssuerSpecAnnotationKeys := []string{}
+			expectedAnnotations := map[string]string{}
+			// Expect the annotations specified on the CredentialIssuer spec to be present.
+			for k, v := range credentialIssuerSpecAnnotations {
+				credentialIssuerSpecAnnotationKeys = append(credentialIssuerSpecAnnotationKeys, k)
+				expectedAnnotations[k] = v
+			}
+			// Aside from the annotations requested on the CredentialIssuer spec, also expect the other annotation to still be there too.
+			for k, v := range otherAnnotations {
+				expectedAnnotations[k] = v
+			}
+			// 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)
+			require.NoError(t, err)
+			expectedAnnotations["credentialissuer.pinniped.dev/annotation-keys"] = string(credentialIssuerSpecAnnotationKeysJSON)
+			return expectedAnnotations
+		}
+
+		otherActorAnnotations := map[string]string{
+			"pinniped.dev/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)
 			applyCredentialIssuerAnnotations(previous.Spec.ImpersonationProxy.Service.DeepCopy().Annotations)
-			waitForServiceAnnotations(previous.Spec.ImpersonationProxy.Service.DeepCopy().Annotations)
+			waitForServiceAnnotations(
+				expectedAnnotations(previous.Spec.ImpersonationProxy.Service.DeepCopy().Annotations, map[string]string{}),
+			)
 		})
 
+		// 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)
+
 		// Set a new annotation in the CredentialIssuer spec.impersonationProxy.service.annotations field.
 		newAnnotationKey := "pinniped.dev/test-" + testlib.RandHex(t, 8)
 		newAnnotationValue := "test-" + testlib.RandHex(t, 8)
@@ -1524,8 +1604,8 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		updatedAnnotations[newAnnotationKey] = newAnnotationValue
 		applyCredentialIssuerAnnotations(updatedAnnotations)
 
-		// Expect it to be applied to the Service.
+		// Expect them to be applied to the Service.
-		waitForServiceAnnotations(updatedAnnotations)
+		waitForServiceAnnotations(expectedAnnotations(updatedAnnotations, otherActorAnnotations))
 	})
 
 	t.Run("running impersonation proxy with ClusterIP service", func(t *testing.T) {