From bbbb40994d085e2f182658dd66aae63f80333755 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 25 Feb 2021 17:03:34 -0800 Subject: [PATCH] Prefer hostnames over IPs when making certs to match load balancer ingress Signed-off-by: Margo Crawford --- .../impersonatorconfig/impersonator_config.go | 86 +++++----- .../impersonator_config_test.go | 152 +++++++++++++++--- 2 files changed, 182 insertions(+), 56 deletions(-) diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index c428b26c..abda84a4 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/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, - "desiredHostnames", desiredHostnames, + "desiredIP", desiredIP, + "desiredHostname", desiredHostname, "actualIPs", actualIPs, "actualHostnames", actualHostnames, "secret", c.tlsSecretName, "namespace", c.namespace) - // TODO handle multiple IPs - 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. + if certHostnameAndIPMatchDesiredState(desiredIP, actualIPs, desiredHostname, actualHostnames) { 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) { - var ips []net.IP - var hostnames []string +func (c *impersonatorConfigController) findTLSCertificateNameFromEndpointConfig(config *impersonator.Config) (net.IP, string, bool, error) { + // TODO Endpoint could have a port number in it, which we should parse out and ignore for this purpose parsedAsIP := net.ParseIP(config.Endpoint) if parsedAsIP != nil { - ips = []net.IP{parsedAsIP} - } else { - hostnames = []string{config.Endpoint} + return parsedAsIP, "", true, nil } - // TODO Endpoint could have a port number in it, which we should parse out and ignore for this purpose - return ips, hostnames, true, nil + return nil, config.Endpoint, true, nil } -func (c *impersonatorConfigController) findTLSCertificateNameFromLoadBalancer() ([]net.IP, []string, bool, error) { - var ips []net.IP - var hostnames []string +func (c *impersonatorConfigController) findTLSCertificateNameFromLoadBalancer() (net.IP, string, bool, error) { 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, nil, false, nil + // Maybe the loadbalancer hasn't been cached in the informer yet. We aren't ready and will try again later. + 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 - if hostname != "" { - return nil, []string{hostname}, true, nil + for _, ingress := range ingresses { + hostname := ingress.Hostname + if hostname != "" { + return nil, hostname, true, nil + } } - ip := ingresses[0].IP - parsedIP := net.ParseIP(ip) - if parsedIP == nil { - return nil, nil, false, fmt.Errorf("could not parse IP address from load balancer %s: %s", lb.Name, ip) + for _, ingress := range ingresses { + ip := ingress.IP + parsedIP := net.ParseIP(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) { diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index b5b26355..20512aad 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/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{ - { - IP: ip, - Hostname: hostname, - }, - }, + Ingress: ingress, }, }, } @@ -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, "", "", kubeAPIClient) + addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "", Hostname: ""}}, kubeInformerClient) + 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, "not-an-ip", "", kubeAPIClient) + addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "not-an-ip"}}, kubeInformerClient) + 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, "127.0.0.1", "", kubeAPIClient) + addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeInformerClient) + 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, "127.0.0.1", "", kubeAPIClient) + addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeInformerClient) + addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeAPIClient) }) it("returns an error and leaves the impersonator running without tls certs", func() {