Add integration and more unit tests

- Add integration test for serving cert auto-generation and rotation
- Add unit test for `WithInitialEvent` of the cert manager controller
- Move UpdateAPIService() into the `apicerts` package, since that is
  the only user of the function.

internal/controller/apicerts/certs_manager.go

@@ -19,7 +19,6 @@ import (
 	aggregatorclient "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset"
 
 	"github.com/suzerain-io/controller-go"
-	"github.com/suzerain-io/placeholder-name/internal/autoregistration"
 	"github.com/suzerain-io/placeholder-name/internal/certauthority"
 	placeholdernamecontroller "github.com/suzerain-io/placeholder-name/internal/controller"
 )
@@ -45,6 +44,7 @@ func NewCertsManagerController(
 	aggregatorClient aggregatorclient.Interface,
 	secretInformer corev1informers.SecretInformer,
 	withInformer placeholdernamecontroller.WithInformerOptionFunc,
+	withInitialEvent placeholdernamecontroller.WithInitialEventOptionFunc,
 ) controller.Controller {
 	return controller.New(
 		controller.Config{
@@ -62,7 +62,7 @@ func NewCertsManagerController(
 			controller.InformerOption{},
 		),
 		// Be sure to run once even if the Secret that the informer is watching doesn't exist.
-		controller.WithInitialEvent(controller.Key{
+		withInitialEvent(controller.Key{
 			Namespace: namespace,
 			Name:      certsSecretName,
 		}),
@@ -123,7 +123,7 @@ func (c *certsManagerController) Sync(ctx controller.Context) error {
 	}
 
 	// Update the APIService to give it the new CA bundle.
-	if err := autoregistration.UpdateAPIService(ctx.Context, c.aggregatorClient, aggregatedAPIServerCA.Bundle()); err != nil {
+	if err := UpdateAPIService(ctx.Context, c.aggregatorClient, aggregatedAPIServerCA.Bundle()); err != nil {
 		return fmt.Errorf("could not update the API service: %w", err)
 	}
 

internal/controller/apicerts/certs_manager_test.go

@@ -31,26 +31,27 @@ import (
 	placeholderv1alpha1 "github.com/suzerain-io/placeholder-name/kubernetes/1.19/api/apis/placeholder/v1alpha1"
 )
 
-// TODO test that it uses controller.WithInitialEvent correctly
-
-func TestManagerControllerInformerFilters(t *testing.T) {
-	spec.Run(t, "informer filters", func(t *testing.T, when spec.G, it spec.S) {
+func TestManagerControllerOptions(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 observableWithInitialEventOption *testutil.ObservableWithInitialEventOption
 		var secretsInformerFilter controller.Filter
 
 		it.Before(func() {
 			r = require.New(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
+				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
 			)
 			secretsInformerFilter = observableWithInformerOption.GetFilterForInformer(secretsInformer)
 		})
@@ -102,6 +103,15 @@ func TestManagerControllerInformerFilters(t *testing.T) {
 				})
 			})
 		})
+
+		when("starting up", func() {
+			it("asks for an initial event because the Secret may not exist yet and it needs to run anyway", func() {
+				r.Equal(controller.Key{
+					Namespace: installedInNamespace,
+					Name:      "api-serving-cert",
+				}, observableWithInitialEventOption.GetInitialEventKey())
+			})
+		})
 	}, spec.Parallel(), spec.Report(report.Terminal{}))
 }
 
@@ -130,6 +140,7 @@ func TestManagerControllerSync(t *testing.T) {
 				aggregatorAPIClient,
 				kubeInformers.Core().V1().Secrets(),
 				controller.WithInformer,
+				controller.WithInitialEvent,
 			)
 
 			// Set this at the last second to support calling subject.Name().

internal/controller/apicerts/update_api_service.go

@@ -0,0 +1,41 @@
+/*
+Copyright 2020 VMware, Inc.
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package apicerts
+
+import (
+	"context"
+	"fmt"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/util/retry"
+	aggregatorclient "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset"
+
+	placeholderv1alpha1 "github.com/suzerain-io/placeholder-name/kubernetes/1.19/api/apis/placeholder/v1alpha1"
+)
+
+// UpdateAPIService updates the APIService's CA bundle.
+func UpdateAPIService(ctx context.Context, aggregatorClient aggregatorclient.Interface, aggregatedAPIServerCA []byte) error {
+	apiServices := aggregatorClient.ApiregistrationV1().APIServices()
+	apiServiceName := placeholderv1alpha1.SchemeGroupVersion.Version + "." + placeholderv1alpha1.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.
+		fetchedAPIService, err := apiServices.Get(ctx, apiServiceName, metav1.GetOptions{})
+		if err != nil {
+			return fmt.Errorf("could not get existing version of API service: %w", err)
+		}
+
+		// Update just the field we care about.
+		fetchedAPIService.Spec.CABundle = aggregatedAPIServerCA
+
+		_, updateErr := apiServices.Update(ctx, fetchedAPIService, metav1.UpdateOptions{})
+		return updateErr
+	}); err != nil {
+		return fmt.Errorf("could not update API service: %w", err)
+	}
+	return nil
+}

internal/controller/apicerts/update_api_service_test.go

@@ -0,0 +1,169 @@
+/*
+Copyright 2020 VMware, Inc.
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package apicerts
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"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"
+	kubetesting "k8s.io/client-go/testing"
+	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
+	aggregatorv1fake "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake"
+)
+
+func TestUpdateAPIService(t *testing.T) {
+	const apiServiceName = "v1alpha1.placeholder.suzerain-io.github.io"
+
+	tests := []struct {
+		name        string
+		mocks       func(*aggregatorv1fake.Clientset)
+		caInput     []byte
+		wantObjects []apiregistrationv1.APIService
+		wantErr     string
+	}{
+		{
+			name: "happy path update when the pre-existing APIService did not already have a CA bundle",
+			mocks: func(c *aggregatorv1fake.Clientset) {
+				_ = c.Tracker().Add(&apiregistrationv1.APIService{
+					ObjectMeta: metav1.ObjectMeta{Name: apiServiceName},
+					Spec: apiregistrationv1.APIServiceSpec{
+						GroupPriorityMinimum: 999,
+						CABundle:             nil,
+					},
+				})
+			},
+			caInput: []byte("some-ca-bundle"),
+			wantObjects: []apiregistrationv1.APIService{{
+				ObjectMeta: metav1.ObjectMeta{Name: apiServiceName},
+				Spec: apiregistrationv1.APIServiceSpec{
+					GroupPriorityMinimum: 999,
+					CABundle:             []byte("some-ca-bundle"),
+				},
+			}},
+		},
+		{
+			name: "happy path update when the pre-existing APIService already had a CA bundle",
+			mocks: func(c *aggregatorv1fake.Clientset) {
+				_ = c.Tracker().Add(&apiregistrationv1.APIService{
+					ObjectMeta: metav1.ObjectMeta{Name: apiServiceName},
+					Spec: apiregistrationv1.APIServiceSpec{
+						GroupPriorityMinimum: 999,
+						CABundle:             []byte("some-other-different-ca-bundle"),
+					},
+				})
+			},
+			caInput: []byte("some-ca-bundle"),
+			wantObjects: []apiregistrationv1.APIService{{
+				ObjectMeta: metav1.ObjectMeta{Name: apiServiceName},
+				Spec: apiregistrationv1.APIServiceSpec{
+					GroupPriorityMinimum: 999,
+					CABundle:             []byte("some-ca-bundle"),
+				},
+			}},
+		},
+		{
+			name: "error on update",
+			mocks: func(c *aggregatorv1fake.Clientset) {
+				_ = c.Tracker().Add(&apiregistrationv1.APIService{
+					ObjectMeta: metav1.ObjectMeta{Name: apiServiceName},
+					Spec:       apiregistrationv1.APIServiceSpec{},
+				})
+				c.PrependReactor("update", "apiservices", func(_ kubetesting.Action) (bool, runtime.Object, error) {
+					return true, nil, fmt.Errorf("error on update")
+				})
+			},
+			wantErr: "could not update API service: error on update",
+		},
+		{
+			name: "error on get",
+			mocks: func(c *aggregatorv1fake.Clientset) {
+				_ = c.Tracker().Add(&apiregistrationv1.APIService{
+					ObjectMeta: metav1.ObjectMeta{Name: apiServiceName},
+					Spec:       apiregistrationv1.APIServiceSpec{},
+				})
+				c.PrependReactor("get", "apiservices", func(_ kubetesting.Action) (bool, runtime.Object, error) {
+					return true, nil, fmt.Errorf("error on get")
+				})
+			},
+			caInput: []byte("some-ca-bundle"),
+			wantErr: "could not update API service: could not get existing version of API service: error on get",
+		},
+		{
+			name: "conflict error on update, followed by successful retry",
+			mocks: func(c *aggregatorv1fake.Clientset) {
+				_ = c.Tracker().Add(&apiregistrationv1.APIService{
+					ObjectMeta: metav1.ObjectMeta{Name: apiServiceName},
+					Spec: apiregistrationv1.APIServiceSpec{
+						GroupPriorityMinimum: 111,
+						CABundle:             nil,
+					},
+				})
+				hit := false
+				c.PrependReactor("update", "apiservices", func(_ kubetesting.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.
+						_ = c.Tracker().Update(schema.GroupVersionResource{
+							Group:    apiregistrationv1.GroupName,
+							Version:  apiregistrationv1.SchemeGroupVersion.Version,
+							Resource: "apiservices",
+						}, &apiregistrationv1.APIService{
+							ObjectMeta: metav1.ObjectMeta{Name: apiServiceName},
+							Spec: apiregistrationv1.APIServiceSpec{
+								GroupPriorityMinimum: 222,
+								CABundle:             nil,
+							},
+						}, "")
+						hit = true
+						return true, nil, apierrors.NewConflict(schema.GroupResource{
+							Group:    apiregistrationv1.GroupName,
+							Resource: "apiservices",
+						}, apiServiceName, fmt.Errorf("there was a conflict"))
+					}
+					return false, nil, nil
+				})
+			},
+			caInput: []byte("some-ca-bundle"),
+			wantObjects: []apiregistrationv1.APIService{{
+				ObjectMeta: metav1.ObjectMeta{Name: apiServiceName},
+				Spec: apiregistrationv1.APIServiceSpec{
+					GroupPriorityMinimum: 222,
+					CABundle:             []byte("some-ca-bundle"),
+				},
+			}},
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := context.Background()
+
+			client := aggregatorv1fake.NewSimpleClientset()
+			if tt.mocks != nil {
+				tt.mocks(client)
+			}
+
+			err := UpdateAPIService(ctx, client, tt.caInput)
+			if tt.wantErr != "" {
+				require.EqualError(t, err, tt.wantErr)
+				return
+			}
+			require.NoError(t, err)
+			if tt.wantObjects != nil {
+				objects, err := client.ApiregistrationV1().APIServices().List(ctx, metav1.ListOptions{})
+				require.NoError(t, err)
+				require.Equal(t, tt.wantObjects, objects.Items)
+			}
+		})
+	}
+}

internal/controller/utils.go

@@ -29,3 +29,6 @@ type WithInformerOptionFunc func(
 	getter controller.InformerGetter,
 	filter controller.Filter,
 	opt controller.InformerOption) controller.Option
+
+// Same signature as controller.WithInitialEvent().
+type WithInitialEventOptionFunc func(key controller.Key) controller.Option