From b4bb0db6e542a7728a74ae9415e1396bf7e1ed41 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Fri, 21 May 2021 09:57:46 -0700 Subject: [PATCH] Refactor some shared code between load balancer and cluster ip creation --- .../impersonatorconfig/impersonator_config.go | 116 ++++++------------ .../impersonator_config_test.go | 35 ++---- 2 files changed, 51 insertions(+), 100 deletions(-) diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index 631795592..92267b120 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -329,8 +329,8 @@ func (c *impersonatorConfigController) shouldHaveTLSSecret(config *v1alpha1.Impe return c.shouldHaveImpersonator(config) } -func (c *impersonatorConfigController) loadBalancerExists() (bool, error) { - _, err := c.servicesInformer.Lister().Services(c.namespace).Get(c.generatedLoadBalancerServiceName) +func (c *impersonatorConfigController) serviceExists(serviceName string) (bool, error) { + _, err := c.servicesInformer.Lister().Services(c.namespace).Get(serviceName) notFound := k8serrors.IsNotFound(err) if notFound { return false, nil @@ -341,40 +341,16 @@ func (c *impersonatorConfigController) loadBalancerExists() (bool, error) { return true, nil } -func (c *impersonatorConfigController) clusterIPExists() (bool, error) { - _, err := c.servicesInformer.Lister().Services(c.namespace).Get(c.generatedClusterIPServiceName) - notFound := k8serrors.IsNotFound(err) - if notFound { - return false, nil - } - if err != nil { - return false, err - } - return true, nil -} - -func (c *impersonatorConfigController) loadBalancerNeedsUpdate(config *v1alpha1.ImpersonationProxySpec) (bool, error) { - lb, err := c.servicesInformer.Lister().Services(c.namespace).Get(c.generatedLoadBalancerServiceName) +func (c *impersonatorConfigController) serviceNeedsUpdate(config *v1alpha1.ImpersonationProxySpec, serviceName string) (bool, error) { + service, err := c.servicesInformer.Lister().Services(c.namespace).Get(serviceName) if err != nil { return false, err } // TODO this will break if anything other than pinniped is adding annotations - if !reflect.DeepEqual(lb.Annotations, config.Service.Annotations) { + if !reflect.DeepEqual(service.Annotations, config.Service.Annotations) { return true, nil } - if lb.Spec.LoadBalancerIP != config.Service.LoadBalancerIP { - return true, nil - } - return false, nil -} - -func (c *impersonatorConfigController) ClusterIPNeedsUpdate(config *v1alpha1.ImpersonationProxySpec) (bool, error) { - clusterIP, err := c.servicesInformer.Lister().Services(c.namespace).Get(c.generatedClusterIPServiceName) - if err != nil { - return false, err - } - // TODO this will break if anything other than pinniped is adding annotations - if !reflect.DeepEqual(clusterIP.Annotations, config.Service.Annotations) { + if service.Spec.LoadBalancerIP != config.Service.LoadBalancerIP { return true, nil } return false, nil @@ -487,33 +463,11 @@ func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.C 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 err - } - return nil - } - plog.Info("creating load balancer for impersonation proxy", - "service", c.generatedLoadBalancerServiceName, - "namespace", c.namespace) - _, err = c.k8sClient.CoreV1().Services(c.namespace).Create(ctx, &loadBalancer, metav1.CreateOptions{}) - return err + return c.createOrUpdateService(ctx, config, &loadBalancer) } func (c *impersonatorConfigController) ensureLoadBalancerIsStopped(ctx context.Context) error { - running, err := c.loadBalancerExists() + running, err := c.serviceExists(c.generatedLoadBalancerServiceName) if err != nil { return err } @@ -548,33 +502,11 @@ func (c *impersonatorConfigController) ensureClusterIPServiceIsStarted(ctx conte Annotations: config.Service.Annotations, }, } - running, err := c.clusterIPExists() - if err != nil { - return err - } - if running { - needsUpdate, err := c.ClusterIPNeedsUpdate(config) - if err != nil { - return err - } - if needsUpdate { - plog.Info("updating cluster ip for impersonation proxy", - "service", c.generatedLoadBalancerServiceName, - "namespace", c.namespace) - _, err = c.k8sClient.CoreV1().Services(c.namespace).Update(ctx, &clusterIP, metav1.UpdateOptions{}) - return err - } - return nil - } - plog.Info("creating cluster ip for impersonation proxy", - "service", c.generatedClusterIPServiceName, - "namespace", c.namespace) - _, err = c.k8sClient.CoreV1().Services(c.namespace).Create(ctx, &clusterIP, metav1.CreateOptions{}) - return err + return c.createOrUpdateService(ctx, config, &clusterIP) } func (c *impersonatorConfigController) ensureClusterIPServiceIsStopped(ctx context.Context) error { - running, err := c.clusterIPExists() + running, err := c.serviceExists(c.generatedClusterIPServiceName) if err != nil { return err } @@ -588,6 +520,34 @@ func (c *impersonatorConfigController) ensureClusterIPServiceIsStopped(ctx conte return c.k8sClient.CoreV1().Services(c.namespace).Delete(ctx, c.generatedClusterIPServiceName, metav1.DeleteOptions{}) } +func (c *impersonatorConfigController) createOrUpdateService(ctx context.Context, config *v1alpha1.ImpersonationProxySpec, service *v1.Service) error { + running, err := c.serviceExists(service.Name) + if err != nil { + return err + } + if running { + needsUpdate, err := c.serviceNeedsUpdate(config, service.Name) + if err != nil { + return err + } + if needsUpdate { + plog.Info("updating service for impersonation proxy", + "service type", service.Spec.Type, + "service", service.Name, + "namespace", c.namespace) + _, err = c.k8sClient.CoreV1().Services(c.namespace).Update(ctx, service, metav1.UpdateOptions{}) + return err + } + return nil + } + plog.Info("creating service for impersonation proxy", + "service type", service.Spec.Type, + "service", service.Name, + "namespace", c.namespace) + _, err = c.k8sClient.CoreV1().Services(c.namespace).Create(ctx, service, metav1.CreateOptions{}) + return err +} + func (c *impersonatorConfigController) ensureTLSSecret(ctx context.Context, nameInfo *certNameInfo, ca *certauthority.CA) error { secretFromInformer, err := c.secretsInformer.Lister().Secrets(c.namespace).Get(c.tlsSecretName) notFound := k8serrors.IsNotFound(err) diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index 2ed70277d..696bcc901 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/internal/controller/impersonatorconfig/impersonator_config_test.go @@ -874,6 +874,14 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.Equal([]v1alpha1.CredentialIssuerStrategy{expectedStrategy}, credentialIssuer.Status.Strategies) } + var requireServiceWasDeleted = func(action coretesting.Action, serviceName string) { + deleteAction, ok := action.(coretesting.DeleteAction) + r.True(ok, "should have been able to cast this action to DeleteAction: %v", action) + r.Equal("delete", deleteAction.GetVerb()) + r.Equal(serviceName, deleteAction.GetName()) + r.Equal("services", deleteAction.GetResource().Resource) + } + 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) @@ -887,14 +895,6 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { return createdLoadBalancerService } - var requireLoadBalancerWasDeleted = func(action coretesting.Action) { - deleteAction, ok := action.(coretesting.DeleteAction) - r.True(ok, "should have been able to cast this action to DeleteAction: %v", action) - r.Equal("delete", deleteAction.GetVerb()) - r.Equal(loadBalancerServiceName, deleteAction.GetName()) - 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) @@ -920,15 +920,6 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { return createdClusterIPService } - var requireClusterIPWasDeleted = func(action coretesting.Action) { - // TODO maybe de-dup this with loadbalancerwasdeleted - deleteAction, ok := action.(coretesting.DeleteAction) - r.True(ok, "should have been able to cast this action to DeleteAction: %v", action) - r.Equal("delete", deleteAction.GetVerb()) - r.Equal(clusterIPServiceName, deleteAction.GetName()) - r.Equal("services", deleteAction.GetResource().Resource) - } - var requireClusterIPWasUpdated = 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) @@ -1162,7 +1153,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSServerWasNeverStarted() r.Len(kubeAPIClient.Actions(), 3) requireNodesListed(kubeAPIClient.Actions()[0]) - requireLoadBalancerWasDeleted(kubeAPIClient.Actions()[1]) + requireServiceWasDeleted(kubeAPIClient.Actions()[1], loadBalancerServiceName) requireTLSSecretWasDeleted(kubeAPIClient.Actions()[2]) requireCredentialIssuer(newAutoDisabledStrategy()) requireSigningCertProviderIsEmpty() @@ -2102,7 +2093,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.NoError(runControllerSync()) requireTLSServerIsNoLongerRunning() r.Len(kubeAPIClient.Actions(), 4) - requireLoadBalancerWasDeleted(kubeAPIClient.Actions()[3]) + requireServiceWasDeleted(kubeAPIClient.Actions()[3], loadBalancerServiceName) requireCredentialIssuer(newManuallyDisabledStrategy()) requireSigningCertProviderIsEmpty() @@ -2168,7 +2159,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.NoError(runControllerSync()) requireTLSServerIsNoLongerRunning() r.Len(kubeAPIClient.Actions(), 4) - requireClusterIPWasDeleted(kubeAPIClient.Actions()[3]) + requireServiceWasDeleted(kubeAPIClient.Actions()[3], clusterIPServiceName) requireCredentialIssuer(newManuallyDisabledStrategy()) requireSigningCertProviderIsEmpty() @@ -2284,7 +2275,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.NoError(runControllerSync()) r.Len(kubeAPIClient.Actions(), 9) - requireLoadBalancerWasDeleted(kubeAPIClient.Actions()[6]) + requireServiceWasDeleted(kubeAPIClient.Actions()[6], loadBalancerServiceName) requireTLSSecretWasDeleted(kubeAPIClient.Actions()[7]) requireTLSSecretWasCreated(kubeAPIClient.Actions()[8], ca) // recreated because the endpoint was updated, reused the old CA requireTLSServerIsRunning(ca, testServerAddr(), nil) @@ -3102,7 +3093,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSServerWasNeverStarted() r.Len(kubeAPIClient.Actions(), 3) requireNodesListed(kubeAPIClient.Actions()[0]) - requireLoadBalancerWasDeleted(kubeAPIClient.Actions()[1]) + requireServiceWasDeleted(kubeAPIClient.Actions()[1], loadBalancerServiceName) requireTLSSecretWasDeleted(kubeAPIClient.Actions()[2]) }) })