From fbae26fc11843000f0d78d0087cc351fe6cf06f7 Mon Sep 17 00:00:00 2001 From: Tarun Pothulapati Date: Fri, 7 Dec 2018 22:50:16 +0530 Subject: [PATCH] env: fix pod envFrom processing --- test/e2e/basic_test.go | 122 ++++++++++++++++++++++++++++++++++++++ test/e2e/framework/env.go | 81 +++++++++++++++++++++++++ vkubelet/env.go | 88 +++++++++++++++++++++------ vkubelet/podcontroller.go | 2 +- 4 files changed, 274 insertions(+), 19 deletions(-) create mode 100644 test/e2e/framework/env.go diff --git a/test/e2e/basic_test.go b/test/e2e/basic_test.go index d92d6a077..62b178dfa 100644 --- a/test/e2e/basic_test.go +++ b/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) { diff --git a/test/e2e/framework/env.go b/test/e2e/framework/env.go new file mode 100644 index 000000000..16a632950 --- /dev/null +++ b/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 +} diff --git a/vkubelet/env.go b/vkubelet/env.go index f63d458f3..629f041dc 100644 --- a/vkubelet/env.go +++ b/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 { + // TODO: Emit an event (the contents of which possibly depend on whether the error is "NotFound", for clarity). + continue + } + // 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) } - - var ok bool - if c.Env[i].Value, ok = cm.Data[vf.Key]; !ok { - 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 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) - if vf.Optional != nil && !*vf.Optional && errors.IsNotFound(err) { - return fmt.Errorf("Secret %s is required by Pod %s and does not exist", vf.Name, pod.Name) + // 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 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 + } + // 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 secret %q not found", vf.Name) + } + return fmt.Errorf("failed to fetch required secret %q: %v", vf.Name, err) } - v, ok := sec.Data[vf.Key] - if !ok { - return fmt.Errorf("Secret %s key %s is required by Pod %s and does not exist", vf.Name, vf.Key, pod.Name) + // 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) } - c.Env[i].Value = string(v) + // Populate the environment variable and continue on to the next reference. + c.Env[i].Value = string(keyValue) continue } diff --git a/vkubelet/podcontroller.go b/vkubelet/podcontroller.go index b2093456b..624dd50aa 100644 --- a/vkubelet/podcontroller.go +++ b/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 }