From c94ee7188cea137716c75741149181c1c8d4b769 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Mon, 1 Mar 2021 15:41:55 -0600 Subject: [PATCH] Factor out issuerconfig.UpdateStrategy helper. Signed-off-by: Matt Moyer --- .../issuerconfig/update_strategy.go | 52 +++++++ .../issuerconfig/update_strategy_test.go | 145 ++++++++++++++++++ .../controller/kubecertagent/annotater.go | 9 +- internal/controller/kubecertagent/creater.go | 15 +- internal/controller/kubecertagent/execer.go | 25 ++- .../controller/kubecertagent/kubecertagent.go | 29 ---- 6 files changed, 234 insertions(+), 41 deletions(-) create mode 100644 internal/controller/issuerconfig/update_strategy.go create mode 100644 internal/controller/issuerconfig/update_strategy_test.go diff --git a/internal/controller/issuerconfig/update_strategy.go b/internal/controller/issuerconfig/update_strategy.go new file mode 100644 index 00000000..33a5ecd8 --- /dev/null +++ b/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] } diff --git a/internal/controller/issuerconfig/update_strategy_test.go b/internal/controller/issuerconfig/update_strategy_test.go new file mode 100644 index 00000000..f261c872 --- /dev/null +++ b/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) + }) + } +} diff --git a/internal/controller/kubecertagent/annotater.go b/internal/controller/kubecertagent/annotater.go index 5ccf95ff..a18cc732 100644 --- a/internal/controller/kubecertagent/annotater.go +++ b/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. diff --git a/internal/controller/kubecertagent/creater.go b/internal/controller/kubecertagent/creater.go index 4db1ba06..6cb37934 100644 --- a/internal/controller/kubecertagent/creater.go +++ b/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 diff --git a/internal/controller/kubecertagent/execer.go b/internal/controller/kubecertagent/execer.go index 457fd5c1..2322103e 100644 --- a/internal/controller/kubecertagent/execer.go +++ b/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 } diff --git a/internal/controller/kubecertagent/kubecertagent.go b/internal/controller/kubecertagent/kubecertagent.go index d7921a45..dff42f63 100644 --- a/internal/controller/kubecertagent/kubecertagent.go +++ b/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,