use update only secret-apply for cert rotation logic

pkg/operator/certrotation/signer.go

@@ -75,7 +75,7 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*
 	// apply necessary metadata (possibly via delete+recreate) if secret exists
 	// this is done before content update to prevent unexpected rollouts
 	if ensureMetadataUpdate(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations) && ensureSecretTLSTypeSet(signingCertKeyPairSecret) {
-		actualSigningCertKeyPairSecret, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret)
+		actualSigningCertKeyPairSecret, _, err := resourceapply.ApplySecretWithoutDelete(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret)
 		if err != nil {
 			return nil, err
 		}
@@ -90,7 +90,7 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*
 
 		LabelAsManagedSecret(signingCertKeyPairSecret, CertificateTypeSigner)
 
-		actualSigningCertKeyPairSecret, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret)
+		actualSigningCertKeyPairSecret, _, err := resourceapply.ApplySecretWithoutDelete(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret)
 		if err != nil {
 			return nil, err
 		}

pkg/operator/certrotation/signer_test.go

@@ -1,16 +1,23 @@
 package certrotation
 
 import (
+	"bytes"
 	"context"
+	"fmt"
 	"strings"
+	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
 	"github.com/davecgh/go-spew/spew"
+	"github.com/google/go-cmp/cmp"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	kubefake "k8s.io/client-go/kubernetes/fake"
+	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 	corev1listers "k8s.io/client-go/listers/core/v1"
 	clienttesting "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/cache"
@@ -19,6 +26,170 @@ import (
 	"github.com/openshift/library-go/pkg/operator/events"
 )
 
+func TestRotatedSigningCASecretWithMultipleControllers(t *testing.T) {
+	ns, name := "ns", "test-signer"
+	// represents a secret that was created before 4.7 and
+	// hasn't been updated until now (upgrade to 4.15)
+	existing := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace:       ns,
+			Name:            name,
+			ResourceVersion: "10",
+			Annotations: map[string]string{
+				"auth.openshift.io/certificate-not-after":  "2108-09-08T22:47:31-07:00",
+				"auth.openshift.io/certificate-not-before": "2108-09-08T20:47:31-07:00",
+			},
+		},
+		Type: "SecretTypeTLS",
+		Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
+	}
+	if err := setSigningCertKeyPairSecret(existing, 24*time.Hour); err != nil {
+		t.Fatal(err)
+	}
+
+	// give it a second so we have a unique signer name,
+	// and also unique not-after, and not-before values
+	<-time.After(2 * time.Second)
+
+	// get the original crt and key bytes to compare later
+	tlsCertWant, ok := existing.Data["tls.crt"]
+	if !ok || len(tlsCertWant) == 0 {
+		t.Fatalf("missing data in 'tls.crt' key of Data: %#v", existing.Data)
+	}
+	tlsKeyWant, ok := existing.Data["tls.key"]
+	if !ok || len(tlsKeyWant) == 0 {
+		t.Fatalf("missing data in 'tls.key' key of Data: %#v", existing.Data)
+	}
+
+	// copy the existing object before test begins, so we can diff it against
+	// the final object on the cluster after the controllers finish
+	secretWant := existing.DeepCopy()
+
+	indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc})
+	if err := indexer.Add(existing); err != nil {
+		t.Fatal(err)
+	}
+	clientset := kubefake.NewSimpleClientset(existing)
+
+	// the list cache is synced as soon as we have a delete, create, or update
+	clientset.PrependReactor("delete", "secrets", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
+		if err = indexer.Delete(existing); err != nil {
+			t.Errorf("unexpected error while syncing the cache, op=delete: %v", err)
+		}
+		return false, nil, nil
+	})
+	clientset.PrependReactor("create", "secrets", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
+		switch action := action.(type) {
+		case clienttesting.CreateActionImpl:
+			indexer.Delete(existing)
+			if err := indexer.Add(action.GetObject()); err != nil {
+				t.Errorf("unexpected error while syncing the cache, op=create: %v", err)
+			}
+			return false, action.GetObject(), nil
+		}
+		t.Errorf("wrong test setup")
+		return false, nil, nil
+	})
+	clientset.PrependReactor("update", "secrets", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
+		switch action := action.(type) {
+		case clienttesting.UpdateActionImpl:
+			indexer.Delete(existing)
+			if err := indexer.Add(action.GetObject()); err != nil {
+				t.Errorf("unexpected error while syncing the cache, op=update: %v", err)
+			}
+			return false, action.GetObject(), nil
+		}
+		t.Errorf("wrong test setup")
+		return false, nil, nil
+	})
+
+	// r := rand.New(rand.NewSource(time.Now().Unix()))
+	options := events.RecommendedClusterSingletonCorrelatorOptions()
+	client := clientset.CoreV1().Secrets(ns)
+	var wg sync.WaitGroup
+	var id int32
+
+	controllers := 20
+	wg.Add(controllers)
+	for i := 1; i <= controllers; i++ {
+		go func() {
+			defer wg.Done()
+
+			controllerName := fmt.Sprintf("controller-%d", atomic.AddInt32(&id, 1))
+			recorder := events.NewKubeRecorderWithOptions(clientset.CoreV1().Events(ns), options, "operator", &corev1.ObjectReference{Name: controllerName, Namespace: ns})
+			wrapped := &wrapped{SecretInterface: client, name: controllerName, t: t}
+			getter := &getter{w: wrapped}
+			ctrl := &RotatedSigningCASecret{
+				Namespace:             ns,
+				Name:                  name,
+				Validity:              24 * time.Hour,
+				Refresh:               12 * time.Hour,
+				Client:                getter,
+				Lister:                corev1listers.NewSecretLister(indexer),
+				AdditionalAnnotations: AdditionalAnnotations{JiraComponent: "test"},
+				Owner:                 &metav1.OwnerReference{Name: "operator"},
+				EventRecorder:         recorder,
+			}
+
+			// <-time.After(time.Microsecond * time.Duration(r.Intn(100)))
+			_, err := ctrl.EnsureSigningCertKeyPair(context.TODO())
+			if err != nil {
+				t.Logf("error from controller 1 - %v", err)
+			}
+		}()
+	}
+
+	wg.Wait()
+	// controllers are done, we don't expect the signer to change
+	secretGot, err := client.Get(context.TODO(), name, metav1.GetOptions{})
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+		return
+	}
+	if tlsCertGot, ok := secretGot.Data["tls.crt"]; !ok || !bytes.Equal(tlsCertWant, tlsCertGot) {
+		t.Errorf("the signer cert has mutated unexpectedly")
+	}
+	if tlsKeyGot, ok := secretGot.Data["tls.key"]; !ok || !bytes.Equal(tlsKeyWant, tlsKeyGot) {
+		t.Errorf("the signer cert has mutated unexpectedly")
+	}
+	if got, exists := secretGot.Annotations["openshift.io/owning-component"]; !exists || got != "test" {
+		t.Errorf("owner annotation is missing: %#v", secretGot.Annotations)
+	}
+	t.Logf("diff: %s", cmp.Diff(secretWant, secretGot))
+}
+
+type getter struct {
+	w *wrapped
+}
+
+func (g *getter) Secrets(string) corev1client.SecretInterface {
+	return g.w
+}
+
+type wrapped struct {
+	corev1client.SecretInterface
+	name string
+	t    *testing.T
+}
+
+func (w wrapped) Create(ctx context.Context, secret *corev1.Secret, opts metav1.CreateOptions) (*corev1.Secret, error) {
+	w.t.Logf("[%s] op=Create, secret=%s/%s", w.name, secret.Namespace, secret.Name)
+	return w.SecretInterface.Create(ctx, secret, opts)
+}
+func (w wrapped) Update(ctx context.Context, secret *corev1.Secret, opts metav1.UpdateOptions) (*corev1.Secret, error) {
+	w.t.Logf("[%s] op=Update, secret=%s/%s", w.name, secret.Namespace, secret.Name)
+	return w.SecretInterface.Update(ctx, secret, opts)
+}
+func (w wrapped) Delete(ctx context.Context, name string, opts metav1.DeleteOptions) error {
+	w.t.Logf("[%s] op=Delete, secret=%s", w.name, name)
+	return w.SecretInterface.Delete(ctx, name, opts)
+}
+func (w wrapped) Get(ctx context.Context, name string, opts metav1.GetOptions) (*corev1.Secret, error) {
+	obj, err := w.SecretInterface.Get(ctx, name, opts)
+	w.t.Logf("[%s] op=Get, secret=%s, err: %v", w.name, name, err)
+	return obj, err
+}
+
 func TestEnsureSigningCertKeyPair(t *testing.T) {
 	tests := []struct {
 		name string

pkg/operator/resource/resourceapply/core.go

@@ -84,7 +84,14 @@ func ApplyConfigMap(ctx context.Context, client coreclientv1.ConfigMapsGetter, r
 
 // ApplySecret merges objectmeta, requires data
 func ApplySecret(ctx context.Context, client coreclientv1.SecretsGetter, recorder events.Recorder, required *corev1.Secret) (*corev1.Secret, bool, error) {
-	return ApplySecretImproved(ctx, client, recorder, required, noCache)
+	return applySecretImproved(ctx, client, recorder, required, noCache, false)
+}
+
+// ApplySecretWithoutDelete applies a secret on the cluster
+// NOTE: DO NOT USE, this is meant to be a short term hack for a very specific use case,
+// and will not work for your application
+func ApplySecretWithoutDelete(ctx context.Context, client coreclientv1.SecretsGetter, recorder events.Recorder, required *corev1.Secret) (*corev1.Secret, bool, error) {
+	return applySecretImproved(ctx, client, recorder, required, noCache, true)
 }
 
 // ApplyNamespace merges objectmeta, does not worry about anything else
@@ -356,6 +363,10 @@ func ApplyConfigMapImproved(ctx context.Context, client coreclientv1.ConfigMapsG
 
 // ApplySecret merges objectmeta, requires data
 func ApplySecretImproved(ctx context.Context, client coreclientv1.SecretsGetter, recorder events.Recorder, requiredInput *corev1.Secret, cache ResourceCache) (*corev1.Secret, bool, error) {
+	return applySecretImproved(ctx, client, recorder, requiredInput, cache, false)
+}
+
+func applySecretImproved(ctx context.Context, client coreclientv1.SecretsGetter, recorder events.Recorder, requiredInput *corev1.Secret, cache ResourceCache, updateOnly bool) (*corev1.Secret, bool, error) {
 	// copy the stringData to data.  Error on a data content conflict inside required.  This is usually a bug.
 
 	existing, err := client.Secrets(requiredInput.Namespace).Get(ctx, requiredInput.Name, metav1.GetOptions{})
@@ -435,6 +446,12 @@ func ApplySecretImproved(ctx context.Context, client coreclientv1.SecretsGetter,
 	 * https://github.com/kubernetes/kubernetes/blob/98e65951dccfd40d3b4f31949c2ab8df5912d93e/pkg/apis/core/validation/validation.go#L5048
 	 * We need to explicitly opt for delete+create in that case.
 	 */
+	if updateOnly {
+		actual, err = client.Secrets(required.Namespace).Update(ctx, existingCopy, metav1.UpdateOptions{})
+		reportUpdateEvent(recorder, existingCopy, err)
+		return actual, err == nil, err
+	}
+
 	if existingCopy.Type == existing.Type {
 		actual, err = client.Secrets(required.Namespace).Update(ctx, existingCopy, metav1.UpdateOptions{})
 		reportUpdateEvent(recorder, existingCopy, err)