Merge pull request #469 from mattmoyer/split-out-credentialissuer-status-helpers

Factor out issuerconfig.UpdateStrategy helper.

internal/controller/issuerconfig/update_strategy.go

@@ -0,0 +1,52 @@
+// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package issuerconfig
+
+import (
+	"context"
+	"sort"
+
+	"go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1"
+	"go.pinniped.dev/generated/latest/client/concierge/clientset/versioned"
+)
+
+// UpdateStrategy creates or updates the desired strategy in the CredentialIssuer status.strategies field.
+// The CredentialIssuer will be created if it does not already exist.
+func UpdateStrategy(ctx context.Context,
+	name string,
+	credentialIssuerLabels map[string]string,
+	pinnipedAPIClient versioned.Interface,
+	strategy v1alpha1.CredentialIssuerStrategy,
+) error {
+	return CreateOrUpdateCredentialIssuerStatus(
+		ctx,
+		name,
+		credentialIssuerLabels,
+		pinnipedAPIClient,
+		func(configToUpdate *v1alpha1.CredentialIssuerStatus) { mergeStrategy(configToUpdate, strategy) },
+	)
+}
+
+func mergeStrategy(configToUpdate *v1alpha1.CredentialIssuerStatus, strategy v1alpha1.CredentialIssuerStrategy) {
+	var existing *v1alpha1.CredentialIssuerStrategy
+	for i := range configToUpdate.Strategies {
+		if configToUpdate.Strategies[i].Type == strategy.Type {
+			existing = &configToUpdate.Strategies[i]
+			break
+		}
+	}
+	if existing != nil {
+		strategy.DeepCopyInto(existing)
+	} else {
+		configToUpdate.Strategies = append(configToUpdate.Strategies, strategy)
+	}
+	sort.Stable(sortableStrategies(configToUpdate.Strategies))
+}
+
+// TODO: sort strategies by server preference rather than alphanumerically by type.
+type sortableStrategies []v1alpha1.CredentialIssuerStrategy
+
+func (s sortableStrategies) Len() int           { return len(s) }
+func (s sortableStrategies) Less(i, j int) bool { return s[i].Type < s[j].Type }
+func (s sortableStrategies) Swap(i, j int)      { s[i], s[j] = s[j], s[i] }

internal/controller/issuerconfig/update_strategy_test.go

@@ -0,0 +1,145 @@
+// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package issuerconfig
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1"
+)
+
+func TestMergeStrategy(t *testing.T) {
+	t1 := metav1.Now()
+	t2 := metav1.NewTime(metav1.Now().Add(-1 * time.Hour))
+
+	tests := []struct {
+		name           string
+		configToUpdate v1alpha1.CredentialIssuerStatus
+		strategy       v1alpha1.CredentialIssuerStrategy
+		expected       v1alpha1.CredentialIssuerStatus
+	}{
+		{
+			name: "new entry",
+			configToUpdate: v1alpha1.CredentialIssuerStatus{
+				Strategies: nil,
+			},
+			strategy: v1alpha1.CredentialIssuerStrategy{
+				Type:           "Type1",
+				Status:         v1alpha1.SuccessStrategyStatus,
+				Reason:         "some reason",
+				Message:        "some message",
+				LastUpdateTime: t1,
+			},
+			expected: v1alpha1.CredentialIssuerStatus{
+				Strategies: []v1alpha1.CredentialIssuerStrategy{
+					{
+						Type:           "Type1",
+						Status:         v1alpha1.SuccessStrategyStatus,
+						Reason:         "some reason",
+						Message:        "some message",
+						LastUpdateTime: t1,
+					},
+				},
+			},
+		},
+		{
+			name: "existing entry to update",
+			configToUpdate: v1alpha1.CredentialIssuerStatus{
+				Strategies: []v1alpha1.CredentialIssuerStrategy{
+					{
+						Type:           "Type1",
+						Status:         v1alpha1.ErrorStrategyStatus,
+						Reason:         "some starting reason",
+						Message:        "some starting message",
+						LastUpdateTime: t2,
+					},
+				},
+			},
+			strategy: v1alpha1.CredentialIssuerStrategy{
+				Type:           "Type1",
+				Status:         v1alpha1.SuccessStrategyStatus,
+				Reason:         "some reason",
+				Message:        "some message",
+				LastUpdateTime: t1,
+			},
+			expected: v1alpha1.CredentialIssuerStatus{
+				Strategies: []v1alpha1.CredentialIssuerStrategy{
+					{
+						Type:           "Type1",
+						Status:         v1alpha1.SuccessStrategyStatus,
+						Reason:         "some reason",
+						Message:        "some message",
+						LastUpdateTime: t1,
+					},
+				},
+			},
+		},
+		{
+			name: "new entry among others",
+			configToUpdate: v1alpha1.CredentialIssuerStatus{
+				Strategies: []v1alpha1.CredentialIssuerStrategy{
+					{
+						Type:           "Type0",
+						Status:         v1alpha1.ErrorStrategyStatus,
+						Reason:         "some starting reason 0",
+						Message:        "some starting message 0",
+						LastUpdateTime: t2,
+					},
+					{
+						Type:           "Type2",
+						Status:         v1alpha1.ErrorStrategyStatus,
+						Reason:         "some starting reason 0",
+						Message:        "some starting message 0",
+						LastUpdateTime: t2,
+					},
+				},
+			},
+			strategy: v1alpha1.CredentialIssuerStrategy{
+				Type:           "Type1",
+				Status:         v1alpha1.SuccessStrategyStatus,
+				Reason:         "some reason",
+				Message:        "some message",
+				LastUpdateTime: t1,
+			},
+			expected: v1alpha1.CredentialIssuerStatus{
+				Strategies: []v1alpha1.CredentialIssuerStrategy{
+					{
+						Type:           "Type0",
+						Status:         v1alpha1.ErrorStrategyStatus,
+						Reason:         "some starting reason 0",
+						Message:        "some starting message 0",
+						LastUpdateTime: t2,
+					},
+					// Expect the Type1 entry to be sorted alphanumerically between the existing entries.
+					{
+						Type:           "Type1",
+						Status:         v1alpha1.SuccessStrategyStatus,
+						Reason:         "some reason",
+						Message:        "some message",
+						LastUpdateTime: t1,
+					},
+					{
+						Type:           "Type2",
+						Status:         v1alpha1.ErrorStrategyStatus,
+						Reason:         "some starting reason 0",
+						Message:        "some starting message 0",
+						LastUpdateTime: t2,
+					},
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			updated := tt.configToUpdate.DeepCopy()
+			mergeStrategy(updated, tt.strategy)
+			require.Equal(t, &tt.expected, updated)
+		})
+	}
+}

internal/controller/kubecertagent/annotater.go

@@ -18,6 +18,7 @@ import (
 
 	pinnipedclientset "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned"
 	pinnipedcontroller "go.pinniped.dev/internal/controller"
+	"go.pinniped.dev/internal/controller/issuerconfig"
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/plog"
 )
@@ -121,7 +122,13 @@ func (c *annotaterController) Sync(ctx controllerlib.Context) error {
 			keyPath,
 		); err != nil {
 			err = fmt.Errorf("cannot update agent pod: %w", err)
-			strategyResultUpdateErr := createOrUpdateCredentialIssuer(ctx.Context, *c.credentialIssuerLocationConfig, nil, c.clock, c.pinnipedAPIClient, err)
+			strategyResultUpdateErr := issuerconfig.UpdateStrategy(
+				ctx.Context,
+				c.credentialIssuerLocationConfig.Name,
+				nil,
+				c.pinnipedAPIClient,
+				strategyError(c.clock, err),
+			)
 			if strategyResultUpdateErr != nil {
 				// If the CI update fails, then we probably want to try again. This controller will get
 				// called again because of the pod create failure, so just try the CI update again then.

internal/controller/kubecertagent/creater.go

@@ -17,6 +17,7 @@ import (
 	pinnipedclientset "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned"
 	"go.pinniped.dev/internal/constable"
 	pinnipedcontroller "go.pinniped.dev/internal/controller"
+	"go.pinniped.dev/internal/controller/issuerconfig"
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/plog"
 )
@@ -96,13 +97,12 @@ func (c *createrController) Sync(ctx controllerlib.Context) error {
 	if len(controllerManagerPods) == 0 {
 		// If there are no controller manager pods, we alert the user that we can't find the keypair via
 		// the CredentialIssuer.
-		return createOrUpdateCredentialIssuer(
+		return issuerconfig.UpdateStrategy(
 			ctx.Context,
-			*c.credentialIssuerLocationConfig,
+			c.credentialIssuerLocationConfig.Name,
 			c.credentialIssuerLabels,
-			c.clock,
 			c.pinnipedAPIClient,
-			constable.Error("did not find kube-controller-manager pod(s)"),
+			strategyError(c.clock, constable.Error("did not find kube-controller-manager pod(s)")),
 		)
 	}
 
@@ -131,13 +131,12 @@ func (c *createrController) Sync(ctx controllerlib.Context) error {
 				Create(ctx.Context, agentPod, metav1.CreateOptions{})
 			if err != nil {
 				err = fmt.Errorf("cannot create agent pod: %w", err)
-				strategyResultUpdateErr := createOrUpdateCredentialIssuer(
+				strategyResultUpdateErr := issuerconfig.UpdateStrategy(
 					ctx.Context,
-					*c.credentialIssuerLocationConfig,
+					c.credentialIssuerLocationConfig.Name,
 					c.credentialIssuerLabels,
-					c.clock,
 					c.pinnipedAPIClient,
-					err,
+					strategyError(c.clock, err),
 				)
 				if strategyResultUpdateErr != nil {
 					// If the CI update fails, then we probably want to try again. This controller will get

internal/controller/kubecertagent/execer.go

@@ -14,6 +14,7 @@ import (
 
 	pinnipedclientset "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned"
 	pinnipedcontroller "go.pinniped.dev/internal/controller"
+	"go.pinniped.dev/internal/controller/issuerconfig"
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/dynamiccert"
 )
@@ -87,21 +88,39 @@ func (c *execerController) Sync(ctx controllerlib.Context) error {
 
 	certPEM, err := c.podCommandExecutor.Exec(agentPod.Namespace, agentPod.Name, "cat", certPath)
 	if err != nil {
-		strategyResultUpdateErr := createOrUpdateCredentialIssuer(ctx.Context, *c.credentialIssuerLocationConfig, nil, c.clock, c.pinnipedAPIClient, err)
+		strategyResultUpdateErr := issuerconfig.UpdateStrategy(
+			ctx.Context,
+			c.credentialIssuerLocationConfig.Name,
+			nil,
+			c.pinnipedAPIClient,
+			strategyError(c.clock, err),
+		)
 		klog.ErrorS(strategyResultUpdateErr, "could not create or update CredentialIssuer with strategy success")
 		return err
 	}
 
 	keyPEM, err := c.podCommandExecutor.Exec(agentPod.Namespace, agentPod.Name, "cat", keyPath)
 	if err != nil {
-		strategyResultUpdateErr := createOrUpdateCredentialIssuer(ctx.Context, *c.credentialIssuerLocationConfig, nil, c.clock, c.pinnipedAPIClient, err)
+		strategyResultUpdateErr := issuerconfig.UpdateStrategy(
+			ctx.Context,
+			c.credentialIssuerLocationConfig.Name,
+			nil,
+			c.pinnipedAPIClient,
+			strategyError(c.clock, err),
+		)
 		klog.ErrorS(strategyResultUpdateErr, "could not create or update CredentialIssuer with strategy success")
 		return err
 	}
 
 	c.dynamicCertProvider.Set([]byte(certPEM), []byte(keyPEM))
 
-	err = createOrUpdateCredentialIssuer(ctx.Context, *c.credentialIssuerLocationConfig, nil, c.clock, c.pinnipedAPIClient, nil)
+	err = issuerconfig.UpdateStrategy(
+		ctx.Context,
+		c.credentialIssuerLocationConfig.Name,
+		nil,
+		c.pinnipedAPIClient,
+		strategySuccess(c.clock),
+	)
 	if err != nil {
 		return err
 	}

internal/controller/kubecertagent/kubecertagent.go

@@ -10,7 +10,6 @@
 package kubecertagent
 
 import (
-	"context"
 	"encoding/hex"
 	"fmt"
 	"hash/fnv"
@@ -25,8 +24,6 @@ import (
 	corev1informers "k8s.io/client-go/informers/core/v1"
 
 	configv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1"
-	pinnipedclientset "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned"
-	"go.pinniped.dev/internal/controller/issuerconfig"
 	"go.pinniped.dev/internal/plog"
 )
 
@@ -280,32 +277,6 @@ func findControllerManagerPodForSpecificAgentPod(
 	return maybeControllerManagerPod, nil
 }
 
-func createOrUpdateCredentialIssuer(ctx context.Context,
-	ciConfig CredentialIssuerLocationConfig,
-	credentialIssuerLabels map[string]string,
-	clock clock.Clock,
-	pinnipedAPIClient pinnipedclientset.Interface,
-	err error,
-) error {
-	return issuerconfig.CreateOrUpdateCredentialIssuerStatus(
-		ctx,
-		ciConfig.Name,
-		credentialIssuerLabels,
-		pinnipedAPIClient,
-		func(configToUpdate *configv1alpha1.CredentialIssuerStatus) {
-			var strategyResult configv1alpha1.CredentialIssuerStrategy
-			if err == nil {
-				strategyResult = strategySuccess(clock)
-			} else {
-				strategyResult = strategyError(clock, err)
-			}
-			configToUpdate.Strategies = []configv1alpha1.CredentialIssuerStrategy{
-				strategyResult,
-			}
-		},
-	)
-}
-
 func strategySuccess(clock clock.Clock) configv1alpha1.CredentialIssuerStrategy {
 	return configv1alpha1.CredentialIssuerStrategy{
 		Type:           configv1alpha1.KubeClusterSigningCertificateStrategyType,