kube_config_info_publisher.go no longer watches cic's with an informer

Simplifies the implementation, makes it more consistent with other
updaters of the cic (CredentialIssuerConfig), and also retries on
update conflicts

Signed-off-by: Ryan Richard <richardry@vmware.com>

internal/controller/issuerconfig/create_or_update_credential_issuer_config.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 
 	"k8s.io/apimachinery/pkg/api/equality"
+
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/util/retry"
@@ -34,36 +35,11 @@ func CreateOrUpdateCredentialIssuerConfig(
 			return fmt.Errorf("get failed: %w", err)
 		}
 
-		return createOrUpdateCredentialIssuerConfig(
-			ctx,
-			existingCredentialIssuerConfig,
-			notFound,
-			credentialIssuerConfigResourceName,
-			credentialIssuerConfigNamespace,
-			pinnipedClient,
-			applyUpdatesToCredentialIssuerConfigFunc)
-	})
-
-	if err != nil {
-		return fmt.Errorf("could not create or update credentialissuerconfig: %w", err)
-	}
-	return nil
-}
-
-func createOrUpdateCredentialIssuerConfig(
-	ctx context.Context,
-	existingCredentialIssuerConfig *configv1alpha1.CredentialIssuerConfig,
-	notFound bool,
-	credentialIssuerConfigName string,
-	credentialIssuerConfigNamespace string,
-	pinnipedClient pinnipedclientset.Interface,
-	applyUpdatesToCredentialIssuerConfigFunc func(configToUpdate *configv1alpha1.CredentialIssuerConfig),
-) error {
 		credentialIssuerConfigsClient := pinnipedClient.ConfigV1alpha1().CredentialIssuerConfigs(credentialIssuerConfigNamespace)
 
 		if notFound {
 			// Create it
-		credentialIssuerConfig := minimalValidCredentialIssuerConfig(credentialIssuerConfigName, credentialIssuerConfigNamespace)
+			credentialIssuerConfig := minimalValidCredentialIssuerConfig(credentialIssuerConfigResourceName, credentialIssuerConfigNamespace)
 			applyUpdatesToCredentialIssuerConfigFunc(credentialIssuerConfig)
 
 			if _, err := credentialIssuerConfigsClient.Create(ctx, credentialIssuerConfig, metav1.CreateOptions{}); err != nil {
@@ -83,7 +59,12 @@ func createOrUpdateCredentialIssuerConfig(
 				return err
 			}
 		}
+		return nil
+	})
 
+	if err != nil {
+		return fmt.Errorf("could not create or update credentialissuerconfig: %w", err)
+	}
 	return nil
 }
 

internal/controller/issuerconfig/kube_config_info_publisher.go

@@ -14,7 +14,6 @@ import (
 
 	configv1alpha1 "go.pinniped.dev/generated/1.19/apis/config/v1alpha1"
 	pinnipedclientset "go.pinniped.dev/generated/1.19/client/clientset/versioned"
-	configv1alpha1informers "go.pinniped.dev/generated/1.19/client/informers/externalversions/config/v1alpha1"
 	pinnipedcontroller "go.pinniped.dev/internal/controller"
 	"go.pinniped.dev/internal/controllerlib"
 )
@@ -31,7 +30,6 @@ type kubeConigInfoPublisherController struct {
 	serverOverride                      *string
 	pinnipedClient                      pinnipedclientset.Interface
 	configMapInformer                   corev1informers.ConfigMapInformer
-	credentialIssuerConfigInformer      configv1alpha1informers.CredentialIssuerConfigInformer
 }
 
 // NewKubeConfigInfoPublisherController returns a controller that syncs the
@@ -43,7 +41,6 @@ func NewKubeConfigInfoPublisherController(
 	serverOverride *string,
 	pinnipedClient pinnipedclientset.Interface,
 	configMapInformer corev1informers.ConfigMapInformer,
-	credentialIssuerConfigInformer configv1alpha1informers.CredentialIssuerConfigInformer, // TODO don't have this informer here
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
 ) controllerlib.Controller {
 	return controllerlib.New(
@@ -55,7 +52,6 @@ func NewKubeConfigInfoPublisherController(
 				serverOverride:                      serverOverride,
 				pinnipedClient:                      pinnipedClient,
 				configMapInformer:                   configMapInformer,
-				credentialIssuerConfigInformer:      credentialIssuerConfigInformer,
 			},
 		},
 		withInformer(
@@ -63,11 +59,6 @@ func NewKubeConfigInfoPublisherController(
 			pinnipedcontroller.NameAndNamespaceExactMatchFilterFactory(clusterInfoName, ClusterInfoNamespace),
 			controllerlib.InformerOption{},
 		),
-		withInformer(
-			credentialIssuerConfigInformer,
-			pinnipedcontroller.NameAndNamespaceExactMatchFilterFactory(credentialIssuerConfigResourceName, credentialIssuerConfigNamespaceName),
-			controllerlib.InformerOption{},
-		),
 	)
 }
 
@@ -112,15 +103,6 @@ func (c *kubeConigInfoPublisherController) Sync(ctx controllerlib.Context) error
 		server = *c.serverOverride
 	}
 
-	existingCredentialIssuerConfigFromInformerCache, err := c.credentialIssuerConfigInformer.
-		Lister().
-		CredentialIssuerConfigs(c.credentialIssuerConfigNamespaceName).
-		Get(c.credentialIssuerConfigResourceName)
-	notFound = k8serrors.IsNotFound(err)
-	if err != nil && !notFound {
-		return fmt.Errorf("could not get credentialissuerconfig: %w", err)
-	}
-
 	updateServerAndCAFunc := func(c *configv1alpha1.CredentialIssuerConfig) {
 		c.Status.KubeConfigInfo = &configv1alpha1.CredentialIssuerConfigKubeConfigInfo{
 			Server:                   server,
@@ -128,17 +110,11 @@ func (c *kubeConigInfoPublisherController) Sync(ctx controllerlib.Context) error
 		}
 	}
 
-	err = createOrUpdateCredentialIssuerConfig(
+	return CreateOrUpdateCredentialIssuerConfig(
 		ctx.Context,
-		existingCredentialIssuerConfigFromInformerCache,
-		notFound,
-		c.credentialIssuerConfigResourceName,
 		c.credentialIssuerConfigNamespaceName,
+		c.credentialIssuerConfigResourceName,
 		c.pinnipedClient,
-		updateServerAndCAFunc)
+		updateServerAndCAFunc,
-
+	)
-	if err != nil {
-		return fmt.Errorf("could not create or update credentialissuerconfig: %w", err)
-	}
-	return nil
 }

internal/controller/issuerconfig/kube_config_info_publisher_test.go

@@ -22,7 +22,6 @@ import (
 
 	configv1alpha1 "go.pinniped.dev/generated/1.19/apis/config/v1alpha1"
 	pinnipedfake "go.pinniped.dev/generated/1.19/client/clientset/versioned/fake"
-	pinnipedinformers "go.pinniped.dev/generated/1.19/client/informers/externalversions"
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/here"
 	"go.pinniped.dev/internal/testutil"
@@ -36,24 +35,20 @@ func TestInformerFilters(t *testing.T) {
 		var r *require.Assertions
 		var observableWithInformerOption *testutil.ObservableWithInformerOption
 		var configMapInformerFilter controllerlib.Filter
-		var credentialIssuerConfigInformerFilter controllerlib.Filter
 
 		it.Before(func() {
 			r = require.New(t)
 			observableWithInformerOption = testutil.NewObservableWithInformerOption()
 			configMapInformer := kubeinformers.NewSharedInformerFactory(nil, 0).Core().V1().ConfigMaps()
-			credentialIssuerConfigInformer := pinnipedinformers.NewSharedInformerFactory(nil, 0).Config().V1alpha1().CredentialIssuerConfigs()
 			_ = NewKubeConfigInfoPublisherController(
 				installedInNamespace,
 				credentialIssuerConfigResourceName,
 				nil,
 				nil,
 				configMapInformer,
-				credentialIssuerConfigInformer,
 				observableWithInformerOption.WithInformer, // make it possible to observe the behavior of the Filters
 			)
 			configMapInformerFilter = observableWithInformerOption.GetFilterForInformer(configMapInformer)
-			credentialIssuerConfigInformerFilter = observableWithInformerOption.GetFilterForInformer(credentialIssuerConfigInformer)
 		})
 
 		when("watching ConfigMap objects", func() {
@@ -103,62 +98,6 @@ func TestInformerFilters(t *testing.T) {
 				})
 			})
 		})
-
-		when("watching CredentialIssuerConfig objects", func() {
-			var subject controllerlib.Filter
-			var target, wrongNamespace, wrongName, unrelated *configv1alpha1.CredentialIssuerConfig
-
-			it.Before(func() {
-				subject = credentialIssuerConfigInformerFilter
-				target = &configv1alpha1.CredentialIssuerConfig{
-					ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerConfigResourceName, Namespace: installedInNamespace},
-				}
-				wrongNamespace = &configv1alpha1.CredentialIssuerConfig{
-					ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerConfigResourceName, Namespace: "wrong-namespace"},
-				}
-				wrongName = &configv1alpha1.CredentialIssuerConfig{
-					ObjectMeta: metav1.ObjectMeta{Name: "wrong-name", Namespace: installedInNamespace},
-				}
-				unrelated = &configv1alpha1.CredentialIssuerConfig{
-					ObjectMeta: metav1.ObjectMeta{Name: "wrong-name", Namespace: "wrong-namespace"},
-				}
-			})
-
-			when("the target CredentialIssuerConfig 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 CredentialIssuerConfig 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 CredentialIssuerConfig 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 CredentialIssuerConfig 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{}))
 }
 
@@ -172,9 +111,7 @@ func TestSync(t *testing.T) {
 		var subject controllerlib.Controller
 		var serverOverride *string
 		var kubeInformerClient *kubernetesfake.Clientset
-		var pinnipedInformerClient *pinnipedfake.Clientset
 		var kubeInformers kubeinformers.SharedInformerFactory
-		var pinnipedInformers pinnipedinformers.SharedInformerFactory
 		var pinnipedAPIClient *pinnipedfake.Clientset
 		var timeoutContext context.Context
 		var timeoutContextCancel context.CancelFunc
@@ -212,7 +149,6 @@ func TestSync(t *testing.T) {
 				serverOverride,
 				pinnipedAPIClient,
 				kubeInformers.Core().V1().ConfigMaps(),
-				pinnipedInformers.Config().V1alpha1().CredentialIssuerConfigs(),
 				controllerlib.WithInformer,
 			)
 
@@ -228,7 +164,6 @@ func TestSync(t *testing.T) {
 
 			// Must start informers before calling TestRunSynchronously()
 			kubeInformers.Start(timeoutContext.Done())
-			pinnipedInformers.Start(timeoutContext.Done())
 			controllerlib.TestRunSynchronously(t, subject)
 		}
 
@@ -240,8 +175,6 @@ func TestSync(t *testing.T) {
 			kubeInformerClient = kubernetesfake.NewSimpleClientset()
 			kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0)
 			pinnipedAPIClient = pinnipedfake.NewSimpleClientset()
-			pinnipedInformerClient = pinnipedfake.NewSimpleClientset()
-			pinnipedInformers = pinnipedinformers.NewSharedInformerFactory(pinnipedInformerClient, 0)
 		})
 
 		it.After(func() {
@@ -288,6 +221,7 @@ func TestSync(t *testing.T) {
 
 						r.Equal(
 							[]coretesting.Action{
+								coretesting.NewGetAction(expectedCredentialIssuerConfigGVR, installedInNamespace, expectedCredentialIssuerConfig.Name),
 								coretesting.NewCreateAction(
 									expectedCredentialIssuerConfigGVR,
 									installedInNamespace,
@@ -334,6 +268,7 @@ func TestSync(t *testing.T) {
 
 							r.Equal(
 								[]coretesting.Action{
+									coretesting.NewGetAction(expectedCredentialIssuerConfigGVR, installedInNamespace, expectedCredentialIssuerConfig.Name),
 									coretesting.NewCreateAction(
 										expectedCredentialIssuerConfigGVR,
 										installedInNamespace,
@@ -348,13 +283,16 @@ func TestSync(t *testing.T) {
 
 				when("the CredentialIssuerConfig already exists", func() {
 					when("the CredentialIssuerConfig is already up to date according to the data in the ConfigMap", func() {
+						var credentialIssuerConfigGVR schema.GroupVersionResource
+						var credentialIssuerConfig *configv1alpha1.CredentialIssuerConfig
+
 						it.Before(func() {
-							_, expectedCredentialIssuerConfig := expectedCredentialIssuerConfig(
+							credentialIssuerConfigGVR, credentialIssuerConfig = expectedCredentialIssuerConfig(
 								installedInNamespace,
 								kubeServerURL,
 								caData,
 							)
-							err := pinnipedInformerClient.Tracker().Add(expectedCredentialIssuerConfig)
+							err := pinnipedAPIClient.Tracker().Add(credentialIssuerConfig)
 							r.NoError(err)
 						})
 
@@ -363,7 +301,12 @@ func TestSync(t *testing.T) {
 							err := controllerlib.TestSync(t, subject, *syncContext)
 							r.NoError(err)
 
-							r.Empty(pinnipedAPIClient.Actions())
+							r.Equal(
+								[]coretesting.Action{
+									coretesting.NewGetAction(credentialIssuerConfigGVR, installedInNamespace, credentialIssuerConfig.Name),
+								},
+								pinnipedAPIClient.Actions(),
+							)
 						})
 					})
 
@@ -375,7 +318,6 @@ func TestSync(t *testing.T) {
 								caData,
 							)
 							expectedCredentialIssuerConfig.Status.KubeConfigInfo.Server = "https://some-other-server"
-							r.NoError(pinnipedInformerClient.Tracker().Add(expectedCredentialIssuerConfig))
 							r.NoError(pinnipedAPIClient.Tracker().Add(expectedCredentialIssuerConfig))
 						})
 
@@ -390,6 +332,7 @@ func TestSync(t *testing.T) {
 								caData,
 							)
 							expectedActions := []coretesting.Action{
+								coretesting.NewGetAction(expectedCredentialIssuerConfigGVR, installedInNamespace, expectedCredentialIssuerConfig.Name),
 								coretesting.NewUpdateAction(
 									expectedCredentialIssuerConfigGVR,
 									installedInNamespace,

internal/controllermanager/prepare_controllers.go

@@ -114,7 +114,6 @@ func PrepareControllers(c *Config) (func(ctx context.Context), error) {
 				c.DiscoveryURLOverride,
 				pinnipedClient,
 				informers.kubePublicNamespaceK8s.Core().V1().ConfigMaps(),
-				informers.installationNamespacePinniped.Config().V1alpha1().CredentialIssuerConfigs(),
 				controllerlib.WithInformer,
 			),
 			singletonWorker,

test/integration/credentialissuerconfig_test.go

@@ -9,10 +9,8 @@ import (
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/rest"
 
 	configv1alpha1 "go.pinniped.dev/generated/1.19/apis/config/v1alpha1"
 	"go.pinniped.dev/test/library"
@@ -50,7 +48,14 @@ func TestCredentialIssuerConfig(t *testing.T) {
 			require.Equal(t, configv1alpha1.FetchedKeyStrategyReason, actualStatusStrategy.Reason)
 			require.Equal(t, "Key was fetched successfully", actualStatusStrategy.Message)
 			// Verify the published kube config info.
-			require.Equal(t, expectedStatusKubeConfigInfo(config), actualStatusKubeConfigInfo)
+			require.Equal(
+				t,
+				&configv1alpha1.CredentialIssuerConfigKubeConfigInfo{
+					Server:                   config.Host,
+					CertificateAuthorityData: base64.StdEncoding.EncodeToString(config.TLSClientConfig.CAData),
+				},
+				actualStatusKubeConfigInfo,
+			)
 		} else {
 			require.Equal(t, configv1alpha1.ErrorStrategyStatus, actualStatusStrategy.Status)
 			require.Equal(t, configv1alpha1.CouldNotFetchKeyStrategyReason, actualStatusStrategy.Reason)
@@ -63,53 +68,4 @@ func TestCredentialIssuerConfig(t *testing.T) {
 
 		require.WithinDuration(t, time.Now(), actualStatusStrategy.LastUpdateTime.Local(), 10*time.Minute)
 	})
-
-	t.Run("reconciling CredentialIssuerConfig", func(t *testing.T) {
-		library.SkipUnlessClusterHasCapability(t, library.ClusterSigningKeyIsAvailable)
-
-		existingConfig, err := client.
-			ConfigV1alpha1().
-			CredentialIssuerConfigs(namespaceName).
-			Get(ctx, "pinniped-config", metav1.GetOptions{})
-		require.NoError(t, err)
-		require.Len(t, existingConfig.Status.Strategies, 1)
-		initialStrategy := existingConfig.Status.Strategies[0]
-
-		// Mutate the existing object. Don't delete it because that would mess up its `Status.Strategies` array,
-		// since the reconciling controller is not currently responsible for that field.
-		updatedServerValue := "https://junk"
-		// TODO maybe mutate the kube-info configmap's CA value instead, because that's the object that we care to check that the controller is watching
-		existingConfig.Status.KubeConfigInfo.Server = updatedServerValue
-		updatedConfig, err := client.
-			ConfigV1alpha1().
-			CredentialIssuerConfigs(namespaceName).
-			Update(ctx, existingConfig, metav1.UpdateOptions{})
-		require.NoError(t, err)
-		require.Equal(t, updatedServerValue, updatedConfig.Status.KubeConfigInfo.Server)
-
-		// Expect that the object's mutated field is set back to what matches its source of truth by the controller.
-		var actualCredentialIssuerConfig *configv1alpha1.CredentialIssuerConfig
-		var configChangesServerField = func() bool {
-			actualCredentialIssuerConfig, err = client.
-				ConfigV1alpha1().
-				CredentialIssuerConfigs(namespaceName).
-				Get(ctx, "pinniped-config", metav1.GetOptions{})
-			return err == nil && actualCredentialIssuerConfig.Status.KubeConfigInfo.Server != updatedServerValue
-		}
-		assert.Eventually(t, configChangesServerField, 10*time.Second, 100*time.Millisecond)
-		require.NoError(t, err) // prints out the error and stops the test in case of failure
-		actualStatusKubeConfigInfo := actualCredentialIssuerConfig.Status.KubeConfigInfo
-		require.Equal(t, expectedStatusKubeConfigInfo(config), actualStatusKubeConfigInfo)
-
-		// The strategies should not have changed during reconciliation.
-		require.Len(t, actualCredentialIssuerConfig.Status.Strategies, 1)
-		require.Equal(t, initialStrategy, actualCredentialIssuerConfig.Status.Strategies[0])
-	})
-}
-
-func expectedStatusKubeConfigInfo(config *rest.Config) *configv1alpha1.CredentialIssuerConfigKubeConfigInfo {
-	return &configv1alpha1.CredentialIssuerConfigKubeConfigInfo{
-		Server:                   config.Host,
-		CertificateAuthorityData: base64.StdEncoding.EncodeToString(config.TLSClientConfig.CAData),
-	}
 }