impersonator_config.go: handle more error cases

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

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.deleteWhenTLSCertificateDoesNotMatchDesiredState(ctx.Context, config, secret); err != nil {
-	if secret, err = c.deleteTLSCertificateWithWrongName(ctx.Context, config, secret); err != nil {
+		return err
-		// TODO
-		// 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.
+		plog.Warning("Found missing or not PEM-encoded data in TLS Secret",
-		return secret, nil // TODO delete this so the controller will create a valid one
+			"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)
+	certPEM, keyPEM, err := certauthority.ToPEM(impersonationCert)
-	// TODO handle err
+	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{})
+	_, err = c.k8sClient.CoreV1().Secrets(c.namespace).Create(ctx, newTLSSecret, metav1.CreateOptions{})
-	// TODO handle error on create
+	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",

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{}))
 }