Refactor env.go

This copies and pastes the loop that used to exist in

func populateEnvironmentVariables(..) {
	...
	for _, env := range container.Env {
		... <--- This code
	}
}

Into getEnvironmentVariableValue. getEnvironmentVariableValue
returns val, err, where val is a pointer to a string
to indicate optionality.

go.mod

@@ -27,6 +27,7 @@ require (
 	k8s.io/klog v1.0.0
 	k8s.io/klog/v2 v2.0.0
 	k8s.io/kubernetes v1.18.4
+	k8s.io/utils v0.0.0-20200603063816-c1c6865ac451
 	sigs.k8s.io/controller-runtime v0.6.3
 )
 

internal/podutils/env.go

@@ -20,6 +20,8 @@ import (
 	"sort"
 	"strings"
 
+	"github.com/virtual-kubelet/virtual-kubelet/internal/manager"
+	"github.com/virtual-kubelet/virtual-kubelet/log"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -31,9 +33,7 @@ import (
 	fieldpath "k8s.io/kubernetes/pkg/fieldpath"
 	"k8s.io/kubernetes/pkg/kubelet/envvars"
 	"k8s.io/kubernetes/third_party/forked/golang/expansion"
-
+	"k8s.io/utils/pointer"
-	"github.com/virtual-kubelet/virtual-kubelet/internal/manager"
-	"github.com/virtual-kubelet/virtual-kubelet/log"
 )
 
 const (
@@ -292,7 +292,6 @@ loop:
 
 // 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 makeEnvironmentMap(ctx context.Context, pod *corev1.Pod, container *corev1.Container, rm *manager.ResourceManager, recorder record.EventRecorder, res map[string]string) error {
-
 	// TODO If pod.Spec.EnableServiceLinks is nil then fail as per 1.14 kubelet.
 	enableServiceLinks := corev1.DefaultEnableServiceLinks
 	if pod.Spec.EnableServiceLinks != nil {
@@ -314,136 +313,13 @@ func makeEnvironmentMap(ctx context.Context, pod *corev1.Pod, container *corev1.
 	mappingFunc := expansion.MappingFuncFor(res, svcEnv)
 
 	// Iterate over environment variables in order to populate the map.
-loop:
 	for _, env := range container.Env {
-		switch {
+		val, err := getEnvironmentVariableValue(ctx, &env, mappingFunc, pod, container, rm, recorder)
-		// Handle values that have been directly provided.
+		if err != nil {
-		case env.Value != "":
+			return err
-			// Expand variable references
+		}
-			res[env.Name] = expansion.Expand(env.Value, mappingFunc)
+		if val != nil {
-			continue loop
+			res[env.Name] = *val
-		// Handle population from a configmap key.
-		case env.ValueFrom != nil && env.ValueFrom.ConfigMapKeyRef != nil:
-			// The environment variable must be set from a configmap.
-			vf := env.ValueFrom.ConfigMapKeyRef
-			// Check whether the key reference is optional.
-			// This will control whether we fail when unable to read the requested key.
-			optional := vf != nil && vf.Optional != nil && *vf.Optional
-			// Try to grab the referenced configmap.
-			m, err := rm.GetConfigMap(vf.Name, pod.Namespace)
-			if err != nil {
-				// We couldn't fetch the configmap.
-				// However, if the key reference is optional we should not fail.
-				if optional {
-					if errors.IsNotFound(err) {
-						recorder.Eventf(pod, corev1.EventTypeWarning, ReasonOptionalConfigMapNotFound, "skipping optional envvar %q: configmap %q not found", env.Name, vf.Name)
-					} else {
-						log.G(ctx).Warnf("failed to read configmap %q: %v", vf.Name, err)
-						recorder.Eventf(pod, corev1.EventTypeWarning, ReasonFailedToReadOptionalConfigMap, "skipping optional envvar %q: failed to read configmap %q", env.Name, vf.Name)
-					}
-					// Continue on to the next reference.
-					continue loop
-				}
-				// At this point we know the key reference is mandatory.
-				// Hence, we should return a meaningful error.
-				if errors.IsNotFound(err) {
-					recorder.Eventf(pod, corev1.EventTypeWarning, ReasonMandatoryConfigMapNotFound, "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 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.
-			var (
-				keyExists bool
-				keyValue  string
-			)
-			if keyValue, keyExists = m.Data[vf.Key]; !keyExists {
-				// The requested key does not exist.
-				// However, we should not fail if the key reference is optional.
-				if optional {
-					// Continue on to the next reference.
-					recorder.Eventf(pod, corev1.EventTypeWarning, ReasonOptionalConfigMapKeyNotFound, "skipping optional envvar %q: key %q does not exist in configmap %q", env.Name, vf.Key, vf.Name)
-					continue 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 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
-			continue loop
-		// Handle population from a secret key.
-		case env.ValueFrom != nil && env.ValueFrom.SecretKeyRef != nil:
-			vf := env.ValueFrom.SecretKeyRef
-			// Check whether the key reference is optional.
-			// This will control whether we fail when unable to read the requested key.
-			optional := vf != nil && vf.Optional != nil && *vf.Optional
-			// Try to grab the referenced secret.
-			s, err := rm.GetSecret(vf.Name, pod.Namespace)
-			if err != nil {
-				// We couldn't fetch the secret.
-				// However, if the key reference is optional we should not fail.
-				if optional {
-					if errors.IsNotFound(err) {
-						recorder.Eventf(pod, corev1.EventTypeWarning, ReasonOptionalSecretNotFound, "skipping optional envvar %q: secret %q not found", env.Name, vf.Name)
-					} else {
-						log.G(ctx).Warnf("failed to read secret %q: %v", vf.Name, err)
-						recorder.Eventf(pod, corev1.EventTypeWarning, ReasonFailedToReadOptionalSecret, "skipping optional envvar %q: failed to read secret %q", env.Name, vf.Name)
-					}
-					// Continue on to the next reference.
-					continue loop
-				}
-				// At this point we know the key reference is mandatory.
-				// Hence, we should return a meaningful error.
-				if errors.IsNotFound(err) {
-					recorder.Eventf(pod, corev1.EventTypeWarning, ReasonMandatorySecretNotFound, "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 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.
-			var (
-				keyExists bool
-				keyValue  []byte
-			)
-			if keyValue, keyExists = s.Data[vf.Key]; !keyExists {
-				// The requested key does not exist.
-				// However, we should not fail if the key reference is optional.
-				if optional {
-					// Continue on to the next reference.
-					recorder.Eventf(pod, corev1.EventTypeWarning, ReasonOptionalSecretKeyNotFound, "skipping optional envvar %q: key %q does not exist in secret %q", env.Name, vf.Key, vf.Name)
-					continue 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 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)
-			continue loop
-		// Handle population from a field (downward API).
-		case env.ValueFrom != nil && env.ValueFrom.FieldRef != nil:
-			// https://github.com/virtual-kubelet/virtual-kubelet/issues/123
-			vf := env.ValueFrom.FieldRef
-
-			runtimeVal, err := podFieldSelectorRuntimeValue(vf, pod)
-			if err != nil {
-				return err
-			}
-
-			res[env.Name] = runtimeVal
-
-			continue loop
-		// Handle population from a resource request/limit.
-		case env.ValueFrom != nil && env.ValueFrom.ResourceFieldRef != nil:
-			// TODO Implement populating resource requests.
-			continue loop
 		}
 	}
 
@@ -457,6 +333,137 @@ loop:
 	return nil
 }
 
+func getEnvironmentVariableValue(ctx context.Context, env *corev1.EnvVar, mappingFunc func(string) string, pod *corev1.Pod, container *corev1.Container, rm *manager.ResourceManager, recorder record.EventRecorder) (*string, error) {
+	switch {
+	// Handle values that have been directly provided.
+	case env.Value != "":
+		// Expand variable references
+		return pointer.StringPtr(expansion.Expand(env.Value, mappingFunc)), nil
+	// Handle population from a configmap key.
+	case env.ValueFrom != nil && env.ValueFrom.ConfigMapKeyRef != nil:
+		// The environment variable must be set from a configmap.
+		vf := env.ValueFrom.ConfigMapKeyRef
+		// Check whether the key reference is optional.
+		// This will control whether we fail when unable to read the requested key.
+		optional := vf != nil && vf.Optional != nil && *vf.Optional
+		// Try to grab the referenced configmap.
+		m, err := rm.GetConfigMap(vf.Name, pod.Namespace)
+		if err != nil {
+			// We couldn't fetch the configmap.
+			// However, if the key reference is optional we should not fail.
+			if optional {
+				if errors.IsNotFound(err) {
+					recorder.Eventf(pod, corev1.EventTypeWarning, ReasonOptionalConfigMapNotFound, "skipping optional envvar %q: configmap %q not found", env.Name, vf.Name)
+				} else {
+					log.G(ctx).Warnf("failed to read configmap %q: %v", vf.Name, err)
+					recorder.Eventf(pod, corev1.EventTypeWarning, ReasonFailedToReadOptionalConfigMap, "skipping optional envvar %q: failed to read configmap %q", env.Name, vf.Name)
+				}
+				// Continue on to the next reference.
+				return nil, nil
+			}
+			// At this point we know the key reference is mandatory.
+			// 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)
+			}
+			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)
+		}
+		// At this point we have successfully fetched the target configmap.
+		// We must now try to grab the requested key.
+		var (
+			keyExists bool
+			keyValue  string
+		)
+		if keyValue, keyExists = m.Data[vf.Key]; !keyExists {
+			// The requested key does not exist.
+			// However, we should not fail if the key reference is optional.
+			if optional {
+				// Continue on to the next reference.
+				recorder.Eventf(pod, corev1.EventTypeWarning, ReasonOptionalConfigMapKeyNotFound, "skipping optional envvar %q: key %q does not exist in configmap %q", env.Name, vf.Key, vf.Name)
+				return nil, nil
+			}
+			// 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)
+		}
+		// Populate the environment variable and continue on to the next reference.
+		return pointer.StringPtr(keyValue), nil
+	// Handle population from a secret key.
+	case env.ValueFrom != nil && env.ValueFrom.SecretKeyRef != nil:
+		vf := env.ValueFrom.SecretKeyRef
+		// Check whether the key reference is optional.
+		// This will control whether we fail when unable to read the requested key.
+		optional := vf != nil && vf.Optional != nil && *vf.Optional
+		// Try to grab the referenced secret.
+		s, err := rm.GetSecret(vf.Name, pod.Namespace)
+		if err != nil {
+			// We couldn't fetch the secret.
+			// However, if the key reference is optional we should not fail.
+			if optional {
+				if errors.IsNotFound(err) {
+					recorder.Eventf(pod, corev1.EventTypeWarning, ReasonOptionalSecretNotFound, "skipping optional envvar %q: secret %q not found", env.Name, vf.Name)
+				} else {
+					log.G(ctx).Warnf("failed to read secret %q: %v", vf.Name, err)
+					recorder.Eventf(pod, corev1.EventTypeWarning, ReasonFailedToReadOptionalSecret, "skipping optional envvar %q: failed to read secret %q", env.Name, vf.Name)
+				}
+				// Continue on to the next reference.
+				return nil, nil
+			}
+			// At this point we know the key reference is mandatory.
+			// 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)
+			}
+			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)
+		}
+		// At this point we have successfully fetched the target secret.
+		// We must now try to grab the requested key.
+		var (
+			keyExists bool
+			keyValue  []byte
+		)
+		if keyValue, keyExists = s.Data[vf.Key]; !keyExists {
+			// The requested key does not exist.
+			// However, we should not fail if the key reference is optional.
+			if optional {
+				// Continue on to the next reference.
+				recorder.Eventf(pod, corev1.EventTypeWarning, ReasonOptionalSecretKeyNotFound, "skipping optional envvar %q: key %q does not exist in secret %q", env.Name, vf.Key, vf.Name)
+				return nil, nil
+			}
+			// 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)
+		}
+		// Populate the environment variable and continue on to the next reference.
+		return pointer.StringPtr(string(keyValue)), nil
+	// Handle population from a field (downward API).
+	case env.ValueFrom != nil && env.ValueFrom.FieldRef != nil:
+		// https://github.com/virtual-kubelet/virtual-kubelet/issues/123
+		vf := env.ValueFrom.FieldRef
+
+		runtimeVal, err := podFieldSelectorRuntimeValue(vf, pod)
+		if err != nil {
+			return nil, err
+		}
+
+		return pointer.StringPtr(runtimeVal), nil
+	// Handle population from a resource request/limit.
+	case env.ValueFrom != nil && env.ValueFrom.ResourceFieldRef != nil:
+		// TODO Implement populating resource requests.
+		return nil, nil
+	default:
+		// TODO: Should we throw an error here?
+		log.G(ctx).WithField("env", env).Error("Unhandled environment variable, do not know how to populate")
+		return nil, nil
+	}
+}
+
 // podFieldSelectorRuntimeValue returns the runtime value of the given
 // selector for a pod.
 func podFieldSelectorRuntimeValue(fs *corev1.ObjectFieldSelector, pod *corev1.Pod) (string, error) {