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 <akeesler@vmware.com>

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

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
+}

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())
+		})
+	}
+}