More WIP managing TLS secrets from the impersonation config controller

Signed-off-by: Margo Crawford <margaretc@vmware.com>

internal/concierge/impersonator/config.go

@@ -45,9 +45,13 @@ type Config struct {
 	// Enable or disable the impersonation proxy. Optional. Defaults to ModeAuto.
 	Mode Mode `json:"mode,omitempty"`
 
-	// The HTTPS URL of the impersonation proxy for clients to use from outside the cluster. Used when creating TLS
+	// Used when creating TLS certificates and for clients to discover the endpoint. Optional. When not specified, if the
-	// certificates and for clients to discover the endpoint. Optional. When not specified, if the impersonation proxy
+	// impersonation proxy is started, then it will automatically create a LoadBalancer Service and use its ingress as the
-	// is started, then it will automatically create a LoadBalancer Service and use its ingress as the endpoint.
+	// endpoint.
+	//
+	// When specified, it may be a hostname or IP address, optionally with a port number, of the impersonation proxy
+	// for clients to use from outside the cluster. E.g. myhost.mycompany.com:8443. Clients should assume that they should
+	// connect via HTTPS to this service.
 	Endpoint string `json:"endpoint,omitempty"`
 
 	// The TLS configuration of the impersonation proxy's endpoints. Optional. When not specified, a CA and TLS

internal/controller/impersonatorconfig/impersonator_config.go

@@ -6,7 +6,9 @@ package impersonatorconfig
 import (
 	"context"
 	"crypto/tls"
+	"crypto/x509"
 	"crypto/x509/pkix"
+	"encoding/pem"
 	"errors"
 	"fmt"
 	"net"
@@ -170,11 +172,13 @@ func (c *impersonatorConfigController) Sync(ctx controllerlib.Context) error {
 	}
 
 	if c.shouldHaveTLSSecret(config) {
-		if err = c.ensureTLSSecretIsCreated(ctx.Context, config); err != nil {
+		err = c.ensureTLSSecret(ctx, config)
-			// return err // TODO
+		if err != nil {
+			return err
 		}
 	} else {
-		if err = c.ensureTLSSecretIsRemoved(ctx.Context); err != nil {
+		err = c.ensureTLSSecretIsRemoved(ctx.Context)
+		if err != nil {
 			return err
 		}
 	}
@@ -184,8 +188,24 @@ func (c *impersonatorConfigController) Sync(ctx controllerlib.Context) error {
 	return nil
 }
 
-func (c *impersonatorConfigController) shouldHaveTLSSecret(config *impersonator.Config) bool {
+func (c *impersonatorConfigController) ensureTLSSecret(ctx controllerlib.Context, config *impersonator.Config) error {
-	return c.shouldHaveImpersonator(config)
+	secret, err := c.secretsInformer.Lister().Secrets(c.namespace).Get(c.tlsSecretName)
+	notFound := k8serrors.IsNotFound(err)
+	if notFound {
+		secret = nil
+	}
+	if !notFound && err != nil {
+		return err
+	}
+	//nolint:staticcheck // TODO remove this nolint when we fix the TODO below
+	if secret, err = c.deleteTLSCertificateWithWrongName(ctx.Context, config, secret); err != nil {
+		// TODO
+		// return err
+	}
+	if err = c.ensureTLSSecretIsCreatedAndLoaded(ctx.Context, config, secret); err != nil {
+		return err
+	}
+	return nil
 }
 
 func (c *impersonatorConfigController) shouldHaveImpersonator(config *impersonator.Config) bool {
@@ -196,6 +216,10 @@ func (c *impersonatorConfigController) shouldHaveLoadBalancer(config *impersonat
 	return c.shouldHaveImpersonator(config) && config.Endpoint == ""
 }
 
+func (c *impersonatorConfigController) shouldHaveTLSSecret(config *impersonator.Config) bool {
+	return c.shouldHaveImpersonator(config) // TODO is this the logic that we want here?
+}
+
 func (c *impersonatorConfigController) ensureImpersonatorIsStopped() error {
 	if c.server != nil {
 		plog.Info("Stopping impersonation proxy", "port", impersonationProxyPort)
@@ -266,26 +290,6 @@ func (c *impersonatorConfigController) tlsSecretExists() (bool, *v1.Secret, erro
 	return true, secret, nil
 }
 
-func (c *impersonatorConfigController) ensureLoadBalancerIsStopped(ctx context.Context) error {
-	running, err := c.isLoadBalancerRunning()
-	if err != nil {
-		return err
-	}
-	if !running {
-		return nil
-	}
-
-	plog.Info("Deleting load balancer for impersonation proxy",
-		"service", c.generatedLoadBalancerServiceName,
-		"namespace", c.namespace)
-	err = c.k8sClient.CoreV1().Services(c.namespace).Delete(ctx, c.generatedLoadBalancerServiceName, metav1.DeleteOptions{})
-	if err != nil {
-		return err
-	}
-
-	return nil
-}
-
 func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.Context) error {
 	running, err := c.isLoadBalancerRunning()
 	if err != nil {
@@ -323,56 +327,147 @@ func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.C
 	return nil
 }
 
-func (c *impersonatorConfigController) ensureTLSSecretIsCreated(ctx context.Context, config *impersonator.Config) error {
+func (c *impersonatorConfigController) ensureLoadBalancerIsStopped(ctx context.Context) error {
-	tlsSecretExists, tlsSecret, err := c.tlsSecretExists()
+	running, err := c.isLoadBalancerRunning()
 	if err != nil {
 		return err
 	}
-	if tlsSecretExists {
+	if !running {
-		certPEM := tlsSecret.Data[v1.TLSCertKey]
-		keyPEM := tlsSecret.Data[v1.TLSPrivateKeyKey]
-		tlsCert, _ := tls.X509KeyPair(certPEM, keyPEM)
-		// TODO handle err, like when the Secret did not contain the fields that we expected
-		c.setTLSCert(&tlsCert)
 		return nil
 	}
-	impersonationCA, err := certauthority.New(pkix.Name{CommonName: "test CA"}, 24*time.Hour) // TODO change the length of this
+
+	plog.Info("Deleting load balancer for impersonation proxy",
+		"service", c.generatedLoadBalancerServiceName,
+		"namespace", c.namespace)
+	err = c.k8sClient.CoreV1().Services(c.namespace).Delete(ctx, c.generatedLoadBalancerServiceName, metav1.DeleteOptions{})
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (c *impersonatorConfigController) deleteTLSCertificateWithWrongName(ctx context.Context, config *impersonator.Config, secret *v1.Secret) (*v1.Secret, error) {
+	if secret == nil {
+		// There is no Secret, so there is nothing to delete.
+		return secret, nil
+	}
+
+	certPEM := secret.Data[v1.TLSCertKey]
+	block, _ := pem.Decode(certPEM)
+	if block == nil {
+		// The certPEM is not valid.
+		return secret, nil // TODO what should we do?
+	}
+	parsed, _ := x509.ParseCertificate(block.Bytes)
+	// TODO handle err
+
+	desiredIPs, nameIsReady, err := c.findTLSCertificateName(config)
+	//nolint:staticcheck // TODO remove this nolint when we fix the TODO below
+	if err != nil {
+		// TODO return err
+	}
+	if !nameIsReady {
+		// We currently have a secret but we are waiting for a load balancer to be assigned an ingress, so
+		// our current secret must be old/unwanted.
+		err = c.ensureTLSSecretIsRemoved(ctx)
+		if err != nil {
+			return secret, err
+		}
+		return nil, nil
+	}
+
+	actualIPs := parsed.IPAddresses
+	// TODO handle multiple IPs, and handle when there is no IP
+	if desiredIPs[0].Equal(actualIPs[0]) {
+		// The cert matches the desired state, so we do not need to delete it.
+		return secret, nil
+	}
+
+	err = c.ensureTLSSecretIsRemoved(ctx)
+	if err != nil {
+		return secret, err
+	}
+	return nil, nil
+}
+
+func (c *impersonatorConfigController) ensureTLSSecretIsCreatedAndLoaded(ctx context.Context, config *impersonator.Config, secret *v1.Secret) error {
+	if secret != nil {
+		err := c.loadTLSCertFromSecret(secret)
+		if err != nil {
+			return err
+		}
+		return nil
+	}
+
+	// TODO create/save/watch the CA separately so we can reuse it to mint tls certs as the settings are dynamically changed,
+	//   so that clients don't need to be updated to use a different CA just because the server-side settings were changed.
+	impersonationCA, err := certauthority.New(pkix.Name{CommonName: "test CA"}, 24*time.Hour) // TODO change the expiration of this to 100 years
 	if err != nil {
 		return fmt.Errorf("could not create impersonation CA: %w", err)
 	}
-	var ips []net.IP
+
-	if config.Endpoint == "" { // TODO are there other cases where we need to do this?
+	ips, nameIsReady, err := c.findTLSCertificateName(config)
-		lb, err := c.servicesInformer.Lister().Services(c.namespace).Get(c.generatedLoadBalancerServiceName)
-		notFound := k8serrors.IsNotFound(err)
-		if notFound {
-			return nil
-		}
 	if err != nil {
 		return err
 	}
-		ingresses := lb.Status.LoadBalancer.Ingress
+	if !nameIsReady {
-		if len(ingresses) == 0 {
+		// Sync will get called again when the load balancer is updated with its ingress info, so this is not an error.
 		return nil
 	}
-		ip := ingresses[0].IP // TODO multiple ips
+
-		ips = []net.IP{net.ParseIP(ip)}
+	newTLSSecret, err := c.createNewTLSSecret(ctx, impersonationCA, ips)
-		// check with informer to get the ip address of the load balancer if its available
-		// if not, return
-	} else {
-		ips = []net.IP{net.ParseIP(config.Endpoint)}
-	}
-	impersonationCert, err := impersonationCA.Issue(pkix.Name{}, nil, ips, 24*time.Hour) // TODO change the length of this
 	if err != nil {
-		return fmt.Errorf("could not create impersonation cert: %w", err)
+		return err
 	}
 
-	c.setTLSCert(impersonationCert)
+	err = c.loadTLSCertFromSecret(newTLSSecret)
+	if err != nil {
+		return err
+	}
 
-	certPEM, keyPEM, err := certauthority.ToPEM(impersonationCert)
+	return nil
-	// TODO error handling?
+}
 
-	// TODO handle error on create
+func (c *impersonatorConfigController) findTLSCertificateName(config *impersonator.Config) ([]net.IP, bool, error) {
-	c.k8sClient.CoreV1().Secrets(c.namespace).Create(ctx, &v1.Secret{
+	var ips []net.IP
+	if config.Endpoint != "" {
+		// TODO Endpoint could be a hostname
+		// TODO Endpoint could have a port number in it, which we should parse out and ignore for this purpose
+		ips = []net.IP{net.ParseIP(config.Endpoint)}
+	} else {
+		lb, err := c.servicesInformer.Lister().Services(c.namespace).Get(c.generatedLoadBalancerServiceName)
+		notFound := k8serrors.IsNotFound(err)
+		if notFound {
+			// TODO is this an error? we should have already created the load balancer, so why would it not exist here?
+			return nil, false, nil
+		}
+		if err != nil {
+			return nil, false, err
+		}
+		ingresses := lb.Status.LoadBalancer.Ingress
+		if len(ingresses) == 0 {
+			plog.Info("load balancer for impersonation proxy does not have an ingress yet, so skipping tls cert generation while we wait",
+				"service", c.generatedLoadBalancerServiceName,
+				"namespace", c.namespace)
+			return nil, false, nil
+		}
+		ip := ingresses[0].IP // TODO handle multiple ips?
+		ips = []net.IP{net.ParseIP(ip)}
+	}
+	return ips, true, nil
+}
+
+func (c *impersonatorConfigController) createNewTLSSecret(ctx context.Context, ca *certauthority.CA, ips []net.IP) (*v1.Secret, error) {
+	impersonationCert, err := ca.Issue(pkix.Name{}, nil, ips, 24*time.Hour) // TODO change the length of this too 100 years for now?
+	if err != nil {
+		return nil, fmt.Errorf("could not create impersonation cert: %w", err)
+	}
+
+	certPEM, keyPEM, _ := certauthority.ToPEM(impersonationCert)
+	// TODO handle err
+
+	newTLSSecret := &v1.Secret{
 		Type: v1.SecretTypeTLS,
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      c.tlsSecretName,
@@ -380,11 +475,30 @@ func (c *impersonatorConfigController) ensureTLSSecretIsCreated(ctx context.Cont
 			Labels:    c.labels,
 		},
 		Data: map[string][]byte{
-			"ca.crt":            impersonationCA.Bundle(),
+			"ca.crt":            ca.Bundle(),
 			v1.TLSPrivateKeyKey: keyPEM,
 			v1.TLSCertKey:       certPEM,
 		},
-	}, metav1.CreateOptions{})
+	}
+	_, _ = c.k8sClient.CoreV1().Secrets(c.namespace).Create(ctx, newTLSSecret, metav1.CreateOptions{})
+	// TODO handle error on create
+	return newTLSSecret, nil
+}
+
+func (c *impersonatorConfigController) loadTLSCertFromSecret(tlsSecret *v1.Secret) error {
+	certPEM := tlsSecret.Data[v1.TLSCertKey]
+	keyPEM := tlsSecret.Data[v1.TLSPrivateKeyKey]
+	tlsCert, err := tls.X509KeyPair(certPEM, keyPEM)
+	if err != nil {
+		plog.Error("Could not parse TLS cert PEM data from Secret",
+			err,
+			"secret", c.tlsSecretName,
+			"namespace", c.namespace,
+		)
+		// TODO clear the secret if it was already set previously... c.setTLSCert(nil)
+		return fmt.Errorf("could not parse TLS cert PEM data from Secret: %w", err)
+	}
+	c.setTLSCert(&tlsCert)
 	return nil
 }
 
@@ -404,6 +518,8 @@ func (c *impersonatorConfigController) ensureTLSSecretIsRemoved(ctx context.Cont
 		return err
 	}
 
+	c.setTLSCert(nil)
+
 	return nil
 }
 

internal/controller/impersonatorconfig/impersonator_config_test.go

@@ -289,7 +289,6 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				return nil, startTLSListenerFuncError
 			}
 			var err error
-			//nolint: gosec // Intentionally binding to all network interfaces.
 			startedTLSListener, err = tls.Listen(network, "127.0.0.1:0", config) // automatically choose the port for unit tests
 			r.NoError(err)
 			return &tlsListenerWrapper{listener: startedTLSListener, closeError: startTLSListenerUponCloseError}, nil
@@ -306,13 +305,26 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			}
 		}
 
-		var requireTLSServerIsRunning = func(caCrt []byte) {
+		var requireTLSServerIsRunning = func(caCrt []byte, addr string, dnsOverrides map[string]string) {
 			r.Greater(startTLSListenerFuncWasCalled, 0)
 
+			realDialer := &net.Dialer{}
+			overrideDialContext := func(ctx context.Context, network, addr string) (net.Conn, error) {
+				replacementAddr, hasKey := dnsOverrides[addr]
+				if hasKey {
+					t.Logf("DialContext replacing addr %s with %s", addr, replacementAddr)
+					addr = replacementAddr
+				} else if dnsOverrides != nil {
+					t.Fatal("dnsOverrides was provided but not used, which was probably a mistake")
+				}
+				return realDialer.DialContext(ctx, network, addr)
+			}
+
 			var tr *http.Transport
 			if caCrt == nil {
 				tr = &http.Transport{
 					TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec
+					DialContext:     overrideDialContext,
 				}
 			} else {
 				rootCAs := x509.NewCertPool()
@@ -320,10 +332,11 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 
 				tr = &http.Transport{
 					TLSClientConfig: &tls.Config{RootCAs: rootCAs},
+					DialContext:     overrideDialContext,
 				}
 			}
 			client := &http.Client{Transport: tr}
-			url := "https://" + startedTLSListener.Addr().String()
+			url := "https://" + addr
 			req, err := http.NewRequestWithContext(context.Background(), "GET", url, nil)
 			r.NoError(err)
 			resp, err := client.Do(req)
@@ -347,7 +360,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			url := "https://" + startedTLSListener.Addr().String()
 			req, err := http.NewRequestWithContext(context.Background(), "GET", url, nil)
 			r.NoError(err)
-			_, err = client.Do(req)
+			_, err = client.Do(req) //nolint:bodyclose
 			r.Error(err)
 			r.Regexp("Get .*: remote error: tls: unrecognized name", err.Error())
 		}
@@ -357,7 +370,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			_, err := tls.Dial(
 				startedTLSListener.Addr().Network(),
 				startedTLSListener.Addr().String(),
-				&tls.Config{InsecureSkipVerify: true}, //nolint:gosec // TODO once we're using certs, do not skip verify
+				&tls.Config{InsecureSkipVerify: true}, //nolint:gosec
 			)
 			r.Error(err)
 			r.Regexp(`dial tcp .*: connect: connection refused`, err.Error())
@@ -380,6 +393,13 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			}, 10*time.Second, time.Millisecond)
 		}
 
+		var waitForTLSCertSecretToBeDeleted = func(informer corev1informers.SecretInformer, name string) {
+			r.Eventually(func() bool {
+				_, err := informer.Lister().Secrets(installedInNamespace).Get(name)
+				return k8serrors.IsNotFound(err)
+			}, 10*time.Second, time.Millisecond)
+		}
+
 		// Defer starting the informers until the last possible moment so that the
 		// nested Before's can keep adding things to the informer caches.
 		var startInformersAndController = func() {
@@ -477,6 +497,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			impersonationCert, err := impersonationCA.Issue(pkix.Name{}, nil, []net.IP{net.ParseIP("127.0.0.1")}, 24*time.Hour)
 			r.NoError(err)
 			certPEM, keyPEM, err := certauthority.ToPEM(impersonationCert)
+			r.NoError(err)
 
 			return &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
@@ -512,6 +533,31 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			r.NoError(client.Tracker().Add(loadBalancerService))
 		}
 
+		var updateLoadBalancerServiceInTracker = func(resourceName, lbIngressIP, newResourceVersion string) {
+			loadBalancerService := &corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            resourceName,
+					Namespace:       installedInNamespace,
+					ResourceVersion: newResourceVersion,
+				},
+				Spec: corev1.ServiceSpec{
+					Type: corev1.ServiceTypeLoadBalancer,
+				},
+				Status: corev1.ServiceStatus{
+					LoadBalancer: corev1.LoadBalancerStatus{
+						Ingress: []corev1.LoadBalancerIngress{
+							{IP: lbIngressIP},
+						},
+					},
+				},
+			}
+			r.NoError(kubeInformerClient.Tracker().Update(
+				schema.GroupVersionResource{Version: "v1", Resource: "services"},
+				loadBalancerService,
+				installedInNamespace,
+			))
+		}
+
 		var addLoadBalancerServiceWithIPToTracker = func(resourceName string, client *kubernetesfake.Clientset) {
 			loadBalancerService := &corev1.Service{
 				ObjectMeta: metav1.ObjectMeta{
@@ -544,6 +590,20 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			))
 		}
 
+		var deleteTLSCertSecretFromTracker = func(resourceName string, client *kubernetesfake.Clientset) {
+			r.NoError(client.Tracker().Delete(
+				schema.GroupVersionResource{Version: "v1", Resource: "secrets"},
+				installedInNamespace,
+				resourceName,
+			))
+		}
+
+		var addSecretFromCreateActionToTracker = func(action coretesting.Action, client *kubernetesfake.Clientset, resourceVersion string) {
+			createdSecret := action.(coretesting.CreateAction).GetObject().(*corev1.Secret)
+			createdSecret.ResourceVersion = resourceVersion
+			r.NoError(client.Tracker().Add(createdSecret))
+		}
+
 		var addNodeWithRoleToTracker = func(role string) {
 			r.NoError(kubeAPIClient.Tracker().Add(
 				&corev1.Node{
@@ -740,18 +800,16 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				r.Equal(1, startTLSListenerFuncWasCalled) // wasn't started a second time
 				r.Len(kubeAPIClient.Actions(), 3)
 				ca := requireTLSSecretWasCreated(kubeAPIClient.Actions()[2])
-				requireTLSServerIsRunning(ca) // running with certs now
+				requireTLSServerIsRunning(ca, startedTLSListener.Addr().String(), nil) // running with certs now
 
 				// update manually because the kubeAPIClient isn't connected to the informer in the tests
-				createdSecret := kubeAPIClient.Actions()[2].(coretesting.CreateAction).GetObject().(*corev1.Secret)
+				addSecretFromCreateActionToTracker(kubeAPIClient.Actions()[2], kubeInformerClient, "1")
-				createdSecret.ResourceVersion = "1"
-				r.NoError(kubeInformerClient.Tracker().Add(createdSecret))
 				waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().Secrets().Informer(), "1")
 
 				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
 				r.Equal(1, startTLSListenerFuncWasCalled)                              // wasn't started a third time
 				r.Len(kubeAPIClient.Actions(), 3)                                      // no more actions
-				requireTLSServerIsRunning(ca)             // still running
+				requireTLSServerIsRunning(ca, startedTLSListener.Addr().String(), nil) // still running
 			})
 		})
 
@@ -794,8 +852,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					addImpersonatorConfigMapToTracker(configMapResourceName, here.Doc(`
 						mode: auto
 						endpoint: 127.0.0.1
-					  `), // TODO what to do about ports
+					  `),
-					// TODO IP address and hostname should work
 					)
 				})
 
@@ -824,7 +881,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 						r.Len(kubeAPIClient.Actions(), 2)
 						requireNodesListed(kubeAPIClient.Actions()[0])
 						ca := requireTLSSecretWasCreated(kubeAPIClient.Actions()[1])
-						requireTLSServerIsRunning(ca)
+						requireTLSServerIsRunning(ca, startedTLSListener.Addr().String(), nil)
 					})
 				})
 			})
@@ -900,12 +957,13 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					})
 				})
 
-				when("a load balancer and a secret already exist", func() {
+				when("a load balancer and a secret already exists", func() {
-					var tlsSecret *corev1.Secret
+					var ca []byte
 					it.Before(func() {
 						addImpersonatorConfigMapToTracker(configMapResourceName, "mode: enabled")
 						addNodeWithRoleToTracker("worker")
-						tlsSecret = createActualTLSSecret(tlsSecretName)
+						tlsSecret := createActualTLSSecret(tlsSecretName)
+						ca = tlsSecret.Data["ca.crt"]
 						r.NoError(kubeAPIClient.Tracker().Add(tlsSecret))
 						r.NoError(kubeInformerClient.Tracker().Add(tlsSecret))
 						addLoadBalancerServiceWithIPToTracker(generatedLoadBalancerServiceName, kubeInformerClient)
@@ -917,7 +975,29 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 						r.NoError(controllerlib.TestSync(t, subject, *syncContext))
 						r.Len(kubeAPIClient.Actions(), 1)
 						requireNodesListed(kubeAPIClient.Actions()[0])
-						requireTLSServerIsRunning(tlsSecret.Data["ca.crt"])
+						requireTLSServerIsRunning(ca, startedTLSListener.Addr().String(), nil)
+					})
+				})
+
+				when("a load balancer and a secret already exists but the tls secret is not valid", func() {
+					var tlsSecret *corev1.Secret
+					it.Before(func() {
+						addImpersonatorConfigMapToTracker(configMapResourceName, "mode: enabled")
+						addNodeWithRoleToTracker("worker")
+						tlsSecret = createStubTLSSecret(tlsSecretName) // secret exists but lacks certs
+						r.NoError(kubeAPIClient.Tracker().Add(tlsSecret))
+						r.NoError(kubeInformerClient.Tracker().Add(tlsSecret))
+						addLoadBalancerServiceWithIPToTracker(generatedLoadBalancerServiceName, kubeInformerClient)
+						addLoadBalancerServiceWithIPToTracker(generatedLoadBalancerServiceName, kubeAPIClient)
+					})
+
+					it("returns an error and leaves the impersonator running without tls certs", func() {
+						startInformersAndController()
+						r.EqualError(controllerlib.TestSync(t, subject, *syncContext),
+							"could not parse TLS cert PEM data from Secret: tls: failed to find any PEM data in certificate input")
+						r.Len(kubeAPIClient.Actions(), 1)
+						requireNodesListed(kubeAPIClient.Actions()[0])
+						requireTLSServerIsRunningWithoutCerts()
 					})
 				})
 			})
@@ -984,39 +1064,67 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				it.Before(func() {
 					addImpersonatorConfigMapToTracker(configMapResourceName, here.Doc(`
 						mode: enabled
-						endpoint: https://proxy.example.com:8443/
+						endpoint: 127.0.0.1
 					  `))
 					addNodeWithRoleToTracker("worker")
 				})
 
-				it("doesn't start, then creates, then deletes the load balancer", func() {
+				it("doesn't create, then creates, then deletes the load balancer", func() {
 					startInformersAndController()
 
+					// Should have started in "enabled" mode with an "endpoint", so no load balancer is needed.
 					r.NoError(controllerlib.TestSync(t, subject, *syncContext))
-
+					r.Len(kubeAPIClient.Actions(), 2)
-					r.Len(kubeAPIClient.Actions(), 1)
 					requireNodesListed(kubeAPIClient.Actions()[0])
+					ca := requireTLSSecretWasCreated(kubeAPIClient.Actions()[1]) // created immediately because "endpoint" was specified
+					requireTLSServerIsRunning(ca, startedTLSListener.Addr().String(), nil)
 
+					// update manually because the kubeAPIClient isn't connected to the informer in the tests
+					addSecretFromCreateActionToTracker(kubeAPIClient.Actions()[1], kubeInformerClient, "1")
+					waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().Secrets().Informer(), "1")
+
+					// Switch to "enabled" mode without an "endpoint", so a load balancer is needed now.
 					updateImpersonatorConfigMapInTracker(configMapResourceName, "mode: enabled", "1")
 					waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().ConfigMaps().Informer(), "1")
 
 					r.NoError(controllerlib.TestSync(t, subject, *syncContext))
-					r.Len(kubeAPIClient.Actions(), 2)
+					r.Len(kubeAPIClient.Actions(), 4)
-					requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1])
+					requireLoadBalancerWasCreated(kubeAPIClient.Actions()[2])
+					requireTLSSecretDeleted(kubeAPIClient.Actions()[3]) // the Secret was deleted because it contained a cert with the wrong IP
+					requireTLSServerIsRunningWithoutCerts()
 
 					// update manually because the kubeAPIClient isn't connected to the informer in the tests
 					addLoadBalancerServiceToTracker(generatedLoadBalancerServiceName, kubeInformerClient)
 					waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().Services().Informer(), "0")
+					deleteTLSCertSecretFromTracker(tlsSecretName, kubeInformerClient)
+					waitForTLSCertSecretToBeDeleted(kubeInformers.Core().V1().Secrets(), tlsSecretName)
 
+					// The controller should be waiting for the load balancer's ingress to become available.
+					r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+					r.Len(kubeAPIClient.Actions(), 4) // no new actions while it is waiting for the load balancer's ingress
+					requireTLSServerIsRunningWithoutCerts()
+
+					// Update the ingress of the LB in the informer's client and run Sync again.
+					fakeIP := "127.0.0.123"
+					updateLoadBalancerServiceInTracker(generatedLoadBalancerServiceName, fakeIP, "1")
+					waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().Services().Informer(), "1")
+					r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+					r.Len(kubeAPIClient.Actions(), 5)
+					ca = requireTLSSecretWasCreated(kubeAPIClient.Actions()[4]) // created because the LB ingress became available
+					// Check that the server is running and that TLS certs that are being served are are for fakeIP.
+					requireTLSServerIsRunning(ca, fakeIP, map[string]string{fakeIP + ":443": startedTLSListener.Addr().String()})
+
+					// Now switch back to having the "endpoint" specified, so the load balancer is not needed anymore.
 					updateImpersonatorConfigMapInTracker(configMapResourceName, here.Doc(`
 						mode: enabled
-						endpoint: https://proxy.example.com:8443/
+						endpoint: 127.0.0.1
 					  `), "2")
 					waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().ConfigMaps().Informer(), "2")
 
 					r.NoError(controllerlib.TestSync(t, subject, *syncContext))
-					r.Len(kubeAPIClient.Actions(), 3)
+					r.Len(kubeAPIClient.Actions(), 7)
-					requireLoadBalancerDeleted(kubeAPIClient.Actions()[2])
+					requireLoadBalancerDeleted(kubeAPIClient.Actions()[5])
+					requireTLSSecretWasCreated(kubeAPIClient.Actions()[6]) // recreated because the endpoint was updated
 				})
 			})
 		})