From 9f80b0ea0048328ac1bbeff4cc7cf00d5a472a02 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 24 Sep 2020 10:40:50 -0400 Subject: [PATCH] Set CIC error statuses in kubecertagent annotater and creater Also fix an instance where we were using an informer in a retry loop. Signed-off-by: Andrew Keesler --- .../controller/kubecertagent/annotater.go | 54 ++- .../kubecertagent/annotater_test.go | 208 +++++++++++ internal/controller/kubecertagent/creater.go | 41 ++- .../controller/kubecertagent/creater_test.go | 339 +++++++++++++++++- internal/controller/kubecertagent/execer.go | 59 ++- .../controller/kubecertagent/kubecertagent.go | 55 ++- .../controllermanager/prepare_controllers.go | 5 + 7 files changed, 693 insertions(+), 68 deletions(-) diff --git a/internal/controller/kubecertagent/annotater.go b/internal/controller/kubecertagent/annotater.go index db9670ac..9e6272bb 100644 --- a/internal/controller/kubecertagent/annotater.go +++ b/internal/controller/kubecertagent/annotater.go @@ -11,11 +11,13 @@ import ( 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" "k8s.io/client-go/util/retry" "k8s.io/klog/v2" + pinnipedclientset "go.pinniped.dev/generated/1.19/client/clientset/versioned" pinnipedcontroller "go.pinniped.dev/internal/controller" "go.pinniped.dev/internal/controllerlib" ) @@ -28,21 +30,29 @@ const ( ) type annotaterController struct { - agentPodConfig *AgentPodConfig - k8sClient kubernetes.Interface - kubeSystemPodInformer corev1informers.PodInformer - agentPodInformer corev1informers.PodInformer + agentPodConfig *AgentPodConfig + credentialIssuerConfigLocationConfig *CredentialIssuerConfigLocationConfig + clock clock.Clock + k8sClient kubernetes.Interface + pinnipedAPIClient pinnipedclientset.Interface + kubeSystemPodInformer corev1informers.PodInformer + agentPodInformer corev1informers.PodInformer } // NewAnnotaterController returns a controller that updates agent pods with the path to the kube // API's certificate and key. // -// This controller will add annotations to agent pods, using the provided -// agentInfo.CertPathAnnotation and agentInfo.KeyPathAnnotation annotation keys, with the best-guess -// paths to the kube API's certificate and key. +// This controller will add annotations to agent pods with the best-guess paths to the kube API's +// certificate and key. +// +// It also is tasked with updating the CredentialIssuerConfig, located via the provided +// credentialIssuerConfigLocationConfig, with any errors that it encounters. func NewAnnotaterController( agentPodConfig *AgentPodConfig, + credentialIssuerConfigLocationConfig *CredentialIssuerConfigLocationConfig, + clock clock.Clock, k8sClient kubernetes.Interface, + pinnipedAPIClient pinnipedclientset.Interface, kubeSystemPodInformer corev1informers.PodInformer, agentPodInformer corev1informers.PodInformer, withInformer pinnipedcontroller.WithInformerOptionFunc, @@ -51,10 +61,13 @@ func NewAnnotaterController( controllerlib.Config{ Name: "kube-cert-agent-annotater-controller", Syncer: &annotaterController{ - agentPodConfig: agentPodConfig, - k8sClient: k8sClient, - kubeSystemPodInformer: kubeSystemPodInformer, - agentPodInformer: agentPodInformer, + agentPodConfig: agentPodConfig, + credentialIssuerConfigLocationConfig: credentialIssuerConfigLocationConfig, + clock: clock, + k8sClient: k8sClient, + pinnipedAPIClient: pinnipedAPIClient, + kubeSystemPodInformer: kubeSystemPodInformer, + agentPodInformer: agentPodInformer, }, }, withInformer( @@ -108,8 +121,21 @@ func (c *annotaterController) Sync(ctx controllerlib.Context) error { certPath, keyPath, ); err != nil { - // TODO Failed, so update the CIC status? - return fmt.Errorf("cannot update agent pod: %w", err) + err = fmt.Errorf("cannot update agent pod: %w", err) + strategyResultUpdateErr := createOrUpdateCredentialIssuerConfig( + ctx.Context, + *c.credentialIssuerConfigLocationConfig, + c.clock, + c.pinnipedAPIClient, + err, + ) + if strategyResultUpdateErr != nil { + // If the CIC update fails, then we probably want to try again. This controller will get + // called again because of the pod create failure, so just try the CIC update again then. + klog.ErrorS(strategyResultUpdateErr, "could not create or update CredentialIssuerConfig") + } + + return err } } @@ -124,7 +150,7 @@ func (c *annotaterController) maybeUpdateAgentPod( keyPath string, ) error { return retry.RetryOnConflict(retry.DefaultRetry, func() error { - agentPod, err := c.agentPodInformer.Lister().Pods(namespace).Get(name) + agentPod, err := c.k8sClient.CoreV1().Pods(namespace).Get(ctx, name, metav1.GetOptions{}) if err != nil { return err } diff --git a/internal/controller/kubecertagent/annotater_test.go b/internal/controller/kubecertagent/annotater_test.go index 793c5aa0..6adba6ae 100644 --- a/internal/controller/kubecertagent/annotater_test.go +++ b/internal/controller/kubecertagent/annotater_test.go @@ -5,6 +5,7 @@ package kubecertagent import ( "context" + "errors" "testing" "time" @@ -12,12 +13,17 @@ import ( "github.com/sclevine/spec/report" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/clock" kubeinformers "k8s.io/client-go/informers" corev1informers "k8s.io/client-go/informers/core/v1" kubernetesfake "k8s.io/client-go/kubernetes/fake" coretesting "k8s.io/client-go/testing" + configv1alpha1 "go.pinniped.dev/generated/1.19/apis/config/v1alpha1" + pinnipedfake "go.pinniped.dev/generated/1.19/client/clientset/versioned/fake" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/testutil" ) @@ -35,7 +41,10 @@ func TestAnnotaterControllerFilter(t *testing.T) { ) { _ = NewAnnotaterController( agentPodConfig, + nil, // credentialIssuerConfigLocationConfig, shouldn't matter + nil, // clock, shouldn't matter nil, // k8sClient, shouldn't matter + nil, // pinnipedClient, shouldn't matter kubeSystemPodInformer, agentPodInformer, observableWithInformerOption.WithInformer, @@ -50,6 +59,8 @@ func TestAnnotaterControllerSync(t *testing.T) { const agentPodNamespace = "agent-pod-namespace" const defaultKubeControllerManagerClusterSigningCertFileFlagValue = "/etc/kubernetes/ca/ca.pem" const defaultKubeControllerManagerClusterSigningKeyFileFlagValue = "/etc/kubernetes/ca/ca.key" + const credentialIssuerConfigNamespaceName = "cic-namespace-name" + const credentialIssuerConfigResourceName = "cic-resource-name" const ( certPath = "some-cert-path" @@ -67,11 +78,14 @@ func TestAnnotaterControllerSync(t *testing.T) { var kubeSystemInformers kubeinformers.SharedInformerFactory var agentInformerClient *kubernetesfake.Clientset var agentInformers kubeinformers.SharedInformerFactory + var pinnipedAPIClient *pinnipedfake.Clientset var timeoutContext context.Context var timeoutContextCancel context.CancelFunc var syncContext *controllerlib.Context var controllerManagerPod, agentPod *corev1.Pod var podsGVR schema.GroupVersionResource + var credentialIssuerConfigGVR schema.GroupVersionResource + var frozenNow time.Time // Defer starting the informers until the last possible moment so that the // nested Before's can keep adding things to the informer caches. @@ -83,7 +97,13 @@ func TestAnnotaterControllerSync(t *testing.T) { ContainerImage: "some-agent-image", PodNamePrefix: "some-agent-name-", }, + &CredentialIssuerConfigLocationConfig{ + Namespace: credentialIssuerConfigNamespaceName, + Name: credentialIssuerConfigResourceName, + }, + clock.NewFakeClock(frozenNow), kubeAPIClient, + pinnipedAPIClient, kubeSystemInformers.Core().V1().Pods(), agentInformers.Core().V1().Pods(), controllerlib.WithInformer, @@ -116,6 +136,8 @@ func TestAnnotaterControllerSync(t *testing.T) { agentInformerClient = kubernetesfake.NewSimpleClientset() agentInformers = kubeinformers.NewSharedInformerFactory(agentInformerClient, 0) + pinnipedAPIClient = pinnipedfake.NewSimpleClientset() + timeoutContext, timeoutContextCancel = context.WithTimeout(context.Background(), time.Second*3) controllerManagerPod, agentPod = exampleControllerManagerAndAgentPods( @@ -128,6 +150,14 @@ func TestAnnotaterControllerSync(t *testing.T) { Resource: "pods", } + credentialIssuerConfigGVR = schema.GroupVersionResource{ + Group: configv1alpha1.GroupName, + Version: configv1alpha1.SchemeGroupVersion.Version, + Resource: "credentialissuerconfigs", + } + + frozenNow = time.Date(2020, time.September, 23, 7, 42, 0, 0, time.Local) + // Add a pod into the test that doesn't matter to make sure we don't accidentally trigger any // logic on this thing. ignorablePod := corev1.Pod{} @@ -163,6 +193,11 @@ func TestAnnotaterControllerSync(t *testing.T) { r.Equal( []coretesting.Action{ + coretesting.NewGetAction( + podsGVR, + agentPodNamespace, + updatedAgentPod.Name, + ), coretesting.NewUpdateAction( podsGVR, agentPodNamespace, @@ -172,6 +207,144 @@ func TestAnnotaterControllerSync(t *testing.T) { kubeAPIClient.Actions(), ) }) + + when("updating the agent pod fails", func() { + it.Before(func() { + kubeAPIClient.PrependReactor( + "update", + "pods", + func(_ coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some update error") + }, + ) + }) + + it("returns the error", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.EqualError(err, "cannot update agent pod: some update error") + }) + + when("there is already a CredentialIssuerConfig", func() { + var initialCredentialIssuerConfig *configv1alpha1.CredentialIssuerConfig + + it.Before(func() { + initialCredentialIssuerConfig = &configv1alpha1.CredentialIssuerConfig{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: credentialIssuerConfigResourceName, + Namespace: credentialIssuerConfigNamespaceName, + }, + Status: configv1alpha1.CredentialIssuerConfigStatus{ + Strategies: []configv1alpha1.CredentialIssuerConfigStrategy{}, + KubeConfigInfo: &configv1alpha1.CredentialIssuerConfigKubeConfigInfo{ + Server: "some-server", + CertificateAuthorityData: "some-ca-value", + }, + }, + } + r.NoError(pinnipedAPIClient.Tracker().Add(initialCredentialIssuerConfig)) + }) + + it("updates the CredentialIssuerConfig status with the error", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + + expectedCredentialIssuerConfig := initialCredentialIssuerConfig.DeepCopy() + expectedCredentialIssuerConfig.Status.Strategies = []configv1alpha1.CredentialIssuerConfigStrategy{ + { + Type: configv1alpha1.KubeClusterSigningCertificateStrategyType, + Status: configv1alpha1.ErrorStrategyStatus, + Reason: configv1alpha1.CouldNotFetchKeyStrategyReason, + Message: "cannot update agent pod: some update error", + LastUpdateTime: metav1.NewTime(frozenNow), + }, + } + expectedGetAction := coretesting.NewGetAction( + credentialIssuerConfigGVR, + credentialIssuerConfigNamespaceName, + credentialIssuerConfigResourceName, + ) + expectedUpdateAction := coretesting.NewUpdateAction( + credentialIssuerConfigGVR, + credentialIssuerConfigNamespaceName, + expectedCredentialIssuerConfig, + ) + + r.EqualError(err, "cannot update agent pod: some update error") + r.Equal( + []coretesting.Action{ + expectedGetAction, + expectedUpdateAction, + }, + pinnipedAPIClient.Actions(), + ) + }) + + when("updating the CredentialIssuerConfig fails", func() { + it.Before(func() { + pinnipedAPIClient.PrependReactor( + "update", + "credentialissuerconfigs", + func(_ coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some update error") + }, + ) + }) + + it.Focus("returns the original pod update error so the controller gets scheduled again", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.EqualError(err, "cannot update agent pod: some update error") + }) + }) + }) + + when("there is not already a CredentialIssuerConfig", func() { + it("creates the CredentialIssuerConfig status with the error", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + + expectedCredentialIssuerConfig := &configv1alpha1.CredentialIssuerConfig{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: credentialIssuerConfigResourceName, + Namespace: credentialIssuerConfigNamespaceName, + }, + Status: configv1alpha1.CredentialIssuerConfigStatus{ + Strategies: []configv1alpha1.CredentialIssuerConfigStrategy{ + { + Type: configv1alpha1.KubeClusterSigningCertificateStrategyType, + Status: configv1alpha1.ErrorStrategyStatus, + Reason: configv1alpha1.CouldNotFetchKeyStrategyReason, + Message: "cannot update agent pod: some update error", + LastUpdateTime: metav1.NewTime(frozenNow), + }, + }, + }, + } + expectedGetAction := coretesting.NewGetAction( + credentialIssuerConfigGVR, + credentialIssuerConfigNamespaceName, + credentialIssuerConfigResourceName, + ) + expectedCreateAction := coretesting.NewCreateAction( + credentialIssuerConfigGVR, + credentialIssuerConfigNamespaceName, + expectedCredentialIssuerConfig, + ) + + r.EqualError(err, "cannot update agent pod: some update error") + r.Equal( + []coretesting.Action{ + expectedGetAction, + expectedCreateAction, + }, + pinnipedAPIClient.Actions(), + ) + }) + }) + }) }) when("there is a controller manager pod with CLI flag values separated by spaces", func() { @@ -195,6 +368,11 @@ func TestAnnotaterControllerSync(t *testing.T) { r.Equal( []coretesting.Action{ + coretesting.NewGetAction( + podsGVR, + agentPodNamespace, + updatedAgentPod.Name, + ), coretesting.NewUpdateAction( podsGVR, agentPodNamespace, @@ -225,6 +403,11 @@ func TestAnnotaterControllerSync(t *testing.T) { r.Equal( []coretesting.Action{ + coretesting.NewGetAction( + podsGVR, + agentPodNamespace, + updatedAgentPod.Name, + ), coretesting.NewUpdateAction( podsGVR, agentPodNamespace, @@ -257,6 +440,11 @@ func TestAnnotaterControllerSync(t *testing.T) { r.Equal( []coretesting.Action{ + coretesting.NewGetAction( + podsGVR, + agentPodNamespace, + updatedAgentPod.Name, + ), coretesting.NewUpdateAction( podsGVR, agentPodNamespace, @@ -289,6 +477,11 @@ func TestAnnotaterControllerSync(t *testing.T) { r.Equal( []coretesting.Action{ + coretesting.NewGetAction( + podsGVR, + agentPodNamespace, + updatedAgentPod.Name, + ), coretesting.NewUpdateAction( podsGVR, agentPodNamespace, @@ -321,6 +514,11 @@ func TestAnnotaterControllerSync(t *testing.T) { r.Equal( []coretesting.Action{ + coretesting.NewGetAction( + podsGVR, + agentPodNamespace, + updatedAgentPod.Name, + ), coretesting.NewUpdateAction( podsGVR, agentPodNamespace, @@ -415,6 +613,11 @@ func TestAnnotaterControllerSync(t *testing.T) { updatedAgentPod.Annotations[certPathAnnotation] = certPath r.Equal( []coretesting.Action{ + coretesting.NewGetAction( + podsGVR, + agentPodNamespace, + updatedAgentPod.Name, + ), coretesting.NewUpdateAction( podsGVR, agentPodNamespace, @@ -449,6 +652,11 @@ func TestAnnotaterControllerSync(t *testing.T) { updatedAgentPod.Annotations[keyPathAnnotation] = keyPath r.Equal( []coretesting.Action{ + coretesting.NewGetAction( + podsGVR, + agentPodNamespace, + updatedAgentPod.Name, + ), coretesting.NewUpdateAction( podsGVR, agentPodNamespace, diff --git a/internal/controller/kubecertagent/creater.go b/internal/controller/kubecertagent/creater.go index 9bdf5333..8354d6fd 100644 --- a/internal/controller/kubecertagent/creater.go +++ b/internal/controller/kubecertagent/creater.go @@ -9,10 +9,13 @@ import ( 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" "k8s.io/klog/v2" + pinnipedclientset "go.pinniped.dev/generated/1.19/client/clientset/versioned" + "go.pinniped.dev/internal/constable" pinnipedcontroller "go.pinniped.dev/internal/controller" "go.pinniped.dev/internal/controllerlib" ) @@ -20,7 +23,9 @@ import ( type createrController struct { agentPodConfig *AgentPodConfig credentialIssuerConfigLocationConfig *CredentialIssuerConfigLocationConfig + clock clock.Clock k8sClient kubernetes.Interface + pinnipedAPIClient pinnipedclientset.Interface kubeSystemPodInformer corev1informers.PodInformer agentPodInformer corev1informers.PodInformer } @@ -28,11 +33,14 @@ type createrController struct { // NewCreaterController returns a controller that creates new kube-cert-agent pods for every known // kube-controller-manager pod. // -// This controller only uses the Template field of the provided agentInfo. +// It also is tasked with updating the CredentialIssuerConfig, located via the provided +// credentialIssuerConfigLocationConfig, with any errors that it encounters. func NewCreaterController( agentPodConfig *AgentPodConfig, credentialIssuerConfigLocationConfig *CredentialIssuerConfigLocationConfig, + clock clock.Clock, k8sClient kubernetes.Interface, + pinnipedAPIClient pinnipedclientset.Interface, kubeSystemPodInformer corev1informers.PodInformer, agentPodInformer corev1informers.PodInformer, withInformer pinnipedcontroller.WithInformerOptionFunc, @@ -44,7 +52,9 @@ func NewCreaterController( Syncer: &createrController{ agentPodConfig: agentPodConfig, credentialIssuerConfigLocationConfig: credentialIssuerConfigLocationConfig, + clock: clock, k8sClient: k8sClient, + pinnipedAPIClient: pinnipedAPIClient, kubeSystemPodInformer: kubeSystemPodInformer, agentPodInformer: agentPodInformer, }, @@ -74,7 +84,17 @@ func (c *createrController) Sync(ctx controllerlib.Context) error { return fmt.Errorf("informer cannot list controller manager pods: %w", err) } - // TODO if controllerManagerPods is empty then update the CIC status with an error message saying that they couldn't be found + if len(controllerManagerPods) == 0 { + // If there are no controller manager pods, we alert the user that we can't find the keypair via + // the CredentialIssuerConfig. + return createOrUpdateCredentialIssuerConfig( + ctx.Context, + *c.credentialIssuerConfigLocationConfig, + c.clock, + c.pinnipedAPIClient, + constable.Error("Controller manager pod(s) could not be found"), + ) + } for _, controllerManagerPod := range controllerManagerPods { agentPod, err := findAgentPodForSpecificControllerManagerPod( @@ -100,8 +120,21 @@ func (c *createrController) Sync(ctx controllerlib.Context) error { Pods(c.agentPodConfig.Namespace). Create(ctx.Context, agentPod, metav1.CreateOptions{}) if err != nil { - // TODO if agent pods fail to create then update the CIC status with an error saying that they couldn't create - return fmt.Errorf("cannot create agent pod: %w", err) + err = fmt.Errorf("cannot create agent pod: %w", err) + strategyResultUpdateErr := createOrUpdateCredentialIssuerConfig( + ctx.Context, + *c.credentialIssuerConfigLocationConfig, + c.clock, + c.pinnipedAPIClient, + err, + ) + if strategyResultUpdateErr != nil { + // If the CIC update fails, then we probably want to try again. This controller will get + // called again because of the pod create failure, so just try the CIC update again then. + klog.ErrorS(strategyResultUpdateErr, "could not create or update CredentialIssuerConfig") + } + + return err } } diff --git a/internal/controller/kubecertagent/creater_test.go b/internal/controller/kubecertagent/creater_test.go index 535363b7..d3d809ea 100644 --- a/internal/controller/kubecertagent/creater_test.go +++ b/internal/controller/kubecertagent/creater_test.go @@ -5,6 +5,7 @@ package kubecertagent import ( "context" + "errors" "testing" "time" @@ -12,12 +13,17 @@ import ( "github.com/sclevine/spec/report" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/clock" kubeinformers "k8s.io/client-go/informers" corev1informers "k8s.io/client-go/informers/core/v1" kubernetesfake "k8s.io/client-go/kubernetes/fake" coretesting "k8s.io/client-go/testing" + configv1alpha1 "go.pinniped.dev/generated/1.19/apis/config/v1alpha1" + pinnipedfake "go.pinniped.dev/generated/1.19/client/clientset/versioned/fake" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/testutil" ) @@ -36,7 +42,9 @@ func TestCreaterControllerFilter(t *testing.T) { _ = NewCreaterController( agentPodConfig, credentialIssuerConfigLocationConfig, + nil, // clock, shound't matter nil, // k8sClient, shouldn't matter + nil, // pinnipedAPIClient, shouldn't matter kubeSystemPodInformer, agentPodInformer, observableWithInformerOption.WithInformer, @@ -49,6 +57,8 @@ func TestCreaterControllerSync(t *testing.T) { spec.Run(t, "CreaterControllerSync", func(t *testing.T, when spec.G, it spec.S) { const kubeSystemNamespace = "kube-system" const agentPodNamespace = "agent-pod-namespace" + const credentialIssuerConfigNamespaceName = "cic-namespace-name" + const credentialIssuerConfigResourceName = "cic-resource-name" var r *require.Assertions @@ -58,11 +68,14 @@ func TestCreaterControllerSync(t *testing.T) { var kubeSystemInformers kubeinformers.SharedInformerFactory var agentInformerClient *kubernetesfake.Clientset var agentInformers kubeinformers.SharedInformerFactory + var pinnipedAPIClient *pinnipedfake.Clientset var timeoutContext context.Context var timeoutContextCancel context.CancelFunc var syncContext *controllerlib.Context var controllerManagerPod, agentPod *corev1.Pod var podsGVR schema.GroupVersionResource + var credentialIssuerConfigGVR schema.GroupVersionResource + var frozenNow time.Time // Defer starting the informers until the last possible moment so that the // nested Before's can keep adding things to the informer caches. @@ -75,10 +88,12 @@ func TestCreaterControllerSync(t *testing.T) { PodNamePrefix: "some-agent-name-", }, &CredentialIssuerConfigLocationConfig{ - Namespace: "not used yet", - Name: "not used yet", + Namespace: credentialIssuerConfigNamespaceName, + Name: credentialIssuerConfigResourceName, }, + clock.NewFakeClock(frozenNow), kubeAPIClient, + pinnipedAPIClient, kubeSystemInformers.Core().V1().Pods(), agentInformers.Core().V1().Pods(), controllerlib.WithInformer, @@ -111,6 +126,8 @@ func TestCreaterControllerSync(t *testing.T) { agentInformerClient = kubernetesfake.NewSimpleClientset() agentInformers = kubeinformers.NewSharedInformerFactory(agentInformerClient, 0) + pinnipedAPIClient = pinnipedfake.NewSimpleClientset() + timeoutContext, timeoutContextCancel = context.WithTimeout(context.Background(), time.Second*3) controllerManagerPod, agentPod = exampleControllerManagerAndAgentPods( @@ -123,6 +140,14 @@ func TestCreaterControllerSync(t *testing.T) { Resource: "pods", } + credentialIssuerConfigGVR = schema.GroupVersionResource{ + Group: configv1alpha1.GroupName, + Version: configv1alpha1.SchemeGroupVersion.Version, + Resource: "credentialissuerconfigs", + } + + frozenNow = time.Date(2020, time.September, 23, 7, 42, 0, 0, time.Local) + // Add a pod into the test that doesn't matter to make sure we don't accidentally trigger any // logic on this thing. ignorablePod := corev1.Pod{} @@ -213,19 +238,313 @@ func TestCreaterControllerSync(t *testing.T) { kubeAPIClient.Actions(), ) }) + + when("creating the matching agent pod fails", func() { + it.Before(func() { + kubeAPIClient.PrependReactor( + "create", + "pods", + func(_ coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some create error") + }, + ) + }) + + when("there is already a CredentialIssuerConfig", func() { + var initialCredentialIssuerConfig *configv1alpha1.CredentialIssuerConfig + + it.Before(func() { + initialCredentialIssuerConfig = &configv1alpha1.CredentialIssuerConfig{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: credentialIssuerConfigResourceName, + Namespace: credentialIssuerConfigNamespaceName, + }, + Status: configv1alpha1.CredentialIssuerConfigStatus{ + Strategies: []configv1alpha1.CredentialIssuerConfigStrategy{}, + KubeConfigInfo: &configv1alpha1.CredentialIssuerConfigKubeConfigInfo{ + Server: "some-server", + CertificateAuthorityData: "some-ca-value", + }, + }, + } + r.NoError(pinnipedAPIClient.Tracker().Add(initialCredentialIssuerConfig)) + }) + + it("updates the CredentialIssuerConfig status saying that controller manager pods couldn't be found", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + + expectedCredentialIssuerConfig := initialCredentialIssuerConfig.DeepCopy() + expectedCredentialIssuerConfig.Status.Strategies = []configv1alpha1.CredentialIssuerConfigStrategy{ + { + Type: configv1alpha1.KubeClusterSigningCertificateStrategyType, + Status: configv1alpha1.ErrorStrategyStatus, + Reason: configv1alpha1.CouldNotFetchKeyStrategyReason, + Message: "cannot create agent pod: some create error", + LastUpdateTime: metav1.NewTime(frozenNow), + }, + } + expectedGetAction := coretesting.NewGetAction( + credentialIssuerConfigGVR, + credentialIssuerConfigNamespaceName, + credentialIssuerConfigResourceName, + ) + expectedUpdateAction := coretesting.NewUpdateAction( + credentialIssuerConfigGVR, + credentialIssuerConfigNamespaceName, + expectedCredentialIssuerConfig, + ) + + r.EqualError(err, "cannot create agent pod: some create error") + r.Equal( + []coretesting.Action{ + expectedGetAction, + expectedUpdateAction, + }, + pinnipedAPIClient.Actions(), + ) + }) + + when("the CredentialIssuerConfig operation fails", func() { + it.Before(func() { + pinnipedAPIClient.PrependReactor( + "update", + "credentialissuerconfigs", + func(_ coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some update error") + }, + ) + + it("still returns the pod create error, since the controller will get rescheduled", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.EqualError(err, "cannot create agent pod: some create error") + }) + }) + }) + }) + + when("there is not already a CredentialIssuerConfig", func() { + it("returns an error and updates the CredentialIssuerConfig status", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + + expectedCredentialIssuerConfig := &configv1alpha1.CredentialIssuerConfig{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: credentialIssuerConfigResourceName, + Namespace: credentialIssuerConfigNamespaceName, + }, + Status: configv1alpha1.CredentialIssuerConfigStatus{ + Strategies: []configv1alpha1.CredentialIssuerConfigStrategy{ + { + Type: configv1alpha1.KubeClusterSigningCertificateStrategyType, + Status: configv1alpha1.ErrorStrategyStatus, + Reason: configv1alpha1.CouldNotFetchKeyStrategyReason, + Message: "cannot create agent pod: some create error", + LastUpdateTime: metav1.NewTime(frozenNow), + }, + }, + }, + } + expectedGetAction := coretesting.NewGetAction( + credentialIssuerConfigGVR, + credentialIssuerConfigNamespaceName, + credentialIssuerConfigResourceName, + ) + expectedCreateAction := coretesting.NewCreateAction( + credentialIssuerConfigGVR, + credentialIssuerConfigNamespaceName, + expectedCredentialIssuerConfig, + ) + + r.EqualError(err, "cannot create agent pod: some create error") + r.Equal( + []coretesting.Action{ + expectedGetAction, + expectedCreateAction, + }, + pinnipedAPIClient.Actions(), + ) + }) + }) + }) }) }) when("there is no controller manager pod", func() { - it("does nothing; the deleter controller will cleanup any leftover agent pods", func() { - startInformersAndController() - err := controllerlib.TestSync(t, subject, *syncContext) + when("there is already a CredentialIssuerConfig", func() { + var initialCredentialIssuerConfig *configv1alpha1.CredentialIssuerConfig - r.NoError(err) - r.Equal( - []coretesting.Action{}, - kubeAPIClient.Actions(), - ) + it.Before(func() { + initialCredentialIssuerConfig = &configv1alpha1.CredentialIssuerConfig{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: credentialIssuerConfigResourceName, + Namespace: credentialIssuerConfigNamespaceName, + }, + Status: configv1alpha1.CredentialIssuerConfigStatus{ + Strategies: []configv1alpha1.CredentialIssuerConfigStrategy{}, + KubeConfigInfo: &configv1alpha1.CredentialIssuerConfigKubeConfigInfo{ + Server: "some-server", + CertificateAuthorityData: "some-ca-value", + }, + }, + } + r.NoError(pinnipedAPIClient.Tracker().Add(initialCredentialIssuerConfig)) + }) + + it("updates the CredentialIssuerConfig status saying that controller manager pods couldn't be found", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + expectedCredentialIssuerConfig := initialCredentialIssuerConfig.DeepCopy() + expectedCredentialIssuerConfig.Status.Strategies = []configv1alpha1.CredentialIssuerConfigStrategy{ + { + Type: configv1alpha1.KubeClusterSigningCertificateStrategyType, + Status: configv1alpha1.ErrorStrategyStatus, + Reason: configv1alpha1.CouldNotFetchKeyStrategyReason, + Message: "Controller manager pod(s) could not be found", + LastUpdateTime: metav1.NewTime(frozenNow), + }, + } + expectedGetAction := coretesting.NewGetAction( + credentialIssuerConfigGVR, + credentialIssuerConfigNamespaceName, + credentialIssuerConfigResourceName, + ) + expectedUpdateAction := coretesting.NewUpdateAction( + credentialIssuerConfigGVR, + credentialIssuerConfigNamespaceName, + expectedCredentialIssuerConfig, + ) + + r.Equal( + []coretesting.Action{ + expectedGetAction, + expectedUpdateAction, + }, + pinnipedAPIClient.Actions(), + ) + }) + + when("when updating the CredentialIssuerConfig fails", func() { + it.Before(func() { + pinnipedAPIClient.PrependReactor( + "update", + "credentialissuerconfigs", + func(_ coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some update error") + }, + ) + }) + + it("returns an error", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.EqualError(err, "could not create or update credentialissuerconfig: some update error") + }) + }) + + when("when getting the CredentialIssuerConfig fails", func() { + it.Before(func() { + pinnipedAPIClient.PrependReactor( + "get", + "credentialissuerconfigs", + func(_ coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some get error") + }, + ) + }) + + it("returns an error", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.EqualError(err, "could not create or update credentialissuerconfig: get failed: some get error") + }) + }) + }) + + when("there is not already a CredentialIssuerConfig", func() { + it("creates the CredentialIssuerConfig status saying that controller manager pods couldn't be found", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + + expectedCredentialIssuerConfig := &configv1alpha1.CredentialIssuerConfig{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: credentialIssuerConfigResourceName, + Namespace: credentialIssuerConfigNamespaceName, + }, + Status: configv1alpha1.CredentialIssuerConfigStatus{ + Strategies: []configv1alpha1.CredentialIssuerConfigStrategy{ + { + Type: configv1alpha1.KubeClusterSigningCertificateStrategyType, + Status: configv1alpha1.ErrorStrategyStatus, + Reason: configv1alpha1.CouldNotFetchKeyStrategyReason, + Message: "Controller manager pod(s) could not be found", + LastUpdateTime: metav1.NewTime(frozenNow), + }, + }, + }, + } + expectedGetAction := coretesting.NewGetAction( + credentialIssuerConfigGVR, + credentialIssuerConfigNamespaceName, + credentialIssuerConfigResourceName, + ) + expectedCreateAction := coretesting.NewCreateAction( + credentialIssuerConfigGVR, + credentialIssuerConfigNamespaceName, + expectedCredentialIssuerConfig, + ) + + r.NoError(err) + r.Equal( + []coretesting.Action{ + expectedGetAction, + expectedCreateAction, + }, + pinnipedAPIClient.Actions(), + ) + }) + + when("when creating the CredentialIssuerConfig fails", func() { + it.Before(func() { + pinnipedAPIClient.PrependReactor( + "create", + "credentialissuerconfigs", + func(_ coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some create error") + }, + ) + }) + + it("returns an error", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.EqualError(err, "could not create or update credentialissuerconfig: create failed: some create error") + }) + }) + + when("when getting the CredentialIssuerConfig fails", func() { + it.Before(func() { + pinnipedAPIClient.PrependReactor( + "get", + "credentialissuerconfigs", + func(_ coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some get error") + }, + ) + }) + + it("returns an error", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.EqualError(err, "could not create or update credentialissuerconfig: get failed: some get error") + }) + }) }) }) }, spec.Parallel(), spec.Report(report.Terminal{})) diff --git a/internal/controller/kubecertagent/execer.go b/internal/controller/kubecertagent/execer.go index 45f9aee3..6b26994e 100644 --- a/internal/controller/kubecertagent/execer.go +++ b/internal/controller/kubecertagent/execer.go @@ -8,15 +8,12 @@ import ( v1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/clock" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/klog/v2" - configv1alpha1 "go.pinniped.dev/generated/1.19/apis/config/v1alpha1" pinnipedclientset "go.pinniped.dev/generated/1.19/client/clientset/versioned" pinnipedcontroller "go.pinniped.dev/internal/controller" - "go.pinniped.dev/internal/controller/issuerconfig" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/dynamiccert" ) @@ -87,21 +84,39 @@ func (c *execerController) Sync(ctx controllerlib.Context) error { certPEM, err := c.podCommandExecutor.Exec(agentPod.Namespace, agentPod.Name, "cat", certPath) if err != nil { - strategyResultUpdateErr := c.createOrUpdateCredentialIssuerConfig(ctx, c.strategyError(err)) + strategyResultUpdateErr := createOrUpdateCredentialIssuerConfig( + ctx.Context, + *c.credentialIssuerConfigLocationConfig, + c.clock, + c.pinnipedAPIClient, + err, + ) klog.ErrorS(strategyResultUpdateErr, "could not create or update CredentialIssuerConfig with strategy success") return err } keyPEM, err := c.podCommandExecutor.Exec(agentPod.Namespace, agentPod.Name, "cat", keyPath) if err != nil { - strategyResultUpdateErr := c.createOrUpdateCredentialIssuerConfig(ctx, c.strategyError(err)) + strategyResultUpdateErr := createOrUpdateCredentialIssuerConfig( + ctx.Context, + *c.credentialIssuerConfigLocationConfig, + c.clock, + c.pinnipedAPIClient, + err, + ) klog.ErrorS(strategyResultUpdateErr, "could not create or update CredentialIssuerConfig with strategy success") return err } c.dynamicCertProvider.Set([]byte(certPEM), []byte(keyPEM)) - err = c.createOrUpdateCredentialIssuerConfig(ctx, c.strategySuccess()) + err = createOrUpdateCredentialIssuerConfig( + ctx.Context, + *c.credentialIssuerConfigLocationConfig, + c.clock, + c.pinnipedAPIClient, + nil, // nil error = success! yay! + ) if err != nil { return err } @@ -109,38 +124,6 @@ func (c *execerController) Sync(ctx controllerlib.Context) error { return nil } -func (c *execerController) createOrUpdateCredentialIssuerConfig(ctx controllerlib.Context, strategyResult configv1alpha1.CredentialIssuerConfigStrategy) error { - return issuerconfig.CreateOrUpdateCredentialIssuerConfig( - ctx.Context, - c.credentialIssuerConfigLocationConfig.Namespace, - c.credentialIssuerConfigLocationConfig.Name, - c.pinnipedAPIClient, - func(configToUpdate *configv1alpha1.CredentialIssuerConfig) { - configToUpdate.Status.Strategies = []configv1alpha1.CredentialIssuerConfigStrategy{strategyResult} - }, - ) -} - -func (c *execerController) strategySuccess() configv1alpha1.CredentialIssuerConfigStrategy { - return configv1alpha1.CredentialIssuerConfigStrategy{ - Type: configv1alpha1.KubeClusterSigningCertificateStrategyType, - Status: configv1alpha1.SuccessStrategyStatus, - Reason: configv1alpha1.FetchedKeyStrategyReason, - Message: "Key was fetched successfully", - LastUpdateTime: metav1.NewTime(c.clock.Now()), - } -} - -func (c *execerController) strategyError(err error) configv1alpha1.CredentialIssuerConfigStrategy { - return configv1alpha1.CredentialIssuerConfigStrategy{ - Type: configv1alpha1.KubeClusterSigningCertificateStrategyType, - Status: configv1alpha1.ErrorStrategyStatus, - Reason: configv1alpha1.CouldNotFetchKeyStrategyReason, - Message: err.Error(), - LastUpdateTime: metav1.NewTime(c.clock.Now()), - } -} - func (c *execerController) getKeypairFilePaths(pod *v1.Pod) (string, string) { annotations := pod.Annotations if annotations == nil { diff --git a/internal/controller/kubecertagent/kubecertagent.go b/internal/controller/kubecertagent/kubecertagent.go index 9ef72b6e..f9ee4b87 100644 --- a/internal/controller/kubecertagent/kubecertagent.go +++ b/internal/controller/kubecertagent/kubecertagent.go @@ -10,18 +10,23 @@ package kubecertagent import ( + "context" "encoding/hex" "fmt" "hash/fnv" - "k8s.io/apimachinery/pkg/api/resource" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" 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/util/clock" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/klog/v2" + + configv1alpha1 "go.pinniped.dev/generated/1.19/apis/config/v1alpha1" + pinnipedclientset "go.pinniped.dev/generated/1.19/client/clientset/versioned" + "go.pinniped.dev/internal/controller/issuerconfig" ) const ( @@ -233,6 +238,52 @@ func findControllerManagerPodForSpecificAgentPod( return maybeControllerManagerPod, nil } +func createOrUpdateCredentialIssuerConfig( + ctx context.Context, + cicConfig CredentialIssuerConfigLocationConfig, + clock clock.Clock, + pinnipedAPIClient pinnipedclientset.Interface, + err error, +) error { + return issuerconfig.CreateOrUpdateCredentialIssuerConfig( + ctx, + cicConfig.Namespace, + cicConfig.Name, + pinnipedAPIClient, + func(configToUpdate *configv1alpha1.CredentialIssuerConfig) { + var strategyResult configv1alpha1.CredentialIssuerConfigStrategy + if err == nil { + strategyResult = strategySuccess(clock) + } else { + strategyResult = strategyError(clock, err) + } + configToUpdate.Status.Strategies = []configv1alpha1.CredentialIssuerConfigStrategy{ + strategyResult, + } + }, + ) +} + +func strategySuccess(clock clock.Clock) configv1alpha1.CredentialIssuerConfigStrategy { + return configv1alpha1.CredentialIssuerConfigStrategy{ + Type: configv1alpha1.KubeClusterSigningCertificateStrategyType, + Status: configv1alpha1.SuccessStrategyStatus, + Reason: configv1alpha1.FetchedKeyStrategyReason, + Message: "Key was fetched successfully", + LastUpdateTime: metav1.NewTime(clock.Now()), + } +} + +func strategyError(clock clock.Clock, err error) configv1alpha1.CredentialIssuerConfigStrategy { + return configv1alpha1.CredentialIssuerConfigStrategy{ + Type: configv1alpha1.KubeClusterSigningCertificateStrategyType, + Status: configv1alpha1.ErrorStrategyStatus, + Reason: configv1alpha1.CouldNotFetchKeyStrategyReason, + Message: err.Error(), + LastUpdateTime: metav1.NewTime(clock.Now()), + } +} + func boolPtr(b bool) *bool { return &b } func hash(controllerManagerPod *corev1.Pod) string { diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index dfb1d934..5f0b955d 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/internal/controllermanager/prepare_controllers.go @@ -174,7 +174,9 @@ func PrepareControllers(c *Config) (func(ctx context.Context), error) { kubecertagent.NewCreaterController( agentPodConfig, credentialIssuerConfigLocationConfig, + clock.RealClock{}, k8sClient, + pinnipedClient, informers.kubeSystemNamespaceK8s.Core().V1().Pods(), informers.installationNamespaceK8s.Core().V1().Pods(), controllerlib.WithInformer, @@ -184,7 +186,10 @@ func PrepareControllers(c *Config) (func(ctx context.Context), error) { WithController( kubecertagent.NewAnnotaterController( agentPodConfig, + credentialIssuerConfigLocationConfig, + clock.RealClock{}, k8sClient, + pinnipedClient, informers.kubeSystemNamespaceK8s.Core().V1().Pods(), informers.installationNamespaceK8s.Core().V1().Pods(), controllerlib.WithInformer,