Allow override of discovery URL via ConfigMap

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

- Seems like the next step is to allow override of the CA bundle; I didn't
  do that here for simplicity of the commit, but seems like it is the right
  thing to do in the future.

deploy/deployment.yaml

@@ -24,6 +24,8 @@ metadata:
 data:
   #@yaml/text-templated-strings
   placeholder-name.yaml: |
+    discovery:
+      url: (@= data.values.discovery_url @)
     webhook:
       url: (@= data.values.webhook_url @)
       caBundle: (@= data.values.webhook_ca_bundle @)

deploy/values.yaml

@@ -12,3 +12,5 @@ image_tag: #! e.g. latest
 
 webhook_url: #! e.g., https://example.com
 webhook_ca_bundle: #! e.g., LS0tLS1CRUdJTiBDRVJUSUZJQ0F...
+
+discovery_url: #! e.g., https://example.com

internal/controller/logindiscovery/publisher.go

@@ -52,6 +52,7 @@ type withInformerOptionFunc func(
 
 type publisherController struct {
 	namespace                    string
+	serverOverride               *string
 	placeholderClient            placeholderclientset.Interface
 	configMapInformer            corev1informers.ConfigMapInformer
 	loginDiscoveryConfigInformer crdsplaceholderv1alpha1informers.LoginDiscoveryConfigInformer
@@ -59,6 +60,7 @@ type publisherController struct {
 
 func NewPublisherController(
 	namespace string,
+	serverOverride *string,
 	placeholderClient placeholderclientset.Interface,
 	configMapInformer corev1informers.ConfigMapInformer,
 	loginDiscoveryConfigInformer crdsplaceholderv1alpha1informers.LoginDiscoveryConfigInformer,
@@ -69,6 +71,7 @@ func NewPublisherController(
 			Name: "publisher-controller",
 			Syncer: &publisherController{
 				namespace:                    namespace,
+				serverOverride:               serverOverride,
 				placeholderClient:            placeholderClient,
 				configMapInformer:            configMapInformer,
 				loginDiscoveryConfigInformer: loginDiscoveryConfigInformer,
@@ -120,6 +123,10 @@ func (c *publisherController) Sync(ctx controller.Context) error {
 		break
 	}
 
+	if c.serverOverride != nil {
+		server = *c.serverOverride
+	}
+
 	discoveryConfig := crdsplaceholderv1alpha1.LoginDiscoveryConfig{
 		TypeMeta: metav1.TypeMeta{},
 		ObjectMeta: metav1.ObjectMeta{

internal/controller/logindiscovery/publisher_test.go

@@ -64,6 +64,7 @@ func TestInformerFilters(t *testing.T) {
 			_ = NewPublisherController(
 				installedInNamespace,
 				nil,
+				nil,
 				configMapInformer,
 				loginDiscoveryConfigInformer,
 				observableWithInformerOption.WithInformer, // make it possible to observe the behavior of the Filters
@@ -185,6 +186,7 @@ func TestSync(t *testing.T) {
 		var r *require.Assertions
 
 		var subject controller.Controller
+		var serverOverride *string
 		var kubeInformerClient *kubernetesfake.Clientset
 		var placeholderInformerClient *placeholderfake.Clientset
 		var kubeInformers kubeinformers.SharedInformerFactory
@@ -216,6 +218,26 @@ func TestSync(t *testing.T) {
 		// Defer starting the informers until the last possible moment so that the
 		// nested Before's can keep adding things to the informer caches.
 		var startInformersAndController = func() {
+			// Set this at the last second to allow for injection of server override.
+			subject = NewPublisherController(
+				installedInNamespace,
+				serverOverride,
+				placeholderAPIClient,
+				kubeInformers.Core().V1().ConfigMaps(),
+				placeholderInformers.Crds().V1alpha1().LoginDiscoveryConfigs(),
+				controller.WithInformer,
+			)
+
+			// Set this at the last second to support calling subject.Name().
+			syncContext = &controller.Context{
+				Context: timeoutContext,
+				Name:    subject.Name(),
+				Key: controller.Key{
+					Namespace: "kube-public",
+					Name:      "cluster-info",
+				},
+			}
+
 			// Must start informers before calling TestRunSynchronously()
 			kubeInformers.Start(timeoutContext.Done())
 			placeholderInformers.Start(timeoutContext.Done())
@@ -232,23 +254,6 @@ func TestSync(t *testing.T) {
 			placeholderAPIClient = placeholderfake.NewSimpleClientset()
 			placeholderInformerClient = placeholderfake.NewSimpleClientset()
 			placeholderInformers = placeholderinformers.NewSharedInformerFactory(placeholderInformerClient, 0)
-
-			subject = NewPublisherController(
-				installedInNamespace,
-				placeholderAPIClient,
-				kubeInformers.Core().V1().ConfigMaps(),
-				placeholderInformers.Crds().V1alpha1().LoginDiscoveryConfigs(),
-				controller.WithInformer,
-			)
-
-			syncContext = &controller.Context{
-				Context: timeoutContext,
-				Name:    subject.Name(),
-				Key: controller.Key{
-					Namespace: "kube-public",
-					Name:      "cluster-info",
-				},
-			}
 		})
 
 		it.After(func() {
@@ -321,6 +326,35 @@ func TestSync(t *testing.T) {
 							r.EqualError(err, "could not create logindiscoveryconfig: create failed")
 						})
 					})
+
+					when("a server override is passed to the controller", func() {
+						it("uses the server override field", func() {
+							serverOverride = new(string)
+							*serverOverride = "https://some-server-override"
+
+							startInformersAndController()
+							err := controller.TestSync(t, subject, *syncContext)
+							r.NoError(err)
+
+							expectedLoginDiscoveryConfigGVR, expectedLoginDiscoveryConfig := expectedLoginDiscoveryConfig(
+								installedInNamespace,
+								kubeServerURL,
+								caData,
+							)
+							expectedLoginDiscoveryConfig.Spec.Server = "https://some-server-override"
+
+							r.Equal(
+								[]coretesting.Action{
+									coretesting.NewCreateAction(
+										expectedLoginDiscoveryConfigGVR,
+										installedInNamespace,
+										expectedLoginDiscoveryConfig,
+									),
+								},
+								placeholderAPIClient.Actions(),
+							)
+						})
+					})
 				})
 
 				when("the LoginDiscoveryConfig already exists", func() {

internal/server/server.go

@@ -241,7 +241,12 @@ func (a *App) run(
 		return fmt.Errorf("could not register API service: %w", err)
 	}
 
-	cmrf := wireControllerManagerRunFunc(serverInstallationNamespace, k8sClient, placeholderClient)
+	cmrf := wireControllerManagerRunFunc(
+		serverInstallationNamespace,
+		cfg.DiscoveryConfig.URL,
+		k8sClient,
+		placeholderClient,
+	)
 	apiServerConfig, err := a.configServer(
 		cert,
 		webhookTokenAuthenticator,
@@ -324,6 +329,7 @@ func createStaticCertKeyProvider(cert *tls.Certificate) (dynamiccertificates.Cer
 
 func wireControllerManagerRunFunc(
 	serverInstallationNamespace string,
+	discoveryURLOverride *string,
 	k8s kubernetes.Interface,
 	placeholder placeholderclientset.Interface,
 ) func(ctx context.Context) {
@@ -344,6 +350,7 @@ func wireControllerManagerRunFunc(
 		WithController(
 			logindiscovery.NewPublisherController(
 				serverInstallationNamespace,
+				discoveryURLOverride,
 				placeholder,
 				k8sInformers.Core().V1().ConfigMaps(),
 				placeholderInformers.Crds().V1alpha1().LoginDiscoveryConfigs(),

pkg/config/api/types.go

@@ -8,6 +8,7 @@ package api
 // Config contains knobs to setup an instance of placeholder-name.
 type Config struct {
 	WebhookConfig   WebhookConfigSpec   `json:"webhook"`
+	DiscoveryConfig DiscoveryConfigSpec `json:"discovery"`
 }
 
 // WebhookConfig contains configuration knobs specific to placeholder-name's use
@@ -21,3 +22,12 @@ type WebhookConfigSpec struct {
 	// to validate TLS connections to the WebhookURL.
 	CABundle []byte `json:"caBundle"`
 }
+
+// DiscoveryConfigSpec contains configuration knobs specific to
+// placeholder-name's publishing of discovery information. These values can be
+// viewed as overrides, i.e., if these are set, then placeholder-name will
+// publish these values in its discovery document instead of the ones it finds.
+type DiscoveryConfigSpec struct {
+	// URL contains the URL at which placeholder-name can be contacted.
+	URL *string `json:"url,omitempty"`
+}

pkg/config/config_test.go

@@ -14,14 +14,50 @@ import (
 )
 
 func TestFromPath(t *testing.T) {
-	expect := require.New(t)
-
-	config, err := FromPath("testdata/happy.yaml")
-	expect.NoError(err)
-	expect.Equal(config, &api.Config{
+	tests := []struct {
+		name       string
+		path       string
+		wantConfig *api.Config
+	}{
+		{
+			name: "Happy",
+			path: "testdata/happy.yaml",
+			wantConfig: &api.Config{
+				DiscoveryConfig: api.DiscoveryConfigSpec{
+					URL: stringPtr("https://some.discovery/url"),
+				},
 				WebhookConfig: api.WebhookConfigSpec{
 					URL:      "https://tuna.com/fish?marlin",
 					CABundle: []byte("-----BEGIN CERTIFICATE-----..."),
 				},
+			},
+		},
+		{
+			name: "NoDiscovery",
+			path: "testdata/no-discovery.yaml",
+			wantConfig: &api.Config{
+				DiscoveryConfig: api.DiscoveryConfigSpec{
+					URL: nil,
+				},
+				WebhookConfig: api.WebhookConfigSpec{
+					URL:      "https://tuna.com/fish?marlin",
+					CABundle: []byte("-----BEGIN CERTIFICATE-----..."),
+				},
+			},
+		},
+	}
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			config, err := FromPath(test.path)
+			require.NoError(t, err)
+			require.Equal(t, test.wantConfig, config)
 		})
 	}
+}
+
+func stringPtr(s string) *string {
+	sPtr := new(string)
+	*sPtr = s
+	return sPtr
+}

pkg/config/testdata/happy.yaml

@@ -1,4 +1,6 @@
 ---
+discovery:
+  url: https://some.discovery/url
 webhook:
   url: https://tuna.com/fish?marlin
   caBundle: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tLi4u

pkg/config/testdata/no-discovery.yaml

@@ -0,0 +1,5 @@
+---
+webhook:
+  url: https://tuna.com/fish?marlin
+  caBundle: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tLi4u
+

test/integration/client_test.go

@@ -7,7 +7,6 @@ package integration
 
 import (
 	"context"
-	"os"
 	"strings"
 	"testing"
 	"time"
@@ -52,8 +51,7 @@ O2D8LtWhMbrYy755Fgq4H9s3vCgfvHY1AQ==
 )
 
 func TestClient(t *testing.T) {
-	tmcClusterToken := os.Getenv("PLACEHOLDER_NAME_TMC_CLUSTER_TOKEN")
-	require.NotEmptyf(t, tmcClusterToken, "must specify PLACEHOLDER_NAME_TMC_CLUSTER_TOKEN env var for integration tests")
+	tmcClusterToken := library.Getenv(t, "PLACEHOLDER_NAME_TMC_CLUSTER_TOKEN")
 
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()

test/integration/deployment_test.go

@@ -7,7 +7,6 @@ package integration
 
 import (
 	"context"
-	"os"
 	"testing"
 	"time"
 
@@ -20,11 +19,8 @@ import (
 )
 
 func TestGetDeployment(t *testing.T) {
-	namespaceName := os.Getenv("PLACEHOLDER_NAME_NAMESPACE")
-	require.NotEmptyf(t, namespaceName, "must specify PLACEHOLDER_NAME_NAMESPACE env var for integration tests")
-
-	deploymentName := os.Getenv("PLACEHOLDER_NAME_DEPLOYMENT")
-	require.NotEmptyf(t, deploymentName, "must specify PLACEHOLDER_NAME_DEPLOYMENT env var for integration tests")
+	namespaceName := library.Getenv(t, "PLACEHOLDER_NAME_NAMESPACE")
+	deploymentName := library.Getenv(t, "PLACEHOLDER_NAME_DEPLOYMENT")
 
 	client := library.NewClientset(t)
 

test/integration/logindiscoveryconfig_test.go

@@ -8,7 +8,6 @@ package integration
 import (
 	"context"
 	"encoding/base64"
-	"os"
 	"testing"
 	"time"
 
@@ -21,8 +20,8 @@ import (
 )
 
 func TestSuccessfulLoginDiscoveryConfig(t *testing.T) {
-	namespaceName := os.Getenv("PLACEHOLDER_NAME_NAMESPACE")
-	require.NotEmptyf(t, namespaceName, "must specify PLACEHOLDER_NAME_NAMESPACE env var for integration tests")
+	namespaceName := library.Getenv(t, "PLACEHOLDER_NAME_NAMESPACE")
+	discoveryURL := library.Getenv(t, "PLACEHOLDER_NAME_DISCOVERY_URL")
 
 	client := library.NewPlaceholderNameClientset(t)
 
@@ -30,7 +29,7 @@ func TestSuccessfulLoginDiscoveryConfig(t *testing.T) {
 	defer cancel()
 
 	config := library.NewClientConfig(t)
-	expectedLDCSpec := expectedLDCSpec(config)
+	expectedLDCSpec := expectedLDCSpec(config, discoveryURL)
 	configList, err := client.
 		CrdsV1alpha1().
 		LoginDiscoveryConfigs(namespaceName).
@@ -41,8 +40,8 @@ func TestSuccessfulLoginDiscoveryConfig(t *testing.T) {
 }
 
 func TestReconcilingLoginDiscoveryConfig(t *testing.T) {
-	namespaceName := os.Getenv("PLACEHOLDER_NAME_NAMESPACE")
-	require.NotEmptyf(t, namespaceName, "must specify PLACEHOLDER_NAME_NAMESPACE env var for integration tests")
+	namespaceName := library.Getenv(t, "PLACEHOLDER_NAME_NAMESPACE")
+	discoveryURL := library.Getenv(t, "PLACEHOLDER_NAME_DISCOVERY_URL")
 
 	client := library.NewPlaceholderNameClientset(t)
 
@@ -56,7 +55,7 @@ func TestReconcilingLoginDiscoveryConfig(t *testing.T) {
 	require.NoError(t, err)
 
 	config := library.NewClientConfig(t)
-	expectedLDCSpec := expectedLDCSpec(config)
+	expectedLDCSpec := expectedLDCSpec(config, discoveryURL)
 
 	var actualLDC *crdsplaceholderv1alpha1.LoginDiscoveryConfig
 	for i := 0; i < 10; i++ {
@@ -73,9 +72,9 @@ func TestReconcilingLoginDiscoveryConfig(t *testing.T) {
 	require.Equal(t, expectedLDCSpec, &actualLDC.Spec)
 }
 
-func expectedLDCSpec(config *rest.Config) *crdsplaceholderv1alpha1.LoginDiscoveryConfigSpec {
+func expectedLDCSpec(config *rest.Config, discoveryURL string) *crdsplaceholderv1alpha1.LoginDiscoveryConfigSpec {
 	return &crdsplaceholderv1alpha1.LoginDiscoveryConfigSpec{
-		Server:                   "https://kind-control-plane:6443", //config.Host, // TODO FIX THIS
+		Server:                   discoveryURL,
 		CertificateAuthorityData: base64.StdEncoding.EncodeToString(config.TLSClientConfig.CAData),
 	}
 }

test/integration/loginrequest_test.go

@@ -8,7 +8,6 @@ package integration
 import (
 	"context"
 	"net/http"
-	"os"
 	"testing"
 	"time"
 
@@ -37,8 +36,7 @@ func makeRequest(t *testing.T, spec v1alpha1.LoginRequestSpec) (*v1alpha1.LoginR
 }
 
 func TestSuccessfulLoginRequest(t *testing.T) {
-	tmcClusterToken := os.Getenv("PLACEHOLDER_NAME_TMC_CLUSTER_TOKEN")
-	require.NotEmptyf(t, tmcClusterToken, "must specify PLACEHOLDER_NAME_TMC_CLUSTER_TOKEN env var for integration tests")
+	tmcClusterToken := library.Getenv(t, "PLACEHOLDER_NAME_TMC_CLUSTER_TOKEN")
 
 	response, err := makeRequest(t, v1alpha1.LoginRequestSpec{
 		Type:  v1alpha1.TokenLoginCredentialType,

test/library/env.go

@@ -0,0 +1,22 @@
+/*
+Copyright 2020 VMware, Inc.
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package library
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// Getenv gets the environment variable with key and asserts that it is not
+// empty. It returns the value of the environment variable.
+func Getenv(t *testing.T, key string) string {
+	t.Helper()
+	value := os.Getenv(key)
+	require.NotEmptyf(t, value, "must specify %s env var for integration tests", key)
+	return value
+}