From 5b01e4be2dfcb3b4eccf529ab8dfd1c583c9df8f Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 26 Feb 2021 10:58:56 -0800 Subject: [PATCH] impersonator_config.go: handle more error cases Signed-off-by: Margo Crawford --- .../impersonatorconfig/impersonator_config.go | 68 +++-- .../impersonator_config_test.go | 235 ++++++++++++++++-- 2 files changed, 260 insertions(+), 43 deletions(-) diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index abda84a4..c7b6d3e2 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -197,10 +197,8 @@ func (c *impersonatorConfigController) ensureTLSSecret(ctx controllerlib.Context 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 secret, err = c.deleteWhenTLSCertificateDoesNotMatchDesiredState(ctx.Context, config, secret); err != nil { + return err } if err = c.ensureTLSSecretIsCreatedAndLoaded(ctx.Context, config, secret); err != nil { return err @@ -357,7 +355,7 @@ func (c *impersonatorConfigController) ensureLoadBalancerIsStopped(ctx context.C return nil } -func (c *impersonatorConfigController) deleteTLSCertificateWithWrongName(ctx context.Context, config *impersonator.Config, secret *v1.Secret) (*v1.Secret, error) { +func (c *impersonatorConfigController) deleteWhenTLSCertificateDoesNotMatchDesiredState(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 @@ -366,16 +364,46 @@ func (c *impersonatorConfigController) deleteTLSCertificateWithWrongName(ctx con certPEM := secret.Data[v1.TLSCertKey] block, _ := pem.Decode(certPEM) if block == nil { - // The certPEM is not valid. - return secret, nil // TODO delete this so the controller will create a valid one + plog.Warning("Found missing or not PEM-encoded data in TLS Secret", + "invalidCertPEM", certPEM, + "secret", c.tlsSecretName, + "namespace", c.namespace) + deleteErr := c.ensureTLSSecretIsRemoved(ctx) + if deleteErr != nil { + return nil, fmt.Errorf("found missing or not PEM-encoded data in TLS Secret, but got error while deleting it: %w", deleteErr) + } + return nil, nil + } + + actualCertFromSecret, err := x509.ParseCertificate(block.Bytes) + if err != nil { + plog.Error("Found invalid PEM data in TLS Secret", err, + "invalidCertPEM", certPEM, + "secret", c.tlsSecretName, + "namespace", c.namespace) + deleteErr := c.ensureTLSSecretIsRemoved(ctx) + if deleteErr != nil { + return nil, fmt.Errorf("PEM data represented an invalid cert, but got error while deleting it: %w", deleteErr) + } + return nil, nil + } + + keyPEM := secret.Data[v1.TLSPrivateKeyKey] + _, err = tls.X509KeyPair(certPEM, keyPEM) + if err != nil { + plog.Error("Found invalid private key PEM data in TLS Secret", err, + "secret", c.tlsSecretName, + "namespace", c.namespace) + deleteErr := c.ensureTLSSecretIsRemoved(ctx) + if deleteErr != nil { + return nil, fmt.Errorf("cert had an invalid private key, but got error while deleting it: %w", deleteErr) + } + return nil, nil } - actualCertFromSecret, _ := x509.ParseCertificate(block.Bytes) - // TODO handle err 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 + return secret, err } if !nameIsReady { // We currently have a secret but we are waiting for a load balancer to be assigned an ingress, so @@ -398,8 +426,10 @@ func (c *impersonatorConfigController) deleteTLSCertificateWithWrongName(ctx con "namespace", c.namespace) if certHostnameAndIPMatchDesiredState(desiredIP, actualIPs, desiredHostname, actualHostnames) { + // The cert already matches the desired state, so there is no need to delete/recreate it. return secret, nil } + err = c.ensureTLSSecretIsRemoved(ctx) if err != nil { return secret, err @@ -509,8 +539,10 @@ func (c *impersonatorConfigController) createNewTLSSecret(ctx context.Context, c return nil, fmt.Errorf("could not create impersonation cert: %w", err) } - certPEM, keyPEM, _ := certauthority.ToPEM(impersonationCert) - // TODO handle err + certPEM, keyPEM, err := certauthority.ToPEM(impersonationCert) + if err != nil { + return nil, err + } newTLSSecret := &v1.Secret{ Type: v1.SecretTypeTLS, @@ -525,13 +557,17 @@ func (c *impersonatorConfigController) createNewTLSSecret(ctx context.Context, c v1.TLSCertKey: certPEM, }, } + plog.Info("Creating TLS certificates for impersonation proxy", "ips", ips, "hostnames", hostnames, "secret", c.tlsSecretName, "namespace", c.namespace) - _, _ = c.k8sClient.CoreV1().Secrets(c.namespace).Create(ctx, newTLSSecret, metav1.CreateOptions{}) - // TODO handle error on create + _, err = c.k8sClient.CoreV1().Secrets(c.namespace).Create(ctx, newTLSSecret, metav1.CreateOptions{}) + if err != nil { + return nil, err + } + return newTLSSecret, nil } @@ -545,7 +581,7 @@ func (c *impersonatorConfigController) loadTLSCertFromSecret(tlsSecret *v1.Secre "secret", c.tlsSecretName, "namespace", c.namespace, ) - // TODO clear the secret if it was already set previously... c.setTLSCert(nil) + c.setTLSCert(nil) return fmt.Errorf("could not parse TLS cert PEM data from Secret: %w", err) } plog.Info("Loading TLS certificates for impersonation proxy", diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index 20512aad..04678706 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/internal/controller/impersonatorconfig/impersonator_config_test.go @@ -896,12 +896,11 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { }) 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") + tlsSecret := createTLSSecretWithMultipleHostnames(tlsSecretName, "127.0.0.1") r.NoError(kubeAPIClient.Tracker().Add(tlsSecret)) r.NoError(kubeInformerClient.Tracker().Add(tlsSecret)) startInformersAndController() @@ -916,6 +915,58 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSServerIsRunning(ca, startedTLSListener.Addr().String(), nil) }) }) + + when("the cert's name needs to change but there is an error while deleting the tls Secret", func() { + it.Before(func() { + addNodeWithRoleToTracker("worker") + addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.42"}}, kubeInformerClient) + addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.42"}}, kubeAPIClient) + tlsSecret := createActualTLSSecret(tlsSecretName) + r.NoError(kubeAPIClient.Tracker().Add(tlsSecret)) + r.NoError(kubeInformerClient.Tracker().Add(tlsSecret)) + kubeAPIClient.PrependReactor("delete", "secrets", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, fmt.Errorf("error on delete") + }) + }) + + it("returns an error and runs the proxy without certs", func() { + startInformersAndController() + r.Error(controllerlib.TestSync(t, subject, *syncContext), "error on delete") + r.Len(kubeAPIClient.Actions(), 2) + requireNodesListed(kubeAPIClient.Actions()[0]) + requireTLSSecretDeleted(kubeAPIClient.Actions()[1]) + requireTLSServerIsRunningWithoutCerts() + }) + }) + + when("the cert's name might need to change but there is an error while determining the new name", func() { + var ca []byte + 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 := createActualTLSSecret(tlsSecretName) + ca = tlsSecret.Data["ca.crt"] + r.NoError(kubeAPIClient.Tracker().Add(tlsSecret)) + r.NoError(kubeInformerClient.Tracker().Add(tlsSecret)) + }) + + it("returns an error and keeps running the proxy with the old cert", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + r.Len(kubeAPIClient.Actions(), 1) + requireNodesListed(kubeAPIClient.Actions()[0]) + requireTLSServerIsRunning(ca, startedTLSListener.Addr().String(), nil) + + updateLoadBalancerServiceInTracker(generatedLoadBalancerServiceName, "not-an-ip", "1") + waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().Services().Informer(), "1") + + r.EqualError(controllerlib.TestSync(t, subject, *syncContext), + "could not find valid IP addresses or hostnames from load balancer some-namespace/some-service-resource-name") + r.Len(kubeAPIClient.Actions(), 1) // no new actions + requireTLSServerIsRunning(ca, startedTLSListener.Addr().String(), nil) + }) + }) }) when("sync is called more than once", func() { @@ -1165,28 +1216,6 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { }) }) - 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)) - 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() { - 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() - }) - }) - when("we have a hostname specified for the endpoint", func() { const fakeHostname = "fake.example.com" it.Before(func() { @@ -1319,7 +1348,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { }) }) - when("the endpoint switches from specified, to not specified, to specified", func() { + when("the endpoint switches from specified, to not specified, to specified again", func() { it.Before(func() { addImpersonatorConfigMapToTracker(configMapResourceName, here.Doc(` mode: enabled @@ -1373,6 +1402,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // 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()}) + // update manually because the kubeAPIClient isn't connected to the informer in the tests + addSecretFromCreateActionToTracker(kubeAPIClient.Actions()[4], kubeInformerClient, "2") + waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().Secrets().Informer(), "2") + // Now switch back to having the "endpoint" specified, so the load balancer is not needed anymore. updateImpersonatorConfigMapInTracker(configMapResourceName, here.Doc(` mode: enabled @@ -1381,9 +1414,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().ConfigMaps().Informer(), "2") r.NoError(controllerlib.TestSync(t, subject, *syncContext)) - r.Len(kubeAPIClient.Actions(), 7) + r.Len(kubeAPIClient.Actions(), 8) requireLoadBalancerDeleted(kubeAPIClient.Actions()[5]) - requireTLSSecretWasCreated(kubeAPIClient.Actions()[6]) // recreated because the endpoint was updated + requireTLSSecretDeleted(kubeAPIClient.Actions()[6]) + requireTLSSecretWasCreated(kubeAPIClient.Actions()[7]) // recreated because the endpoint was updated }) }) }) @@ -1402,6 +1436,25 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { }) }) + when("there is an error creating the tls secret", func() { + it.Before(func() { + addImpersonatorConfigMapToTracker(configMapResourceName, "{mode: enabled, endpoint: example.com}") + addNodeWithRoleToTracker("control-plane") + kubeAPIClient.PrependReactor("create", "secrets", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, fmt.Errorf("error on create") + }) + }) + + it("starts the impersonator without certs and returns an error", func() { + startInformersAndController() + r.EqualError(controllerlib.TestSync(t, subject, *syncContext), "error on create") + requireTLSServerIsRunningWithoutCerts() + r.Len(kubeAPIClient.Actions(), 2) + requireNodesListed(kubeAPIClient.Actions()[0]) + requireTLSSecretWasCreated(kubeAPIClient.Actions()[1]) + }) + }) + when("there is an error deleting the tls secret", func() { it.Before(func() { addNodeWithRoleToTracker("control-plane") @@ -1425,5 +1478,133 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSSecretDeleted(kubeAPIClient.Actions()[2]) }) }) + + when("the PEM formatted data in the Secret is not a valid cert", func() { + it.Before(func() { + addImpersonatorConfigMapToTracker(configMapResourceName, "{mode: enabled, endpoint: 127.0.0.1}") + addNodeWithRoleToTracker("worker") + tlsSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: tlsSecretName, + Namespace: installedInNamespace, + }, + Data: map[string][]byte{ + // "aGVsbG8gd29ybGQK" is "hello world" base64 encoded + corev1.TLSCertKey: []byte("-----BEGIN CERTIFICATE-----\naGVsbG8gd29ybGQK\n-----END CERTIFICATE-----\n"), + }, + } + r.NoError(kubeAPIClient.Tracker().Add(tlsSecret)) + r.NoError(kubeInformerClient.Tracker().Add(tlsSecret)) + }) + + it("deletes the invalid certs, creates new certs, and starts the impersonator", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + r.Len(kubeAPIClient.Actions(), 3) + requireNodesListed(kubeAPIClient.Actions()[0]) + requireTLSSecretDeleted(kubeAPIClient.Actions()[1]) // deleted the bad cert + ca := requireTLSSecretWasCreated(kubeAPIClient.Actions()[2]) + requireTLSServerIsRunning(ca, startedTLSListener.Addr().String(), nil) + }) + + when("there is an error while the invalid cert is being deleted", func() { + it.Before(func() { + kubeAPIClient.PrependReactor("delete", "secrets", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, fmt.Errorf("error on delete") + }) + }) + + it("tries to delete the invalid cert, starts the impersonator without certs, and returns an error", func() { + startInformersAndController() + r.EqualError(controllerlib.TestSync(t, subject, *syncContext), "PEM data represented an invalid cert, but got error while deleting it: error on delete") + requireTLSServerIsRunningWithoutCerts() + r.Len(kubeAPIClient.Actions(), 2) + requireNodesListed(kubeAPIClient.Actions()[0]) + requireTLSSecretDeleted(kubeAPIClient.Actions()[1]) // tried deleted the bad cert, which failed + requireTLSServerIsRunningWithoutCerts() + }) + }) + }) + + when("a tls secret already exists but it is not valid", func() { + 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)) + addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeInformerClient) + addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeAPIClient) + }) + + it("deletes the invalid certs, creates new certs, and starts the impersonator", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + r.Len(kubeAPIClient.Actions(), 3) + requireNodesListed(kubeAPIClient.Actions()[0]) + requireTLSSecretDeleted(kubeAPIClient.Actions()[1]) // deleted the bad cert + ca := requireTLSSecretWasCreated(kubeAPIClient.Actions()[2]) + requireTLSServerIsRunning(ca, startedTLSListener.Addr().String(), nil) + }) + + when("there is an error while the invalid cert is being deleted", func() { + it.Before(func() { + kubeAPIClient.PrependReactor("delete", "secrets", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, fmt.Errorf("error on delete") + }) + }) + + it("tries to delete the invalid cert, starts the impersonator without certs, and returns an error", func() { + startInformersAndController() + r.EqualError(controllerlib.TestSync(t, subject, *syncContext), "found missing or not PEM-encoded data in TLS Secret, but got error while deleting it: error on delete") + requireTLSServerIsRunningWithoutCerts() + r.Len(kubeAPIClient.Actions(), 2) + requireNodesListed(kubeAPIClient.Actions()[0]) + requireTLSSecretDeleted(kubeAPIClient.Actions()[1]) // tried deleted the bad cert, which failed + requireTLSServerIsRunningWithoutCerts() + }) + }) + }) + + when("a tls secret already exists but the private key is not valid", func() { + it.Before(func() { + addImpersonatorConfigMapToTracker(configMapResourceName, "mode: enabled") + addNodeWithRoleToTracker("worker") + tlsSecret := createActualTLSSecret(tlsSecretName) + tlsSecret.Data["tls.key"] = nil + r.NoError(kubeAPIClient.Tracker().Add(tlsSecret)) + r.NoError(kubeInformerClient.Tracker().Add(tlsSecret)) + addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeInformerClient) + addLoadBalancerServiceWithIngressToTracker(generatedLoadBalancerServiceName, []corev1.LoadBalancerIngress{{IP: "127.0.0.1"}}, kubeAPIClient) + }) + + it("deletes the invalid certs, creates new certs, and starts the impersonator", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + r.Len(kubeAPIClient.Actions(), 3) + requireNodesListed(kubeAPIClient.Actions()[0]) + requireTLSSecretDeleted(kubeAPIClient.Actions()[1]) // deleted the bad cert + ca := requireTLSSecretWasCreated(kubeAPIClient.Actions()[2]) + requireTLSServerIsRunning(ca, startedTLSListener.Addr().String(), nil) + }) + + when("there is an error while the invalid cert is being deleted", func() { + it.Before(func() { + kubeAPIClient.PrependReactor("delete", "secrets", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, fmt.Errorf("error on delete") + }) + }) + + it("tries to delete the invalid cert, starts the impersonator without certs, and returns an error", func() { + startInformersAndController() + r.EqualError(controllerlib.TestSync(t, subject, *syncContext), "cert had an invalid private key, but got error while deleting it: error on delete") + requireTLSServerIsRunningWithoutCerts() + r.Len(kubeAPIClient.Actions(), 2) + requireNodesListed(kubeAPIClient.Actions()[0]) + requireTLSSecretDeleted(kubeAPIClient.Actions()[1]) // tried deleted the bad cert, which failed + requireTLSServerIsRunningWithoutCerts() + }) + }) + }) }, spec.Parallel(), spec.Report(report.Terminal{})) }