Change Preferred Controller Config to Use Secret Instead of ConfigMap (…

…#318) * add test for kapp-controller getting secret over configmap Co-Authored-By: Joe Kimmel <[email protected]> * add ability for kapp-controller to fetch controller config from k8s secret Co-Authored-By: Joe Kimmel <[email protected]> * added unit tests for when only secret or configmap exist for kapp-controller config Co-Authored-By: Joe Kimmel <[email protected]> * add all keys from secret to configmap Co-Authored-By: Joe Kimmel <[email protected]> * initial feedback Co-Authored-By: Joe Kimmel <[email protected]> * remove base64 and convert Config to a struct with explicit fields populated from Either a secret or configmap not both * rewrote findExternalConfig to avoid nested ifs * remove stale TODO * change config-map.yml to secret-config.yml * add copyright header to config_test.go * convert helper funcs to Config struct funcs * remove checking for key existence * remove keys list from addSecretDataToConfig Co-authored-by: Joe Kimmel <[email protected]> Co-authored-by: Joe Kimmel <[email protected]>
carvel-dev · Jul 28, 2021 · 9214dec · 9214dec
1 parent 60d8e36
commit 9214dec
Show file tree

Hide file tree

Showing 3 changed files with 180 additions and 19 deletions.
diff --git a/config-test/config-map.yml → config-test/secret-config.yml b/config-test/config-map.yml → config-test/secret-config.yml
@@ -1,13 +1,13 @@
 #@ load("@ytt:data", "data")
 
 apiVersion: v1
-kind: ConfigMap
+kind: Secret
 metadata:
   name: kapp-controller-config
   namespace: #@ data.values.namespace
   annotations:
     kapp.k14s.io/change-group: "kapp-controller.carvel.dev/config"
-data:
+stringData:
   #! Must match the second cert in the cert chain in kapp-controller/test/e2e/assets/self-signed-https-server.yml
   caCerts: |
     -----BEGIN CERTIFICATE-----

diff --git a/pkg/config/config.go b/pkg/config/config.go
@@ -9,15 +9,16 @@ import (
 	"os"
 	"strings"
 
-	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/clientcmd"
 )
 
 const (
-	configMapName = "kapp-controller-config"
+	kcConfigName = "kapp-controller-config"
 
 	caCertsKey      = "caCerts"
 	systemCertsFile = "/etc/pki/tls/certs/ca-bundle.crt"
@@ -32,10 +33,43 @@ const (
 	skipTLSVerifyKey = "dangerousSkipTLSVerify"
 )
 
+// Config is populated from the cluster's Secret or ConfigMap and sets behavior of kapp-controller.
+// NOTE because config may be populated from a Secret use caution if you're tempted to serialize.
 type Config struct {
-	configMap *v1.ConfigMap
+	caCerts       string
+	httpProxy     string
+	httpsProxy    string
+	noProxy       string
+	skipTLSVerify string
+	populated     bool
 }
 
+// findExternalConfig will populate exactly one of its return values and the others will be nil.
+// we prefer to populate secret, fall back to configMap, and return unrecoverable errors if they occur.
+func findExternalConfig(namespace string, client kubernetes.Interface) (*v1.Secret, *v1.ConfigMap, error) {
+	secret, err := client.CoreV1().Secrets(namespace).Get(context.Background(), kcConfigName, metav1.GetOptions{})
+	// NOTE: to avoid nested ifs we are checking err == nil,  instead of != nil.
+	if err == nil { // happy path return
+		return secret, nil, nil
+	}
+	if !errors.IsNotFound(err) { // other error than NotFound so we're not gonna look for configMap
+		return nil, nil, err
+	}
+
+	configMap, err := client.CoreV1().ConfigMaps(namespace).Get(context.Background(), kcConfigName, metav1.GetOptions{})
+	if err == nil { // second happiest path return
+		return nil, configMap, nil
+	}
+	if !errors.IsNotFound(err) { // other error than NotFound
+		return nil, nil, err
+	}
+
+	// nothing found, no errors, return triple-nil
+	return nil, nil, nil
+}
+
+// GetConfig populates the Config struct from k8s resources.
+// GetConfig prefers a secret named kcConfigName but if that does not exist falls back to a configMap of same name.
 func GetConfig(client kubernetes.Interface) (*Config, error) {
 	loadingRules := clientcmd.NewDefaultClientConfigLoadingRules()
 	kubeConf := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, &clientcmd.ConfigOverrides{})
@@ -44,42 +78,44 @@ func GetConfig(client kubernetes.Interface) (*Config, error) {
 		return nil, fmt.Errorf("Getting namespace: %s", err)
 	}
 
-	configMap, err := client.CoreV1().ConfigMaps(namespace).Get(context.Background(), configMapName, metav1.GetOptions{})
-
-	if errors.IsNotFound(err) {
-		return &Config{}, nil
-	}
-
+	secret, configMap, err := findExternalConfig(namespace, client)
 	if err != nil {
 		return nil, err
 	}
 
-	return &Config{configMap}, nil
+	config := &Config{}
+
+	if secret != nil {
+		config.addSecretDataToConfig(secret)
+	} else if configMap != nil {
+		config.addConfigMapDataToConfig(configMap)
+	}
+
+	return config, nil
 }
 
 func (gc *Config) Apply() error {
-	if gc.configMap == nil {
+	if !gc.populated {
 		return nil
 	}
 
-	configMap := gc.configMap
-	err := gc.addTrustedCerts(gc.configMap.Data[caCertsKey])
+	err := gc.addTrustedCerts(gc.caCerts)
 	if err != nil {
 		return fmt.Errorf("Adding trusted certs: %s", err)
 	}
 
-	gc.configureProxies(configMap.Data[httpProxyKey], configMap.Data[httpsProxyKey], configMap.Data[noProxyKey])
+	gc.configureProxies(gc.httpProxy, gc.httpsProxy, gc.noProxy)
 
 	return nil
 }
 
 func (gc *Config) ShouldSkipTLSForDomain(candidateDomain string) bool {
-	if gc.configMap == nil {
+	if !gc.populated {
 		return false
 	}
 
-	domains, exists := gc.configMap.Data[skipTLSVerifyKey]
-	if !exists {
+	domains := gc.skipTLSVerify
+	if len(domains) == 0 {
 		return false
 	}
 
@@ -129,3 +165,25 @@ func (gc *Config) configureProxies(httpProxy, httpsProxy, noProxy string) {
 		os.Setenv(strings.ToUpper(noProxyEnvVar), noProxy)
 	}
 }
+
+func (gc *Config) addSecretDataToConfig(secret *v1.Secret) {
+	extractedValues := map[string]string{}
+	for key, value := range secret.Data {
+		extractedValues[key] = string(value)
+	}
+
+	gc.addDataToConfig(extractedValues)
+}
+
+func (gc *Config) addConfigMapDataToConfig(configMap *v1.ConfigMap) {
+	gc.addDataToConfig(configMap.Data)
+}
+
+func (gc *Config) addDataToConfig(data map[string]string) {
+	gc.caCerts = data[caCertsKey]
+	gc.httpProxy = data[httpProxyKey]
+	gc.httpsProxy = data[httpsProxyKey]
+	gc.noProxy = data[noProxyKey]
+	gc.skipTLSVerify = data[skipTLSVerifyKey]
+	gc.populated = true
+}
diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go
@@ -0,0 +1,103 @@
+// Copyright 2021 VMware, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package config_test
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	kcconfig "github.com/vmware-tanzu/carvel-kapp-controller/pkg/config"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	k8sfake "k8s.io/client-go/kubernetes/fake"
+)
+
+func Test_GetConfig_ReturnsSecret_WhenBothConfigMapAndSecretExist(t *testing.T) {
+	configMap := &v1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "kapp-controller-config",
+			Namespace: "default",
+		},
+		Data: map[string]string{
+			"httpProxy": "wrong-proxy",
+		},
+	}
+
+	secret := &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "kapp-controller-config",
+			Namespace: "default",
+		},
+		Data: map[string][]byte{
+			"httpProxy": []byte("proxy-svc.proxy-server.svc.cluster.local:80"),
+		},
+	}
+
+	defer os.Unsetenv("http_proxy")
+
+	k8scs := k8sfake.NewSimpleClientset(configMap, secret)
+
+	config, err := kcconfig.GetConfig(k8scs)
+	assert.Nil(t, err, "unexpected error after running config.GetConfig()", err)
+
+	assert.Nil(t, config.Apply(), "unexpected error after running config.Apply()", err)
+
+	expected := "proxy-svc.proxy-server.svc.cluster.local:80"
+	httpProxyActual := os.Getenv("http_proxy")
+
+	assert.Equal(t, expected, httpProxyActual)
+}
+
+func Test_GetConfig_ReturnsConfigMap_WhenOnlyConfigMapExists(t *testing.T) {
+	configMap := &v1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "kapp-controller-config",
+			Namespace: "default",
+		},
+		Data: map[string]string{
+			"httpProxy": "proxy-svc.proxy-server.svc.cluster.local:80",
+		},
+	}
+
+	defer os.Unsetenv("http_proxy")
+
+	k8scs := k8sfake.NewSimpleClientset(configMap)
+
+	config, err := kcconfig.GetConfig(k8scs)
+	assert.Nil(t, err, "unexpected error after running config.GetConfig()", err)
+
+	assert.Nil(t, config.Apply(), "unexpected error after running config.Apply()", err)
+
+	expected := "proxy-svc.proxy-server.svc.cluster.local:80"
+	httpProxyActual := os.Getenv("http_proxy")
+
+	assert.Equal(t, expected, httpProxyActual)
+}
+
+func Test_GetConfig_ReturnsSecret_WhenOnlySecretExists(t *testing.T) {
+	secret := &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "kapp-controller-config",
+			Namespace: "default",
+		},
+		Data: map[string][]byte{
+			"httpProxy": []byte("proxy-svc.proxy-server.svc.cluster.local:80"),
+		},
+	}
+
+	defer os.Unsetenv("http_proxy")
+
+	k8scs := k8sfake.NewSimpleClientset(secret)
+
+	config, err := kcconfig.GetConfig(k8scs)
+	assert.Nil(t, err, "unexpected error after running config.GetConfig()", err)
+
+	assert.Nil(t, config.Apply(), "unexpected error after running config.Apply()", err)
+
+	expected := "proxy-svc.proxy-server.svc.cluster.local:80"
+	httpProxyActual := os.Getenv("http_proxy")
+
+	assert.Equal(t, expected, httpProxyActual)
+}