From 04d54e622aeada4a1745c34332e0d8f30d43a4a9 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 17 Dec 2020 07:41:53 -0500 Subject: [PATCH] Only set single secret status field in FederationDomainSecretsController This implementation is janky because I wanted to make the smallest change possible to try to get the code back to stable so we can release. Also deep copy an object so we aren't mutating the cache. Signed-off-by: Andrew Keesler --- cmd/pinniped-supervisor/main.go | 14 ++- .../generator/oidc_provider_secrets.go | 10 ++- .../generator/oidc_provider_secrets_test.go | 86 +++++++++++++++---- 3 files changed, 91 insertions(+), 19 deletions(-) diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index d9508290..b125e966 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -15,9 +15,8 @@ import ( "strings" "time" - "go.pinniped.dev/internal/secret" - appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/clock" kubeinformers "k8s.io/client-go/informers" @@ -29,6 +28,7 @@ import ( "k8s.io/klog/v2" "k8s.io/klog/v2/klogr" + configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1" pinnipedclientset "go.pinniped.dev/generated/1.19/client/supervisor/clientset/versioned" pinnipedinformers "go.pinniped.dev/generated/1.19/client/supervisor/informers/externalversions" "go.pinniped.dev/internal/config/supervisor" @@ -42,6 +42,7 @@ import ( "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidc/provider/manager" "go.pinniped.dev/internal/plog" + "go.pinniped.dev/internal/secret" ) const ( @@ -173,6 +174,9 @@ func startControllers( secretCache.SetTokenHMACKey(federationDomainIssuer, symmetricKey) }, ), + func(fd *configv1alpha1.FederationDomain) *corev1.LocalObjectReference { + return &fd.Status.Secrets.TokenSigningKey + }, kubeClient, pinnipedClient, secretInformer, @@ -193,6 +197,9 @@ func startControllers( secretCache.SetStateEncoderHashKey(federationDomainIssuer, symmetricKey) }, ), + func(fd *configv1alpha1.FederationDomain) *corev1.LocalObjectReference { + return &fd.Status.Secrets.StateSigningKey + }, kubeClient, pinnipedClient, secretInformer, @@ -213,6 +220,9 @@ func startControllers( secretCache.SetStateEncoderBlockKey(federationDomainIssuer, symmetricKey) }, ), + func(fd *configv1alpha1.FederationDomain) *corev1.LocalObjectReference { + return &fd.Status.Secrets.StateEncryptionKey + }, kubeClient, pinnipedClient, secretInformer, diff --git a/internal/controller/supervisorconfig/generator/oidc_provider_secrets.go b/internal/controller/supervisorconfig/generator/oidc_provider_secrets.go index 227f55d4..146e4541 100644 --- a/internal/controller/supervisorconfig/generator/oidc_provider_secrets.go +++ b/internal/controller/supervisorconfig/generator/oidc_provider_secrets.go @@ -26,6 +26,7 @@ import ( type federationDomainSecretsController struct { secretHelper SecretHelper + secretRefFunc func(domain *configv1alpha1.FederationDomain) *corev1.LocalObjectReference kubeClient kubernetes.Interface pinnipedClient pinnipedclientset.Interface opcInformer configinformers.FederationDomainInformer @@ -37,6 +38,7 @@ type federationDomainSecretsController struct { // provides the parent/child mapping logic. func NewFederationDomainSecretsController( secretHelper SecretHelper, + secretRefFunc func(domain *configv1alpha1.FederationDomain) *corev1.LocalObjectReference, kubeClient kubernetes.Interface, pinnipedClient pinnipedclientset.Interface, secretInformer corev1informers.SecretInformer, @@ -48,6 +50,7 @@ func NewFederationDomainSecretsController( Name: fmt.Sprintf("%s%s", secretHelper.NamePrefix(), "controller"), Syncer: &federationDomainSecretsController{ secretHelper: secretHelper, + secretRefFunc: secretRefFunc, kubeClient: kubeClient, pinnipedClient: pinnipedClient, secretInformer: secretInformer, @@ -102,6 +105,7 @@ func (c *federationDomainSecretsController) Sync(ctx controllerlib.Context) erro return nil } + op = op.DeepCopy() newSecret, err := c.secretHelper.Generate(op) if err != nil { return fmt.Errorf("failed to generate secret: %w", err) @@ -221,11 +225,13 @@ func (c *federationDomainSecretsController) updateFederationDomain( return fmt.Errorf("failed to get federationdomain %s/%s: %w", newOP.Namespace, newOP.Name, err) } - if reflect.DeepEqual(newOP.Status.Secrets, oldOP.Status.Secrets) { + oldOPSecretRef := c.secretRefFunc(oldOP) + newOPSecretRef := c.secretRefFunc(newOP) + if reflect.DeepEqual(oldOPSecretRef, newOPSecretRef) { return nil } - oldOP.Status.Secrets = newOP.Status.Secrets + *oldOPSecretRef = *newOPSecretRef _, err = opcClient.Update(ctx, oldOP, metav1.UpdateOptions{}) return err }) diff --git a/internal/controller/supervisorconfig/generator/oidc_provider_secrets_test.go b/internal/controller/supervisorconfig/generator/oidc_provider_secrets_test.go index d4e90863..e56125a1 100644 --- a/internal/controller/supervisorconfig/generator/oidc_provider_secrets_test.go +++ b/internal/controller/supervisorconfig/generator/oidc_provider_secrets_test.go @@ -157,6 +157,7 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) { withInformer := testutil.NewObservableWithInformerOption() _ = NewFederationDomainSecretsController( secretHelper, + nil, // secretRefFunc, not needed nil, // kubeClient, not needed nil, // pinnipedClient, not needed secretInformer, @@ -216,6 +217,7 @@ func TestNewFederationDomainSecretsControllerFilterOPC(t *testing.T) { withInformer := testutil.NewObservableWithInformerOption() _ = NewFederationDomainSecretsController( secretHelper, + nil, // secretRefFunc, not needed nil, // kubeClient, not needed nil, // pinnipedClient, not needed secretInformer, @@ -293,8 +295,14 @@ func TestFederationDomainSecretsControllerSync(t *testing.T) { }, } - goodOPWithStatus := goodOP.DeepCopy() - goodOPWithStatus.Status.Secrets.TokenSigningKey.Name = goodSecret.Name + goodOPWithTokenSigningKey := goodOP.DeepCopy() + goodOPWithTokenSigningKey.Status.Secrets.TokenSigningKey.Name = goodSecret.Name + + goodOPWithJWKS := goodOP.DeepCopy() + goodOPWithJWKS.Status.Secrets.JWKS.Name = "some-jwks-key" + + goodOPWithJWKSAndTokenSigningKey := goodOPWithJWKS.DeepCopy() + goodOPWithJWKSAndTokenSigningKey.Status.Secrets.TokenSigningKey = goodOPWithTokenSigningKey.Status.Secrets.TokenSigningKey invalidSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -343,11 +351,56 @@ func TestFederationDomainSecretsControllerSync(t *testing.T) { }, secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) - secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithStatus) + secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithTokenSigningKey) + }, + wantOPActions: []kubetesting.Action{ + kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), + kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithTokenSigningKey), + }, + wantSecretActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), + kubetesting.NewCreateAction(secretGVR, namespace, goodSecret), + }, + }, + { + name: "FederationDomain exists and secret does not exist and upon updating FederationDomain we learn a new status field has been set", + storage: func(op **configv1alpha1.FederationDomain, s **corev1.Secret) { + *s = nil + }, + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) + secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithTokenSigningKey) + }, + client: func(c *pinnipedfake.Clientset, _ *kubernetesfake.Clientset) { + c.PrependReactor("get", "federationdomains", func(_ kubetesting.Action) (bool, runtime.Object, error) { + return true, goodOPWithJWKS, nil + }) + }, + wantOPActions: []kubetesting.Action{ + kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), + kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithJWKSAndTokenSigningKey), + }, + wantSecretActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), + kubetesting.NewCreateAction(secretGVR, namespace, goodSecret), + }, + }, + { + name: "FederationDomain exists and secret does not exist and upon updating FederationDomain we learn all status fields have been set", + storage: func(op **configv1alpha1.FederationDomain, s **corev1.Secret) { + *s = nil + }, + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) + secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithTokenSigningKey) + }, + client: func(c *pinnipedfake.Clientset, _ *kubernetesfake.Clientset) { + c.PrependReactor("get", "federationdomains", func(_ kubetesting.Action) (bool, runtime.Object, error) { + return true, goodOPWithJWKSAndTokenSigningKey, nil + }) }, wantOPActions: []kubetesting.Action{ kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), - kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithStatus), }, wantSecretActions: []kubetesting.Action{ kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), @@ -362,11 +415,11 @@ func TestFederationDomainSecretsControllerSync(t *testing.T) { secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) secretHelper.EXPECT().IsValid(goodOP, invalidSecret).Times(2).Return(false) - secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithStatus) + secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithTokenSigningKey) }, wantOPActions: []kubetesting.Action{ kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), - kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithStatus), + kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithTokenSigningKey), }, wantSecretActions: []kubetesting.Action{ kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), @@ -389,11 +442,11 @@ func TestFederationDomainSecretsControllerSync(t *testing.T) { secretHelper.EXPECT().Generate(goodOP).Times(1).Return(otherSecret, nil) secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(1).Return(false) secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(1).Return(true) - secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithStatus) + secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithTokenSigningKey) }, wantOPActions: []kubetesting.Action{ kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), - kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithStatus), + kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithTokenSigningKey), }, wantSecretActions: []kubetesting.Action{ kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), @@ -459,7 +512,7 @@ func TestFederationDomainSecretsControllerSync(t *testing.T) { secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) secretHelper.EXPECT().IsValid(goodOP, invalidSecret).Times(3).Return(false) - secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithStatus) + secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithTokenSigningKey) }, client: func(_ *pinnipedfake.Clientset, c *kubernetesfake.Clientset) { once := sync.Once{} @@ -471,7 +524,7 @@ func TestFederationDomainSecretsControllerSync(t *testing.T) { }, wantOPActions: []kubetesting.Action{ kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), - kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithStatus), + kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithTokenSigningKey), }, wantSecretActions: []kubetesting.Action{ kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), @@ -488,7 +541,7 @@ func TestFederationDomainSecretsControllerSync(t *testing.T) { secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) secretHelper.EXPECT().IsValid(goodOP, invalidSecret).Times(2).Return(false) - secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithStatus) + secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithTokenSigningKey) }, client: func(c *pinnipedfake.Clientset, _ *kubernetesfake.Clientset) { c.PrependReactor("get", "federationdomains", func(_ kubetesting.Action) (bool, runtime.Object, error) { @@ -502,7 +555,7 @@ func TestFederationDomainSecretsControllerSync(t *testing.T) { kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret), }, - wantError: fmt.Sprintf("failed to update federationdomain: failed to get federationdomain %s/%s: some get error", goodOPWithStatus.Namespace, goodOPWithStatus.Name), + wantError: fmt.Sprintf("failed to update federationdomain: failed to get federationdomain %s/%s: some get error", goodOPWithTokenSigningKey.Namespace, goodOPWithTokenSigningKey.Name), }, { name: "FederationDomain exists and invalid secret exists and updating FederationDomain fails due to conflict", @@ -512,7 +565,7 @@ func TestFederationDomainSecretsControllerSync(t *testing.T) { secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) secretHelper.EXPECT().IsValid(goodOP, invalidSecret).Times(2).Return(false) - secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithStatus) + secretHelper.EXPECT().ObserveActiveSecretAndUpdateParentFederationDomain(goodOP, goodSecret).Times(1).Return(goodOPWithTokenSigningKey) }, client: func(c *pinnipedfake.Clientset, _ *kubernetesfake.Clientset) { once := sync.Once{} @@ -524,9 +577,9 @@ func TestFederationDomainSecretsControllerSync(t *testing.T) { }, wantOPActions: []kubetesting.Action{ kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), - kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithStatus), + kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithTokenSigningKey), kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), - kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithStatus), + kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithTokenSigningKey), }, wantSecretActions: []kubetesting.Action{ kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), @@ -585,6 +638,9 @@ func TestFederationDomainSecretsControllerSync(t *testing.T) { c := NewFederationDomainSecretsController( secretHelper, + func(fd *configv1alpha1.FederationDomain) *corev1.LocalObjectReference { + return &fd.Status.Secrets.TokenSigningKey + }, kubeAPIClient, pinnipedAPIClient, kubeInformers.Core().V1().Secrets(),