From c3f73ffb57310e402ed7d5f9bc1fae25ad89d13d Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 10 Dec 2020 11:54:36 -0500 Subject: [PATCH] Check in some musings on a symmetric key generator controller There is still a test failing, but I am sure it is a simple fix hiding in the code. I think this is the general shape of the controller that we want. Signed-off-by: Andrew Keesler --- go.mod | 1 + .../secretgenerator/secretgenerator.go | 166 ++++++++++ .../secretgenerator/secretgenerator_test.go | 294 ++++++++++++++++++ 3 files changed, 461 insertions(+) create mode 100644 internal/controller/supervisorconfig/secretgenerator/secretgenerator.go create mode 100644 internal/controller/supervisorconfig/secretgenerator/secretgenerator_test.go diff --git a/go.mod b/go.mod index 84237fd7..3abd0ba1 100644 --- a/go.mod +++ b/go.mod @@ -19,6 +19,7 @@ require ( github.com/ory/fosite v0.35.1 github.com/pkg/browser v0.0.0-20180916011732-0a3d74bf9ce4 github.com/pkg/errors v0.9.1 + github.com/prometheus/common v0.10.0 github.com/sclevine/agouti v3.0.0+incompatible github.com/sclevine/spec v1.4.0 github.com/spf13/cobra v1.0.0 diff --git a/internal/controller/supervisorconfig/secretgenerator/secretgenerator.go b/internal/controller/supervisorconfig/secretgenerator/secretgenerator.go new file mode 100644 index 00000000..89df14e1 --- /dev/null +++ b/internal/controller/supervisorconfig/secretgenerator/secretgenerator.go @@ -0,0 +1,166 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package secretgenerator provides a controller that can ensure existence of a generated secret. +package secretgenerator + +import ( + "context" + "crypto/rand" + "fmt" + + corev1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + corev1informers "k8s.io/client-go/informers/core/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/util/retry" + "k8s.io/klog/v2" + + pinnipedcontroller "go.pinniped.dev/internal/controller" + "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/plog" +) + +const ( + symmetricKeySecretType = "secrets.pinniped.dev/symmetric" + symmetricKeySecretDataKey = "key" + + symmetricKeySize = 32 // TODO: what should this be? +) + +// generateKey is stubbed out for the purpose of testing. The default behavior is to generate a symmetric key. +//nolint:gochecknoglobals +var generateKey = generateSymmetricKey + +func generateSymmetricKey() ([]byte, error) { + b := make([]byte, symmetricKeySize) + if _, err := rand.Read(b); err != nil { + return nil, err + } + return b, nil +} + +type controller struct { + secretNamePrefix string + client kubernetes.Interface + secrets corev1informers.SecretInformer +} + +// New instantiates a new controllerlib.Controller which will ensure existence of a generated secret. +func New(secretNamePrefix string, client kubernetes.Interface, secrets corev1informers.SecretInformer) controllerlib.Controller { + c := controller{ + secretNamePrefix: secretNamePrefix, + client: client, + secrets: secrets, + } + filter := pinnipedcontroller.SimpleFilterWithSingletonQueue(isOwnee) + return controllerlib.New( + controllerlib.Config{Name: secretNamePrefix + "-secrets-generator", Syncer: &c}, + controllerlib.WithInformer(secrets, filter, controllerlib.InformerOption{}), + ) +} + +// Sync implements controllerlib.Syncer.Sync(). +func (c *controller) Sync(ctx controllerlib.Context) error { + secret, err := c.secrets.Lister().Secrets(ctx.Key.Namespace).Get(ctx.Key.Name) + isNotFound := k8serrors.IsNotFound(err) + if !isNotFound && err != nil { + return fmt.Errorf("failed to list secret %s/%s: %w", ctx.Key.Namespace, ctx.Key.Name, err) + } + + secretNeedsUpdate := isNotFound || !c.isValid(secret) + if !secretNeedsUpdate { + plog.Debug("secret is up to date", "secret", klog.KObj(secret)) + return nil + } + + newSecret, err := c.generateSecret(ctx.Key.Namespace) + if err != nil { + return fmt.Errorf("failed to generate secret: %w", err) + } + + if isNotFound { + err = c.createSecret(ctx.Context, newSecret) + } else { + err = c.updateSecret(ctx.Context, newSecret, ctx.Key.Name) + } + if err != nil { + return fmt.Errorf("failed to create/update secret %s/%s: %w", ctx.Key.Namespace, ctx.Key.Name, err) + } + + return nil +} + +func (c *controller) isValid(secret *corev1.Secret) bool { + if secret.Type != symmetricKeySecretType { + return false + } + + data, ok := secret.Data[symmetricKeySecretDataKey] + if !ok { + return false + } + if len(data) != symmetricKeySize { + return false + } + + return true +} + +func (c *controller) generateSecret(namespace string) (*corev1.Secret, error) { + symmetricKey, err := generateKey() + if err != nil { + return nil, err + } + + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: c.secretNamePrefix, + Namespace: namespace, + }, + Type: symmetricKeySecretType, + Data: map[string][]byte{ + symmetricKeySecretDataKey: symmetricKey, + }, + }, nil +} + +func (c *controller) createSecret(ctx context.Context, newSecret *corev1.Secret) error { + _, err := c.client.CoreV1().Secrets(newSecret.Namespace).Create(ctx, newSecret, metav1.CreateOptions{}) + return err +} + +func (c *controller) updateSecret(ctx context.Context, newSecret *corev1.Secret, secretName string) error { + secrets := c.client.CoreV1().Secrets(newSecret.Namespace) + return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + currentSecret, err := secrets.Get(ctx, secretName, metav1.GetOptions{}) + isNotFound := k8serrors.IsNotFound(err) + if !isNotFound && err != nil { + return fmt.Errorf("failed to get secret: %w", err) + } + + if isNotFound { + if err := c.createSecret(ctx, newSecret); err != nil { + return fmt.Errorf("failed to create secret: %w", err) + } + return nil + } + + if c.isValid(currentSecret) { + return nil + } + + currentSecret.Type = newSecret.Type + currentSecret.Data = newSecret.Data + + _, err = secrets.Update(ctx, currentSecret, metav1.UpdateOptions{}) + return err + }) +} + +// isOwnee returns whether the provided obj is owned by this controller. +func isOwnee(obj metav1.Object) bool { + // TODO: how do we say we are owned by our Deployment? + return true +} diff --git a/internal/controller/supervisorconfig/secretgenerator/secretgenerator_test.go b/internal/controller/supervisorconfig/secretgenerator/secretgenerator_test.go new file mode 100644 index 00000000..f1e5a3de --- /dev/null +++ b/internal/controller/supervisorconfig/secretgenerator/secretgenerator_test.go @@ -0,0 +1,294 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package secretgenerator + +import ( + "context" + "errors" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + kubeinformers "k8s.io/client-go/informers" + kubernetesfake "k8s.io/client-go/kubernetes/fake" + kubetesting "k8s.io/client-go/testing" + + "go.pinniped.dev/internal/controllerlib" +) + +func TestController(t *testing.T) { + const ( + generatedSecretNamespace = "some-namespace" + generatedSecretNamePrefix = "some-name-" + generatedSecretName = "some-name-abc123" + ) + + var ( + secretsGVR = schema.GroupVersionResource{ + Group: corev1.SchemeGroupVersion.Group, + Version: corev1.SchemeGroupVersion.Version, + Resource: "secrets", + } + + generatedSymmetricKey = []byte("some-neato-32-byte-generated-key") + + generatedSecret = &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: generatedSecretNamePrefix, + Namespace: generatedSecretNamespace, + }, + Type: "secrets.pinniped.dev/symmetric", + Data: map[string][]byte{ + "key": generatedSymmetricKey, + }, + } + ) + + generatedSecretWithName := generatedSecret.DeepCopy() + generatedSecretWithName.Name = generatedSecretName + + once := sync.Once{} + + tests := []struct { + name string + storedSecret func(**corev1.Secret) + generateKey func() ([]byte, error) + apiClient func(*testing.T, *kubernetesfake.Clientset) + wantError string + wantActions []kubetesting.Action + }{ + { + name: "when the secrets does not exist, it gets generated", + storedSecret: func(secret **corev1.Secret) { + *secret = nil + }, + wantActions: []kubetesting.Action{ + kubetesting.NewCreateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + }, + }, + { + name: "when a valid secret exists, nothing happens", + }, + { + name: "secret gets updated when the type is wrong", + storedSecret: func(secret **corev1.Secret) { + (*secret).Type = "wrong" + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName), + }, + }, + { + name: "secret gets updated when the key data does not exist", + storedSecret: func(secret **corev1.Secret) { + delete((*secret).Data, "key") + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName), + }, + }, + { + name: "secret gets updated when the key data is too short", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName), + }, + }, + { + name: "an error is returned when creating fails", + storedSecret: func(secret **corev1.Secret) { + *secret = nil + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("create", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some create error") + }) + }, + wantActions: []kubetesting.Action{ + kubetesting.NewCreateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + }, + wantError: "failed to create/update secret some-namespace/some-name-abc123: some create error", + }, + { + name: "an error is returned when updating fails", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") // force updating + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("update", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some update error") + }) + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName), + }, + wantError: "failed to create/update secret some-namespace/some-name-abc123: some update error", + }, + { + name: "an error is returned when getting fails", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") // force updating + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("get", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some get error") + }) + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + }, + wantError: "failed to create/update secret some-namespace/some-name-abc123: failed to get secret: some get error", + }, + { + name: "the update is retried when it fails due to a conflict", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") // force updating + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("update", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + var err error + once.Do(func() { + err = k8serrors.NewConflict(secretsGVR.GroupResource(), generatedSecretName, errors.New("some error")) + }) + return true, nil, err + }) + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName), + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName), + }, + }, + { + name: "upon updating we discover that a valid secret exists", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") // force updating + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("get", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, generatedSecretWithName, nil + }) + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + }, + }, + { + name: "upon updating we discover that the secret has been deleted", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") // force updating + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("get", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, k8serrors.NewNotFound(secretsGVR.GroupResource(), generatedSecretName) + }) + client.PrependReactor("create", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, nil + }) + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewCreateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + }, + }, + { + name: "upon updating we discover that the secret has been deleted and our create fails", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") // force updating + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("get", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, k8serrors.NewNotFound(secretsGVR.GroupResource(), generatedSecretName) + }) + client.PrependReactor("create", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some create error") + }) + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewCreateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + }, + wantError: "failed to create/update secret some-namespace/some-name-abc123: failed to create secret: some create error", + }, + { + name: "when generating the secret fails, we return an error", + generateKey: func() ([]byte, error) { + return nil, errors.New("some generate error") + }, + wantError: "failed to generate secret: some generate error", + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + // We cannot currently run this test in parallel since it uses the global generateKey function. + + ctx, cancel := context.WithTimeout(context.Background(), time.Second*3) + defer cancel() + + if test.generateKey != nil { + generateKey = test.generateKey + } else { + generateKey = func() ([]byte, error) { + return generatedSymmetricKey, nil + } + } + + apiClient := kubernetesfake.NewSimpleClientset() + if test.apiClient != nil { + test.apiClient(t, apiClient) + } + informerClient := kubernetesfake.NewSimpleClientset() + + storedSecret := generatedSecretWithName.DeepCopy() + if test.storedSecret != nil { + test.storedSecret(&storedSecret) + } + if storedSecret != nil { + require.NoError(t, apiClient.Tracker().Add(storedSecret)) + require.NoError(t, informerClient.Tracker().Add(storedSecret)) + } + + informers := kubeinformers.NewSharedInformerFactory(informerClient, 0) + secrets := informers.Core().V1().Secrets() + + c := New(generatedSecretNamePrefix, apiClient, secrets) + + // Must start informers before calling TestRunSynchronously(). + informers.Start(ctx.Done()) + controllerlib.TestRunSynchronously(t, c) + + err := controllerlib.TestSync(t, c, controllerlib.Context{ + Context: ctx, + Key: controllerlib.Key{ + Namespace: generatedSecretNamespace, + Name: generatedSecretName, + }, + }) + if test.wantError != "" { + require.EqualError(t, err, test.wantError) + } else { + require.NoError(t, err) + } + + if test.wantActions == nil { + test.wantActions = []kubetesting.Action{} + } + require.Equal(t, test.wantActions, apiClient.Actions()) + }) + } +}