Prefactor: Move updating of APIService to a separate controller

- The certs manager controller, along with its sibling certs expirer
  and certs observer controllers, are generally useful for any process
  that wants to create its own CA and TLS certs, but only if the
  updating of the APIService is not included in those controllers
- So that functionality for updating APIServices is moved to a new
  controller which watches the same Secret which is used by those
  other controllers
- Also parameterize `NewCertsManagerController` with the service name
  and the CA common name to make the controller more reusable

internal/controller/apicerts/apiservice_updater.go

@@ -0,0 +1,69 @@
+/*
+Copyright 2020 VMware, Inc.
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package apicerts
+
+import (
+	"fmt"
+
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+	corev1informers "k8s.io/client-go/informers/core/v1"
+	"k8s.io/klog/v2"
+	aggregatorclient "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset"
+
+	pinnipedcontroller "github.com/suzerain-io/pinniped/internal/controller"
+	"github.com/suzerain-io/pinniped/internal/controllerlib"
+)
+
+type apiServiceUpdaterController struct {
+	namespace        string
+	aggregatorClient aggregatorclient.Interface
+	secretInformer   corev1informers.SecretInformer
+}
+
+func NewAPIServiceUpdaterController(
+	namespace string,
+	aggregatorClient aggregatorclient.Interface,
+	secretInformer corev1informers.SecretInformer,
+	withInformer pinnipedcontroller.WithInformerOptionFunc,
+) controllerlib.Controller {
+	return controllerlib.New(
+		controllerlib.Config{
+			Name: "certs-manager-controller",
+			Syncer: &apiServiceUpdaterController{
+				namespace:        namespace,
+				aggregatorClient: aggregatorClient,
+				secretInformer:   secretInformer,
+			},
+		},
+		withInformer(
+			secretInformer,
+			pinnipedcontroller.NameAndNamespaceExactMatchFilterFactory(certsSecretName, namespace),
+			controllerlib.InformerOption{},
+		),
+	)
+}
+
+func (c *apiServiceUpdaterController) Sync(ctx controllerlib.Context) error {
+	// Try to get the secret from the informer cache.
+	certSecret, err := c.secretInformer.Lister().Secrets(c.namespace).Get(certsSecretName)
+	notFound := k8serrors.IsNotFound(err)
+	if err != nil && !notFound {
+		return fmt.Errorf("failed to get %s/%s secret: %w", c.namespace, certsSecretName, err)
+	}
+	if notFound {
+		// The secret does not exist yet, so nothing to do.
+		klog.Info("apiServiceUpdaterController Sync found that the secret does not exist yet or was deleted")
+		return nil
+	}
+
+	// Update the APIService to give it the new CA bundle.
+	if err := UpdateAPIService(ctx.Context, c.aggregatorClient, certSecret.Data[caCertificateSecretKey]); err != nil {
+		return fmt.Errorf("could not update the API service: %w", err)
+	}
+
+	klog.Info("apiServiceUpdaterController Sync successfully updated API service")
+	return nil
+}

internal/controller/apicerts/apiservice_updater_test.go

@@ -0,0 +1,277 @@
+/*
+Copyright 2020 VMware, Inc.
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package apicerts
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+
+	"github.com/sclevine/spec"
+	"github.com/sclevine/spec/report"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	kubeinformers "k8s.io/client-go/informers"
+	kubernetesfake "k8s.io/client-go/kubernetes/fake"
+	coretesting "k8s.io/client-go/testing"
+	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
+	aggregatorfake "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake"
+
+	pinnipedv1alpha1 "github.com/suzerain-io/pinniped/generated/1.19/apis/pinniped/v1alpha1"
+	"github.com/suzerain-io/pinniped/internal/controllerlib"
+	"github.com/suzerain-io/pinniped/internal/testutil"
+)
+
+func TestAPIServiceUpdaterControllerOptions(t *testing.T) {
+	spec.Run(t, "options", func(t *testing.T, when spec.G, it spec.S) {
+		const installedInNamespace = "some-namespace"
+
+		var r *require.Assertions
+		var observableWithInformerOption *testutil.ObservableWithInformerOption
+		var secretsInformerFilter controllerlib.Filter
+
+		it.Before(func() {
+			r = require.New(t)
+			observableWithInformerOption = testutil.NewObservableWithInformerOption()
+			secretsInformer := kubeinformers.NewSharedInformerFactory(nil, 0).Core().V1().Secrets()
+			_ = NewAPIServiceUpdaterController(
+				installedInNamespace,
+				nil,
+				secretsInformer,
+				observableWithInformerOption.WithInformer, // make it possible to observe the behavior of the Filters
+			)
+			secretsInformerFilter = observableWithInformerOption.GetFilterForInformer(secretsInformer)
+		})
+
+		when("watching Secret objects", func() {
+			var subject controllerlib.Filter
+			var target, wrongNamespace, wrongName, unrelated *corev1.Secret
+
+			it.Before(func() {
+				subject = secretsInformerFilter
+				target = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "api-serving-cert", Namespace: installedInNamespace}}
+				wrongNamespace = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "api-serving-cert", Namespace: "wrong-namespace"}}
+				wrongName = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "wrong-name", Namespace: installedInNamespace}}
+				unrelated = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "wrong-name", Namespace: "wrong-namespace"}}
+			})
+
+			when("the target Secret changes", func() {
+				it("returns true to trigger the sync method", func() {
+					r.True(subject.Add(target))
+					r.True(subject.Update(target, unrelated))
+					r.True(subject.Update(unrelated, target))
+					r.True(subject.Delete(target))
+				})
+			})
+
+			when("a Secret from another namespace changes", func() {
+				it("returns false to avoid triggering the sync method", func() {
+					r.False(subject.Add(wrongNamespace))
+					r.False(subject.Update(wrongNamespace, unrelated))
+					r.False(subject.Update(unrelated, wrongNamespace))
+					r.False(subject.Delete(wrongNamespace))
+				})
+			})
+
+			when("a Secret with a different name changes", func() {
+				it("returns false to avoid triggering the sync method", func() {
+					r.False(subject.Add(wrongName))
+					r.False(subject.Update(wrongName, unrelated))
+					r.False(subject.Update(unrelated, wrongName))
+					r.False(subject.Delete(wrongName))
+				})
+			})
+
+			when("a Secret with a different name and a different namespace changes", func() {
+				it("returns false to avoid triggering the sync method", func() {
+					r.False(subject.Add(unrelated))
+					r.False(subject.Update(unrelated, unrelated))
+					r.False(subject.Delete(unrelated))
+				})
+			})
+		})
+	}, spec.Parallel(), spec.Report(report.Terminal{}))
+}
+
+func TestAPIServiceUpdaterControllerSync(t *testing.T) {
+	spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) {
+		const installedInNamespace = "some-namespace"
+
+		var r *require.Assertions
+
+		var subject controllerlib.Controller
+		var aggregatorAPIClient *aggregatorfake.Clientset
+		var kubeInformerClient *kubernetesfake.Clientset
+		var kubeInformers kubeinformers.SharedInformerFactory
+		var timeoutContext context.Context
+		var timeoutContextCancel context.CancelFunc
+		var syncContext *controllerlib.Context
+
+		// Defer starting the informers until the last possible moment so that the
+		// nested Before's can keep adding things to the informer caches.
+		var startInformersAndController = func() {
+			// Set this at the last second to allow for injection of server override.
+			subject = NewAPIServiceUpdaterController(
+				installedInNamespace,
+				aggregatorAPIClient,
+				kubeInformers.Core().V1().Secrets(),
+				controllerlib.WithInformer,
+			)
+
+			// Set this at the last second to support calling subject.Name().
+			syncContext = &controllerlib.Context{
+				Context: timeoutContext,
+				Name:    subject.Name(),
+				Key: controllerlib.Key{
+					Namespace: installedInNamespace,
+					Name:      "api-serving-cert",
+				},
+			}
+
+			// Must start informers before calling TestRunSynchronously()
+			kubeInformers.Start(timeoutContext.Done())
+			controllerlib.TestRunSynchronously(t, subject)
+		}
+
+		it.Before(func() {
+			r = require.New(t)
+
+			timeoutContext, timeoutContextCancel = context.WithTimeout(context.Background(), time.Second*3)
+
+			kubeInformerClient = kubernetesfake.NewSimpleClientset()
+			kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0)
+			aggregatorAPIClient = aggregatorfake.NewSimpleClientset()
+		})
+
+		it.After(func() {
+			timeoutContextCancel()
+		})
+
+		when("there is not yet an api-serving-cert Secret in the installation namespace or it was deleted", func() {
+			it.Before(func() {
+				unrelatedSecret := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "some other secret",
+						Namespace: installedInNamespace,
+					},
+				}
+				err := kubeInformerClient.Tracker().Add(unrelatedSecret)
+				r.NoError(err)
+			})
+
+			it("does not need to make any API calls with its API client", func() {
+				startInformersAndController()
+				err := controllerlib.TestSync(t, subject, *syncContext)
+				r.NoError(err)
+				r.Empty(aggregatorAPIClient.Actions())
+			})
+		})
+
+		when("there is an api-serving-cert Secret already in the installation namespace", func() {
+			it.Before(func() {
+				apiServingCertSecret := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "api-serving-cert",
+						Namespace: installedInNamespace,
+					},
+					Data: map[string][]byte{
+						"caCertificate":       []byte("fake CA cert"),
+						"tlsPrivateKey":       []byte("fake private key"),
+						"tlsCertificateChain": []byte("fake cert chain"),
+					},
+				}
+				err := kubeInformerClient.Tracker().Add(apiServingCertSecret)
+				r.NoError(err)
+			})
+
+			when("the APIService exists", func() {
+				it.Before(func() {
+					apiService := &apiregistrationv1.APIService{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: pinnipedv1alpha1.SchemeGroupVersion.Version + "." + pinnipedv1alpha1.GroupName,
+						},
+						Spec: apiregistrationv1.APIServiceSpec{
+							CABundle:        nil,
+							VersionPriority: 1234,
+						},
+					}
+					err := aggregatorAPIClient.Tracker().Add(apiService)
+					r.NoError(err)
+				})
+
+				it("updates the APIService's ca bundle", func() {
+					startInformersAndController()
+					err := controllerlib.TestSync(t, subject, *syncContext)
+					r.NoError(err)
+
+					// Make sure we updated the APIService caBundle and left it otherwise unchanged
+					r.Len(aggregatorAPIClient.Actions(), 2)
+					r.Equal("get", aggregatorAPIClient.Actions()[0].GetVerb())
+					expectedAPIServiceName := pinnipedv1alpha1.SchemeGroupVersion.Version + "." + pinnipedv1alpha1.GroupName
+					expectedUpdateAction := coretesting.NewUpdateAction(
+						schema.GroupVersionResource{
+							Group:    apiregistrationv1.GroupName,
+							Version:  "v1",
+							Resource: "apiservices",
+						},
+						"",
+						&apiregistrationv1.APIService{
+							ObjectMeta: metav1.ObjectMeta{
+								Name:      expectedAPIServiceName,
+								Namespace: "",
+							},
+							Spec: apiregistrationv1.APIServiceSpec{
+								VersionPriority: 1234, // only the CABundle is updated, this other field is left unchanged
+								CABundle:        []byte("fake CA cert"),
+							},
+						},
+					)
+					r.Equal(expectedUpdateAction, aggregatorAPIClient.Actions()[1])
+				})
+
+				when("updating the APIService fails", func() {
+					it.Before(func() {
+						aggregatorAPIClient.PrependReactor(
+							"update",
+							"apiservices",
+							func(_ coretesting.Action) (bool, runtime.Object, error) {
+								return true, nil, errors.New("update failed")
+							},
+						)
+					})
+
+					it("returns the update error", func() {
+						startInformersAndController()
+						err := controllerlib.TestSync(t, subject, *syncContext)
+						r.EqualError(err, "could not update the API service: could not update API service: update failed")
+					})
+				})
+			})
+
+			when("the APIService does not exist", func() {
+				it.Before(func() {
+					unrelatedAPIService := &apiregistrationv1.APIService{
+						ObjectMeta: metav1.ObjectMeta{Name: "some other api service"},
+						Spec:       apiregistrationv1.APIServiceSpec{},
+					}
+					err := aggregatorAPIClient.Tracker().Add(unrelatedAPIService)
+					r.NoError(err)
+				})
+
+				it("returns an error", func() {
+					startInformersAndController()
+					err := controllerlib.TestSync(t, subject, *syncContext)
+					r.Error(err)
+					r.Regexp("could not get existing version of API service: .* not found", err.Error())
+				})
+			})
+		})
+	}, spec.Parallel(), spec.Report(report.Terminal{}))
+}

internal/controller/apicerts/certs_expirer.go

@@ -69,7 +69,7 @@ func (c *certsExpirerController) Sync(ctx controllerlib.Context) error {
 		return fmt.Errorf("failed to get %s/%s secret: %w", c.namespace, certsSecretName, err)
 	}
 	if notFound {
-		klog.Info("certsExpirerController Sync() found that the secret does not exist yet or was deleted")
+		klog.Info("certsExpirerController Sync found that the secret does not exist yet or was deleted")
 		return nil
 	}
 
@@ -78,13 +78,13 @@ func (c *certsExpirerController) Sync(ctx controllerlib.Context) error {
 		// If we can't read the cert, then really all we can do is log something,
 		// since if we returned an error then the controller lib would just call us
 		// again and again, which would probably yield the same results.
-		klog.Warningf("certsExpirerController Sync() found that the secret is malformed: %s", err.Error())
+		klog.Warningf("certsExpirerController Sync found that the secret is malformed: %s", err.Error())
 		return nil
 	}
 
 	certAge := time.Since(notBefore)
 	renewDelta := certAge - c.renewBefore
-	klog.Infof("certsExpirerController Sync() found a renew delta of %s", renewDelta)
+	klog.Infof("certsExpirerController Sync found a renew delta of %s", renewDelta)
 	if renewDelta >= 0 || time.Now().After(notAfter) {
 		err := c.k8sClient.
 			CoreV1().

internal/controller/apicerts/certs_manager.go

@@ -16,7 +16,6 @@ import (
 	corev1informers "k8s.io/client-go/informers/core/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/klog/v2"
-	aggregatorclient "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset"
 
 	"github.com/suzerain-io/pinniped/internal/certauthority"
 	pinnipedcontroller "github.com/suzerain-io/pinniped/internal/controller"
@@ -32,34 +31,37 @@ const (
 )
 
 type certsManagerController struct {
-	namespace        string
-	k8sClient        kubernetes.Interface
-	aggregatorClient aggregatorclient.Interface
-	secretInformer   corev1informers.SecretInformer
+	namespace      string
+	k8sClient      kubernetes.Interface
+	secretInformer corev1informers.SecretInformer
 
 	// certDuration is the lifetime of both the serving certificate and its CA
 	// certificate that this controller will use when issuing the certificates.
 	certDuration time.Duration
+
+	generatedCACommonName                 string
+	serviceNameForGeneratedCertCommonName string
 }
 
-func NewCertsManagerController(
-	namespace string,
+func NewCertsManagerController(namespace string,
 	k8sClient kubernetes.Interface,
-	aggregatorClient aggregatorclient.Interface,
 	secretInformer corev1informers.SecretInformer,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
 	withInitialEvent pinnipedcontroller.WithInitialEventOptionFunc,
 	certDuration time.Duration,
+	generatedCACommonName string,
+	serviceNameForGeneratedCertCommonName string,
 ) controllerlib.Controller {
 	return controllerlib.New(
 		controllerlib.Config{
 			Name: "certs-manager-controller",
 			Syncer: &certsManagerController{
-				namespace:        namespace,
-				k8sClient:        k8sClient,
-				aggregatorClient: aggregatorClient,
-				secretInformer:   secretInformer,
-				certDuration:     certDuration,
+				namespace:                             namespace,
+				k8sClient:                             k8sClient,
+				secretInformer:                        secretInformer,
+				certDuration:                          certDuration,
+				generatedCACommonName:                 generatedCACommonName,
+				serviceNameForGeneratedCertCommonName: serviceNameForGeneratedCertCommonName,
 			},
 		},
 		withInformer(
@@ -88,16 +90,13 @@ func (c *certsManagerController) Sync(ctx controllerlib.Context) error {
 	}
 
 	// Create a CA.
-	aggregatedAPIServerCA, err := certauthority.New(pkix.Name{CommonName: "Pinniped CA"}, c.certDuration)
+	aggregatedAPIServerCA, err := certauthority.New(pkix.Name{CommonName: c.generatedCACommonName}, c.certDuration)
 	if err != nil {
 		return fmt.Errorf("could not initialize CA: %w", err)
 	}
 
-	// This string must match the name of the Service declared in the deployment yaml.
-	const serviceName = "pinniped-api"
-
 	// Using the CA from above, create a TLS server cert for the aggregated API server to use.
-	serviceEndpoint := serviceName + "." + c.namespace + ".svc"
+	serviceEndpoint := c.serviceNameForGeneratedCertCommonName + "." + c.namespace + ".svc"
 	aggregatedAPIServerTLSCert, err := aggregatedAPIServerCA.Issue(
 		pkix.Name{CommonName: serviceEndpoint},
 		[]string{serviceEndpoint},
@@ -129,11 +128,6 @@ func (c *certsManagerController) Sync(ctx controllerlib.Context) error {
 		return fmt.Errorf("could not create secret: %w", err)
 	}
 
-	// Update the APIService to give it the new CA bundle.
-	if err := UpdateAPIService(ctx.Context, c.aggregatorClient, aggregatedAPIServerCA.Bundle()); err != nil {
-		return fmt.Errorf("could not update the API service: %w", err)
-	}
-
-	klog.Info("certsManagerController Sync successfully created secret and updated API service")
+	klog.Info("certsManagerController Sync successfully created secret")
 	return nil
 }

internal/controller/apicerts/certs_manager_test.go

@@ -21,10 +21,7 @@ import (
 	kubeinformers "k8s.io/client-go/informers"
 	kubernetesfake "k8s.io/client-go/kubernetes/fake"
 	coretesting "k8s.io/client-go/testing"
-	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
-	aggregatorfake "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake"
 
-	pinnipedv1alpha1 "github.com/suzerain-io/pinniped/generated/1.19/apis/pinniped/v1alpha1"
 	"github.com/suzerain-io/pinniped/internal/controllerlib"
 	"github.com/suzerain-io/pinniped/internal/testutil"
 )
@@ -43,15 +40,7 @@ func TestManagerControllerOptions(t *testing.T) {
 			observableWithInformerOption = testutil.NewObservableWithInformerOption()
 			observableWithInitialEventOption = testutil.NewObservableWithInitialEventOption()
 			secretsInformer := kubeinformers.NewSharedInformerFactory(nil, 0).Core().V1().Secrets()
-			_ = NewCertsManagerController(
-				installedInNamespace,
-				nil,
-				nil,
-				secretsInformer,
-				observableWithInformerOption.WithInformer,         // make it possible to observe the behavior of the Filters
-				observableWithInitialEventOption.WithInitialEvent, // make it possible to observe the behavior of the initial event
-				0, // certDuration, not needed for this test
-			)
+			_ = NewCertsManagerController(installedInNamespace, nil, secretsInformer, observableWithInformerOption.WithInformer, observableWithInitialEventOption.WithInitialEvent, 0, "Pinniped CA", "pinniped-api")
 			secretsInformerFilter = observableWithInformerOption.GetFilterForInformer(secretsInformer)
 		})
 
@@ -123,7 +112,6 @@ func TestManagerControllerSync(t *testing.T) {
 
 		var subject controllerlib.Controller
 		var kubeAPIClient *kubernetesfake.Clientset
-		var aggregatorAPIClient *aggregatorfake.Clientset
 		var kubeInformerClient *kubernetesfake.Clientset
 		var kubeInformers kubeinformers.SharedInformerFactory
 		var timeoutContext context.Context
@@ -137,11 +125,12 @@ func TestManagerControllerSync(t *testing.T) {
 			subject = NewCertsManagerController(
 				installedInNamespace,
 				kubeAPIClient,
-				aggregatorAPIClient,
 				kubeInformers.Core().V1().Secrets(),
 				controllerlib.WithInformer,
 				controllerlib.WithInitialEvent,
 				certDuration,
+				"Pinniped CA",
+				"pinniped-api",
 			)
 
 			// Set this at the last second to support calling subject.Name().
@@ -167,7 +156,6 @@ func TestManagerControllerSync(t *testing.T) {
 			kubeInformerClient = kubernetesfake.NewSimpleClientset()
 			kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0)
 			kubeAPIClient = kubernetesfake.NewSimpleClientset()
-			aggregatorAPIClient = aggregatorfake.NewSimpleClientset()
 		})
 
 		it.After(func() {
@@ -186,111 +174,35 @@ func TestManagerControllerSync(t *testing.T) {
 				r.NoError(err)
 			})
 
-			when("the APIService exists", func() {
-				it.Before(func() {
-					apiService := &apiregistrationv1.APIService{
-						ObjectMeta: metav1.ObjectMeta{
-							Name: pinnipedv1alpha1.SchemeGroupVersion.Version + "." + pinnipedv1alpha1.GroupName,
-						},
-						Spec: apiregistrationv1.APIServiceSpec{
-							CABundle:        nil,
-							VersionPriority: 1234,
-						},
-					}
-					err := aggregatorAPIClient.Tracker().Add(apiService)
-					r.NoError(err)
-				})
+			it("creates the api-serving-cert Secret", func() {
+				startInformersAndController()
+				err := controllerlib.TestSync(t, subject, *syncContext)
+				r.NoError(err)
 
-				it("creates the api-serving-cert Secret and updates the APIService's ca bundle", func() {
-					startInformersAndController()
-					err := controllerlib.TestSync(t, subject, *syncContext)
-					r.NoError(err)
+				// Check all the relevant fields from the create Secret action
+				r.Len(kubeAPIClient.Actions(), 1)
+				actualAction := kubeAPIClient.Actions()[0].(coretesting.CreateActionImpl)
+				r.Equal(schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}, actualAction.GetResource())
+				r.Equal(installedInNamespace, actualAction.GetNamespace())
+				actualSecret := actualAction.GetObject().(*corev1.Secret)
+				r.Equal("api-serving-cert", actualSecret.Name)
+				r.Equal(installedInNamespace, actualSecret.Namespace)
+				actualCACert := actualSecret.StringData["caCertificate"]
+				actualPrivateKey := actualSecret.StringData["tlsPrivateKey"]
+				actualCertChain := actualSecret.StringData["tlsCertificateChain"]
+				r.NotEmpty(actualCACert)
+				r.NotEmpty(actualPrivateKey)
+				r.NotEmpty(actualCertChain)
 
-					// Check all the relevant fields from the create Secret action
-					r.Len(kubeAPIClient.Actions(), 1)
-					actualAction := kubeAPIClient.Actions()[0].(coretesting.CreateActionImpl)
-					r.Equal(schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}, actualAction.GetResource())
-					r.Equal(installedInNamespace, actualAction.GetNamespace())
-					actualSecret := actualAction.GetObject().(*corev1.Secret)
-					r.Equal("api-serving-cert", actualSecret.Name)
-					r.Equal(installedInNamespace, actualSecret.Namespace)
-					actualCACert := actualSecret.StringData["caCertificate"]
-					actualPrivateKey := actualSecret.StringData["tlsPrivateKey"]
-					actualCertChain := actualSecret.StringData["tlsCertificateChain"]
-					r.NotEmpty(actualCACert)
-					r.NotEmpty(actualPrivateKey)
-					r.NotEmpty(actualCertChain)
+				// Validate the created CA's lifetime.
+				validCACert := testutil.ValidateCertificate(t, actualCACert, actualCACert)
+				validCACert.RequireLifetime(time.Now(), time.Now().Add(certDuration), 6*time.Minute)
 
-					// Validate the created CA's lifetime.
-					validCACert := testutil.ValidateCertificate(t, actualCACert, actualCACert)
-					validCACert.RequireLifetime(time.Now(), time.Now().Add(certDuration), 6*time.Minute)
-
-					// Validate the created cert using the CA, and also validate the cert's hostname
-					validCert := testutil.ValidateCertificate(t, actualCACert, actualCertChain)
-					validCert.RequireDNSName("pinniped-api." + installedInNamespace + ".svc")
-					validCert.RequireLifetime(time.Now(), time.Now().Add(certDuration), 6*time.Minute)
-					validCert.RequireMatchesPrivateKey(actualPrivateKey)
-
-					// Make sure we updated the APIService caBundle and left it otherwise unchanged
-					r.Len(aggregatorAPIClient.Actions(), 2)
-					r.Equal("get", aggregatorAPIClient.Actions()[0].GetVerb())
-					expectedAPIServiceName := pinnipedv1alpha1.SchemeGroupVersion.Version + "." + pinnipedv1alpha1.GroupName
-					expectedUpdateAction := coretesting.NewUpdateAction(
-						schema.GroupVersionResource{
-							Group:    apiregistrationv1.GroupName,
-							Version:  "v1",
-							Resource: "apiservices",
-						},
-						"",
-						&apiregistrationv1.APIService{
-							ObjectMeta: metav1.ObjectMeta{
-								Name:      expectedAPIServiceName,
-								Namespace: "",
-							},
-							Spec: apiregistrationv1.APIServiceSpec{
-								VersionPriority: 1234, // only the CABundle is updated, this other field is left unchanged
-								CABundle:        []byte(actualCACert),
-							},
-						},
-					)
-					r.Equal(expectedUpdateAction, aggregatorAPIClient.Actions()[1])
-				})
-
-				when("updating the APIService fails", func() {
-					it.Before(func() {
-						aggregatorAPIClient.PrependReactor(
-							"update",
-							"apiservices",
-							func(_ coretesting.Action) (bool, runtime.Object, error) {
-								return true, nil, errors.New("update failed")
-							},
-						)
-					})
-
-					it("returns the update error", func() {
-						startInformersAndController()
-						err := controllerlib.TestSync(t, subject, *syncContext)
-						r.EqualError(err, "could not update the API service: could not update API service: update failed")
-					})
-				})
-			})
-
-			when("the APIService does not exist", func() {
-				it.Before(func() {
-					unrelatedAPIService := &apiregistrationv1.APIService{
-						ObjectMeta: metav1.ObjectMeta{Name: "some other api service"},
-						Spec:       apiregistrationv1.APIServiceSpec{},
-					}
-					err := aggregatorAPIClient.Tracker().Add(unrelatedAPIService)
-					r.NoError(err)
-				})
-
-				it("returns an error", func() {
-					startInformersAndController()
-					err := controllerlib.TestSync(t, subject, *syncContext)
-					r.Error(err)
-					r.Regexp("could not get existing version of API service: .* not found", err.Error())
-				})
+				// Validate the created cert using the CA, and also validate the cert's hostname
+				validCert := testutil.ValidateCertificate(t, actualCACert, actualCertChain)
+				validCert.RequireDNSName("pinniped-api." + installedInNamespace + ".svc")
+				validCert.RequireLifetime(time.Now(), time.Now().Add(certDuration), 6*time.Minute)
+				validCert.RequireMatchesPrivateKey(actualPrivateKey)
 			})
 
 			when("creating the Secret fails", func() {
@@ -304,11 +216,10 @@ func TestManagerControllerSync(t *testing.T) {
 					)
 				})
 
-				it("returns the create error and does not update the APIService", func() {
+				it("returns the create error", func() {
 					startInformersAndController()
 					err := controllerlib.TestSync(t, subject, *syncContext)
 					r.EqualError(err, "could not create secret: create failed")
-					r.Empty(aggregatorAPIClient.Actions())
 				})
 			})
 		})
@@ -325,12 +236,11 @@ func TestManagerControllerSync(t *testing.T) {
 				r.NoError(err)
 			})
 
-			it("does not need to make any API calls with its API clients", func() {
+			it("does not need to make any API calls with its API client", func() {
 				startInformersAndController()
 				err := controllerlib.TestSync(t, subject, *syncContext)
 				r.NoError(err)
 				r.Empty(kubeAPIClient.Actions())
-				r.Empty(aggregatorAPIClient.Actions())
 			})
 		})
 	}, spec.Parallel(), spec.Report(report.Terminal{}))

internal/controller/apicerts/certs_observer.go

@@ -54,7 +54,7 @@ func (c *certsObserverController) Sync(_ controllerlib.Context) error {
 		return fmt.Errorf("failed to get %s/%s secret: %w", c.namespace, certsSecretName, err)
 	}
 	if notFound {
-		klog.Info("certsObserverController Sync() found that the secret does not exist yet or was deleted")
+		klog.Info("certsObserverController Sync found that the secret does not exist yet or was deleted")
 		// The secret does not exist yet or was deleted.
 		c.dynamicCertProvider.Set(nil, nil)
 		return nil

internal/controller/apicerts/update_api_service.go

@@ -6,6 +6,7 @@ SPDX-License-Identifier: Apache-2.0
 package apicerts
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 
@@ -28,6 +29,11 @@ func UpdateAPIService(ctx context.Context, aggregatorClient aggregatorclient.Int
 			return fmt.Errorf("could not get existing version of API service: %w", err)
 		}
 
+		if bytes.Equal(fetchedAPIService.Spec.CABundle, aggregatedAPIServerCA) {
+			// Already has the same value, perhaps because another process already updated the object, so no need to update.
+			return nil
+		}
+
 		// Update just the field we care about.
 		fetchedAPIService.Spec.CABundle = aggregatedAPIServerCA
 

internal/controller/apicerts/update_api_service_test.go

@@ -70,17 +70,44 @@ func TestUpdateAPIService(t *testing.T) {
 				},
 			}},
 		},
+		{
+			name: "happy path update when the pre-existing APIService already has the same CA bundle so there is no need to update",
+			mocks: func(c *aggregatorv1fake.Clientset) {
+				_ = c.Tracker().Add(&apiregistrationv1.APIService{
+					ObjectMeta: metav1.ObjectMeta{Name: apiServiceName},
+					Spec: apiregistrationv1.APIServiceSpec{
+						GroupPriorityMinimum: 999,
+						CABundle:             []byte("some-ca-bundle"),
+					},
+				})
+				c.PrependReactor("update", "apiservices", func(_ kubetesting.Action) (bool, runtime.Object, error) {
+					return true, nil, fmt.Errorf("should not encounter this error because update should be skipped in this case")
+				})
+			},
+			caInput: []byte("some-ca-bundle"),
+			wantObjects: []apiregistrationv1.APIService{{
+				ObjectMeta: metav1.ObjectMeta{Name: apiServiceName},
+				Spec: apiregistrationv1.APIServiceSpec{
+					GroupPriorityMinimum: 999,
+					CABundle:             []byte("some-ca-bundle"), // unchanged
+				},
+			}},
+		},
 		{
 			name: "error on update",
 			mocks: func(c *aggregatorv1fake.Clientset) {
 				_ = c.Tracker().Add(&apiregistrationv1.APIService{
 					ObjectMeta: metav1.ObjectMeta{Name: apiServiceName},
-					Spec:       apiregistrationv1.APIServiceSpec{},
+					Spec: apiregistrationv1.APIServiceSpec{
+						GroupPriorityMinimum: 999,
+						CABundle:             []byte("some-other-different-ca-bundle"),
+					},
 				})
 				c.PrependReactor("update", "apiservices", func(_ kubetesting.Action) (bool, runtime.Object, error) {
 					return true, nil, fmt.Errorf("error on update")
 				})
 			},
+			caInput: []byte("some-ca-bundle"),
 			wantErr: "could not update API service: error on update",
 		},
 		{
@@ -143,6 +170,7 @@ func TestUpdateAPIService(t *testing.T) {
 			}},
 		},
 	}
+
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {

internal/controllermanager/prepare_controllers.go

@@ -47,6 +47,9 @@ func PrepareControllers(
 	kubePublicNamespaceK8sInformers, installationNamespaceK8sInformers, installationNamespacePinnipedInformers :=
 		createInformers(serverInstallationNamespace, k8sClient, pinnipedClient)
 
+	// This string must match the name of the Service declared in the deployment yaml.
+	const serviceName = "pinniped-api"
+
 	// Create controller manager.
 	controllerManager := controllerlib.
 		NewManager().
@@ -65,11 +68,21 @@ func PrepareControllers(
 			apicerts.NewCertsManagerController(
 				serverInstallationNamespace,
 				k8sClient,
-				aggregatorClient,
 				installationNamespaceK8sInformers.Core().V1().Secrets(),
 				controllerlib.WithInformer,
 				controllerlib.WithInitialEvent,
 				servingCertDuration,
+				"Pinniped CA",
+				serviceName,
+			),
+			singletonWorker,
+		).
+		WithController(
+			apicerts.NewAPIServiceUpdaterController(
+				serverInstallationNamespace,
+				aggregatorClient,
+				installationNamespaceK8sInformers.Core().V1().Secrets(),
+				controllerlib.WithInformer,
 			),
 			singletonWorker,
 		).