diff --git a/pkg/operator/certrotation/signer_test.go b/pkg/operator/certrotation/signer_test.go index f58e8c41ba..43b6de5a8d 100644 --- a/pkg/operator/certrotation/signer_test.go +++ b/pkg/operator/certrotation/signer_test.go @@ -1,16 +1,24 @@ package certrotation import ( + "bytes" "context" + "fmt" + "math/rand" "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 +27,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 diff --git a/pkg/operator/resource/resourceapply/core.go b/pkg/operator/resource/resourceapply/core.go index 588d7b0622..a7b12dd00c 100644 --- a/pkg/operator/resource/resourceapply/core.go +++ b/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)