env: fix pod envFrom processing

test/e2e/basic_test.go

@@ -189,6 +189,128 @@ func TestPodLifecycle(t *testing.T) {
 	}
 }
 
+// TestCreatePodWithOptionalInexistantSecrets tries to create a pod referencing optional, inexistant secrets.
+// It then verifies that the pod is created successfully.
+func TestCreatePodWithOptionalInexistantSecrets(t *testing.T) {
+	// Create a pod with a single container referencing optional, inexistant secrets.
+	pod, err := f.CreatePod(f.CreatePodObjectWithOptionalSecretKey())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Delete the pod after the test finishes.
+	defer func() {
+		if err := f.DeletePod(pod.Namespace, pod.Name); err != nil && !apierrors.IsNotFound(err) {
+			t.Error(err)
+		}
+	}()
+
+	// Wait for the pod to be reported as running and ready.
+	if err := f.WaitUntilPodReady(pod.Namespace, pod.Name); err != nil {
+		t.Fatal(err)
+	}
+
+	// Check that the pod is known to the provider.
+	stats, err := f.GetStatsSummary()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := findPodInPodStats(stats, pod); err != nil {
+		t.Fatal(err)
+	}
+}
+
+// TestCreatePodWithMandatoryInexistantSecrets tries to create a pod referencing inexistant secrets.
+// It then verifies that the pod is not created.
+func TestCreatePodWithMandatoryInexistantSecrets(t *testing.T) {
+	// Create a pod with a single container referencing inexistant secrets.
+	pod, err := f.CreatePod(f.CreatePodObjectWithMandatorySecretKey())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Delete the pod after the test finishes.
+	defer func() {
+		if err := f.DeletePodImmediately(pod.Namespace, pod.Name); err != nil && !apierrors.IsNotFound(err) {
+			t.Error(err)
+		}
+	}()
+
+	// Wait for a small period so we can later assert the pod is not running.
+	// TODO (@pires) Wait for event to be reported.
+	time.Sleep(5 * time.Second)
+
+	// Check that the pod is NOT known to the provider.
+	stats, err := f.GetStatsSummary()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := findPodInPodStats(stats, pod); err == nil {
+		t.Fatalf("Expecting to NOT find pod \"%s/%s\" having mandatory, inexistant secrets.", pod.Namespace, pod.Name)
+	}
+}
+
+// TestCreatePodWithOptionalInexistantConfigMap tries to create a pod referencing optional, inexistant config map.
+// It then verifies that the pod is created successfully.
+func TestCreatePodWithOptionalInexistantConfigMap(t *testing.T) {
+	// Create a pod with a single container referencing optional, inexistant config map.
+	pod, err := f.CreatePod(f.CreatePodObjectWithOptionalConfigMapKey())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Delete the pod after the test finishes.
+	defer func() {
+		if err := f.DeletePod(pod.Namespace, pod.Name); err != nil && !apierrors.IsNotFound(err) {
+			t.Error(err)
+		}
+	}()
+
+	// Wait for the pod to be reported as running and ready.
+	if err := f.WaitUntilPodReady(pod.Namespace, pod.Name); err != nil {
+		t.Fatal(err)
+	}
+
+	// Check that the pod is known to the provider.
+	stats, err := f.GetStatsSummary()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := findPodInPodStats(stats, pod); err != nil {
+		t.Fatal(err)
+	}
+}
+
+// TestCreatePodWithMandatoryInexistantConfigMap tries to create a pod referencing inexistant secrets.
+// It then verifies that the pod is not created.
+func TestCreatePodWithMandatoryInexistantConfigMap(t *testing.T) {
+	// Create a pod with a single container referencing inexistant config map.
+	pod, err := f.CreatePod(f.CreatePodObjectWithMandatoryConfigMapKey())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Delete the pod after the test finishes.
+	defer func() {
+		if err := f.DeletePodImmediately(pod.Namespace, pod.Name); err != nil && !apierrors.IsNotFound(err) {
+			t.Error(err)
+		}
+	}()
+
+	// Wait for a small period so we can later assert the pod is not running.
+	// TODO (@pires) Wait for event to be reported.
+	time.Sleep(5 * time.Second)
+
+	// Check that the pod is NOT known to the provider.
+	stats, err := f.GetStatsSummary()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := findPodInPodStats(stats, pod); err == nil {
+		t.Fatalf("Expecting to NOT find pod \"%s/%s\" having mandatory, inexistant config map.", pod.Namespace, pod.Name)
+	}
+}
+
 // findPodInPodStats returns the index of the specified pod in the .pods field of the specified Summary object.
 // It returns an error if the specified pod is not found.
 func findPodInPodStats(summary *v1alpha1.Summary, pod *v1.Pod) (int, error) {

test/e2e/framework/env.go

@@ -0,0 +1,81 @@
+package framework
+
+import (
+	corev1 "k8s.io/api/core/v1"
+)
+
+var (
+	bFalse = false
+	bTrue  = true
+)
+
+// CreatePodObjectWithMandatoryConfigMapKey creates a pod object that references the "key_0" key from the "config-map-0" config map as mandatory.
+func (f *Framework) CreatePodObjectWithMandatoryConfigMapKey() *corev1.Pod {
+	return f.CreatePodObjectWithEnv([]corev1.EnvVar{
+		{
+			Name: "CONFIG_MAP_0_KEY_0",
+			ValueFrom: &corev1.EnvVarSource{
+				ConfigMapKeyRef: &corev1.ConfigMapKeySelector{
+					LocalObjectReference: corev1.LocalObjectReference{Name: "config-map-0"},
+					Key:                  "key_0",
+					Optional:             &bFalse,
+				},
+			},
+		},
+	})
+}
+
+// CreatePodObjectWithOptionalConfigMapKey creates a pod object that references the "key_0" key from the "config-map-0" config map as optional.
+func (f *Framework) CreatePodObjectWithOptionalConfigMapKey() *corev1.Pod {
+	return f.CreatePodObjectWithEnv([]corev1.EnvVar{
+		{
+			Name: "CONFIG_MAP_0_KEY_0",
+			ValueFrom: &corev1.EnvVarSource{
+				ConfigMapKeyRef: &corev1.ConfigMapKeySelector{
+					LocalObjectReference: corev1.LocalObjectReference{Name: "config-map-0"},
+					Key:                  "key_0",
+					Optional:             &bTrue,
+				},
+			},
+		},
+	})
+}
+
+// CreatePodObjectWithMandatorySecretKey creates a pod object that references the "key_0" key from the "secret-0" config map as mandatory.
+func (f *Framework) CreatePodObjectWithMandatorySecretKey() *corev1.Pod {
+	return f.CreatePodObjectWithEnv([]corev1.EnvVar{
+		{
+			Name: "SECRET_0_KEY_0",
+			ValueFrom: &corev1.EnvVarSource{
+				SecretKeyRef: &corev1.SecretKeySelector{
+					LocalObjectReference: corev1.LocalObjectReference{Name: "secret-0"},
+					Key:                  "key_0",
+					Optional:             &bFalse,
+				},
+			},
+		},
+	})
+}
+
+// CreatePodObjectWithOptionalSecretKey creates a pod object that references the "key_0" key from the "secret-0" config map as optional.
+func (f *Framework) CreatePodObjectWithOptionalSecretKey() *corev1.Pod {
+	return f.CreatePodObjectWithEnv([]corev1.EnvVar{
+		{
+			Name: "SECRET_0_KEY_0",
+			ValueFrom: &corev1.EnvVarSource{
+				SecretKeyRef: &corev1.SecretKeySelector{
+					LocalObjectReference: corev1.LocalObjectReference{Name: "secret-0"},
+					Key:                  "key_0",
+					Optional:             &bTrue,
+				},
+			},
+		},
+	})
+}
+
+// CreatePodObjectWithEnv creates a pod object whose name starts with "env-test-" and that uses the specified environment configuration for its first container.
+func (f *Framework) CreatePodObjectWithEnv(env []corev1.EnvVar) *corev1.Pod {
+	pod := f.CreateDummyPodObjectWithPrefix("env-test-", "foo")
+	pod.Spec.Containers[0].Env = env
+	return pod
+}

vkubelet/env.go

@@ -2,7 +2,6 @@ package vkubelet
 
 import (
 	"fmt"
-
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 )
@@ -15,34 +14,87 @@ func (s *Server) populateEnvironmentVariables(pod *corev1.Pod) error {
 				// Populate ConfigMaps to Env
 				if e.ValueFrom.ConfigMapKeyRef != nil {
 					vf := e.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
+					// Try to grab the referenced config map.
 					cm, err := s.resourceManager.GetConfigMap(vf.Name, pod.Namespace)
-					if vf.Optional != nil && !*vf.Optional && errors.IsNotFound(err) {
-						return fmt.Errorf("ConfigMap %s is required by Pod %s and does not exist", vf.Name, pod.Name)
-					}
-
 					if err != nil {
-						return fmt.Errorf("Error retrieving ConfigMap %s required by Pod %s: %s", vf.Name, pod.Name, err)
+						// We couldn't fetch the config map.
-					}
+						// However, if the key reference is optional we should not fail.
-
+						if optional {
-					var ok bool
+							// TODO: Emit an event (the contents of which possibly depend on whether the error is "NotFound", for clarity).
-					if c.Env[i].Value, ok = cm.Data[vf.Key]; !ok {
+							continue
-						return fmt.Errorf("ConfigMap %s key %s is required by Pod %s and does not exist", vf.Name, vf.Key, pod.Name)
+						}
-					}
+						// At this point we know the key reference is mandatory.
+						// Hence, we should return a meaningful error.
+						if errors.IsNotFound(err) {
+							return fmt.Errorf("required configmap %q not found", vf.Name)
+						}
+						return fmt.Errorf("failed to fetch required configmap %q: %v", vf.Name, err)
+					}
+					// At this point we have successfully fetched the target config map.
+					// We must now try to grab the requested key.
+					var (
+						keyExists bool
+						keyValue  string
+					)
+					if keyValue, keyExists = cm.Data[vf.Key]; !keyExists {
+						// The requested key does not exist.
+						// However, we should not fail if the key reference is optional.
+						if optional {
+							// TODO: Emit an event.
+							continue
+						}
+						// At this point we know the key reference is mandatory.
+						// Hence, we should fail.
+						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.
+					c.Env[i].Value = keyValue
 					continue
 				}
-
 				// Populate Secrets to Env
 				if e.ValueFrom.SecretKeyRef != nil {
 					vf := e.ValueFrom.SecretKeyRef
-					sec, err := s.resourceManager.GetSecret(vf.Name, pod.Namespace)
+					// Check whether the key reference is optional.
-					if vf.Optional != nil && !*vf.Optional && errors.IsNotFound(err) {
+					// This will control whether we fail when unable to read the requested key.
-						return fmt.Errorf("Secret %s is required by Pod %s and does not exist", vf.Name, pod.Name)
+					optional := vf != nil && *vf.Optional
+					// Try to grab the referenced secret.
+					cm, err := s.resourceManager.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 {
+							// TODO: Emit an event (the contents of which possibly depend on whether the error is "NotFound", for clarity).
+							continue
 						}
-					v, ok := sec.Data[vf.Key]
+						// At this point we know the key reference is mandatory.
-					if !ok {
+						// Hence, we should return a meaningful error.
-						return fmt.Errorf("Secret %s key %s is required by Pod %s and does not exist", vf.Name, vf.Key, pod.Name)
+						if errors.IsNotFound(err) {
+							return fmt.Errorf("required secret %q not found", vf.Name)
 						}
-					c.Env[i].Value = string(v)
+						return fmt.Errorf("failed to fetch required 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 = cm.Data[vf.Key]; !keyExists {
+						// The requested key does not exist.
+						// However, we should not fail if the key reference is optional.
+						if optional {
+							// TODO: Emit an event.
+							continue
+						}
+						// At this point we know the key reference is mandatory.
+						// Hence, we should fail.
+						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.
+					c.Env[i].Value = string(keyValue)
 					continue
 				}
 

vkubelet/podcontroller.go

@@ -195,7 +195,7 @@ func (pc *PodController) processNextWorkItem(ctx context.Context, workerId strin
 		if err := pc.syncHandler(ctx, key); err != nil {
 			if pc.workqueue.NumRequeues(key) < maxRetries {
 				// Put the item back on the work queue to handle any transient errors.
-				log.G(ctx).Warnf("requeuing %q due to failed sync", key)
+				log.G(ctx).Warnf("requeuing %q due to failed sync: %v", key, err)
 				pc.workqueue.AddRateLimited(key)
 				return nil
 			}