From 94c370ac856b728902d4c88ed80ce458520063ff Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Tue, 18 May 2021 16:54:59 -0700 Subject: [PATCH] Annotations for impersonation load balancer --- .../impersonatorconfig/impersonator_config.go | 55 +++++-- .../impersonator_config_test.go | 148 +++++++++++++++--- .../concierge_impersonation_proxy_test.go | 10 ++ 3 files changed, 175 insertions(+), 38 deletions(-) diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index 1d29fdd1..beb466e1 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/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 { - running, err := c.loadBalancerExists() - if err != nil { - return err - } - if running { - return nil - } +func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.Context, config *v1alpha1.ImpersonationProxySpec) error { appNameLabel := c.labels[appLabelKey] loadBalancer := v1.Service{ Spec: v1.ServiceSpec{ @@ -425,17 +431,34 @@ func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.C Protocol: v1.ProtocolTCP, }, }, - Selector: map[string]string{appLabelKey: appNameLabel}, + 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{ - "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout": "4000", // AWS' default is to time out after 60 seconds idle. Prevent that. - }, + Name: c.generatedLoadBalancerServiceName, + Namespace: c.namespace, + Labels: c.labels, + Annotations: config.Service.Annotations, }, } + 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) diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index 08acd916..3fa247e2 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/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{ @@ -1594,7 +1636,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { Mode: v1alpha1.ImpersonationProxyModeEnabled, ExternalEndpoint: fakeHostnameWithPort, Service: v1alpha1.ImpersonationProxyServiceSpec{ - Type: v1alpha1.ImpersonationProxyServiceTypeLoadBalancer, + 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,25 +2874,27 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { }) }) - when("the impersonator is enabled but the service type is none and the external endpoint is empty", func() { - it.Before(func() { - addSecretToTrackers(signingCASecret, kubeInformerClient) - addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ - ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ - Mode: v1alpha1.ImpersonationProxyModeEnabled, - ExternalEndpoint: "", - Service: v1alpha1.ImpersonationProxyServiceSpec{ - Type: v1alpha1.ImpersonationProxyServiceTypeNone, + 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) + addCredentialIssuerToTracker(credentialIssuerResourceName, v1alpha1.CredentialIssuerSpec{ + ImpersonationProxy: v1alpha1.ImpersonationProxySpec{ + Mode: v1alpha1.ImpersonationProxyModeEnabled, + ExternalEndpoint: "", + Service: v1alpha1.ImpersonationProxyServiceSpec{ + Type: v1alpha1.ImpersonationProxyServiceTypeNone, + }, }, - }, - }, pinnipedInformerClient) - addNodeWithRoleToTracker("control-plane", kubeAPIClient) - }) + }, pinnipedInformerClient) + addNodeWithRoleToTracker("control-plane", kubeAPIClient) + }) - it("returns a validation error", func() { - startInformersAndController() - r.EqualError(runControllerSync(), "invalid impersonator configuration: invalid impersonation proxy configuration: must specify an external endpoint or set a service type") - r.Len(kubeAPIClient.Actions(), 0) + it("returns a validation error", func() { + startInformersAndController() + r.EqualError(runControllerSync(), "invalid impersonator configuration: invalid impersonation proxy configuration: must specify an external endpoint or set a service type") + r.Len(kubeAPIClient.Actions(), 0) + }) }) }) }, spec.Report(report.Terminal{})) diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index 6e7153c6..e3273cab 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/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.