From f76b9857daf0396760e9f34391f05f55a99befbd Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 30 Oct 2020 17:41:17 -0700 Subject: [PATCH 1/2] Don't use custom labels when selecting an agent pod And delete the agent pod when it needs its custom labels to be updated, so that the creator controller will notice that it is missing and immediately create it with the new custom labels. --- .../controller/kubecertagent/annotater.go | 4 +- .../kubecertagent/annotater_test.go | 44 ++++ internal/controller/kubecertagent/creater.go | 5 +- .../controller/kubecertagent/creater_test.go | 22 +- internal/controller/kubecertagent/deleter.go | 4 +- .../controller/kubecertagent/deleter_test.go | 249 +++++++----------- .../controller/kubecertagent/kubecertagent.go | 28 +- 7 files changed, 183 insertions(+), 173 deletions(-) diff --git a/internal/controller/kubecertagent/annotater.go b/internal/controller/kubecertagent/annotater.go index d7727784..d5a63c87 100644 --- a/internal/controller/kubecertagent/annotater.go +++ b/internal/controller/kubecertagent/annotater.go @@ -10,7 +10,6 @@ import ( "github.com/spf13/pflag" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/clock" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" @@ -85,11 +84,10 @@ func NewAnnotaterController( // Sync implements controllerlib.Syncer. func (c *annotaterController) Sync(ctx controllerlib.Context) error { - agentSelector := labels.SelectorFromSet(c.agentPodConfig.Labels()) agentPods, err := c.agentPodInformer. Lister(). Pods(c.agentPodConfig.Namespace). - List(agentSelector) + List(c.agentPodConfig.AgentSelector()) if err != nil { return fmt.Errorf("informer cannot list agent pods: %w", err) } diff --git a/internal/controller/kubecertagent/annotater_test.go b/internal/controller/kubecertagent/annotater_test.go index 28cd8d3d..cb3161e1 100644 --- a/internal/controller/kubecertagent/annotater_test.go +++ b/internal/controller/kubecertagent/annotater_test.go @@ -96,6 +96,10 @@ func TestAnnotaterControllerSync(t *testing.T) { Namespace: agentPodNamespace, ContainerImage: "some-agent-image", PodNamePrefix: "some-agent-name-", + AdditionalLabels: map[string]string{ + "myLabelKey1": "myLabelValue1", + "myLabelKey2": "myLabelValue2", + }, }, &CredentialIssuerConfigLocationConfig{ Namespace: credentialIssuerConfigNamespaceName, @@ -565,6 +569,46 @@ func TestAnnotaterControllerSync(t *testing.T) { }) }) + when("there is an agent pod without annotations set which does not have the configured additional labels", func() { + it.Before(func() { + delete(agentPod.ObjectMeta.Labels, "myLabelKey1") + r.NoError(agentInformerClient.Tracker().Add(agentPod)) + r.NoError(kubeAPIClient.Tracker().Add(agentPod)) + }) + + when("there is a matching controller manager pod", func() { + it.Before(func() { + r.NoError(kubeSystemInformerClient.Tracker().Add(controllerManagerPod)) + r.NoError(kubeAPIClient.Tracker().Add(controllerManagerPod)) + }) + + it("updates the annotations according to the controller manager pod", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + updatedAgentPod := agentPod.DeepCopy() + updatedAgentPod.Annotations[certPathAnnotation] = certPath + updatedAgentPod.Annotations[keyPathAnnotation] = keyPath + + r.Equal( + []coretesting.Action{ + coretesting.NewGetAction( + podsGVR, + agentPodNamespace, + updatedAgentPod.Name, + ), + coretesting.NewUpdateAction( + podsGVR, + agentPodNamespace, + updatedAgentPod, + ), + }, + kubeAPIClient.Actions(), + ) + }) + }) + }) + when("there is an agent pod with correct annotations set", func() { it.Before(func() { agentPod.Annotations = make(map[string]string) diff --git a/internal/controller/kubecertagent/creater.go b/internal/controller/kubecertagent/creater.go index e284f110..028a72b3 100644 --- a/internal/controller/kubecertagent/creater.go +++ b/internal/controller/kubecertagent/creater.go @@ -110,7 +110,7 @@ func (c *createrController) Sync(ctx controllerlib.Context) error { controllerManagerPod, c.kubeSystemPodInformer, c.agentPodInformer, - c.agentPodConfig.Labels(), + c.agentPodConfig.AgentSelector(), ) if err != nil { return err @@ -158,9 +158,8 @@ func findAgentPodForSpecificControllerManagerPod( controllerManagerPod *corev1.Pod, kubeSystemPodInformer corev1informers.PodInformer, agentPodInformer corev1informers.PodInformer, - agentLabels map[string]string, + agentSelector labels.Selector, ) (*corev1.Pod, error) { - agentSelector := labels.SelectorFromSet(agentLabels) agentPods, err := agentPodInformer. Lister(). List(agentSelector) diff --git a/internal/controller/kubecertagent/creater_test.go b/internal/controller/kubecertagent/creater_test.go index e6fb80c3..8c380dcf 100644 --- a/internal/controller/kubecertagent/creater_test.go +++ b/internal/controller/kubecertagent/creater_test.go @@ -223,10 +223,24 @@ func TestCreaterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{}, - kubeAPIClient.Actions(), - ) + r.Empty(kubeAPIClient.Actions()) + }) + }) + + when("there is a matching agent pod that is missing some of the configured additional labels", func() { + it.Before(func() { + nonMatchingAgentPod := agentPod.DeepCopy() + delete(nonMatchingAgentPod.ObjectMeta.Labels, "myLabelKey1") + r.NoError(agentInformerClient.Tracker().Add(nonMatchingAgentPod)) + r.NoError(kubeAPIClient.Tracker().Add(nonMatchingAgentPod)) + }) + + it("does nothing because the deleter controller is responsible for deleting it", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + + r.NoError(err) + r.Empty(kubeAPIClient.Actions()) }) }) diff --git a/internal/controller/kubecertagent/deleter.go b/internal/controller/kubecertagent/deleter.go index 99bf09e8..992cd998 100644 --- a/internal/controller/kubecertagent/deleter.go +++ b/internal/controller/kubecertagent/deleter.go @@ -7,7 +7,6 @@ import ( "fmt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" "k8s.io/klog/v2" @@ -57,11 +56,10 @@ func NewDeleterController( // Sync implements controllerlib.Syncer. func (c *deleterController) Sync(ctx controllerlib.Context) error { - agentSelector := labels.SelectorFromSet(c.agentPodConfig.Labels()) agentPods, err := c.agentPodInformer. Lister(). Pods(c.agentPodConfig.Namespace). - List(agentSelector) + List(c.agentPodConfig.AgentSelector()) if err != nil { return fmt.Errorf("informer cannot list agent pods: %w", err) } diff --git a/internal/controller/kubecertagent/deleter_test.go b/internal/controller/kubecertagent/deleter_test.go index 065370fc..30dff398 100644 --- a/internal/controller/kubecertagent/deleter_test.go +++ b/internal/controller/kubecertagent/deleter_test.go @@ -72,6 +72,10 @@ func TestDeleterControllerSync(t *testing.T) { Namespace: agentPodNamespace, ContainerImage: "some-agent-image", PodNamePrefix: "some-agent-name-", + AdditionalLabels: map[string]string{ + "myLabelKey1": "myLabelValue1", + "myLabelKey2": "myLabelValue2", + }, }, kubeAPIClient, kubeSystemInformers.Core().V1().Pods(), @@ -95,6 +99,13 @@ func TestDeleterControllerSync(t *testing.T) { controllerlib.TestRunSynchronously(t, subject) } + var requireAgentPodWasDeleted = func() { + r.Equal( + []coretesting.Action{coretesting.NewDeleteAction(podsGVR, agentPodNamespace, agentPod.Name)}, + kubeAPIClient.Actions(), + ) + } + it.Before(func() { r = require.New(t) @@ -148,19 +159,12 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{}, - kubeAPIClient.Actions(), - ) + r.Empty(kubeAPIClient.Actions()) }) when("the agent pod is out of sync with the controller manager via volume mounts", func() { it.Before(func() { - controllerManagerPod.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ - { - Name: "some-other-volume-mount", - }, - } + controllerManagerPod.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{{Name: "some-other-volume-mount"}} r.NoError(kubeSystemInformerClient.Tracker().Update(podsGVR, controllerManagerPod, controllerManagerPod.Namespace)) r.NoError(kubeAPIClient.Tracker().Update(podsGVR, controllerManagerPod, controllerManagerPod.Namespace)) }) @@ -170,26 +174,13 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{ - coretesting.NewDeleteAction( - podsGVR, - agentPodNamespace, - agentPod.Name, - ), - }, - kubeAPIClient.Actions(), - ) + requireAgentPodWasDeleted() }) }) when("the agent pod is out of sync with the controller manager via volumes", func() { it.Before(func() { - controllerManagerPod.Spec.Volumes = []corev1.Volume{ - { - Name: "some-other-volume", - }, - } + controllerManagerPod.Spec.Volumes = []corev1.Volume{{Name: "some-other-volume"}} r.NoError(kubeSystemInformerClient.Tracker().Update(podsGVR, controllerManagerPod, controllerManagerPod.Namespace)) r.NoError(kubeAPIClient.Tracker().Update(podsGVR, controllerManagerPod, controllerManagerPod.Namespace)) }) @@ -199,16 +190,7 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{ - coretesting.NewDeleteAction( - podsGVR, - agentPodNamespace, - agentPod.Name, - ), - }, - kubeAPIClient.Actions(), - ) + requireAgentPodWasDeleted() }) }) @@ -226,16 +208,7 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{ - coretesting.NewDeleteAction( - podsGVR, - agentPodNamespace, - agentPod.Name, - ), - }, - kubeAPIClient.Actions(), - ) + requireAgentPodWasDeleted() }) }) @@ -251,26 +224,13 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{ - coretesting.NewDeleteAction( - podsGVR, - agentPodNamespace, - agentPod.Name, - ), - }, - kubeAPIClient.Actions(), - ) + requireAgentPodWasDeleted() }) }) when("the agent pod is out of sync with the controller manager via tolerations", func() { it.Before(func() { - controllerManagerPod.Spec.Tolerations = []corev1.Toleration{ - { - Key: "some-other-toleration-key", - }, - } + controllerManagerPod.Spec.Tolerations = []corev1.Toleration{{Key: "some-other-toleration-key"}} r.NoError(kubeSystemInformerClient.Tracker().Update(podsGVR, controllerManagerPod, controllerManagerPod.Namespace)) r.NoError(kubeAPIClient.Tracker().Update(podsGVR, controllerManagerPod, controllerManagerPod.Namespace)) }) @@ -280,16 +240,7 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{ - coretesting.NewDeleteAction( - podsGVR, - agentPodNamespace, - agentPod.Name, - ), - }, - kubeAPIClient.Actions(), - ) + requireAgentPodWasDeleted() }) }) @@ -306,16 +257,7 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{ - coretesting.NewDeleteAction( - podsGVR, - agentPodNamespace, - agentPod.Name, - ), - }, - kubeAPIClient.Actions(), - ) + requireAgentPodWasDeleted() }) }) @@ -332,16 +274,7 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{ - coretesting.NewDeleteAction( - podsGVR, - agentPodNamespace, - agentPod.Name, - ), - }, - kubeAPIClient.Actions(), - ) + requireAgentPodWasDeleted() }) }) @@ -358,16 +291,7 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{ - coretesting.NewDeleteAction( - podsGVR, - agentPodNamespace, - agentPod.Name, - ), - }, - kubeAPIClient.Actions(), - ) + requireAgentPodWasDeleted() }) }) @@ -384,16 +308,74 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{ - coretesting.NewDeleteAction( - podsGVR, - agentPodNamespace, - agentPod.Name, - ), - }, - kubeAPIClient.Actions(), - ) + requireAgentPodWasDeleted() + }) + }) + + when("the agent pod is out of sync with the template via labels", func() { + when("an additional label's value was changed", func() { + it.Before(func() { + updatedAgentPod := agentPod.DeepCopy() + updatedAgentPod.ObjectMeta.Labels = map[string]string{ + agentPodLabelKey: agentPodLabelValue, + // the value of a label is wrong so the pod should be deleted so it can get recreated with the new labels + "myLabelKey1": "myLabelValue1-outdated-value", + "myLabelKey2": "myLabelValue2-outdated-value", + } + r.NoError(agentInformerClient.Tracker().Update(podsGVR, updatedAgentPod, updatedAgentPod.Namespace)) + r.NoError(kubeAPIClient.Tracker().Update(podsGVR, updatedAgentPod, updatedAgentPod.Namespace)) + }) + + it("deletes the agent pod", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + + r.NoError(err) + requireAgentPodWasDeleted() + }) + }) + + when("an additional label was added", func() { + it.Before(func() { + updatedAgentPod := agentPod.DeepCopy() + updatedAgentPod.ObjectMeta.Labels = map[string]string{ + agentPodLabelKey: agentPodLabelValue, + "myLabelKey1": "myLabelValue1", + // "myLabelKey2" is missing so the pod should be deleted so it can get recreated with the new labels + } + r.NoError(agentInformerClient.Tracker().Update(podsGVR, updatedAgentPod, updatedAgentPod.Namespace)) + r.NoError(kubeAPIClient.Tracker().Update(podsGVR, updatedAgentPod, updatedAgentPod.Namespace)) + }) + + it("deletes the agent pod", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + + r.NoError(err) + requireAgentPodWasDeleted() + }) + }) + + when("the agent pod has extra labels that seem unrelated to the additional labels", func() { + it.Before(func() { + updatedAgentPod := agentPod.DeepCopy() + updatedAgentPod.ObjectMeta.Labels = map[string]string{ + agentPodLabelKey: agentPodLabelValue, + "myLabelKey1": "myLabelValue1", + "myLabelKey2": "myLabelValue2", + "extra-label": "not-related-to-the-sepcified-additional-labels", + } + r.NoError(agentInformerClient.Tracker().Update(podsGVR, updatedAgentPod, updatedAgentPod.Namespace)) + r.NoError(kubeAPIClient.Tracker().Update(podsGVR, updatedAgentPod, updatedAgentPod.Namespace)) + }) + + it("does not delete the agent pod because someone else might have put those labels on it", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + + r.NoError(err) + r.Empty(kubeAPIClient.Actions()) + }) }) }) @@ -410,16 +392,7 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{ - coretesting.NewDeleteAction( - podsGVR, - agentPodNamespace, - agentPod.Name, - ), - }, - kubeAPIClient.Actions(), - ) + requireAgentPodWasDeleted() }) }) }) @@ -436,16 +409,7 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{ - coretesting.NewDeleteAction( - podsGVR, - agentPodNamespace, - agentPod.Name, - ), - }, - kubeAPIClient.Actions(), - ) + requireAgentPodWasDeleted() }) }) @@ -461,16 +425,7 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{ - coretesting.NewDeleteAction( - podsGVR, - agentPodNamespace, - agentPod.Name, - ), - }, - kubeAPIClient.Actions(), - ) + requireAgentPodWasDeleted() }) }) @@ -480,16 +435,7 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{ - coretesting.NewDeleteAction( - podsGVR, - agentPodNamespace, - agentPod.Name, - ), - }, - kubeAPIClient.Actions(), - ) + requireAgentPodWasDeleted() }) }) }) @@ -500,10 +446,7 @@ func TestDeleterControllerSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.Equal( - []coretesting.Action{}, - kubeAPIClient.Actions(), - ) + r.Empty(kubeAPIClient.Actions()) }) }) }, spec.Parallel(), spec.Report(report.Terminal{})) diff --git a/internal/controller/kubecertagent/kubecertagent.go b/internal/controller/kubecertagent/kubecertagent.go index 7f816b39..5b8b8e10 100644 --- a/internal/controller/kubecertagent/kubecertagent.go +++ b/internal/controller/kubecertagent/kubecertagent.go @@ -20,6 +20,7 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/clock" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/klog/v2" @@ -81,13 +82,17 @@ type CredentialIssuerConfigLocationConfig struct { } func (c *AgentPodConfig) Labels() map[string]string { - labels := map[string]string{ + allLabels := map[string]string{ agentPodLabelKey: agentPodLabelValue, } for k, v := range c.AdditionalLabels { - labels[k] = v + allLabels[k] = v } - return labels + return allLabels +} + +func (c *AgentPodConfig) AgentSelector() labels.Selector { + return labels.SelectorFromSet(map[string]string{agentPodLabelKey: agentPodLabelValue}) } func (c *AgentPodConfig) PodTemplate() *corev1.Pod { @@ -164,10 +169,19 @@ func newAgentPod( } func isAgentPodUpToDate(actualAgentPod, expectedAgentPod *corev1.Pod) bool { - return equality.Semantic.DeepEqual( - actualAgentPod.Spec.Containers[0].VolumeMounts, - expectedAgentPod.Spec.Containers[0].VolumeMounts, - ) && + requiredLabelsAllPresentWithCorrectValues := true + actualLabels := actualAgentPod.ObjectMeta.Labels + for expectedLabelKey, expectedLabelValue := range expectedAgentPod.ObjectMeta.Labels { + if actualLabels[expectedLabelKey] != expectedLabelValue { + requiredLabelsAllPresentWithCorrectValues = false + break + } + } + return requiredLabelsAllPresentWithCorrectValues && + equality.Semantic.DeepEqual( + actualAgentPod.Spec.Containers[0].VolumeMounts, + expectedAgentPod.Spec.Containers[0].VolumeMounts, + ) && equality.Semantic.DeepEqual( actualAgentPod.Spec.Containers[0].Name, expectedAgentPod.Spec.Containers[0].Name, From 7597b12a511e755b3bc176ba894494a3f6beed10 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 2 Nov 2020 08:40:39 -0800 Subject: [PATCH 2/2] Small unit test changes for deleter_test.go --- .../controller/kubecertagent/deleter_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/controller/kubecertagent/deleter_test.go b/internal/controller/kubecertagent/deleter_test.go index 30dff398..d8d528ca 100644 --- a/internal/controller/kubecertagent/deleter_test.go +++ b/internal/controller/kubecertagent/deleter_test.go @@ -317,7 +317,7 @@ func TestDeleterControllerSync(t *testing.T) { it.Before(func() { updatedAgentPod := agentPod.DeepCopy() updatedAgentPod.ObjectMeta.Labels = map[string]string{ - agentPodLabelKey: agentPodLabelValue, + "kube-cert-agent.pinniped.dev": "true", // the value of a label is wrong so the pod should be deleted so it can get recreated with the new labels "myLabelKey1": "myLabelValue1-outdated-value", "myLabelKey2": "myLabelValue2-outdated-value", @@ -335,12 +335,12 @@ func TestDeleterControllerSync(t *testing.T) { }) }) - when("an additional label was added", func() { + when("an additional custom label was added since the agent pod was created", func() { it.Before(func() { updatedAgentPod := agentPod.DeepCopy() updatedAgentPod.ObjectMeta.Labels = map[string]string{ - agentPodLabelKey: agentPodLabelValue, - "myLabelKey1": "myLabelValue1", + "kube-cert-agent.pinniped.dev": "true", + "myLabelKey1": "myLabelValue1", // "myLabelKey2" is missing so the pod should be deleted so it can get recreated with the new labels } r.NoError(agentInformerClient.Tracker().Update(podsGVR, updatedAgentPod, updatedAgentPod.Namespace)) @@ -360,10 +360,10 @@ func TestDeleterControllerSync(t *testing.T) { it.Before(func() { updatedAgentPod := agentPod.DeepCopy() updatedAgentPod.ObjectMeta.Labels = map[string]string{ - agentPodLabelKey: agentPodLabelValue, - "myLabelKey1": "myLabelValue1", - "myLabelKey2": "myLabelValue2", - "extra-label": "not-related-to-the-sepcified-additional-labels", + "kube-cert-agent.pinniped.dev": "true", + "myLabelKey1": "myLabelValue1", + "myLabelKey2": "myLabelValue2", + "extra-label": "not-related-to-the-sepcified-additional-labels", } r.NoError(agentInformerClient.Tracker().Update(podsGVR, updatedAgentPod, updatedAgentPod.Namespace)) r.NoError(kubeAPIClient.Tracker().Update(podsGVR, updatedAgentPod, updatedAgentPod.Namespace))