Allow composing env var from existing env vars. (#643)

Example:
env name:  FOO
    value: "foo"
    name:  BAR
    value: "bar"
    name:  FOOBAR
    value: "${FOO}${BAR}" <-- should expand to: "foobar"

Added testcase for the same as well.
Change is based on kubelet_pods.go.
Simplified some of the existing code.

Gopkg.lock

@@ -1272,7 +1272,7 @@
   revision = "0317810137be915b9cf888946c6e115c1bfac693"
 
 [[projects]]
-  digest = "1:4ff70ea1888545c3a245a76657ba38a308d2c068bd26fa4310f63560ebaf265c"
+  digest = "1:b78561dbff036b36f419a270993ca22bb6fdafd84085686f24e4a5f8e31056be"
   name = "k8s.io/kubernetes"
   packages = [
     "pkg/api/v1/pod",
@@ -1285,6 +1285,7 @@
     "pkg/kubelet/apis/stats/v1alpha1",
     "pkg/kubelet/envvars",
     "pkg/kubelet/server/remotecommand",
+    "third_party/forked/golang/expansion",
   ]
   pruneopts = "NUT"
   revision = "c27b913fddd1a6c480c229191a087698aa92f0b1"
@@ -1409,6 +1410,7 @@
     "k8s.io/kubernetes/pkg/kubelet/apis/stats/v1alpha1",
     "k8s.io/kubernetes/pkg/kubelet/envvars",
     "k8s.io/kubernetes/pkg/kubelet/server/remotecommand",
+    "k8s.io/kubernetes/third_party/forked/golang/expansion",
   ]
   solver-name = "gps-cdcl"
   solver-version = 1

vendor/k8s.io/kubernetes/third_party/forked/golang/expansion/expand.go

@@ -0,0 +1,102 @@
+package expansion
+
+import (
+	"bytes"
+)
+
+const (
+	operator        = '$'
+	referenceOpener = '('
+	referenceCloser = ')'
+)
+
+// syntaxWrap returns the input string wrapped by the expansion syntax.
+func syntaxWrap(input string) string {
+	return string(operator) + string(referenceOpener) + input + string(referenceCloser)
+}
+
+// MappingFuncFor returns a mapping function for use with Expand that
+// implements the expansion semantics defined in the expansion spec; it
+// returns the input string wrapped in the expansion syntax if no mapping
+// for the input is found.
+func MappingFuncFor(context ...map[string]string) func(string) string {
+	return func(input string) string {
+		for _, vars := range context {
+			val, ok := vars[input]
+			if ok {
+				return val
+			}
+		}
+
+		return syntaxWrap(input)
+	}
+}
+
+// Expand replaces variable references in the input string according to
+// the expansion spec using the given mapping function to resolve the
+// values of variables.
+func Expand(input string, mapping func(string) string) string {
+	var buf bytes.Buffer
+	checkpoint := 0
+	for cursor := 0; cursor < len(input); cursor++ {
+		if input[cursor] == operator && cursor+1 < len(input) {
+			// Copy the portion of the input string since the last
+			// checkpoint into the buffer
+			buf.WriteString(input[checkpoint:cursor])
+
+			// Attempt to read the variable name as defined by the
+			// syntax from the input string
+			read, isVar, advance := tryReadVariableName(input[cursor+1:])
+
+			if isVar {
+				// We were able to read a variable name correctly;
+				// apply the mapping to the variable name and copy the
+				// bytes into the buffer
+				buf.WriteString(mapping(read))
+			} else {
+				// Not a variable name; copy the read bytes into the buffer
+				buf.WriteString(read)
+			}
+
+			// Advance the cursor in the input string to account for
+			// bytes consumed to read the variable name expression
+			cursor += advance
+
+			// Advance the checkpoint in the input string
+			checkpoint = cursor + 1
+		}
+	}
+
+	// Return the buffer and any remaining unwritten bytes in the
+	// input string.
+	return buf.String() + input[checkpoint:]
+}
+
+// tryReadVariableName attempts to read a variable name from the input
+// string and returns the content read from the input, whether that content
+// represents a variable name to perform mapping on, and the number of bytes
+// consumed in the input string.
+//
+// The input string is assumed not to contain the initial operator.
+func tryReadVariableName(input string) (string, bool, int) {
+	switch input[0] {
+	case operator:
+		// Escaped operator; return it.
+		return input[0:1], false, 1
+	case referenceOpener:
+		// Scan to expression closer
+		for i := 1; i < len(input); i++ {
+			if input[i] == referenceCloser {
+				return input[1:i], true, i + 1
+			}
+		}
+
+		// Incomplete reference; return it.
+		return string(operator) + string(referenceOpener), false, 1
+	default:
+		// Not the beginning of an expression, ie, an operator
+		// that doesn't begin an expression.  Return the operator
+		// and the first rune in the string.
+		return (string(operator) + string(input[0])), false, 1
+	}
+}

vkubelet/env.go

@@ -16,6 +16,7 @@ import (
 	v1helper "k8s.io/kubernetes/pkg/apis/core/v1/helper"
 	fieldpath "k8s.io/kubernetes/pkg/fieldpath"
 	"k8s.io/kubernetes/pkg/kubelet/envvars"
+	"k8s.io/kubernetes/third_party/forked/golang/expansion"
 
 	"github.com/virtual-kubelet/virtual-kubelet/log"
 	"github.com/virtual-kubelet/virtual-kubelet/manager"
@@ -78,12 +79,13 @@ func populateEnvironmentVariables(ctx context.Context, pod *corev1.Pod, rm *mana
 // populateContainerEnvironment populates the environment of a single container in the specified pod.
 func populateContainerEnvironment(ctx context.Context, pod *corev1.Pod, container *corev1.Container, rm *manager.ResourceManager, recorder record.EventRecorder) error {
 	// Create an "environment map" based on the value of the specified container's ".envFrom" field.
-	envFrom, err := makeEnvironmentMapBasedOnEnvFrom(ctx, pod, container, rm, recorder)
+	tmpEnv, err := makeEnvironmentMapBasedOnEnvFrom(ctx, pod, container, rm, recorder)
 	if err != nil {
 		return err
 	}
-	// Create an "environment map" based on the value of the specified container's ".env" field.
+	// Create the final "environment map" for the container using the ".env" and ".envFrom" field
-	env, err := makeEnvironmentMapBasedOnEnv(ctx, pod, container, rm, recorder)
+	// and service environment variables.
+	err = makeEnvironmentMap(ctx, pod, container, rm, recorder, tmpEnv)
 	if err != nil {
 		return err
 	}
@@ -92,7 +94,17 @@ func populateContainerEnvironment(ctx context.Context, pod *corev1.Pod, containe
 	// This is in accordance with what the Kubelet itself does.
 	// https://github.com/kubernetes/kubernetes/blob/v1.13.1/pkg/kubelet/kubelet_pods.go#L557-L558
 	container.EnvFrom = []corev1.EnvFromSource{}
-	container.Env = mergeEnvironments(envFrom, env)
+
+	res := make([]corev1.EnvVar, 0)
+
+	for key, val := range tmpEnv {
+		res = append(res, corev1.EnvVar{
+			Name:  key,
+			Value: val,
+		})
+	}
+	container.Env = res
+
 	return nil
 }
 
@@ -265,17 +277,37 @@ loop:
 	return res, nil
 }
 
-// makeEnvironmentMapBasedOnEnv returns a map representing the resolved environment of the specified container after being populated from the entries in the ".env" field.
+// makeEnvironmentMap returns a map representing the resolved environment of the specified container after being populated from the entries in the ".env" and ".envFrom" field.
-func makeEnvironmentMapBasedOnEnv(ctx context.Context, pod *corev1.Pod, container *corev1.Container, rm *manager.ResourceManager, recorder record.EventRecorder) (map[string]string, error) {
+func makeEnvironmentMap(ctx context.Context, pod *corev1.Pod, container *corev1.Container, rm *manager.ResourceManager, recorder record.EventRecorder, res map[string]string) error {
-	// Create a map to hold the resolved environment variables.
+
-	res := make(map[string]string, len(container.Env))
+	// TODO If pod.Spec.EnableServiceLinks is nil then fail as per 1.14 kubelet.
+	enableServiceLinks := corev1.DefaultEnableServiceLinks
+	if pod.Spec.EnableServiceLinks != nil {
+		enableServiceLinks = *pod.Spec.EnableServiceLinks
+	}
+
+	// Note that there is a race between Kubelet seeing the pod and kubelet seeing the service.
+	// To avoid this users can: (1) wait between starting a service and starting; or (2) detect
+	// missing service env var and exit and be restarted; or (3) use DNS instead of env vars
+	// and keep trying to resolve the DNS name of the service (recommended).
+	svcEnv, err := getServiceEnvVarMap(rm, pod.Namespace, enableServiceLinks)
+	if err != nil {
+		return err
+	}
+
+	// If the variable's Value is set, expand the `$(var)` references to other
+	// variables in the .Value field; the sources of variables are the declared
+	// variables of the container and the service environment variables.
+	mappingFunc := expansion.MappingFuncFor(res, svcEnv)
+
 	// Iterate over environment variables in order to populate the map.
 loop:
 	for _, env := range container.Env {
 		switch {
 		// Handle values that have been directly provided.
 		case env.Value != "":
-			res[env.Name] = env.Value
+			// Expand variable references
+			res[env.Name] = expansion.Expand(env.Value, mappingFunc)
 			continue loop
 		// Handle population from a configmap key.
 		case env.ValueFrom != nil && env.ValueFrom.ConfigMapKeyRef != nil:
@@ -303,10 +335,10 @@ loop:
 				// Hence, we should return a meaningful error.
 				if errors.IsNotFound(err) {
 					recorder.Eventf(pod, corev1.EventTypeWarning, ReasonMandatoryConfigMapNotFound, "configmap %q not found", vf.Name)
-					return nil, fmt.Errorf("configmap %q not found", vf.Name)
+					return fmt.Errorf("configmap %q not found", vf.Name)
 				}
 				recorder.Eventf(pod, corev1.EventTypeWarning, ReasonFailedToReadMandatoryConfigMap, "failed to read configmap %q", vf.Name)
-				return nil, fmt.Errorf("failed to read configmap %q: %v", vf.Name, err)
+				return fmt.Errorf("failed to read configmap %q: %v", vf.Name, err)
 			}
 			// At this point we have successfully fetched the target configmap.
 			// We must now try to grab the requested key.
@@ -325,7 +357,7 @@ loop:
 				// At this point we know the key reference is mandatory.
 				// Hence, we should fail.
 				recorder.Eventf(pod, corev1.EventTypeWarning, ReasonMandatoryConfigMapKeyNotFound, "key %q does not exist in configmap %q", vf.Key, vf.Name)
-				return nil, fmt.Errorf("configmap %q doesn't contain the %q key required by pod %s", vf.Name, vf.Key, pod.Name)
+				return fmt.Errorf("configmap %q doesn't contain the %q key required by pod %s", vf.Name, vf.Key, pod.Name)
 			}
 			// Populate the environment variable and continue on to the next reference.
 			res[env.Name] = keyValue
@@ -355,10 +387,10 @@ loop:
 				// Hence, we should return a meaningful error.
 				if errors.IsNotFound(err) {
 					recorder.Eventf(pod, corev1.EventTypeWarning, ReasonMandatorySecretNotFound, "secret %q not found", vf.Name)
-					return nil, fmt.Errorf("secret %q not found", vf.Name)
+					return fmt.Errorf("secret %q not found", vf.Name)
 				}
 				recorder.Eventf(pod, corev1.EventTypeWarning, ReasonFailedToReadMandatorySecret, "failed to read secret %q", vf.Name)
-				return nil, fmt.Errorf("failed to read secret %q: %v", vf.Name, err)
+				return fmt.Errorf("failed to read secret %q: %v", vf.Name, err)
 			}
 			// At this point we have successfully fetched the target secret.
 			// We must now try to grab the requested key.
@@ -377,7 +409,7 @@ loop:
 				// At this point we know the key reference is mandatory.
 				// Hence, we should fail.
 				recorder.Eventf(pod, corev1.EventTypeWarning, ReasonMandatorySecretKeyNotFound, "key %q does not exist in secret %q", vf.Key, vf.Name)
-				return nil, fmt.Errorf("secret %q doesn't contain the %q key required by pod %s", vf.Name, vf.Key, pod.Name)
+				return fmt.Errorf("secret %q doesn't contain the %q key required by pod %s", vf.Name, vf.Key, pod.Name)
 			}
 			// Populate the environment variable and continue on to the next reference.
 			res[env.Name] = string(keyValue)
@@ -389,7 +421,7 @@ loop:
 
 			runtimeVal, err := podFieldSelectorRuntimeValue(vf, pod)
 			if err != nil {
-				return res, err
+				return err
 			}
 
 			res[env.Name] = runtimeVal
@@ -402,21 +434,6 @@ loop:
 		}
 	}
 
-	// TODO If pod.Spec.EnableServiceLinks is nil then fail as per 1.14 kubelet.
-	enableServiceLinks := corev1.DefaultEnableServiceLinks
-	if pod.Spec.EnableServiceLinks != nil {
-		enableServiceLinks = *pod.Spec.EnableServiceLinks
-	}
-
-	// Note that there is a race between Kubelet seeing the pod and kubelet seeing the service.
-	// To avoid this users can: (1) wait between starting a service and starting; or (2) detect
-	// missing service env var and exit and be restarted; or (3) use DNS instead of env vars
-	// and keep trying to resolve the DNS name of the service (recommended).
-	svcEnv, err := getServiceEnvVarMap(rm, pod.Namespace, enableServiceLinks)
-	if err != nil {
-		return nil, err
-	}
-
 	// Append service env vars.
 	for k, v := range svcEnv {
 		if _, present := res[k]; !present {
@@ -424,8 +441,7 @@ loop:
 		}
 	}
 
-	// Return the populated environment.
+	return nil
-	return res, nil
 }
 
 // podFieldSelectorRuntimeValue returns the runtime value of the given
@@ -444,25 +460,3 @@ func podFieldSelectorRuntimeValue(fs *corev1.ObjectFieldSelector, pod *corev1.Po
 	}
 	return fieldpath.ExtractFieldPathAsString(pod, internalFieldPath)
 }
-
-// mergeEnvironments creates the final environment for a container by merging "envFrom" and "env".
-// Values in "env" override any values with the same key defined in "envFrom".
-// This is in accordance with what the Kubelet itself does.
-// https://github.com/kubernetes/kubernetes/blob/v1.13.1/pkg/kubelet/kubelet_pods.go#L557-L558
-func mergeEnvironments(envFrom map[string]string, env map[string]string) []corev1.EnvVar {
-	tmp := make(map[string]string, 0)
-	res := make([]corev1.EnvVar, 0)
-	for key, val := range envFrom {
-		tmp[key] = val
-	}
-	for key, val := range env {
-		tmp[key] = val
-	}
-	for key, val := range tmp {
-		res = append(res, corev1.EnvVar{
-			Name:  key,
-			Value: val,
-		})
-	}
-	return res
-}

vkubelet/env_internal_test.go

@@ -23,6 +23,10 @@ const (
 	envVarValue1 = "foo_value"
 	// envVarName2 is a string that can be used as the name of an environment value.
 	envVarName2 = "BAR"
+	// envVarValue2 is a string meant to be used as the value of the "envVarName2" environment value.
+	envVarValue2 = "bar_value"
+	// envVarName12 is a key that can be used as the name of an environment variable.
+	envVarName12 = "FOOBAR"
 	// envVarName3 is a string that can be used as the name of an environment value.
 	envVarName3 = "CHO"
 	// envVarName4 is a string that can be used as the name of an environment value.
@@ -1011,3 +1015,64 @@ func TestServiceEnvVar(t *testing.T) {
 	}
 
 }
+
+// TestComposingEnv tests that env var can be composed from the existing env vars.
+func TestComposingEnv(t *testing.T) {
+	rm := testutil.FakeResourceManager()
+	er := testutil.FakeEventRecorder(defaultEventRecorderBufferSize)
+
+	// Create a pod object having a single container.
+	// The container's third environment variable is composed of the previous two.
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: namespace,
+			Name:      "pod-0",
+		},
+		Spec: corev1.PodSpec{
+			Containers: []corev1.Container{
+				{
+					Env: []corev1.EnvVar{
+						{
+							Name:  envVarName1,
+							Value: envVarValue1,
+						},
+						{
+							Name:  envVarName2,
+							Value: envVarValue2,
+						},
+						{
+							Name:  envVarName12,
+							Value: "$(" + envVarName1 + ")$(" + envVarName2 + ")", // "$(envVarName1)$(envVarName2)"
+						},
+					},
+				},
+			},
+			EnableServiceLinks: &bFalse,
+		},
+	}
+
+	// Populate the pods's environment.
+	err := populateEnvironmentVariables(context.Background(), pod, rm, er)
+	assert.Check(t, err)
+
+	// Make sure that the container's environment contains all the expected keys and values.
+	assert.Check(t, is.DeepEqual(pod.Spec.Containers[0].Env, []corev1.EnvVar{
+		{
+			Name:  envVarName1,
+			Value: envVarValue1,
+		},
+		{
+			Name:  envVarName2,
+			Value: envVarValue2,
+		},
+		{
+			Name:  envVarName12,
+			Value: envVarValue1 + envVarValue2,
+		},
+	},
+		sortOpt,
+	))
+
+	// Make sure that no events have been recorded.
+	assert.Check(t, is.Len(er.Events, 0))
+}