From 2a29303e3fdc391a44e112cc312b929e114bcb3b Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 2 Mar 2021 13:59:46 -0600 Subject: [PATCH] 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 --- internal/controller/kubecertagent/annotater.go | 5 ++++- internal/controller/kubecertagent/annotater_test.go | 13 +++++++++++-- internal/controller/kubecertagent/execer.go | 11 +++++++---- internal/controller/kubecertagent/execer_test.go | 12 +++++++++--- internal/controllermanager/prepare_controllers.go | 2 ++ 5 files changed, 33 insertions(+), 10 deletions(-) diff --git a/internal/controller/kubecertagent/annotater.go b/internal/controller/kubecertagent/annotater.go index a18cc732..fa640d21 100644 --- a/internal/controller/kubecertagent/annotater.go +++ b/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), ) diff --git a/internal/controller/kubecertagent/annotater_test.go b/internal/controller/kubecertagent/annotater_test.go index b60cfad8..bb767a47 100644 --- a/internal/controller/kubecertagent/annotater_test.go +++ b/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{ diff --git a/internal/controller/kubecertagent/execer.go b/internal/controller/kubecertagent/execer.go index 0c1e7638..b8ff1c83 100644 --- a/internal/controller/kubecertagent/execer.go +++ b/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, diff --git a/internal/controller/kubecertagent/execer_test.go b/internal/controller/kubecertagent/execer_test.go index 485e9c6f..b3789176 100644 --- a/internal/controller/kubecertagent/execer_test.go +++ b/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{ diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index cd00604b..61990a39 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/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),