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.

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.
+		// Retrieve the latest version of the Service.
-		// RetryOnConflict uses exponential backoff to avoid exhausting the API server.
 		fetchedAPIService, err := apiServices.Get(ctx, apiServiceName, metav1.GetOptions{})
 		if err != nil {
 			return fmt.Errorf("could not get existing version of API service: %w", err)

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.
+		notFound := k8serrors.IsNotFound(err)
-		CrdV1alpha1().
+		if err != nil && !notFound {
-		CredentialIssuerConfigs(credentialIssuerConfigNamespace).
+			return fmt.Errorf("get failed: %w", err)
-		Get(ctx, credentialIssuerConfigName, metav1.GetOptions{})
+		}
 
-	notFound := k8serrors.IsNotFound(err)
+		return createOrUpdateCredentialIssuerConfig(
-	if err != nil && !notFound {
+			ctx,
-		return fmt.Errorf("could not get credentialissuerconfig: %w", err)
+			existingCredentialIssuerConfig,
+			notFound,
+			configName,
+			credentialIssuerConfigNamespace,
+			pinnipedClient,
+			applyUpdatesToCredentialIssuerConfigFunc)
+	})
+
+	if err != nil {
+		return fmt.Errorf("could not create or update credentialissuerconfig: %w", err)
 	}
-
+	return nil
-	return createOrUpdateCredentialIssuerConfig(
-		ctx,
-		existingCredentialIssuerConfig,
-		notFound,
-		credentialIssuerConfigName,
-		credentialIssuerConfigNamespace,
-		pinnipedClient,
-		applyUpdatesToCredentialIssuerConfigFunc)
 }
 
 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
 		}
 	}
 

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

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
 }

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