logindiscovery: add tests for conditional update and error cases

- Also add some log lines for better observability of behavior.

Signed-off-by: Andrew Keesler <akeesler@vmware.com>

go.mod

@@ -11,7 +11,7 @@ require (
 	github.com/spf13/cobra v1.0.0
 	github.com/stretchr/testify v1.6.1
 	github.com/suzerain-io/controller-go v0.0.0-20200728175738-b49edda60499
-	github.com/suzerain-io/placeholder-name-api v0.0.0-20200729202220-f1696913d7c9
+	github.com/suzerain-io/placeholder-name-api v0.0.0-20200730131400-4a1da8d7e70b
 	github.com/suzerain-io/placeholder-name-client-go v0.0.0-20200729202601-9b4b6d38494c
 	k8s.io/api v0.19.0-rc.0
 	k8s.io/apimachinery v0.19.0-rc.0

go.sum

@@ -534,6 +534,8 @@ github.com/suzerain-io/controller-go v0.0.0-20200728175738-b49edda60499 h1:2NQMY
 github.com/suzerain-io/controller-go v0.0.0-20200728175738-b49edda60499/go.mod h1:+v9upryFWBJac6KXKlheGHr7e3kqpk1ldH1iIMFopMs=
 github.com/suzerain-io/placeholder-name-api v0.0.0-20200729202220-f1696913d7c9 h1:xnco3XJMrvlwyQJfKoyVPciATvCJ3Y6SY2D8gI2DT2E=
 github.com/suzerain-io/placeholder-name-api v0.0.0-20200729202220-f1696913d7c9/go.mod h1:OuYBJDpMMnvMUoBn+XeMWtHghuYk0cq9bNkNa3T8j/g=
+github.com/suzerain-io/placeholder-name-api v0.0.0-20200730131400-4a1da8d7e70b h1:7Fuizf0c3ffqyHj7X4AvXnYNFJHbSgHKjuDxDsxeQ8A=
+github.com/suzerain-io/placeholder-name-api v0.0.0-20200730131400-4a1da8d7e70b/go.mod h1:OuYBJDpMMnvMUoBn+XeMWtHghuYk0cq9bNkNa3T8j/g=
 github.com/suzerain-io/placeholder-name-client-go v0.0.0-20200729202601-9b4b6d38494c h1:UQVAfjF71g+v2L0ZT8MedKxrdIpx4KrHqvefnmW1AoU=
 github.com/suzerain-io/placeholder-name-client-go v0.0.0-20200729202601-9b4b6d38494c/go.mod h1:J/D+4tWlKpLpTd8Jotb7FnLl1OeT2KrUOP3Wc3ooqis=
 github.com/tdakkota/asciicheck v0.0.0-20200416190851-d7f85be797a2 h1:Xr9gkxfOP0KQWXKNqmwe8vEeSUiUj4Rlee9CMVX2ZUQ=

internal/controller/logindiscovery/publisher.go

@@ -6,11 +6,15 @@ SPDX-License-Identifier: Apache-2.0
 package logindiscovery
 
 import (
+	"context"
 	"encoding/base64"
+	"fmt"
 
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/clientcmd"
+	"k8s.io/klog/v2"
 
 	"github.com/suzerain-io/controller-go"
 	placeholderv1alpha1 "github.com/suzerain-io/placeholder-name-api/pkg/apis/placeholder/v1alpha1"
@@ -45,13 +49,27 @@ func NewPublisherController(namespace string, kubeClient kubernetes.Interface, p
 }
 
 func (c *publisherController) Sync(ctx controller.Context) error {
-	configMap, err := c.kubeClient.CoreV1().ConfigMaps(clusterInfoNamespace).Get(ctx.Context, clusterInfoName, metav1.GetOptions{})
+	configMap, err := c.kubeClient.
-	if err != nil {
+		CoreV1().
-		return nil // TODO should this return an error? and should it log?
+		ConfigMaps(clusterInfoNamespace).
+		Get(ctx.Context, clusterInfoName, metav1.GetOptions{})
+	notFound := k8serrors.IsNotFound(err)
+	if err != nil && !notFound {
+		return fmt.Errorf("failed to get %s configmap: %w", clusterInfoName, err)
 	}
+	if notFound {
+		klog.InfoS(
+			"could not find config map",
+			"configmap",
+			klog.KRef(clusterInfoNamespace, clusterInfoName),
+		)
+		return nil
+	}
+
 	kubeConfig, kubeConfigPresent := configMap.Data[clusterInfoConfigMapKey]
 	if !kubeConfigPresent {
-		return nil // TODO should this return an error? and should it log?
+		klog.InfoS("could not find kubeconfig configmap key")
+		return nil
 	}
 
 	config, _ := clientcmd.Load([]byte(kubeConfig))
@@ -74,10 +92,57 @@ func (c *publisherController) Sync(ctx controller.Context) error {
 			CertificateAuthorityData: certificateAuthorityData,
 		},
 	}
-	_, _ = c.placeholderClient.
+	if err := c.createOrUpdateLoginDiscoveryConfig(ctx.Context, &discoveryConfig); err != nil {
-		PlaceholderV1alpha1().
+		return err
-		LoginDiscoveryConfigs(c.namespace).
+	}
-		Create(ctx.Context, &discoveryConfig, metav1.CreateOptions{})
 
 	return nil
 }
+
+func (c *publisherController) createOrUpdateLoginDiscoveryConfig(
+	ctx context.Context,
+	discoveryConfig *placeholderv1alpha1.LoginDiscoveryConfig,
+) error {
+	loginDiscoveryConfigs := c.placeholderClient.
+		PlaceholderV1alpha1().
+		LoginDiscoveryConfigs(c.namespace)
+
+	existingDiscoveryConfig, err := loginDiscoveryConfigs.Get(
+		ctx,
+		discoveryConfig.Name,
+		metav1.GetOptions{},
+	)
+	notFound := k8serrors.IsNotFound(err)
+	if err != nil && !notFound {
+		return fmt.Errorf("could not get logindiscoveryconfig: %w", err)
+	}
+
+	if notFound {
+		if _, err := loginDiscoveryConfigs.Create(
+			ctx,
+			discoveryConfig,
+			metav1.CreateOptions{},
+		); err != nil {
+			return fmt.Errorf("could not create logindiscoveryconfig: %w", err)
+		}
+	} else if !equal(existingDiscoveryConfig, discoveryConfig) {
+		// Update just the fields we care about.
+		existingDiscoveryConfig.Spec.Server = discoveryConfig.Spec.Server
+		existingDiscoveryConfig.Spec.CertificateAuthorityData = discoveryConfig.Spec.CertificateAuthorityData
+
+		if _, err := loginDiscoveryConfigs.Update(
+			ctx,
+			existingDiscoveryConfig,
+			metav1.UpdateOptions{},
+		); err != nil {
+			return fmt.Errorf("could not update logindiscoveryconfig: %w", err)
+		}
+	}
+
+	return nil
+}
+
+func equal(a, b *placeholderv1alpha1.LoginDiscoveryConfig) bool {
+	return a.Spec.Server == b.Spec.Server &&
+		a.Spec.CertificateAuthorityData == b.Spec.CertificateAuthorityData
+}

internal/controller/logindiscovery/publisher_test.go

@@ -7,6 +7,7 @@ package logindiscovery
 
 import (
 	"context"
+	"errors"
 	"strings"
 	"testing"
 	"time"
@@ -16,6 +17,7 @@ import (
 	"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"
 	kubernetesfake "k8s.io/client-go/kubernetes/fake"
 	coretesting "k8s.io/client-go/testing"
@@ -106,38 +108,144 @@ func TestRun(t *testing.T) {
 						err := controller.TestSync(t, subject, *controllerContext)
 						r.NoError(err)
 
-						expectedLoginDiscoveryConfigGVR, expectedLoginDiscoveryConfig := expectedLoginDiscoveryConfig(installedInNamespace, kubeServerURL, caData)
+						expectedLoginDiscoveryConfigGVR, expectedLoginDiscoveryConfig := expectedLoginDiscoveryConfig(
+							installedInNamespace,
+							kubeServerURL,
+							caData,
+						)
 						expectedActions := []coretesting.Action{
-							coretesting.NewCreateAction(expectedLoginDiscoveryConfigGVR, installedInNamespace, expectedLoginDiscoveryConfig),
+							coretesting.NewGetAction(
+								expectedLoginDiscoveryConfigGVR,
+								installedInNamespace,
+								expectedLoginDiscoveryConfig.Name,
+							),
+							coretesting.NewCreateAction(
+								expectedLoginDiscoveryConfigGVR,
+								installedInNamespace,
+								expectedLoginDiscoveryConfig,
+							),
 						}
-
-						// Expect a LoginDiscoveryConfig to be created with the fields from the cluster-info ConfigMap
-						actualCreatedObject := placeholderClient.Actions()[0].(coretesting.CreateActionImpl).Object
-						r.Equal(expectedLoginDiscoveryConfig, actualCreatedObject)
 						r.Equal(expectedActions, placeholderClient.Actions())
 					})
+
+					when("creating the LoginDiscoveryConfig fails", func() {
+						it.Before(func() {
+							placeholderClient.PrependReactor(
+								"create",
+								"logindiscoveryconfigs",
+								func(_ coretesting.Action) (bool, runtime.Object, error) {
+									return true, nil, errors.New("create failed")
+								},
+							)
+						})
+
+						it("returns the create error", func() {
+							err := controller.TestSync(t, subject, *controllerContext)
+							r.EqualError(err, "could not create logindiscoveryconfig: create failed")
+						})
+					})
 				})
 
 				when("the LoginDiscoveryConfig already exists", func() {
 					when("the LoginDiscoveryConfig is already up to date according to the data in the ConfigMap", func() {
 						it.Before(func() {
-							// TODO add a fake LoginDiscoveryConfig to the placeholderClient whose data matches the ConfigMap's data
+							_, expectedLoginDiscoveryConfig := expectedLoginDiscoveryConfig(
+								installedInNamespace,
+								kubeServerURL,
+								caData,
+							)
+							err := placeholderClient.Tracker().Add(expectedLoginDiscoveryConfig)
+							r.NoError(err)
 						})
 
-						it.Pend("does not update the LoginDiscoveryConfig to avoid unnecessary etcd writes", func() {
+						it("does not update the LoginDiscoveryConfig to avoid unnecessary etcd writes/api calls", func() {
 							err := controller.TestSync(t, subject, *controllerContext)
 							r.NoError(err)
-							r.Empty(placeholderClient.Actions())
+
+							expectedLoginDiscoveryConfigGVR, expectedLoginDiscoveryConfig := expectedLoginDiscoveryConfig(
+								installedInNamespace,
+								kubeServerURL,
+								caData,
+							)
+							expectedActions := []coretesting.Action{
+								coretesting.NewGetAction(
+									expectedLoginDiscoveryConfigGVR,
+									installedInNamespace,
+									expectedLoginDiscoveryConfig.Name,
+								),
+							}
+							r.Equal(expectedActions, placeholderClient.Actions())
+						})
+
+						when("getting the LoginDiscoveryConfig fails", func() {
+							it.Before(func() {
+								placeholderClient.PrependReactor(
+									"get",
+									"logindiscoveryconfigs",
+									func(_ coretesting.Action) (bool, runtime.Object, error) {
+										return true, nil, errors.New("get failed")
+									},
+								)
+							})
+
+							it("returns the get error", func() {
+								err := controller.TestSync(t, subject, *controllerContext)
+								r.EqualError(err, "could not get logindiscoveryconfig: get failed")
+							})
 						})
 					})
 
 					when("the LoginDiscoveryConfig is stale compared to the data in the ConfigMap", func() {
 						it.Before(func() {
-							// TODO add a fake LoginDiscoveryConfig to the placeholderClient whose data does not match the ConfigMap's data
+							_, expectedLoginDiscoveryConfig := expectedLoginDiscoveryConfig(
+								installedInNamespace,
+								kubeServerURL,
+								caData,
+							)
+							expectedLoginDiscoveryConfig.Spec.Server = "https://some-other-server"
+							err := placeholderClient.Tracker().Add(expectedLoginDiscoveryConfig)
+							r.NoError(err)
 						})
 
-						it.Pend("updates the existing LoginDiscoveryConfig", func() {
+						it("updates the existing LoginDiscoveryConfig", func() {
-							// TODO
+							err := controller.TestSync(t, subject, *controllerContext)
+							r.NoError(err)
+
+							expectedLoginDiscoveryConfigGVR, expectedLoginDiscoveryConfig := expectedLoginDiscoveryConfig(
+								installedInNamespace,
+								kubeServerURL,
+								caData,
+							)
+							expectedActions := []coretesting.Action{
+								coretesting.NewGetAction(
+									expectedLoginDiscoveryConfigGVR,
+									installedInNamespace,
+									expectedLoginDiscoveryConfig.Name,
+								),
+								coretesting.NewUpdateAction(
+									expectedLoginDiscoveryConfigGVR,
+									installedInNamespace,
+									expectedLoginDiscoveryConfig,
+								),
+							}
+							r.Equal(expectedActions, placeholderClient.Actions())
+						})
+
+						when("updating the LoginDiscoveryConfig fails", func() {
+							it.Before(func() {
+								placeholderClient.PrependReactor(
+									"update",
+									"logindiscoveryconfigs",
+									func(_ coretesting.Action) (bool, runtime.Object, error) {
+										return true, nil, errors.New("update failed")
+									},
+								)
+							})
+
+							it("returns the update error", func() {
+								err := controller.TestSync(t, subject, *controllerContext)
+								r.EqualError(err, "could not update logindiscoveryconfig: update failed")
+							})
 						})
 					})
 				})
@@ -181,5 +289,22 @@ func TestRun(t *testing.T) {
 				r.Empty(placeholderClient.Actions())
 			})
 		})
+
+		when("getting the cluster-info ConfigMap in the kube-public namespace fails", func() {
+			it.Before(func() {
+				kubeClient.PrependReactor(
+					"get",
+					"configmaps",
+					func(_ coretesting.Action) (bool, runtime.Object, error) {
+						return true, nil, errors.New("get failed")
+					},
+				)
+			})
+
+			it("returns an error", func() {
+				err := controller.TestSync(t, subject, *controllerContext)
+				r.EqualError(err, "failed to get cluster-info configmap: get failed")
+			})
+		})
 	}, spec.Parallel(), spec.Report(report.Terminal{}))
 }