From cbc80d5bc4f3c7d61c4e82dc6d9488e0c838bb5c Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 27 Aug 2020 17:11:10 -0700 Subject: [PATCH] RetryOnConflict when updating CredentialIssuerConfig from outside any controller - Controllers will automatically run again when there's an error, but when we want to update CredentialIssuerConfig from server.go we should be careful to retry on conflicts - Add unit tests for `issuerconfig.CreateOrUpdateCredentialIssuerConfig()` which was covered by integration tests in previous commits, but not covered by units tests yet. --- .../controller/apicerts/update_api_service.go | 3 +- ...eate_or_update_credential_issuer_config.go | 44 ++-- ...or_update_credential_issuer_config_test.go | 246 ++++++++++++++++++ internal/controller/issuerconfig/publisher.go | 7 +- .../controller/issuerconfig/publisher_test.go | 4 +- 5 files changed, 280 insertions(+), 24 deletions(-) create mode 100644 internal/controller/issuerconfig/create_or_update_credential_issuer_config_test.go diff --git a/internal/controller/apicerts/update_api_service.go b/internal/controller/apicerts/update_api_service.go index 19797007..faf6eb55 100644 --- a/internal/controller/apicerts/update_api_service.go +++ b/internal/controller/apicerts/update_api_service.go @@ -22,8 +22,7 @@ func UpdateAPIService(ctx context.Context, aggregatorClient aggregatorclient.Int apiServiceName := pinnipedv1alpha1.SchemeGroupVersion.Version + "." + pinnipedv1alpha1.GroupName if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - // Retrieve the latest version of the Service before attempting update. - // RetryOnConflict uses exponential backoff to avoid exhausting the API server. + // Retrieve the latest version of the Service. fetchedAPIService, err := apiServices.Get(ctx, apiServiceName, metav1.GetOptions{}) if err != nil { return fmt.Errorf("could not get existing version of API service: %w", err) diff --git a/internal/controller/issuerconfig/create_or_update_credential_issuer_config.go b/internal/controller/issuerconfig/create_or_update_credential_issuer_config.go index 738c965f..07138789 100644 --- a/internal/controller/issuerconfig/create_or_update_credential_issuer_config.go +++ b/internal/controller/issuerconfig/create_or_update_credential_issuer_config.go @@ -12,6 +12,7 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/util/retry" crdpinnipedv1alpha1 "github.com/suzerain-io/pinniped/generated/1.19/apis/crdpinniped/v1alpha1" pinnipedclientset "github.com/suzerain-io/pinniped/generated/1.19/client/clientset/versioned" @@ -23,26 +24,31 @@ func CreateOrUpdateCredentialIssuerConfig( pinnipedClient pinnipedclientset.Interface, applyUpdatesToCredentialIssuerConfigFunc func(configToUpdate *crdpinnipedv1alpha1.CredentialIssuerConfig), ) error { - credentialIssuerConfigName := configName + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + existingCredentialIssuerConfig, err := pinnipedClient. + CrdV1alpha1(). + CredentialIssuerConfigs(credentialIssuerConfigNamespace). + Get(ctx, configName, metav1.GetOptions{}) - existingCredentialIssuerConfig, err := pinnipedClient. - CrdV1alpha1(). - CredentialIssuerConfigs(credentialIssuerConfigNamespace). - Get(ctx, credentialIssuerConfigName, metav1.GetOptions{}) + notFound := k8serrors.IsNotFound(err) + if err != nil && !notFound { + return fmt.Errorf("get failed: %w", err) + } - notFound := k8serrors.IsNotFound(err) - if err != nil && !notFound { - return fmt.Errorf("could not get credentialissuerconfig: %w", err) + return createOrUpdateCredentialIssuerConfig( + ctx, + existingCredentialIssuerConfig, + notFound, + configName, + credentialIssuerConfigNamespace, + pinnipedClient, + applyUpdatesToCredentialIssuerConfigFunc) + }) + + if err != nil { + return fmt.Errorf("could not create or update credentialissuerconfig: %w", err) } - - return createOrUpdateCredentialIssuerConfig( - ctx, - existingCredentialIssuerConfig, - notFound, - credentialIssuerConfigName, - credentialIssuerConfigNamespace, - pinnipedClient, - applyUpdatesToCredentialIssuerConfigFunc) + return nil } func createOrUpdateCredentialIssuerConfig( @@ -62,7 +68,7 @@ func createOrUpdateCredentialIssuerConfig( applyUpdatesToCredentialIssuerConfigFunc(credentialIssuerConfig) if _, err := credentialIssuerConfigsClient.Create(ctx, credentialIssuerConfig, metav1.CreateOptions{}); err != nil { - return fmt.Errorf("could not create credentialissuerconfig: %w", err) + return fmt.Errorf("create failed: %w", err) } } else { // Already exists, so check to see if we need to update it @@ -75,7 +81,7 @@ func createOrUpdateCredentialIssuerConfig( } if _, err := credentialIssuerConfigsClient.Update(ctx, credentialIssuerConfig, metav1.UpdateOptions{}); err != nil { - return fmt.Errorf("could not update credentialissuerconfig: %w", err) + return err } } diff --git a/internal/controller/issuerconfig/create_or_update_credential_issuer_config_test.go b/internal/controller/issuerconfig/create_or_update_credential_issuer_config_test.go new file mode 100644 index 00000000..2a5e915b --- /dev/null +++ b/internal/controller/issuerconfig/create_or_update_credential_issuer_config_test.go @@ -0,0 +1,246 @@ +/* +Copyright 2020 VMware, Inc. +SPDX-License-Identifier: Apache-2.0 +*/ + +package issuerconfig + +import ( + "context" + "fmt" + "testing" + + "github.com/sclevine/spec" + "github.com/sclevine/spec/report" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + coretesting "k8s.io/client-go/testing" + apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" + + "github.com/suzerain-io/pinniped/generated/1.19/apis/crdpinniped/v1alpha1" + crdpinnipedv1alpha1 "github.com/suzerain-io/pinniped/generated/1.19/apis/crdpinniped/v1alpha1" + pinnipedfake "github.com/suzerain-io/pinniped/generated/1.19/client/clientset/versioned/fake" +) + +func TestCreateOrUpdateCredentialIssuerConfig(t *testing.T) { + spec.Run(t, "specs", func(t *testing.T, when spec.G, it spec.S) { + var r *require.Assertions + var ctx context.Context + var pinnipedAPIClient *pinnipedfake.Clientset + var credentialIssuerConfigGVR schema.GroupVersionResource + const installationNamespace = "some-namespace" + const configName = "pinniped-config" + + it.Before(func() { + r = require.New(t) + ctx = context.Background() + pinnipedAPIClient = pinnipedfake.NewSimpleClientset() + credentialIssuerConfigGVR = schema.GroupVersionResource{ + Group: crdpinnipedv1alpha1.GroupName, + Version: crdpinnipedv1alpha1.SchemeGroupVersion.Version, + Resource: "credentialissuerconfigs", + } + }) + + when("the config does not exist", func() { + it("creates a new config which includes only the updates made by the func parameter", func() { + err := CreateOrUpdateCredentialIssuerConfig(ctx, installationNamespace, pinnipedAPIClient, + func(configToUpdate *v1alpha1.CredentialIssuerConfig) { + configToUpdate.Status.KubeConfigInfo = &crdpinnipedv1alpha1.CredentialIssuerConfigKubeConfigInfo{ + CertificateAuthorityData: "some-ca-value", + } + }, + ) + r.NoError(err) + + expectedGetAction := coretesting.NewGetAction(credentialIssuerConfigGVR, installationNamespace, configName) + + expectedCreateAction := coretesting.NewCreateAction( + credentialIssuerConfigGVR, + installationNamespace, + &crdpinnipedv1alpha1.CredentialIssuerConfig{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: configName, + Namespace: installationNamespace, + }, + Status: crdpinnipedv1alpha1.CredentialIssuerConfigStatus{ + Strategies: []crdpinnipedv1alpha1.CredentialIssuerConfigStrategy{}, + KubeConfigInfo: &crdpinnipedv1alpha1.CredentialIssuerConfigKubeConfigInfo{ + Server: "", + CertificateAuthorityData: "some-ca-value", + }, + }, + }, + ) + + r.Equal([]coretesting.Action{expectedGetAction, expectedCreateAction}, pinnipedAPIClient.Actions()) + }) + + when("there is an unexpected error while creating the existing object", func() { + it.Before(func() { + pinnipedAPIClient.PrependReactor("create", "credentialissuerconfigs", func(_ coretesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("error on create") + }) + }) + + it("returns an error", func() { + err := CreateOrUpdateCredentialIssuerConfig(ctx, installationNamespace, pinnipedAPIClient, + func(configToUpdate *v1alpha1.CredentialIssuerConfig) {}, + ) + r.EqualError(err, "could not create or update credentialissuerconfig: create failed: error on create") + }) + }) + }) + + when("the config already exists", func() { + var existingConfig *crdpinnipedv1alpha1.CredentialIssuerConfig + + it.Before(func() { + existingConfig = &crdpinnipedv1alpha1.CredentialIssuerConfig{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: configName, + Namespace: installationNamespace, + }, + Status: crdpinnipedv1alpha1.CredentialIssuerConfigStatus{ + Strategies: []crdpinnipedv1alpha1.CredentialIssuerConfigStrategy{ + { + Type: crdpinnipedv1alpha1.KubeClusterSigningCertificateStrategyType, + Status: crdpinnipedv1alpha1.SuccessStrategyStatus, + Reason: crdpinnipedv1alpha1.FetchedKeyStrategyReason, + Message: "initial-message", + LastUpdateTime: metav1.Now(), + }, + }, + KubeConfigInfo: &crdpinnipedv1alpha1.CredentialIssuerConfigKubeConfigInfo{ + Server: "initial-server-value", + CertificateAuthorityData: "initial-ca-value", + }, + }, + } + r.NoError(pinnipedAPIClient.Tracker().Add(existingConfig)) + }) + + it("updates the existing config to only apply the updates made by the func parameter", func() { + err := CreateOrUpdateCredentialIssuerConfig(ctx, installationNamespace, pinnipedAPIClient, + func(configToUpdate *v1alpha1.CredentialIssuerConfig) { + configToUpdate.Status.KubeConfigInfo.CertificateAuthorityData = "new-ca-value" + }, + ) + r.NoError(err) + + expectedGetAction := coretesting.NewGetAction(credentialIssuerConfigGVR, installationNamespace, configName) + + // Only the edited field should be changed. + expectedUpdatedConfig := existingConfig.DeepCopy() + expectedUpdatedConfig.Status.KubeConfigInfo.CertificateAuthorityData = "new-ca-value" + expectedUpdateAction := coretesting.NewUpdateAction(credentialIssuerConfigGVR, installationNamespace, expectedUpdatedConfig) + + r.Equal([]coretesting.Action{expectedGetAction, expectedUpdateAction}, pinnipedAPIClient.Actions()) + }) + + it("avoids the cost of an update if the local updates made by the func parameter did not actually change anything", func() { + err := CreateOrUpdateCredentialIssuerConfig(ctx, installationNamespace, pinnipedAPIClient, + func(configToUpdate *v1alpha1.CredentialIssuerConfig) { + configToUpdate.Status.KubeConfigInfo.CertificateAuthorityData = "initial-ca-value" + }, + ) + r.NoError(err) + + expectedGetAction := coretesting.NewGetAction(credentialIssuerConfigGVR, installationNamespace, configName) + r.Equal([]coretesting.Action{expectedGetAction}, pinnipedAPIClient.Actions()) + }) + + when("there is an unexpected error while getting the existing object", func() { + it.Before(func() { + pinnipedAPIClient.PrependReactor("get", "credentialissuerconfigs", func(_ coretesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("error on get") + }) + }) + + it("returns an error", func() { + err := CreateOrUpdateCredentialIssuerConfig(ctx, installationNamespace, pinnipedAPIClient, + func(configToUpdate *v1alpha1.CredentialIssuerConfig) {}, + ) + r.EqualError(err, "could not create or update credentialissuerconfig: get failed: error on get") + }) + }) + + when("there is an unexpected error while updating the existing object", func() { + it.Before(func() { + pinnipedAPIClient.PrependReactor("update", "credentialissuerconfigs", func(_ coretesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("error on update") + }) + }) + + it("returns an error", func() { + err := CreateOrUpdateCredentialIssuerConfig(ctx, installationNamespace, pinnipedAPIClient, + func(configToUpdate *v1alpha1.CredentialIssuerConfig) { + configToUpdate.Status.KubeConfigInfo.CertificateAuthorityData = "new-ca-value" + }, + ) + r.EqualError(err, "could not create or update credentialissuerconfig: error on update") + }) + }) + + when("there is a conflict error while updating the existing object on the first try and the next try succeeds", func() { + var slightlyDifferentExistingConfig *crdpinnipedv1alpha1.CredentialIssuerConfig + + it.Before(func() { + hit := false + slightlyDifferentExistingConfig = existingConfig.DeepCopy() + slightlyDifferentExistingConfig.Status.KubeConfigInfo.Server = "some-other-server-value-from-conflicting-update" + + pinnipedAPIClient.PrependReactor("update", "credentialissuerconfigs", func(_ coretesting.Action) (bool, runtime.Object, error) { + // Return an error on the first call, then fall through to the default (successful) response. + if !hit { + // Before the update fails, also change the object that will be returned by the next Get(), + // to make sure that the production code does a fresh Get() after detecting a conflict. + r.NoError(pinnipedAPIClient.Tracker().Update(credentialIssuerConfigGVR, slightlyDifferentExistingConfig, installationNamespace)) + hit = true + return true, nil, apierrors.NewConflict(schema.GroupResource{ + Group: apiregistrationv1.GroupName, + Resource: "credentialissuerconfigs", + }, "alphav1.pinniped.dev", fmt.Errorf("there was a conflict")) + } + return false, nil, nil + }) + }) + + it("retries updates on conflict", func() { + err := CreateOrUpdateCredentialIssuerConfig(ctx, installationNamespace, pinnipedAPIClient, + func(configToUpdate *v1alpha1.CredentialIssuerConfig) { + configToUpdate.Status.KubeConfigInfo.CertificateAuthorityData = "new-ca-value" + }, + ) + r.NoError(err) + + expectedGetAction := coretesting.NewGetAction(credentialIssuerConfigGVR, installationNamespace, configName) + + // The first attempted update only includes its own edits. + firstExpectedUpdatedConfig := existingConfig.DeepCopy() + firstExpectedUpdatedConfig.Status.KubeConfigInfo.CertificateAuthorityData = "new-ca-value" + firstExpectedUpdateAction := coretesting.NewUpdateAction(credentialIssuerConfigGVR, installationNamespace, firstExpectedUpdatedConfig) + + // Both the edits made by this update and the edits made by the conflicting update should be included. + secondExpectedUpdatedConfig := existingConfig.DeepCopy() + secondExpectedUpdatedConfig.Status.KubeConfigInfo.Server = "some-other-server-value-from-conflicting-update" + secondExpectedUpdatedConfig.Status.KubeConfigInfo.CertificateAuthorityData = "new-ca-value" + secondExpectedUpdateAction := coretesting.NewUpdateAction(credentialIssuerConfigGVR, installationNamespace, secondExpectedUpdatedConfig) + + expectedActions := []coretesting.Action{ + expectedGetAction, + firstExpectedUpdateAction, + expectedGetAction, + secondExpectedUpdateAction, + } + r.Equal(expectedActions, pinnipedAPIClient.Actions()) + }) + }) + }) + }, spec.Parallel(), spec.Report(report.Terminal{})) +} diff --git a/internal/controller/issuerconfig/publisher.go b/internal/controller/issuerconfig/publisher.go index 7d1b862f..c1ba4742 100644 --- a/internal/controller/issuerconfig/publisher.go +++ b/internal/controller/issuerconfig/publisher.go @@ -126,6 +126,7 @@ func (c *publisherController) Sync(ctx controller.Context) error { CertificateAuthorityData: certificateAuthorityData, } } + err = createOrUpdateCredentialIssuerConfig( ctx.Context, existingCredentialIssuerConfigFromInformerCache, @@ -134,5 +135,9 @@ func (c *publisherController) Sync(ctx controller.Context) error { c.namespace, c.pinnipedClient, updateServerAndCAFunc) - return err + + if err != nil { + return fmt.Errorf("could not create or update credentialissuerconfig: %w", err) + } + return nil } diff --git a/internal/controller/issuerconfig/publisher_test.go b/internal/controller/issuerconfig/publisher_test.go index 79fd87cd..4fe5e68b 100644 --- a/internal/controller/issuerconfig/publisher_test.go +++ b/internal/controller/issuerconfig/publisher_test.go @@ -309,7 +309,7 @@ func TestSync(t *testing.T) { it("returns the create error", func() { startInformersAndController() err := controller.TestSync(t, subject, *syncContext) - r.EqualError(err, "could not create credentialissuerconfig: create failed") + r.EqualError(err, "could not create or update credentialissuerconfig: create failed: create failed") }) }) @@ -410,7 +410,7 @@ func TestSync(t *testing.T) { it("returns the update error", func() { startInformersAndController() err := controller.TestSync(t, subject, *syncContext) - r.EqualError(err, "could not update credentialissuerconfig: update failed") + r.EqualError(err, "could not create or update credentialissuerconfig: update failed") }) }) })