Fix label handling in kubecertagent controllers.

These controllers were a bit inconsistent. There were cases where the controllers ran out of the expected order and the custom labels might not have been applied.

We should still plan to remove this label handling or move responsibility into the middleware layer, but this avoids any regression.

Signed-off-by: Matt Moyer <moyerm@vmware.com>

internal/controller/kubecertagent/annotater.go

@@ -33,6 +33,7 @@ const (
 type annotaterController struct {
 	agentPodConfig                 *AgentPodConfig
 	credentialIssuerLocationConfig *CredentialIssuerLocationConfig
+	credentialIssuerLabels         map[string]string
 	clock                          clock.Clock
 	k8sClient                      kubernetes.Interface
 	pinnipedAPIClient              pinnipedclientset.Interface
@@ -51,6 +52,7 @@ type annotaterController struct {
 func NewAnnotaterController(
 	agentPodConfig *AgentPodConfig,
 	credentialIssuerLocationConfig *CredentialIssuerLocationConfig,
+	credentialIssuerLabels map[string]string,
 	clock clock.Clock,
 	k8sClient kubernetes.Interface,
 	pinnipedAPIClient pinnipedclientset.Interface,
@@ -64,6 +66,7 @@ func NewAnnotaterController(
 			Syncer: &annotaterController{
 				agentPodConfig:                 agentPodConfig,
 				credentialIssuerLocationConfig: credentialIssuerLocationConfig,
+				credentialIssuerLabels:         credentialIssuerLabels,
 				clock:                          clock,
 				k8sClient:                      k8sClient,
 				pinnipedAPIClient:              pinnipedAPIClient,
@@ -125,7 +128,7 @@ func (c *annotaterController) Sync(ctx controllerlib.Context) error {
 			strategyResultUpdateErr := issuerconfig.UpdateStrategy(
 				ctx.Context,
 				c.credentialIssuerLocationConfig.Name,
-				nil,
+				c.credentialIssuerLabels,
 				c.pinnipedAPIClient,
 				strategyError(c.clock, err),
 			)

internal/controller/kubecertagent/annotater_test.go

@@ -41,6 +41,7 @@ func TestAnnotaterControllerFilter(t *testing.T) {
 		) {
 			_ = NewAnnotaterController(
 				agentPodConfig,
+				nil, // credentialIssuerLabels, shouldn't matter
 				nil, // credentialIssuerLocationConfig, shouldn't matter
 				nil, // clock, shouldn't matter
 				nil, // k8sClient, shouldn't matter
@@ -85,6 +86,7 @@ func TestAnnotaterControllerSync(t *testing.T) {
 		var podsGVR schema.GroupVersionResource
 		var credentialIssuerGVR schema.GroupVersionResource
 		var frozenNow time.Time
+		var credentialIssuerLabels map[string]string
 
 		// Defer starting the informers until the last possible moment so that the
 		// nested Before's can keep adding things to the informer caches.
@@ -103,6 +105,7 @@ func TestAnnotaterControllerSync(t *testing.T) {
 				&CredentialIssuerLocationConfig{
 					Name: credentialIssuerResourceName,
 				},
+				credentialIssuerLabels,
 				clock.NewFakeClock(frozenNow),
 				kubeAPIClient,
 				pinnipedAPIClient,
@@ -297,6 +300,10 @@ func TestAnnotaterControllerSync(t *testing.T) {
 					})
 
 					when("there is not already a CredentialIssuer", func() {
+						it.Before(func() {
+							credentialIssuerLabels = map[string]string{"foo": "bar"}
+						})
+
 						it("creates the CredentialIssuer status with the error", func() {
 							startInformersAndController()
 							err := controllerlib.TestSync(t, subject, *syncContext)
@@ -304,14 +311,16 @@ func TestAnnotaterControllerSync(t *testing.T) {
 							expectedCreateCredentialIssuer := &configv1alpha1.CredentialIssuer{
 								TypeMeta: metav1.TypeMeta{},
 								ObjectMeta: metav1.ObjectMeta{
-									Name: credentialIssuerResourceName,
+									Name:   credentialIssuerResourceName,
+									Labels: map[string]string{"foo": "bar"},
 								},
 							}
 
 							expectedCredentialIssuer := &configv1alpha1.CredentialIssuer{
 								TypeMeta: metav1.TypeMeta{},
 								ObjectMeta: metav1.ObjectMeta{
-									Name: credentialIssuerResourceName,
+									Name:   credentialIssuerResourceName,
+									Labels: map[string]string{"foo": "bar"},
 								},
 								Status: configv1alpha1.CredentialIssuerStatus{
 									Strategies: []configv1alpha1.CredentialIssuerStrategy{

internal/controller/kubecertagent/execer.go

@@ -31,6 +31,7 @@ const (
 
 type execerController struct {
 	credentialIssuerLocationConfig *CredentialIssuerLocationConfig
+	credentialIssuerLabels         map[string]string
 	discoveryURLOverride           *string
 	dynamicCertProvider            dynamiccert.Provider
 	podCommandExecutor             PodCommandExecutor
@@ -48,6 +49,7 @@ type execerController struct {
 // credentialIssuerLocationConfig, with any errors that it encounters.
 func NewExecerController(
 	credentialIssuerLocationConfig *CredentialIssuerLocationConfig,
+	credentialIssuerLabels map[string]string,
 	discoveryURLOverride *string,
 	dynamicCertProvider dynamiccert.Provider,
 	podCommandExecutor PodCommandExecutor,
@@ -62,6 +64,7 @@ func NewExecerController(
 			Name: "kube-cert-agent-execer-controller",
 			Syncer: &execerController{
 				credentialIssuerLocationConfig: credentialIssuerLocationConfig,
+				credentialIssuerLabels:         credentialIssuerLabels,
 				discoveryURLOverride:           discoveryURLOverride,
 				dynamicCertProvider:            dynamicCertProvider,
 				podCommandExecutor:             podCommandExecutor,
@@ -112,7 +115,7 @@ func (c *execerController) Sync(ctx controllerlib.Context) error {
 		strategyResultUpdateErr := issuerconfig.UpdateStrategy(
 			ctx.Context,
 			c.credentialIssuerLocationConfig.Name,
-			nil,
+			c.credentialIssuerLabels,
 			c.pinnipedAPIClient,
 			strategyError(c.clock, err),
 		)
@@ -125,7 +128,7 @@ func (c *execerController) Sync(ctx controllerlib.Context) error {
 		strategyResultUpdateErr := issuerconfig.UpdateStrategy(
 			ctx.Context,
 			c.credentialIssuerLocationConfig.Name,
-			nil,
+			c.credentialIssuerLabels,
 			c.pinnipedAPIClient,
 			strategyError(c.clock, err),
 		)
@@ -140,7 +143,7 @@ func (c *execerController) Sync(ctx controllerlib.Context) error {
 		strategyResultUpdateErr := issuerconfig.UpdateStrategy(
 			ctx.Context,
 			c.credentialIssuerLocationConfig.Name,
-			nil,
+			c.credentialIssuerLabels,
 			c.pinnipedAPIClient,
 			configv1alpha1.CredentialIssuerStrategy{
 				Type:           configv1alpha1.KubeClusterSigningCertificateStrategyType,
@@ -157,7 +160,7 @@ func (c *execerController) Sync(ctx controllerlib.Context) error {
 	return issuerconfig.UpdateStrategy(
 		ctx.Context,
 		c.credentialIssuerLocationConfig.Name,
-		nil,
+		c.credentialIssuerLabels,
 		c.pinnipedAPIClient,
 		configv1alpha1.CredentialIssuerStrategy{
 			Type:           configv1alpha1.KubeClusterSigningCertificateStrategyType,

internal/controller/kubecertagent/execer_test.go

@@ -49,6 +49,7 @@ func TestExecerControllerOptions(t *testing.T) {
 				&CredentialIssuerLocationConfig{
 					Name: "ignored by this test",
 				},
+				nil, // credentialIssuerLabels, not needed for this test
 				nil, // discoveryURLOverride, not needed for this test
 				nil, // dynamicCertProvider, not needed for this test
 				nil, // podCommandExecutor, not needed for this test
@@ -152,6 +153,7 @@ func TestManagerControllerSync(t *testing.T) {
 		var kubeInformerFactory kubeinformers.SharedInformerFactory
 		var kubeClientset *kubernetesfake.Clientset
 		var fakeExecutor *fakePodExecutor
+		var credentialIssuerLabels map[string]string
 		var discoveryURLOverride *string
 		var dynamicCertProvider dynamiccert.Provider
 		var fakeCertPEM, fakeKeyPEM string
@@ -166,6 +168,7 @@ func TestManagerControllerSync(t *testing.T) {
 				&CredentialIssuerLocationConfig{
 					Name: credentialIssuerResourceName,
 				},
+				credentialIssuerLabels,
 				discoveryURLOverride,
 				dynamicCertProvider,
 				fakeExecutor,
@@ -516,23 +519,26 @@ func TestManagerControllerSync(t *testing.T) {
 						it.Before(func() {
 							server := "https://overridden-server-url.example.com"
 							discoveryURLOverride = &server
+							credentialIssuerLabels = map[string]string{"foo": "bar"}
 							startInformersAndController()
 						})
 
-						it("also creates the the CredentialIssuer with the appropriate status field", func() {
+						it("also creates the the CredentialIssuer with the appropriate status field and labels", func() {
 							r.NoError(controllerlib.TestSync(t, subject, *syncContext))
 
 							expectedCreateCredentialIssuer := &configv1alpha1.CredentialIssuer{
 								TypeMeta: metav1.TypeMeta{},
 								ObjectMeta: metav1.ObjectMeta{
-									Name: credentialIssuerResourceName,
+									Name:   credentialIssuerResourceName,
+									Labels: map[string]string{"foo": "bar"},
 								},
 							}
 
 							expectedCredentialIssuer := &configv1alpha1.CredentialIssuer{
 								TypeMeta: metav1.TypeMeta{},
 								ObjectMeta: metav1.ObjectMeta{
-									Name: credentialIssuerResourceName,
+									Name:   credentialIssuerResourceName,
+									Labels: map[string]string{"foo": "bar"},
 								},
 								Status: configv1alpha1.CredentialIssuerStatus{
 									Strategies: []configv1alpha1.CredentialIssuerStrategy{

internal/controllermanager/prepare_controllers.go

@@ -204,6 +204,7 @@ func PrepareControllers(c *Config) (func(ctx context.Context), error) {
 			kubecertagent.NewAnnotaterController(
 				agentPodConfig,
 				credentialIssuerLocationConfig,
+				c.Labels,
 				clock.RealClock{},
 				client.Kubernetes,
 				client.PinnipedConcierge,
@@ -216,6 +217,7 @@ func PrepareControllers(c *Config) (func(ctx context.Context), error) {
 		WithController(
 			kubecertagent.NewExecerController(
 				credentialIssuerLocationConfig,
+				c.Labels,
 				c.DiscoveryURLOverride,
 				c.DynamicSigningCertProvider,
 				kubecertagent.NewPodCommandExecutor(client.JSONConfig, client.Kubernetes),