Refactor some shared code between load balancer and cluster ip creation

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) {
+func (c *impersonatorConfigController) serviceExists(serviceName string) (bool, error) {
-	_, err := c.servicesInformer.Lister().Services(c.namespace).Get(c.generatedLoadBalancerServiceName)
+	_, 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) {
+func (c *impersonatorConfigController) serviceNeedsUpdate(config *v1alpha1.ImpersonationProxySpec, serviceName string) (bool, error) {
-	_, err := c.servicesInformer.Lister().Services(c.namespace).Get(c.generatedClusterIPServiceName)
+	service, err := c.servicesInformer.Lister().Services(c.namespace).Get(serviceName)
-	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)
 	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 {
+	if service.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) {
 		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()
+	return c.createOrUpdateService(ctx, config, &loadBalancer)
-	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
 }
 
 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()
+	return c.createOrUpdateService(ctx, config, &clusterIP)
-	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
 }
 
 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)

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])
 			})
 		})