diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 8e61cc38..e5b5a6b4 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -16,6 +16,8 @@ import ( "go.pinniped.dev/internal/secret" + appsv1 "k8s.io/api/apps/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/clock" kubeinformers "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" @@ -30,6 +32,7 @@ import ( pinnipedinformers "go.pinniped.dev/generated/1.19/client/supervisor/informers/externalversions" "go.pinniped.dev/internal/config/supervisor" "go.pinniped.dev/internal/controller/supervisorconfig" + "go.pinniped.dev/internal/controller/supervisorconfig/secretgenerator" "go.pinniped.dev/internal/controller/supervisorconfig/upstreamwatcher" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/downward" @@ -78,6 +81,8 @@ func startControllers( dynamicJWKSProvider jwks.DynamicJWKSProvider, dynamicTLSCertProvider provider.DynamicTLSCertProvider, dynamicUpstreamIDPProvider provider.DynamicUpstreamIDPProvider, + secretCache *secret.Cache, + supervisorDeployment *appsv1.Deployment, kubeClient kubernetes.Interface, pinnipedClient pinnipedclientset.Interface, kubeInformers kubeinformers.SharedInformerFactory, @@ -126,6 +131,18 @@ func startControllers( ), singletonWorker, ). + WithController( + secretgenerator.New( + supervisorDeployment, + kubeClient, + kubeInformers.Core().V1().Secrets(), + func(secret []byte) { + plog.Debug("setting csrf cookie secret") + secretCache.SetCSRFCookieEncoderHashKey(secret) + }, + ), + singletonWorker, + ). WithController( upstreamwatcher.New( dynamicUpstreamIDPProvider, @@ -145,6 +162,41 @@ func startControllers( go controllerManager.Start(ctx) } +func getSupervisorDeployment( + ctx context.Context, + kubeClient kubernetes.Interface, + podInfo *downward.PodInfo, +) (*appsv1.Deployment, error) { + ns := podInfo.Namespace + + pod, err := kubeClient.CoreV1().Pods(ns).Get(ctx, podInfo.Name, metav1.GetOptions{}) + if err != nil { + return nil, fmt.Errorf("could not get pod: %w", err) + } + + podOwner := metav1.GetControllerOf(pod) + if podOwner == nil { + return nil, fmt.Errorf("pod %s/%s is missing owner", ns, podInfo.Name) + } + + rs, err := kubeClient.AppsV1().ReplicaSets(ns).Get(ctx, podOwner.Name, metav1.GetOptions{}) + if err != nil { + return nil, fmt.Errorf("could not get replicaset: %w", err) + } + + rsOwner := metav1.GetControllerOf(rs) + if rsOwner == nil { + return nil, fmt.Errorf("replicaset %s/%s is missing owner", ns, podInfo.Name) + } + + d, err := kubeClient.AppsV1().Deployments(ns).Get(ctx, rsOwner.Name, metav1.GetOptions{}) + if err != nil { + return nil, fmt.Errorf("could not get deployment: %w", err) + } + + return d, nil +} + func newClients() (kubernetes.Interface, pinnipedclientset.Interface, error) { kubeConfig, err := restclient.InClusterConfig() if err != nil { @@ -166,7 +218,9 @@ func newClients() (kubernetes.Interface, pinnipedclientset.Interface, error) { return kubeClient, pinnipedClient, nil } -func run(serverInstallationNamespace string, cfg *supervisor.Config) error { +func run(podInfo *downward.PodInfo, cfg *supervisor.Config) error { + serverInstallationNamespace := podInfo.Namespace + ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -196,19 +250,22 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error { dynamicJWKSProvider := jwks.NewDynamicJWKSProvider() dynamicTLSCertProvider := provider.NewDynamicTLSCertProvider() dynamicUpstreamIDPProvider := provider.NewDynamicUpstreamIDPProvider() - cache := secret.Cache{} - - cache.SetCSRFCookieEncoderHashKey([]byte("fake-csrf-hash-secret")) // TODO fetch from `Secret` + secretCache := secret.Cache{} // OIDC endpoints will be served by the oidProvidersManager, and any non-OIDC paths will fallback to the healthMux. oidProvidersManager := manager.NewManager( healthMux, dynamicJWKSProvider, dynamicUpstreamIDPProvider, - cache, + &secretCache, kubeClient.CoreV1().Secrets(serverInstallationNamespace), ) + supervisorDeployment, err := getSupervisorDeployment(ctx, kubeClient, podInfo) + if err != nil { + return fmt.Errorf("cannot get supervisor deployment: %w", err) + } + startControllers( ctx, cfg, @@ -216,6 +273,8 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error { dynamicJWKSProvider, dynamicTLSCertProvider, dynamicUpstreamIDPProvider, + &secretCache, + supervisorDeployment, kubeClient, pinnipedClient, kubeInformers, @@ -284,7 +343,7 @@ func main() { klog.Fatal(fmt.Errorf("could not load config: %w", err)) } - if err := run(podInfo.Namespace, cfg); err != nil { + if err := run(podInfo, cfg); err != nil { klog.Fatal(err) } } diff --git a/deploy/supervisor/deployment.yaml b/deploy/supervisor/deployment.yaml index 28b8d93f..1e0c75c0 100644 --- a/deploy/supervisor/deployment.yaml +++ b/deploy/supervisor/deployment.yaml @@ -132,6 +132,9 @@ spec: - path: "namespace" fieldRef: fieldPath: metadata.namespace + - path: "name" + fieldRef: + fieldPath: metadata.name #! This will help make sure our multiple pods run on different nodes, making #! our deployment "more" "HA". affinity: diff --git a/deploy/supervisor/rbac.yaml b/deploy/supervisor/rbac.yaml index 33e86585..b66c00fc 100644 --- a/deploy/supervisor/rbac.yaml +++ b/deploy/supervisor/rbac.yaml @@ -25,6 +25,14 @@ rules: - apiGroups: [idp.supervisor.pinniped.dev] resources: [upstreamoidcproviders/status] verbs: [get, patch, update] + #! We want to be able to read pods/replicasets/deployment so we can learn who our deployment is to set + #! as an owner reference. + - apiGroups: [""] + resources: [pods] + verbs: [get] + - apiGroups: [apps] + resources: [replicasets,deployments] + verbs: [get] --- kind: RoleBinding apiVersion: rbac.authorization.k8s.io/v1 diff --git a/internal/controller/supervisorconfig/secretgenerator/secretgenerator.go b/internal/controller/supervisorconfig/secretgenerator/secretgenerator.go index 89df14e1..5a767ce0 100644 --- a/internal/controller/supervisorconfig/secretgenerator/secretgenerator.go +++ b/internal/controller/supervisorconfig/secretgenerator/secretgenerator.go @@ -9,9 +9,11 @@ import ( "crypto/rand" "fmt" + appsv1 "k8s.io/api/apps/v1" 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/schema" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" "k8s.io/client-go/util/retry" @@ -42,22 +44,35 @@ func generateSymmetricKey() ([]byte, error) { } type controller struct { - secretNamePrefix string + owner *appsv1.Deployment client kubernetes.Interface secrets corev1informers.SecretInformer + onCreateOrUpdate func(secret []byte) } // 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 { +func New( + owner *appsv1.Deployment, + client kubernetes.Interface, + secrets corev1informers.SecretInformer, + onCreateOrUpdate func(secret []byte), +) controllerlib.Controller { c := controller{ - secretNamePrefix: secretNamePrefix, + owner: owner, client: client, secrets: secrets, + onCreateOrUpdate: onCreateOrUpdate, } - filter := pinnipedcontroller.SimpleFilterWithSingletonQueue(isOwnee) + filter := pinnipedcontroller.SimpleFilter(func(obj metav1.Object) bool { + return metav1.IsControlledBy(obj, owner) + }, nil) return controllerlib.New( - controllerlib.Config{Name: secretNamePrefix + "-secrets-generator", Syncer: &c}, + controllerlib.Config{Name: owner.Name + "-secret-generator", Syncer: &c}, controllerlib.WithInformer(secrets, filter, controllerlib.InformerOption{}), + controllerlib.WithInitialEvent(controllerlib.Key{ + Namespace: owner.Namespace, + Name: owner.Name + "-keys", + }), ) } @@ -72,10 +87,11 @@ func (c *controller) Sync(ctx controllerlib.Context) error { secretNeedsUpdate := isNotFound || !c.isValid(secret) if !secretNeedsUpdate { plog.Debug("secret is up to date", "secret", klog.KObj(secret)) + c.onCreateOrUpdate(secret.Data[symmetricKeySecretDataKey]) return nil } - newSecret, err := c.generateSecret(ctx.Key.Namespace) + newSecret, err := c.generateSecret(ctx.Key.Namespace, ctx.Key.Name) if err != nil { return fmt.Errorf("failed to generate secret: %w", err) } @@ -83,12 +99,14 @@ func (c *controller) Sync(ctx controllerlib.Context) error { if isNotFound { err = c.createSecret(ctx.Context, newSecret) } else { - err = c.updateSecret(ctx.Context, newSecret, ctx.Key.Name) + 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 fmt.Errorf("failed to create/update secret %s/%s: %w", newSecret.Namespace, newSecret.Name, err) } + c.onCreateOrUpdate(newSecret.Data[symmetricKeySecretDataKey]) + return nil } @@ -108,16 +126,24 @@ func (c *controller) isValid(secret *corev1.Secret) bool { return true } -func (c *controller) generateSecret(namespace string) (*corev1.Secret, error) { +func (c *controller) generateSecret(namespace, name string) (*corev1.Secret, error) { symmetricKey, err := generateKey() if err != nil { return nil, err } + deploymentGVK := schema.GroupVersionKind{ + Group: appsv1.SchemeGroupVersion.Group, + Version: appsv1.SchemeGroupVersion.Version, + Kind: "Deployment", + } return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - GenerateName: c.secretNamePrefix, - Namespace: namespace, + Name: name, + Namespace: namespace, + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(c.owner, deploymentGVK), + }, }, Type: symmetricKeySecretType, Data: map[string][]byte{ @@ -131,8 +157,8 @@ func (c *controller) createSecret(ctx context.Context, newSecret *corev1.Secret) return err } -func (c *controller) updateSecret(ctx context.Context, newSecret *corev1.Secret, secretName string) error { - secrets := c.client.CoreV1().Secrets(newSecret.Namespace) +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) @@ -141,26 +167,21 @@ func (c *controller) updateSecret(ctx context.Context, newSecret *corev1.Secret, } if isNotFound { - if err := c.createSecret(ctx, newSecret); err != nil { + if err := c.createSecret(ctx, *newSecret); err != nil { return fmt.Errorf("failed to create secret: %w", err) } return nil } if c.isValid(currentSecret) { + *newSecret = currentSecret return nil } - currentSecret.Type = newSecret.Type - currentSecret.Data = newSecret.Data + 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 index 884e2049..f753fcf8 100644 --- a/internal/controller/supervisorconfig/secretgenerator/secretgenerator_test.go +++ b/internal/controller/supervisorconfig/secretgenerator/secretgenerator_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -25,9 +26,9 @@ import ( func TestController(t *testing.T) { const ( - generatedSecretNamespace = "some-namespace" - generatedSecretNamePrefix = "some-name-" - generatedSecretName = "some-name-abc123" + generatedSecretNamespace = "some-namespace" + generatedSecretName = "some-name-abc123" + otherGeneratedSecretName = "some-other-name-abc123" ) var ( @@ -37,32 +38,60 @@ func TestController(t *testing.T) { Resource: "secrets", } - generatedSymmetricKey = []byte("some-neato-32-byte-generated-key") + owner = &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-owner-name", + UID: "some-owner-uid", + }, + } + ownerGVK = schema.GroupVersionKind{ + Group: appsv1.SchemeGroupVersion.Group, + Version: appsv1.SchemeGroupVersion.Version, + Kind: "Deployment", + } + + generatedSymmetricKey = []byte("some-neato-32-byte-generated-key") + otherGeneratedSymmetricKey = []byte("some-funio-32-byte-generated-key") generatedSecret = &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - GenerateName: generatedSecretNamePrefix, - Namespace: generatedSecretNamespace, + Name: generatedSecretName, + Namespace: generatedSecretNamespace, + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(owner, ownerGVK), + }, }, Type: "secrets.pinniped.dev/symmetric", Data: map[string][]byte{ "key": generatedSymmetricKey, }, } - ) - generatedSecretWithName := generatedSecret.DeepCopy() - generatedSecretWithName.Name = generatedSecretName + otherGeneratedSecret = &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: generatedSecretName, + Namespace: generatedSecretNamespace, + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(owner, ownerGVK), + }, + }, + Type: "secrets.pinniped.dev/symmetric", + Data: map[string][]byte{ + "key": otherGeneratedSymmetricKey, + }, + } + ) 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 string + storedSecret func(**corev1.Secret) + generateKey func() ([]byte, error) + apiClient func(*testing.T, *kubernetesfake.Clientset) + wantError string + wantActions []kubetesting.Action + wantCallbackSecret []byte }{ { name: "when the secrets does not exist, it gets generated", @@ -72,9 +101,11 @@ func TestController(t *testing.T) { wantActions: []kubetesting.Action{ kubetesting.NewCreateAction(secretsGVR, generatedSecretNamespace, generatedSecret), }, + wantCallbackSecret: generatedSymmetricKey, }, { - name: "when a valid secret exists, nothing happens", + name: "when a valid secret exists, nothing happens", + wantCallbackSecret: generatedSymmetricKey, }, { name: "secret gets updated when the type is wrong", @@ -83,8 +114,9 @@ func TestController(t *testing.T) { }, wantActions: []kubetesting.Action{ kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), - kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), }, + wantCallbackSecret: generatedSymmetricKey, }, { name: "secret gets updated when the key data does not exist", @@ -93,8 +125,9 @@ func TestController(t *testing.T) { }, wantActions: []kubetesting.Action{ kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), - kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), }, + wantCallbackSecret: generatedSymmetricKey, }, { name: "secret gets updated when the key data is too short", @@ -103,8 +136,9 @@ func TestController(t *testing.T) { }, wantActions: []kubetesting.Action{ kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), - kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), }, + wantCallbackSecret: generatedSymmetricKey, }, { name: "an error is returned when creating fails", @@ -133,7 +167,7 @@ func TestController(t *testing.T) { }, wantActions: []kubetesting.Action{ kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), - kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), }, wantError: "failed to create/update secret some-namespace/some-name-abc123: some update error", }, @@ -168,10 +202,11 @@ func TestController(t *testing.T) { }, wantActions: []kubetesting.Action{ kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), - kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), - kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), }, + wantCallbackSecret: generatedSymmetricKey, }, { name: "upon updating we discover that a valid secret exists", @@ -180,12 +215,13 @@ func TestController(t *testing.T) { }, apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { client.PrependReactor("get", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { - return true, generatedSecretWithName, nil + return true, otherGeneratedSecret, nil }) }, wantActions: []kubetesting.Action{ kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), }, + wantCallbackSecret: otherGeneratedSymmetricKey, }, { name: "upon updating we discover that the secret has been deleted", @@ -204,6 +240,7 @@ func TestController(t *testing.T) { kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), kubetesting.NewCreateAction(secretsGVR, generatedSecretNamespace, generatedSecret), }, + wantCallbackSecret: generatedSymmetricKey, }, { name: "upon updating we discover that the secret has been deleted and our create fails", @@ -257,7 +294,7 @@ func TestController(t *testing.T) { } informerClient := kubernetesfake.NewSimpleClientset() - storedSecret := generatedSecretWithName.DeepCopy() + storedSecret := generatedSecret.DeepCopy() if test.storedSecret != nil { test.storedSecret(&storedSecret) } @@ -269,7 +306,11 @@ func TestController(t *testing.T) { informers := kubeinformers.NewSharedInformerFactory(informerClient, 0) secrets := informers.Core().V1().Secrets() - c := New(generatedSecretNamePrefix, apiClient, secrets) + var callbackSecret []byte + c := New(owner, apiClient, secrets, func(secret []byte) { + require.Nil(t, callbackSecret, "callback was called twice") + callbackSecret = secret + }) // Must start informers before calling TestRunSynchronously(). informers.Start(ctx.Done()) @@ -292,6 +333,8 @@ func TestController(t *testing.T) { test.wantActions = []kubetesting.Action{} } require.Equal(t, test.wantActions, apiClient.Actions()) + + require.Equal(t, test.wantCallbackSecret, callbackSecret) }) } } diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index ad33b408..6043ba7c 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -37,7 +37,7 @@ type Manager struct { nextHandler http.Handler // the next handler in a chain, called when this manager didn't know how to handle a request dynamicJWKSProvider jwks.DynamicJWKSProvider // in-memory cache of per-issuer JWKS data idpListGetter oidc.IDPListGetter // in-memory cache of upstream IDPs - cache secret.Cache // in-memory cache of cryptographic material + cache *secret.Cache // in-memory cache of cryptographic material secretsClient corev1client.SecretInterface } @@ -49,7 +49,7 @@ func NewManager( nextHandler http.Handler, dynamicJWKSProvider jwks.DynamicJWKSProvider, idpListGetter oidc.IDPListGetter, - cache secret.Cache, + cache *secret.Cache, secretsClient corev1client.SecretInterface, ) *Manager { return &Manager{ diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index 997f5f61..0a59abb0 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -246,7 +246,7 @@ func TestManager(t *testing.T) { cache := secret.Cache{} cache.SetCSRFCookieEncoderHashKey([]byte("fake-csrf-hash-secret")) - subject = NewManager(nextHandler, dynamicJWKSProvider, idpListGetter, cache, secretsClient) + subject = NewManager(nextHandler, dynamicJWKSProvider, idpListGetter, &cache, secretsClient) }) when("given no providers via SetProviders()", func() {