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

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,

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

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()
+	goodOPWithTokenSigningKey := goodOP.DeepCopy()
-	goodOPWithStatus.Status.Secrets.TokenSigningKey.Name = goodSecret.Name
+	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(),