Annotations for impersonation load balancer

internal/controller/impersonatorconfig/impersonator_config.go

@@ -11,6 +11,7 @@ import (
 	"encoding/pem"
 	"fmt"
 	"net"
+	"reflect"
 	"strings"
 	"time"
 
@@ -222,7 +223,7 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context) (*v
 	}
 
 	if c.shouldHaveLoadBalancer(config) {
-		if err = c.ensureLoadBalancerIsStarted(ctx); err != nil {
+		if err = c.ensureLoadBalancerIsStarted(ctx, config); err != nil {
 			return nil, err
 		}
 	} else {
@@ -321,6 +322,18 @@ func (c *impersonatorConfigController) loadBalancerExists() (bool, error) {
 	return true, nil
 }
 
+func (c *impersonatorConfigController) loadBalancerNeedsUpdate(config *v1alpha1.ImpersonationProxySpec) (bool, error) {
+	lb, err := c.servicesInformer.Lister().Services(c.namespace).Get(c.generatedLoadBalancerServiceName)
+	if err != nil {
+		return false, err
+	}
+	if !reflect.DeepEqual(lb.Annotations, config.Service.Annotations) {
+		return true, nil
+	}
+	// TODO also check for loadBalancerIP
+	return false, nil
+}
+
 func (c *impersonatorConfigController) tlsSecretExists() (bool, *v1.Secret, error) {
 	secret, err := c.secretsInformer.Lister().Secrets(c.namespace).Get(c.tlsSecretName)
 	notFound := k8serrors.IsNotFound(err)
@@ -406,14 +419,7 @@ func (c *impersonatorConfigController) ensureImpersonatorIsStopped(shouldCloseEr
 	return stopErr
 }
 
-func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.Context) error {
+func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.Context, config *v1alpha1.ImpersonationProxySpec) error {
-	running, err := c.loadBalancerExists()
-	if err != nil {
-		return err
-	}
-	if running {
-		return nil
-	}
 	appNameLabel := c.labels[appLabelKey]
 	loadBalancer := v1.Service{
 		Spec: v1.ServiceSpec{
@@ -425,17 +431,34 @@ func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.C
 					Protocol:   v1.ProtocolTCP,
 				},
 			},
+			LoadBalancerIP: config.Service.LoadBalancerIP,
 			Selector:       map[string]string{appLabelKey: appNameLabel},
 		},
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        c.generatedLoadBalancerServiceName,
 			Namespace:   c.namespace,
 			Labels:      c.labels,
-			Annotations: map[string]string{
+			Annotations: config.Service.Annotations,
-				"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "4000", // AWS' default is to time out after 60 seconds idle. Prevent that.
-			},
 		},
 	}
+	running, err := c.loadBalancerExists()
+	if err != nil {
+		return err
+	}
+	if running {
+		needsUpdate, err := c.loadBalancerNeedsUpdate(config)
+		if err != nil {
+			return err
+		}
+		if needsUpdate {
+			plog.Info("updating load balancer for impersonation proxy",
+				"service", c.generatedLoadBalancerServiceName,
+				"namespace", c.namespace)
+			_, err = c.k8sClient.CoreV1().Services(c.namespace).Update(ctx, &loadBalancer, metav1.UpdateOptions{})
+
+		}
+		return nil
+	}
 	plog.Info("creating load balancer for impersonation proxy",
 		"service", c.generatedLoadBalancerServiceName,
 		"namespace", c.namespace)

internal/controller/impersonatorconfig/impersonator_config_test.go

@@ -856,7 +856,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			r.Equal([]v1alpha1.CredentialIssuerStrategy{expectedStrategy}, credentialIssuer.Status.Strategies)
 		}
 
-		var requireLoadBalancerWasCreated = func(action coretesting.Action) {
+		var requireLoadBalancerWasCreated = func(action coretesting.Action) *corev1.Service {
 			createAction, ok := action.(coretesting.CreateAction)
 			r.True(ok, "should have been able to cast this action to CreateAction: %v", action)
 			r.Equal("create", createAction.GetVerb())
@@ -866,7 +866,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			r.Equal(corev1.ServiceTypeLoadBalancer, createdLoadBalancerService.Spec.Type)
 			r.Equal("app-name", createdLoadBalancerService.Spec.Selector["app"])
 			r.Equal(labels, createdLoadBalancerService.Labels)
-			r.Equal(map[string]string{"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "4000"}, createdLoadBalancerService.Annotations)
+			return createdLoadBalancerService
 		}
 
 		var requireLoadBalancerWasDeleted = func(action coretesting.Action) {
@@ -877,6 +877,19 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			r.Equal("services", deleteAction.GetResource().Resource)
 		}
 
+		var requireLoadBalancerWasUpdated = func(action coretesting.Action) *corev1.Service {
+			updateAction, ok := action.(coretesting.UpdateAction)
+			r.True(ok, "should have been able to cast this action to UpdateAction: %v", action)
+			r.Equal("update", updateAction.GetVerb())
+			updatedLoadBalancerService := updateAction.GetObject().(*corev1.Service)
+			r.Equal(loadBalancerServiceName, updatedLoadBalancerService.Name)
+			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
+		}
+
 		var requireTLSSecretWasDeleted = func(action coretesting.Action) {
 			deleteAction, ok := action.(coretesting.DeleteAction)
 			r.True(ok, "should have been able to cast this action to DeleteAction: %v", action)
@@ -1469,6 +1482,35 @@ 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() {
+					addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{
+						ImpersonationProxy: v1alpha1.ImpersonationProxySpec{
+							Mode: v1alpha1.ImpersonationProxyModeEnabled,
+							Service: v1alpha1.ImpersonationProxyServiceSpec{
+								Type:        v1alpha1.ImpersonationProxyServiceTypeLoadBalancer,
+								Annotations: annotations,
+							},
+						},
+					}, pinnipedInformerClient)
+					addNodeWithRoleToTracker("worker", kubeAPIClient)
+				})
+
+				it("starts the impersonator, generates a valid cert for the specified hostname, starts a loadbalancer", func() {
+					startInformersAndController()
+					r.NoError(runControllerSync())
+					r.Len(kubeAPIClient.Actions(), 3)
+					requireNodesListed(kubeAPIClient.Actions()[0])
+					lbService := requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
+					require.Equal(t, lbService.Annotations, annotations)
+					requireCASecretWasCreated(kubeAPIClient.Actions()[2])
+					requireTLSServerIsRunningWithoutCerts()
+					requireCredentialIssuer(newPendingStrategy())
+					requireSigningCertProviderIsEmpty()
+				})
+			})
+
 			when("the CredentialIssuer has a hostname specified and service type none", func() {
 				const fakeHostname = "fake.example.com"
 				it.Before(func() {
@@ -1586,7 +1628,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				})
 			})
 
-			when("the CredentialIssuer has a endpoint which is a hostname with a port, service type loadbalancer", func() {
+			when("the CredentialIssuer has a endpoint which is a hostname with a port, service type loadbalancer with loadbalancerip", func() {
 				const fakeHostnameWithPort = "fake.example.com:3000"
 				it.Before(func() {
 					addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{
@@ -1595,6 +1637,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 							ExternalEndpoint: fakeHostnameWithPort,
 							Service: v1alpha1.ImpersonationProxyServiceSpec{
 								Type:           v1alpha1.ImpersonationProxyServiceTypeLoadBalancer,
+								LoadBalancerIP: localhostIP,
 							},
 						},
 					}, pinnipedInformerClient)
@@ -1606,7 +1649,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					r.NoError(runControllerSync())
 					r.Len(kubeAPIClient.Actions(), 4)
 					requireNodesListed(kubeAPIClient.Actions()[0])
-					requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
+					lbService := requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
+					require.Equal(t, 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.
@@ -2011,6 +2055,64 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			})
 		})
 
+		when("requesting a load balancer via CredentialIssuer, then updating the annotations", func() {
+			it.Before(func() {
+				addSecretToTrackers(signingCASecret, kubeInformerClient)
+				addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{
+					ImpersonationProxy: v1alpha1.ImpersonationProxySpec{
+						Mode:             v1alpha1.ImpersonationProxyModeEnabled,
+						ExternalEndpoint: localhostIP,
+						Service: v1alpha1.ImpersonationProxyServiceSpec{
+							Type: v1alpha1.ImpersonationProxyServiceTypeLoadBalancer,
+						},
+					},
+				}, pinnipedInformerClient)
+				addNodeWithRoleToTracker("worker", kubeAPIClient)
+			})
+
+			it("creates the load balancer without annotations, then adds them", 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(nil), lbService.Annotations) // there should be no 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 the informer cache's background update from its watch.
+				addObjectFromCreateActionToInformerAndWait(kubeAPIClient.Actions()[1], 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.
+				annotations := 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,
+						},
+					},
+				}, 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, annotations, lbService.Annotations) // now the annotations should exist on the load balancer
+				requireTLSServerIsRunning(ca, testServerAddr(), nil)
+				requireCredentialIssuer(newSuccessStrategy(localhostIP, ca))
+				requireSigningCertProviderHasLoadedCerts(signingCACertPEM, signingCAKeyPEM)
+			})
+		})
+
 		when("sync is called more than once", func() {
 			it.Before(func() {
 				addSecretToTrackers(signingCASecret, kubeInformerClient)
@@ -2772,6 +2874,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			})
 		})
 
+		when("CredentialIssuer spec validation", func() {
 			when("the impersonator is enabled but the service type is none and the external endpoint is empty", func() {
 				it.Before(func() {
 					addSecretToTrackers(signingCASecret, kubeInformerClient)
@@ -2793,6 +2896,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					r.Len(kubeAPIClient.Actions(), 0)
 				})
 			})
+		})
 	}, spec.Report(report.Terminal{}))
 }
 

test/integration/concierge_impersonation_proxy_test.go

@@ -191,6 +191,16 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 	// this point depending on the capabilities of the cluster under test. We handle each possible case here.
 	switch {
 	case impersonatorShouldHaveStartedAutomaticallyByDefault && clusterSupportsLoadBalancers:
+		// configure the credential issuer spec to have the impersonation proxy in auto mode
+		updateCredentialIssuer(ctx, t, env, adminConciergeClient, conciergev1alpha.CredentialIssuerSpec{
+			ImpersonationProxy: conciergev1alpha.ImpersonationProxySpec{
+				Mode: conciergev1alpha.ImpersonationProxyModeAuto,
+				Service: conciergev1alpha.ImpersonationProxyServiceSpec{
+					Type:        conciergev1alpha.ImpersonationProxyServiceTypeLoadBalancer,
+					Annotations: map[string]string{"service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "4000"},
+				},
+			},
+		})
 		// Auto mode should have decided that the impersonator will run and should have started a load balancer,
 		// and we will be able to use the load balancer to access the impersonator. (e.g. GKE, AKS, EKS)
 		// Check that load balancer has been automatically created by the impersonator's "auto" mode.