Merge pull request #295 from ankeesler/fix-secret-status

Only set single secret status field in FederationDomainSecretsController

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(),