Prefer hostnames over IPs when making certs to match load balancer ingress

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

internal/controller/impersonatorconfig/impersonator_config.go

@@ -217,7 +217,17 @@ func (c *impersonatorConfigController) shouldHaveLoadBalancer(config *impersonat
 }
 
 func (c *impersonatorConfigController) shouldHaveTLSSecret(config *impersonator.Config) bool {
-	return c.shouldHaveImpersonator(config) // TODO is this the logic that we want here?
+	return c.shouldHaveImpersonator(config)
+}
+
+func certHostnameAndIPMatchDesiredState(desiredIP net.IP, actualIPs []net.IP, desiredHostname string, actualHostnames []string) bool {
+	if desiredIP != nil && len(actualIPs) == 1 && desiredIP.Equal(actualIPs[0]) && len(actualHostnames) == 0 {
+		return true
+	}
+	if desiredHostname != "" && len(actualHostnames) == 1 && desiredHostname == actualHostnames[0] && len(actualIPs) == 0 {
+		return true
+	}
+	return false
 }
 
 func (c *impersonatorConfigController) ensureImpersonatorIsStopped() error {
@@ -357,12 +367,12 @@ func (c *impersonatorConfigController) deleteTLSCertificateWithWrongName(ctx con
 	block, _ := pem.Decode(certPEM)
 	if block == nil {
 		// The certPEM is not valid.
-		return secret, nil // TODO what should we do?
+		return secret, nil // TODO delete this so the controller will create a valid one
 	}
 	actualCertFromSecret, _ := x509.ParseCertificate(block.Bytes)
 	// TODO handle err
 
-	desiredIPs, desiredHostnames, nameIsReady, err := c.findTLSCertificateName(config)
+	desiredIP, desiredHostname, nameIsReady, err := c.findDesiredTLSCertificateName(config)
 	//nolint:staticcheck // TODO remove this nolint when we fix the TODO below
 	if err != nil {
 		// TODO return err
@@ -380,22 +390,16 @@ func (c *impersonatorConfigController) deleteTLSCertificateWithWrongName(ctx con
 	actualIPs := actualCertFromSecret.IPAddresses
 	actualHostnames := actualCertFromSecret.DNSNames
 	plog.Info("Checking TLS certificate names",
-		"desiredIPs", desiredIPs,
+		"desiredIP", desiredIP,
-		"desiredHostnames", desiredHostnames,
+		"desiredHostname", desiredHostname,
 		"actualIPs", actualIPs,
 		"actualHostnames", actualHostnames,
 		"secret", c.tlsSecretName,
 		"namespace", c.namespace)
 
-	// TODO handle multiple IPs
+	if certHostnameAndIPMatchDesiredState(desiredIP, actualIPs, desiredHostname, actualHostnames) {
-	if len(desiredIPs) == len(actualIPs) && len(desiredIPs) == 1 && desiredIPs[0].Equal(actualIPs[0]) { //nolint:gocritic
-		// The cert matches the desired state, so we do not need to delete it.
 		return secret, nil
 	}
-	if len(desiredHostnames) == len(actualHostnames) && len(desiredHostnames) == 1 && desiredHostnames[0] == actualHostnames[0] { //nolint:gocritic
-		return secret, nil
-	}
-
 	err = c.ensureTLSSecretIsRemoved(ctx)
 	if err != nil {
 		return secret, err
@@ -419,7 +423,7 @@ func (c *impersonatorConfigController) ensureTLSSecretIsCreatedAndLoaded(ctx con
 		return fmt.Errorf("could not create impersonation CA: %w", err)
 	}
 
-	ips, hostnames, nameIsReady, err := c.findTLSCertificateName(config)
+	ip, hostname, nameIsReady, err := c.findDesiredTLSCertificateName(config)
 	if err != nil {
 		return err
 	}
@@ -428,6 +432,14 @@ func (c *impersonatorConfigController) ensureTLSSecretIsCreatedAndLoaded(ctx con
 		return nil
 	}
 
+	var hostnames []string
+	var ips []net.IP
+	if hostname != "" {
+		hostnames = []string{hostname}
+	}
+	if ip != nil {
+		ips = []net.IP{ip}
+	}
 	newTLSSecret, err := c.createNewTLSSecret(ctx, impersonationCA, ips, hostnames)
 	if err != nil {
 		return err
@@ -441,56 +453,54 @@ func (c *impersonatorConfigController) ensureTLSSecretIsCreatedAndLoaded(ctx con
 	return nil
 }
 
-func (c *impersonatorConfigController) findTLSCertificateName(config *impersonator.Config) ([]net.IP, []string, bool, error) {
+func (c *impersonatorConfigController) findDesiredTLSCertificateName(config *impersonator.Config) (net.IP, string, bool, error) {
 	if config.Endpoint != "" {
 		return c.findTLSCertificateNameFromEndpointConfig(config)
 	}
 	return c.findTLSCertificateNameFromLoadBalancer()
 }
 
-func (c *impersonatorConfigController) findTLSCertificateNameFromEndpointConfig(config *impersonator.Config) ([]net.IP, []string, bool, error) {
+func (c *impersonatorConfigController) findTLSCertificateNameFromEndpointConfig(config *impersonator.Config) (net.IP, string, bool, error) {
-	var ips []net.IP
+	// TODO Endpoint could have a port number in it, which we should parse out and ignore for this purpose
-	var hostnames []string
 	parsedAsIP := net.ParseIP(config.Endpoint)
 	if parsedAsIP != nil {
-		ips = []net.IP{parsedAsIP}
+		return parsedAsIP, "", true, nil
-	} else {
-		hostnames = []string{config.Endpoint}
 	}
-	// TODO Endpoint could have a port number in it, which we should parse out and ignore for this purpose
+	return nil, config.Endpoint, true, nil
-	return ips, hostnames, true, nil
 }
 
-func (c *impersonatorConfigController) findTLSCertificateNameFromLoadBalancer() ([]net.IP, []string, bool, error) {
+func (c *impersonatorConfigController) findTLSCertificateNameFromLoadBalancer() (net.IP, string, bool, error) {
-	var ips []net.IP
-	var hostnames []string
 	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?
+		// Maybe the loadbalancer hasn't been cached in the informer yet. We aren't ready and will try again later.
-		return nil, nil, false, nil
+		return nil, "", false, nil
 	}
 	if err != nil {
-		return nil, nil, false, err
+		return nil, "", false, err
 	}
 	ingresses := lb.Status.LoadBalancer.Ingress
 	if len(ingresses) == 0 || (ingresses[0].Hostname == "" && ingresses[0].IP == "") {
 		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, nil, false, nil
+		return nil, "", false, nil
 	}
-	hostname := ingresses[0].Hostname
+	for _, ingress := range ingresses {
-	if hostname != "" {
+		hostname := ingress.Hostname
-		return nil, []string{hostname}, true, nil
+		if hostname != "" {
+			return nil, hostname, true, nil
+		}
 	}
-	ip := ingresses[0].IP
+	for _, ingress := range ingresses {
-	parsedIP := net.ParseIP(ip)
+		ip := ingress.IP
-	if parsedIP == nil {
+		parsedIP := net.ParseIP(ip)
-		return nil, nil, false, fmt.Errorf("could not parse IP address from load balancer %s: %s", lb.Name, ip)
+		if parsedIP != nil {
+			return parsedIP, "", true, nil
+		}
 	}
-	ips = []net.IP{parsedIP}
+
-	return ips, hostnames, true, nil
+	return nil, "", false, fmt.Errorf("could not find valid IP addresses or hostnames from load balancer %s/%s", c.namespace, lb.Name)
 }
 
 func (c *impersonatorConfigController) createNewTLSSecret(ctx context.Context, ca *certauthority.CA, ips []net.IP, hostnames []string) (*v1.Secret, error) {

internal/controller/impersonatorconfig/impersonator_config_test.go

@@ -516,6 +516,31 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			}
 		}
 
+		var createTLSSecretWithMultipleHostnames = func(resourceName string, ip string) *corev1.Secret {
+			impersonationCA, err := certauthority.New(pkix.Name{CommonName: "test CA"}, 24*time.Hour)
+			r.NoError(err)
+			impersonationCert, err := impersonationCA.Issue(pkix.Name{}, []string{"foo", "bar"}, []net.IP{net.ParseIP(ip)}, 24*time.Hour)
+			r.NoError(err)
+			certPEM, keyPEM, err := certauthority.ToPEM(impersonationCert)
+			r.NoError(err)
+
+			return &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      resourceName,
+					Namespace: installedInNamespace,
+					// Note that this seems to be ignored by the informer during initial creation, so actually
+					// the informer will see this as resource version "". Leaving it here to express the intent
+					// that the initial version is version 0.
+					ResourceVersion: "0",
+				},
+				Data: map[string][]byte{
+					"ca.crt":                impersonationCA.Bundle(),
+					corev1.TLSPrivateKeyKey: keyPEM,
+					corev1.TLSCertKey:       certPEM,
+				},
+			}
+		}
+
 		var addLoadBalancerServiceToTracker = func(resourceName string, client *kubernetesfake.Clientset) {
 			loadBalancerService := &corev1.Service{
 				ObjectMeta: metav1.ObjectMeta{
@@ -558,7 +583,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			))
 		}
 
-		var addLoadBalancerServiceWithIngressToTracker = func(resourceName string, ip string, hostname string, client *kubernetesfake.Clientset) {
+		var addLoadBalancerServiceWithIngressToTracker = func(resourceName string, ingress []corev1.LoadBalancerIngress, client *kubernetesfake.Clientset) {
 			loadBalancerService := &corev1.Service{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      resourceName,
@@ -573,12 +598,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				},
 				Status: corev1.ServiceStatus{
 					LoadBalancer: corev1.LoadBalancerStatus{
-						Ingress: []corev1.LoadBalancerIngress{
+						Ingress: ingress,
-							{
-								IP:       ip,
-								Hostname: hostname,
-							},
-						},
 					},
 				},
 			}
@@ -766,8 +786,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			when("there are not visible control plane nodes and a load balancer already exists with empty ingress", func() {
 				it.Before(func() {
 					addNodeWithRoleToTracker("worker")
-					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, "", "", kubeInformerClient)
+					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "", Hostname: ""}}, kubeInformerClient)
-					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, "", "", kubeAPIClient)
+					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "", Hostname: ""}}, kubeAPIClient)
 					startInformersAndController()
 					r.NoError(controllerlib.TestSync(t, subject, *syncContext))
 				})
@@ -785,10 +805,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			when("there are not visible control plane nodes and a load balancer already exists with invalid ip", func() {
 				it.Before(func() {
 					addNodeWithRoleToTracker("worker")
-					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, "not-an-ip", "", kubeInformerClient)
+					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "not-an-ip"}}, kubeInformerClient)
-					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, "not-an-ip", "", kubeAPIClient)
+					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "not-an-ip"}}, kubeAPIClient)
 					startInformersAndController()
-					r.EqualError(controllerlib.TestSync(t, subject, *syncContext), "could not parse IP address from load balancer some-service-resource-name: not-an-ip")
+					r.EqualError(controllerlib.TestSync(t, subject, *syncContext), "could not find valid IP addresses or hostnames from load balancer some-namespace/some-service-resource-name")
 				})
 
 				it("starts the impersonator without tls certs", func() {
@@ -800,6 +820,102 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					requireNodesListed(kubeAPIClient.Actions()[0])
 				})
 			})
+
+			when("there are not visible control plane nodes and a load balancer already exists with multiple ips", func() {
+				it.Before(func() {
+					addNodeWithRoleToTracker("worker")
+					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.123"}, {IP: "127.0.0.456"}}, kubeInformerClient)
+					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.123"}, {IP: "127.0.0.456"}}, kubeAPIClient)
+					startInformersAndController()
+					r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+				})
+
+				it("starts the impersonator with certs that match the first IP address", func() {
+					r.Len(kubeAPIClient.Actions(), 2)
+					requireNodesListed(kubeAPIClient.Actions()[0])
+					ca := requireTLSSecretWasCreated(kubeAPIClient.Actions()[1])
+					requireTLSServerIsRunning(ca, "127.0.0.123", map[string]string{"127.0.0.123:443": startedTLSListener.Addr().String()})
+				})
+
+				it("keeps the secret around after resync", func() {
+					addSecretFromCreateActionToTracker(kubeAPIClient.Actions()[1], kubeInformerClient, "0")
+					waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().Secrets().Informer(), "0")
+					r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+					r.Len(kubeAPIClient.Actions(), 2) // nothing changed
+				})
+			})
+
+			when("there are not visible control plane nodes and a load balancer already exists with multiple hostnames", func() {
+				firstHostname := "fake-1.example.com"
+				it.Before(func() {
+					addNodeWithRoleToTracker("worker")
+					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{Hostname: firstHostname}, {Hostname: "fake-2.example.com"}}, kubeInformerClient)
+					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{Hostname: firstHostname}, {Hostname: "fake-2.example.com"}}, kubeAPIClient)
+					startInformersAndController()
+					r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+				})
+
+				it("starts the impersonator with certs that match the first hostname", func() {
+					r.Len(kubeAPIClient.Actions(), 2)
+					requireNodesListed(kubeAPIClient.Actions()[0])
+					ca := requireTLSSecretWasCreated(kubeAPIClient.Actions()[1])
+					requireTLSServerIsRunning(ca, firstHostname, map[string]string{firstHostname + ":443": startedTLSListener.Addr().String()})
+				})
+
+				it("keeps the secret around after resync", func() {
+					addSecretFromCreateActionToTracker(kubeAPIClient.Actions()[1], kubeInformerClient, "0")
+					waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().Secrets().Informer(), "0")
+					r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+					r.Len(kubeAPIClient.Actions(), 2) // nothing changed
+				})
+			})
+
+			when("there are not visible control plane nodes and a load balancer already exists with hostnames and ips", func() {
+				firstHostname := "fake-1.example.com"
+				it.Before(func() {
+					addNodeWithRoleToTracker("worker")
+					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.254"}, {Hostname: firstHostname}}, kubeInformerClient)
+					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.254"}, {Hostname: firstHostname}}, kubeAPIClient)
+					startInformersAndController()
+					r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+				})
+
+				it("starts the impersonator with certs that match the first hostname", func() {
+					r.Len(kubeAPIClient.Actions(), 2)
+					requireNodesListed(kubeAPIClient.Actions()[0])
+					ca := requireTLSSecretWasCreated(kubeAPIClient.Actions()[1])
+					requireTLSServerIsRunning(ca, firstHostname, map[string]string{firstHostname + ":443": startedTLSListener.Addr().String()})
+				})
+
+				it("keeps the secret around after resync", func() {
+					addSecretFromCreateActionToTracker(kubeAPIClient.Actions()[1], kubeInformerClient, "0")
+					waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().Secrets().Informer(), "0")
+					r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+					r.Len(kubeAPIClient.Actions(), 2) // nothing changed
+				})
+			})
+
+			when("there are not visible control plane nodes, a secret exists with multiple hostnames and an IP", func() {
+				var tlsSecret *corev1.Secret
+				it.Before(func() {
+					addNodeWithRoleToTracker("worker")
+					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeInformerClient)
+					addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeAPIClient)
+					tlsSecret = createTLSSecretWithMultipleHostnames(tlsSecretName, "127.0.0.1")
+					r.NoError(kubeAPIClient.Tracker().Add(tlsSecret))
+					r.NoError(kubeInformerClient.Tracker().Add(tlsSecret))
+					startInformersAndController()
+					r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+				})
+
+				it("deletes and recreates the secret to match the IP in the load balancer without the extra hostnames", func() {
+					r.Len(kubeAPIClient.Actions(), 3)
+					requireNodesListed(kubeAPIClient.Actions()[0])
+					requireTLSSecretDeleted(kubeAPIClient.Actions()[1])
+					ca := requireTLSSecretWasCreated(kubeAPIClient.Actions()[2])
+					requireTLSServerIsRunning(ca, startedTLSListener.Addr().String(), nil)
+				})
+			})
 		})
 
 		when("sync is called more than once", func() {
@@ -834,7 +950,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				requireTLSServerIsRunningWithoutCerts()
 
 				// update manually because the kubeAPIClient isn't connected to the informer in the tests
-				addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, "127.0.0.1", "", kubeInformerClient)
+				addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeInformerClient)
 				waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().Services().Informer(), "0")
 
 				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
@@ -863,7 +979,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				requireTLSServerIsRunningWithoutCerts()
 
 				// update manually because the kubeAPIClient isn't connected to the informer in the tests
-				addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, "127.0.0.1", hostname, kubeInformerClient)
+				addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1", Hostname: hostname}}, kubeInformerClient)
 				waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().Services().Informer(), "0")
 
 				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
@@ -1036,8 +1152,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 						ca = tlsSecret.Data["ca.crt"]
 						r.NoError(kubeAPIClient.Tracker().Add(tlsSecret))
 						r.NoError(kubeInformerClient.Tracker().Add(tlsSecret))
-						addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, "127.0.0.1", "", kubeInformerClient)
+						addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeInformerClient)
-						addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, "127.0.0.1", "", kubeAPIClient)
+						addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeAPIClient)
 					})
 
 					it("starts the impersonator with the existing tls certs, does not start loadbalancer or make tls secret", func() {
@@ -1057,8 +1173,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 						tlsSecret = createStubTLSSecret(tlsSecretName) // secret exists but lacks certs
 						r.NoError(kubeAPIClient.Tracker().Add(tlsSecret))
 						r.NoError(kubeInformerClient.Tracker().Add(tlsSecret))
-						addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, "127.0.0.1", "", kubeInformerClient)
+						addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeInformerClient)
-						addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, "127.0.0.1", "", kubeAPIClient)
+						addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeAPIClient)
 					})
 
 					it("returns an error and leaves the impersonator running without tls certs", func() {