From 8b4ed86071c33b3f7cc7756b8c45a69bd92358e9 Mon Sep 17 00:00:00 2001
From: Monis Khan <i@monis.app>
Date: Wed, 28 Jul 2021 09:26:19 -0400
Subject: [PATCH] certs_expirer: be specific about what secret to delete

This change fixes a race that can occur because we have multiple
writers with no leader election lock.

1. TestAPIServingCertificateAutoCreationAndRotation/automatic
   expires the current serving certificate
2. CertsExpirerController 1 deletes expired serving certificate
3. CertsExpirerController 2 starts deletion of expired serving
   certificate but has not done so yet
4. CertsManagerController 1 creates new serving certificate
5. TestAPIServingCertificateAutoCreationAndRotation/automatic
   records the new serving certificate
6. CertsExpirerController 2 finishes deletion, and thus deletes the
   newly created serving certificate instead of the old one
7. CertsManagerController 2 creates new serving certificate
8. TestAPIServingCertificateAutoCreationAndRotation/automatic keeps
   running and eventually times out because it is expecting the
   serving certificate created by CertsManagerController 2 to match
   the value it recorded from CertsManagerController 1 (which will
   never happen since that certificate was incorrectly deleted).

Signed-off-by: Monis Khan <mok@vmware.com>
---
 internal/controller/apicerts/certs_expirer.go |  7 ++-
 .../controller/apicerts/certs_expirer_test.go | 56 ++++++++++++++++++-
 2 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/internal/controller/apicerts/certs_expirer.go b/internal/controller/apicerts/certs_expirer.go
index 2b643c3e..c04d27bc 100644
--- a/internal/controller/apicerts/certs_expirer.go
+++ b/internal/controller/apicerts/certs_expirer.go
@@ -90,7 +90,12 @@ func (c *certsExpirerController) Sync(ctx controllerlib.Context) error {
 		err := c.k8sClient.
 			CoreV1().
 			Secrets(c.namespace).
-			Delete(ctx.Context, c.certsSecretResourceName, metav1.DeleteOptions{})
+			Delete(ctx.Context, c.certsSecretResourceName, metav1.DeleteOptions{
+				Preconditions: &metav1.Preconditions{
+					UID:             &secret.UID,
+					ResourceVersion: &secret.ResourceVersion,
+				},
+			})
 		if err != nil {
 			// Do return an error here so that the controller library will reschedule
 			// us to try deleting this cert again.
diff --git a/internal/controller/apicerts/certs_expirer_test.go b/internal/controller/apicerts/certs_expirer_test.go
index 7e82819a..da508c55 100644
--- a/internal/controller/apicerts/certs_expirer_test.go
+++ b/internal/controller/apicerts/certs_expirer_test.go
@@ -18,8 +18,11 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	kubeinformers "k8s.io/client-go/informers"
+	"k8s.io/client-go/kubernetes"
 	kubernetesfake "k8s.io/client-go/kubernetes/fake"
+	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 	kubetesting "k8s.io/client-go/testing"
 
 	"go.pinniped.dev/internal/controllerlib"
@@ -223,14 +226,19 @@ func TestExpirerControllerSync(t *testing.T) {
 				test.configKubeAPIClient(kubeAPIClient)
 			}
 
+			testRV := "rv_001"
+			testUID := types.UID("uid_002")
+
 			kubeInformerClient := kubernetesfake.NewSimpleClientset()
 			name := certsSecretResourceName
 			namespace := "some-namespace"
 			if test.fillSecretData != nil {
 				secret := &corev1.Secret{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:      name,
-						Namespace: namespace,
+						Name:            name,
+						Namespace:       namespace,
+						ResourceVersion: testRV,
+						UID:             testUID,
 					},
 					Data: map[string][]byte{},
 				}
@@ -245,10 +253,12 @@ func TestExpirerControllerSync(t *testing.T) {
 				0,
 			)
 
+			trackDeleteClient := &clientWrapper{Interface: kubeAPIClient, opts: &[]metav1.DeleteOptions{}}
+
 			c := NewCertsExpirerController(
 				namespace,
 				certsSecretResourceName,
-				kubeAPIClient,
+				trackDeleteClient,
 				kubeInformers.Core().V1().Secrets(),
 				controllerlib.WithInformer,
 				test.renewBefore,
@@ -285,6 +295,46 @@ func TestExpirerControllerSync(t *testing.T) {
 			}
 			acActions := kubeAPIClient.Actions()
 			require.Equal(t, exActions, acActions)
+
+			if test.wantDelete {
+				require.Len(t, *trackDeleteClient.opts, 1)
+				require.Equal(t, metav1.DeleteOptions{
+					Preconditions: &metav1.Preconditions{
+						UID:             &testUID,
+						ResourceVersion: &testRV,
+					},
+				}, (*trackDeleteClient.opts)[0])
+			} else {
+				require.Len(t, *trackDeleteClient.opts, 0)
+			}
 		})
 	}
 }
+
+type clientWrapper struct {
+	kubernetes.Interface
+	opts *[]metav1.DeleteOptions
+}
+
+func (c *clientWrapper) CoreV1() corev1client.CoreV1Interface {
+	return &coreWrapper{CoreV1Interface: c.Interface.CoreV1(), opts: c.opts}
+}
+
+type coreWrapper struct {
+	corev1client.CoreV1Interface
+	opts *[]metav1.DeleteOptions
+}
+
+func (c *coreWrapper) Secrets(namespace string) corev1client.SecretInterface {
+	return &secretsWrapper{SecretInterface: c.CoreV1Interface.Secrets(namespace), opts: c.opts}
+}
+
+type secretsWrapper struct {
+	corev1client.SecretInterface
+	opts *[]metav1.DeleteOptions
+}
+
+func (s *secretsWrapper) Delete(ctx context.Context, name string, opts metav1.DeleteOptions) error {
+	*s.opts = append(*s.opts, opts)
+	return s.SecretInterface.Delete(ctx, name, opts)
+}