From e2fad6932f09a39802dff717d5544c8f885872bf Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Tue, 25 May 2021 17:01:42 -0700 Subject: [PATCH] multiple cluster ips --- .../impersonatorconfig/impersonator_config.go | 46 ++++++++++--------- .../impersonator_config_test.go | 44 +++++++++++++++++- 2 files changed, 68 insertions(+), 22 deletions(-) diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index 3d3bcd3c..2e9b1861 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -193,7 +193,7 @@ type certNameInfo struct { // The IP address or hostname which was selected to be used as the name in the cert. // Either selectedIP or selectedHostname will be set, but not both. - selectedIP net.IP + selectedIPs []net.IP selectedHostname string // The name of the endpoint to which a client should connect to talk to the impersonator. @@ -636,14 +636,14 @@ func (c *impersonatorConfigController) deleteTLSSecretWhenCertificateDoesNotMatc actualIPs := actualCertFromSecret.IPAddresses actualHostnames := actualCertFromSecret.DNSNames plog.Info("Checking TLS certificate names", - "desiredIP", nameInfo.selectedIP, + "desiredIPs", nameInfo.selectedIPs, "desiredHostname", nameInfo.selectedHostname, "actualIPs", actualIPs, "actualHostnames", actualHostnames, "secret", c.tlsSecretName, "namespace", c.namespace) - if certHostnameAndIPMatchDesiredState(nameInfo.selectedIP, actualIPs, nameInfo.selectedHostname, actualHostnames) { + if certHostnameAndIPMatchDesiredState(nameInfo.selectedIPs, actualIPs, nameInfo.selectedHostname, actualHostnames) { // The cert already matches the desired state, so there is no need to delete/recreate it. return false, nil } @@ -654,8 +654,13 @@ func (c *impersonatorConfigController) deleteTLSSecretWhenCertificateDoesNotMatc return true, nil } -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 { +func certHostnameAndIPMatchDesiredState(desiredIPs []net.IP, actualIPs []net.IP, desiredHostname string, actualHostnames []string) bool { + if len(desiredIPs) > 0 && len(actualIPs) > 0 && len(actualIPs) == len(desiredIPs) && len(actualHostnames) == 0 { + for i := range desiredIPs { + if !actualIPs[i].Equal(desiredIPs[i]) { + return false + } + } return true } if desiredHostname != "" && len(actualHostnames) == 1 && desiredHostname == actualHostnames[0] && len(actualIPs) == 0 { @@ -677,7 +682,7 @@ func (c *impersonatorConfigController) ensureTLSSecretIsCreatedAndLoaded(ctx con return nil } - newTLSSecret, err := c.createNewTLSSecret(ctx, ca, nameInfo.selectedIP, nameInfo.selectedHostname) + newTLSSecret, err := c.createNewTLSSecret(ctx, ca, nameInfo.selectedIPs, nameInfo.selectedHostname) if err != nil { return err } @@ -746,12 +751,6 @@ func (c *impersonatorConfigController) createCASecret(ctx context.Context) (*cer } func (c *impersonatorConfigController) findDesiredTLSCertificateName(config *v1alpha1.ImpersonationProxySpec) (*certNameInfo, error) { - // possible valid options: - // - you have a loadbalancer and are autoconfiguring the endpoint -> get cert info based on load balancer ip/hostnome - // - you have a loadbalancer AND an external endpoint -> either should work since they should be the same - // - external endpoint no loadbalancer or other service -> use the endpoint config - // - external endpoint and ClusterIP -> use external endpoint? - // - clusterip and no external endpoint if config.ExternalEndpoint != "" { return c.findTLSCertificateNameFromEndpointConfig(config), nil } else if config.Service.Type == v1alpha1.ImpersonationProxyServiceTypeClusterIP { @@ -765,7 +764,7 @@ func (c *impersonatorConfigController) findTLSCertificateNameFromEndpointConfig( endpointWithoutPort := strings.Split(endpointMaybeWithPort, ":")[0] parsedAsIP := net.ParseIP(endpointWithoutPort) if parsedAsIP != nil { - return &certNameInfo{ready: true, selectedIP: parsedAsIP, clientEndpoint: endpointMaybeWithPort} + return &certNameInfo{ready: true, selectedIPs: []net.IP{parsedAsIP}, clientEndpoint: endpointMaybeWithPort} } return &certNameInfo{ready: true, selectedHostname: endpointWithoutPort, clientEndpoint: endpointMaybeWithPort} } @@ -797,7 +796,7 @@ func (c *impersonatorConfigController) findTLSCertificateNameFromLoadBalancer() ip := ingress.IP parsedIP := net.ParseIP(ip) if parsedIP != nil { - return &certNameInfo{ready: true, selectedIP: parsedIP, clientEndpoint: ip}, nil + return &certNameInfo{ready: true, selectedIPs: []net.IP{parsedIP}, clientEndpoint: ip}, nil } } @@ -815,22 +814,27 @@ func (c *impersonatorConfigController) findTLSCertificateNameFromClusterIPServic return nil, err } ip := clusterIP.Spec.ClusterIP + ips := clusterIP.Spec.ClusterIPs if ip != "" { - parsedIP := net.ParseIP(ip) - return &certNameInfo{ready: true, selectedIP: parsedIP, clientEndpoint: ip}, nil + // clusterIP will always exist when clusterIPs does, but not vice versa + var parsedIPs []net.IP + if len(ips) > 0 { + for _, ipFromIPs := range ips { + parsedIPs = append(parsedIPs, net.ParseIP(ipFromIPs)) + } + } else { + parsedIPs = []net.IP{net.ParseIP(ip)} + } + return &certNameInfo{ready: true, selectedIPs: parsedIPs, clientEndpoint: ip}, nil } return &certNameInfo{ready: false}, nil } -func (c *impersonatorConfigController) createNewTLSSecret(ctx context.Context, ca *certauthority.CA, ip net.IP, hostname string) (*v1.Secret, error) { +func (c *impersonatorConfigController) createNewTLSSecret(ctx context.Context, ca *certauthority.CA, ips []net.IP, hostname string) (*v1.Secret, error) { var hostnames []string - var ips []net.IP if hostname != "" { hostnames = []string{hostname} } - if ip != nil { - ips = []net.IP{ip} - } impersonationCert, err := ca.IssueServerCert(hostnames, ips, approximatelyOneHundredYears) if err != nil { diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index fc8e346b..f188a4cf 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/internal/controller/impersonatorconfig/impersonator_config_test.go @@ -427,7 +427,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { 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") + t.Fatalf("dnsOverrides was provided but not used, which was probably a mistake. addr %s", addr) } return realDialer.DialContext(ctx, network, addr) } @@ -747,6 +747,15 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.NoError(client.Tracker().Add(clusterIPService)) } + var addDualStackClusterIPServiceToTracker = func(resourceName string, clusterIP0 string, clusterIP1 string, client *kubernetesfake.Clientset) { + clusterIPService := newClusterIPService(resourceName, corev1.ServiceStatus{}, corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + ClusterIP: clusterIP0, + ClusterIPs: []string{clusterIP0, clusterIP1}, + }) + r.NoError(client.Tracker().Add(clusterIPService)) + } + var addSecretToTrackers = func(secret *corev1.Secret, clients ...*kubernetesfake.Clientset) { for _, client := range clients { secretCopy := secret.DeepCopy() @@ -1541,6 +1550,39 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { }) }) + when("a clusterip service exists with dual stack ips", func() { + const fakeIP1 = "127.0.0.123" + const fakeIP2 = "127.0.0.234" // TODO test that this works for an IPv6 address, which is the actual use case for multiple clusterips + it.Before(func() { + addCredentialIssuerToTrackers(v1alpha1.CredentialIssuer{ + ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerResourceName}, + Spec: v1alpha1.CredentialIssuerSpec{ + ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{ + Mode: v1alpha1.ImpersonationProxyModeEnabled, + Service: v1alpha1.ImpersonationProxyServiceSpec{ + Type: v1alpha1.ImpersonationProxyServiceTypeClusterIP, + }, + }, + }, + }, pinnipedInformerClient, pinnipedAPIClient) + addNodeWithRoleToTracker("worker", kubeAPIClient) + addDualStackClusterIPServiceToTracker(clusterIPServiceName, fakeIP1, fakeIP2, kubeInformerClient) + addDualStackClusterIPServiceToTracker(clusterIPServiceName, fakeIP1, fakeIP2, kubeAPIClient) + }) + + it("certs are valid for both ip addresses", func() { + startInformersAndController() + r.NoError(runControllerSync()) + r.Len(kubeAPIClient.Actions(), 3) + requireNodesListed(kubeAPIClient.Actions()[0]) + ca := requireCASecretWasCreated(kubeAPIClient.Actions()[1]) + requireTLSSecretWasCreated(kubeAPIClient.Actions()[2], ca) + requireTLSServerIsRunning(ca, fakeIP2, map[string]string{fakeIP2 + ":443": testServerAddr()}) + requireTLSServerIsRunning(ca, fakeIP1, map[string]string{fakeIP1 + ":443": testServerAddr()}) + requireCredentialIssuer(newSuccessStrategy(fakeIP1, ca)) + }) + }) + when("a load balancer and a secret already exists", func() { var caCrt []byte it.Before(func() {