From 69137fb6b983117dba913d3636f29d9b33a6e39a Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 24 Sep 2020 09:19:57 -0700 Subject: [PATCH] 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 --- ...eate_or_update_credential_issuer_config.go | 71 ++++++---------- .../kube_config_info_publisher.go | 32 +------ .../kube_config_info_publisher_test.go | 85 +++---------------- .../controllermanager/prepare_controllers.go | 1 - .../credentialissuerconfig_test.go | 60 ++----------- 5 files changed, 52 insertions(+), 197 deletions(-) diff --git a/internal/controller/issuerconfig/create_or_update_credential_issuer_config.go b/internal/controller/issuerconfig/create_or_update_credential_issuer_config.go index acaaa3d21..f420b850b 100644 --- a/internal/controller/issuerconfig/create_or_update_credential_issuer_config.go +++ b/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,14 +35,31 @@ func CreateOrUpdateCredentialIssuerConfig( return fmt.Errorf("get failed: %w", err) } - return createOrUpdateCredentialIssuerConfig( - ctx, - existingCredentialIssuerConfig, - notFound, - credentialIssuerConfigResourceName, - credentialIssuerConfigNamespace, - pinnipedClient, - applyUpdatesToCredentialIssuerConfigFunc) + credentialIssuerConfigsClient := pinnipedClient.ConfigV1alpha1().CredentialIssuerConfigs(credentialIssuerConfigNamespace) + + if notFound { + // Create it + credentialIssuerConfig := minimalValidCredentialIssuerConfig(credentialIssuerConfigResourceName, credentialIssuerConfigNamespace) + applyUpdatesToCredentialIssuerConfigFunc(credentialIssuerConfig) + + if _, err := credentialIssuerConfigsClient.Create(ctx, credentialIssuerConfig, metav1.CreateOptions{}); err != nil { + return fmt.Errorf("create failed: %w", err) + } + } else { + // Already exists, so check to see if we need to update it + credentialIssuerConfig := existingCredentialIssuerConfig.DeepCopy() + applyUpdatesToCredentialIssuerConfigFunc(credentialIssuerConfig) + + if equality.Semantic.DeepEqual(existingCredentialIssuerConfig, credentialIssuerConfig) { + // Nothing interesting would change as a result of this update, so skip it + return nil + } + + if _, err := credentialIssuerConfigsClient.Update(ctx, credentialIssuerConfig, metav1.UpdateOptions{}); err != nil { + return err + } + } + return nil }) if err != nil { @@ -50,43 +68,6 @@ func CreateOrUpdateCredentialIssuerConfig( 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) - applyUpdatesToCredentialIssuerConfigFunc(credentialIssuerConfig) - - if _, err := credentialIssuerConfigsClient.Create(ctx, credentialIssuerConfig, metav1.CreateOptions{}); err != nil { - return fmt.Errorf("create failed: %w", err) - } - } else { - // Already exists, so check to see if we need to update it - credentialIssuerConfig := existingCredentialIssuerConfig.DeepCopy() - applyUpdatesToCredentialIssuerConfigFunc(credentialIssuerConfig) - - if equality.Semantic.DeepEqual(existingCredentialIssuerConfig, credentialIssuerConfig) { - // Nothing interesting would change as a result of this update, so skip it - return nil - } - - if _, err := credentialIssuerConfigsClient.Update(ctx, credentialIssuerConfig, metav1.UpdateOptions{}); err != nil { - return err - } - } - - return nil -} - func minimalValidCredentialIssuerConfig( credentialIssuerConfigName string, credentialIssuerConfigNamespace string, diff --git a/internal/controller/issuerconfig/kube_config_info_publisher.go b/internal/controller/issuerconfig/kube_config_info_publisher.go index 516c0a8f4..3705a3d09 100644 --- a/internal/controller/issuerconfig/kube_config_info_publisher.go +++ b/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) - - if err != nil { - return fmt.Errorf("could not create or update credentialissuerconfig: %w", err) - } - return nil + updateServerAndCAFunc, + ) } diff --git a/internal/controller/issuerconfig/kube_config_info_publisher_test.go b/internal/controller/issuerconfig/kube_config_info_publisher_test.go index 1ceade66e..f3750c1a4 100644 --- a/internal/controller/issuerconfig/kube_config_info_publisher_test.go +++ b/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, diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index 5f0b955df..18a64d7b3 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/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, diff --git a/test/integration/credentialissuerconfig_test.go b/test/integration/credentialissuerconfig_test.go index 3cde0579f..f07dd81aa 100644 --- a/test/integration/credentialissuerconfig_test.go +++ b/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), - } }