From e3b0f65c4c5dcb93843f63795c86f9ca2d145462 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 26 Mar 2026 10:52:41 -0700 Subject: [PATCH 1/2] impersonator config controller ignores service labels added by others --- .../impersonatorconfig/impersonator_config.go | 130 ++++++++++++------ .../impersonator_config_test.go | 100 ++++++++------ 2 files changed, 149 insertions(+), 81 deletions(-) diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index d1d56b944..e0edffeb9 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/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 +} diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index e7f108afc..e598ebc1a 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/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) From 20b9a8a4bb351587bf3e4a33d8dbb04b44610ed7 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 26 Mar 2026 13:15:25 -0700 Subject: [PATCH 2/2] integration test for ignoring impersonator service label added by others --- .../concierge_impersonation_proxy_test.go | 83 +++++++++++++++---- 1 file changed, 68 insertions(+), 15 deletions(-) diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index 95815d1da..450c3b924 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/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) {