Repository: dexidp/dex
Pull request #2092 (2021-04-20 13:58:27)

fix: read namespace from file for Kubernetes storage client
Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

<!--
Thank you for sending a pull request! Here some tips for contributors:

1. Fill the description template below.
2. Sign a DCO (if you haven't already signed it).
3. Include appropriate tests (if necessary). Make sure that all CI checks are passed.
4. If the Pull Request is a work in progress, use GitHub's "Draft PR" feature and mark it as such.
-->

#### Overview
If Dex fails to get namespace from the token or variable name, read it from the service account namespace file

#### What this PR does / why we need it
Related https://github.com/dexidp/dex/issues/2082

#### Special notes for your reviewer
Fixed some minor issues in this PR:
* Dial -> DialContext (because Dial is deprecated and DialContext is a direct substitution)
* context -> k8sContext to avoid overlapping with imported package
* Make some variables constant

#### Does this PR introduce a user-facing change?

```release-note
Get namespace for Kubernetes storage client in a cluster from the service account namespace file as a fallback.
```



Patch:
diff --git a/storage/kubernetes/client.go b/storage/kubernetes/client.go
index 593f1c03..b0ace6c1 100644
--- a/storage/kubernetes/client.go
+++ b/storage/kubernetes/client.go
@@ -307,10 +307,10 @@ func newClient(cluster k8sapi.Cluster, user k8sapi.AuthInfo, namespace string, l
 	var t http.RoundTripper
 	httpTransport := &http.Transport{
 		Proxy: http.ProxyFromEnvironment,
-		Dial: (&net.Dialer{
+		DialContext: (&net.Dialer{
 			Timeout:   30 * time.Second,
 			KeepAlive: 30 * time.Second,
-		}).Dial,
+		}).DialContext,
 		TLSClientConfig:       tlsConfig,
 		TLSHandshakeTimeout:   10 * time.Second,
 		ExpectContinueTimeout: 1 * time.Second,
@@ -423,34 +423,76 @@ func namespaceFromServiceAccountJWT(s string) (string, error) {
 	return data.Namespace, nil
 }
 
-func inClusterConfig() (cluster k8sapi.Cluster, user k8sapi.AuthInfo, namespace string, err error) {
-	host, port := os.Getenv("KUBERNETES_SERVICE_HOST"), os.Getenv("KUBERNETES_SERVICE_PORT")
+func namespaceFromFile(path string) (string, error) {
+	data, err := ioutil.ReadFile(path)
+	if err != nil {
+		return "", err
+	}
+
+	return string(data), nil
+}
+
+func getInClusterConfigNamespace(token, namespaceENV, namespacePath string) (string, error) {
+	namespace := os.Getenv(namespaceENV)
+	if namespace != "" {
+		return namespace, nil
+	}
+
+	namespace, err := namespaceFromServiceAccountJWT(token)
+	if err == nil {
+		return namespace, nil
+	}
+
+	err = fmt.Errorf("inspect service account token: %v", err)
+	namespace, fileErr := namespaceFromFile(namespacePath)
+	if fileErr == nil {
+		return namespace, nil
+	}
+
+	return "", fmt.Errorf("%v: trying to get namespace from file: %v", err, fileErr)
+}
+
+func inClusterConfig() (k8sapi.Cluster, k8sapi.AuthInfo, string, error) {
+	const (
+		serviceAccountPath          = "/var/run/secrets/kubernetes.io/serviceaccount/"
+		serviceAccountTokenPath     = serviceAccountPath + "token"
+		serviceAccountCAPath        = serviceAccountPath + "ca.crt"
+		serviceAccountNamespacePath = serviceAccountPath + "namespace"
+
+		kubernetesServiceHostENV  = "KUBERNETES_SERVICE_HOST"
+		kubernetesServicePortENV  = "KUBERNETES_SERVICE_PORT"
+		kubernetesPodNamespaceENV = "KUBERNETES_POD_NAMESPACE"
+	)
+
+	host, port := os.Getenv(kubernetesServiceHostENV), os.Getenv(kubernetesServicePortENV)
 	if len(host) == 0 || len(port) == 0 {
-		err = fmt.Errorf("unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined")
-		return
+		return k8sapi.Cluster{}, k8sapi.AuthInfo{}, "", fmt.Errorf(
+			"unable to load in-cluster configuration, %s and %s must be defined",
+			kubernetesServiceHostENV,
+			kubernetesServicePortENV,
+		)
 	}
 	// we need to wrap IPv6 addresses in square brackets
 	// IPv4 also works with square brackets
 	host = "[" + host + "]"
-	cluster = k8sapi.Cluster{
+	cluster := k8sapi.Cluster{
 		Server:               "https://" + host + ":" + port,
-		CertificateAuthority: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt",
+		CertificateAuthority: serviceAccountCAPath,
 	}
-	token, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/token")
+
+	token, err := ioutil.ReadFile(serviceAccountTokenPath)
 	if err != nil {
-		return
+		return cluster, k8sapi.AuthInfo{}, "", err
 	}
-	user = k8sapi.AuthInfo{Token: string(token)}
 
-	if namespace = os.Getenv("KUBERNETES_POD_NAMESPACE"); namespace == "" {
-		namespace, err = namespaceFromServiceAccountJWT(user.Token)
-		if err != nil {
-			err = fmt.Errorf("failed to inspect service account token: %v", err)
-			return
-		}
+	user := k8sapi.AuthInfo{Token: string(token)}
+
+	namespace, err := getInClusterConfigNamespace(user.Token, kubernetesPodNamespaceENV, serviceAccountNamespacePath)
+	if err != nil {
+		return cluster, user, "", err
 	}
 
-	return
+	return cluster, user, namespace, nil
 }
 
 func currentContext(config *k8sapi.Config) (cluster k8sapi.Cluster, user k8sapi.AuthInfo, ns string, err error) {
@@ -461,7 +503,7 @@ func currentContext(config *k8sapi.Config) (cluster k8sapi.Cluster, user k8sapi.
 			return cluster, user, "", errors.New("kubeconfig has no current context")
 		}
 	}
-	context, ok := func() (k8sapi.Context, bool) {
+	k8sContext, ok := func() (k8sapi.Context, bool) {
 		for _, namedContext := range config.Contexts {
 			if namedContext.Name == config.CurrentContext {
 				return namedContext.Context, true
@@ -475,26 +517,26 @@ func currentContext(config *k8sapi.Config) (cluster k8sapi.Cluster, user k8sapi.
 
 	cluster, ok = func() (k8sapi.Cluster, bool) {
 		for _, namedCluster := range config.Clusters {
-			if namedCluster.Name == context.Cluster {
+			if namedCluster.Name == k8sContext.Cluster {
 				return namedCluster.Cluster, true
 			}
 		}
 		return k8sapi.Cluster{}, false
 	}()
 	if !ok {
-		return cluster, user, "", fmt.Errorf("no cluster named %q found", context.Cluster)
+		return cluster, user, "", fmt.Errorf("no cluster named %q found", k8sContext.Cluster)
 	}
 
 	user, ok = func() (k8sapi.AuthInfo, bool) {
 		for _, namedAuthInfo := range config.AuthInfos {
-			if namedAuthInfo.Name == context.AuthInfo {
+			if namedAuthInfo.Name == k8sContext.AuthInfo {
 				return namedAuthInfo.AuthInfo, true
 			}
 		}
 		return k8sapi.AuthInfo{}, false
 	}()
 	if !ok {
-		return cluster, user, "", fmt.Errorf("no user named %q found", context.AuthInfo)
+		return cluster, user, "", fmt.Errorf("no user named %q found", k8sContext.AuthInfo)
 	}
-	return cluster, user, context.Namespace, nil
+	return cluster, user, k8sContext.Namespace, nil
 }


Test patch:
diff --git a/storage/kubernetes/client_test.go b/storage/kubernetes/client_test.go
index 5793d7d1..0f7b0789 100644
--- a/storage/kubernetes/client_test.go
+++ b/storage/kubernetes/client_test.go
@@ -3,8 +3,12 @@ package kubernetes
 import (
 	"hash"
 	"hash/fnv"
+	"io/ioutil"
+	"os"
 	"sync"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 // This test does not have an explicit error condition but is used
@@ -42,18 +46,101 @@ func TestOfflineTokenName(t *testing.T) {
 	}
 }
 
-func TestNamespaceFromServiceAccountJWT(t *testing.T) {
-	namespace, err := namespaceFromServiceAccountJWT(serviceAccountToken)
-	if err != nil {
-		t.Fatal(err)
+func TestGetClusterConfigNamespace(t *testing.T) {
+	const namespaceENVVariableName = "TEST_GET_CLUSTER_CONFIG_NAMESPACE"
+	{
+		os.Setenv(namespaceENVVariableName, "namespace-from-env")
+		defer os.Unsetenv(namespaceENVVariableName)
 	}
-	wantNamespace := "dex-test-namespace"
-	if namespace != wantNamespace {
-		t.Errorf("expected namespace %q got %q", wantNamespace, namespace)
+
+	var namespaceFile string
+	{
+		tmpfile, err := ioutil.TempFile(os.TempDir(), "test-get-cluster-config-namespace")
+		require.NoError(t, err)
+
+		_, err = tmpfile.Write([]byte("namespace-from-file"))
+		require.NoError(t, err)
+
+		namespaceFile = tmpfile.Name()
+		defer os.Remove(namespaceFile)
+	}
+
+	tests := []struct {
+		name        string
+		token       string
+		fileName    string
+		envVariable string
+
+		expectedError     bool
+		expectedNamespace string
+	}{
+		{
+			name:        "With env variable",
+			envVariable: "TEST_GET_CLUSTER_CONFIG_NAMESPACE",
+
+			expectedNamespace: "namespace-from-env",
+		},
+		{
+			name:  "With token",
+			token: serviceAccountToken,
+
+			expectedNamespace: "dex-test-namespace",
+		},
+		{
+			name:     "With namespace file",
+			fileName: namespaceFile,
+
+			expectedNamespace: "namespace-from-file",
+		},
+		{
+			name:     "With file and token",
+			fileName: namespaceFile,
+			token:    serviceAccountToken,
+
+			expectedNamespace: "dex-test-namespace",
+		},
+		{
+			name:        "With file and env",
+			fileName:    namespaceFile,
+			envVariable: "TEST_GET_CLUSTER_CONFIG_NAMESPACE",
+
+			expectedNamespace: "namespace-from-env",
+		},
+		{
+			name:        "With token and env",
+			envVariable: "TEST_GET_CLUSTER_CONFIG_NAMESPACE",
+			token:       serviceAccountToken,
+
+			expectedNamespace: "namespace-from-env",
+		},
+		{
+			name:        "With file, token and env",
+			fileName:    namespaceFile,
+			token:       serviceAccountToken,
+			envVariable: "TEST_GET_CLUSTER_CONFIG_NAMESPACE",
+
+			expectedNamespace: "namespace-from-env",
+		},
+		{
+			name:          "Without anything",
+			expectedError: true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			namespace, err := getInClusterConfigNamespace(tc.token, tc.envVariable, tc.fileName)
+			if tc.expectedError {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+			require.Equal(t, namespace, tc.expectedNamespace)
+		})
 	}
 }
 
-var serviceAccountToken = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJkZXgtdGVzdC1uYW1lc3BhY2UiLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlY3JldC5uYW1lIjoiZG90aGVyb2JvdC1zZWNyZXQiLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlcnZpY2UtYWNjb3VudC5uYW1lIjoiZG90aGVyb2JvdCIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50LnVpZCI6IjQyYjJhOTRmLTk4MjAtMTFlNi1iZDc0LTJlZmQzOGYxMjYxYyIsInN1YiI6InN5c3RlbTpzZXJ2aWNlYWNjb3VudDpkZXgtdGVzdC1uYW1lc3BhY2U6ZG90aGVyb2JvdCJ9.KViBpPwCiBwxDvAjYUUXoVvLVwqV011aLlYQpNtX12Bh8M-QAFch-3RWlo_SR00bcdFg_nZo9JKACYlF_jHMEsf__PaYms9r7vEaSg0jPfkqnL2WXZktzQRyLBr0n-bxeUrbwIWsKOAC0DfFB5nM8XoXljRmq8yAx8BAdmQp7MIFb4EOV9nYthhua6pjzYyaFSiDiYTjw7HtXOvoL8oepodJ3-37pUKS8vdBvnvUoqC4M1YAhkO5L36JF6KV_RfmG8GPEdNQfXotHcsR-3jKi1n8S5l7Xd-rhrGOhSGQizH3dORzo9GvBAhYeqbq1O-NLzm2EQUiMQayIUx7o4g3Kw"
+const serviceAccountToken = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJkZXgtdGVzdC1uYW1lc3BhY2UiLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlY3JldC5uYW1lIjoiZG90aGVyb2JvdC1zZWNyZXQiLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlcnZpY2UtYWNjb3VudC5uYW1lIjoiZG90aGVyb2JvdCIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50LnVpZCI6IjQyYjJhOTRmLTk4MjAtMTFlNi1iZDc0LTJlZmQzOGYxMjYxYyIsInN1YiI6InN5c3RlbTpzZXJ2aWNlYWNjb3VudDpkZXgtdGVzdC1uYW1lc3BhY2U6ZG90aGVyb2JvdCJ9.KViBpPwCiBwxDvAjYUUXoVvLVwqV011aLlYQpNtX12Bh8M-QAFch-3RWlo_SR00bcdFg_nZo9JKACYlF_jHMEsf__PaYms9r7vEaSg0jPfkqnL2WXZktzQRyLBr0n-bxeUrbwIWsKOAC0DfFB5nM8XoXljRmq8yAx8BAdmQp7MIFb4EOV9nYthhua6pjzYyaFSiDiYTjw7HtXOvoL8oepodJ3-37pUKS8vdBvnvUoqC4M1YAhkO5L36JF6KV_RfmG8GPEdNQfXotHcsR-3jKi1n8S5l7Xd-rhrGOhSGQizH3dORzo9GvBAhYeqbq1O-NLzm2EQUiMQayIUx7o4g3Kw"
 
 // The following program was used to generate the example token. Since we don't want to
 // import Kubernetes, just leave it as a comment.