From c437e05ad0de7c681bf1da8647977cfaf88eacdf Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Sat, 24 Oct 2020 12:07:27 -0700 Subject: [PATCH] Move env var code into its own package This creates a new package -- podutils. The env var related code doesn't really have any business being part of the node package, and to create a separation of concerns, faster tests, and just general code isolation and cleanliness, we can move the env var related code into this package. This change is purely hygiene, and not logic related. For node, the package is under internal, because the constructor references manager, which is an internal package. --- {node => internal/podutils}/env.go | 7 ++--- .../podutils}/env_internal_test.go | 28 +++++++++---------- node/pod.go | 3 +- test/e2e/basic.go | 10 +++---- 4 files changed, 24 insertions(+), 24 deletions(-) rename {node => internal/podutils}/env.go (98%) rename {node => internal/podutils}/env_internal_test.go (97%) diff --git a/node/env.go b/internal/podutils/env.go similarity index 98% rename from node/env.go rename to internal/podutils/env.go index c06ca2ab6..9c4a6bcc8 100755 --- a/node/env.go +++ b/internal/podutils/env.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package node +package podutils import ( "context" @@ -71,9 +71,8 @@ const ( var masterServices = sets.NewString("kubernetes") -// populateEnvironmentVariables populates the environment of each container (and init container) in the specified pod. -// TODO Make this the single exported function of a "pkg/environment" package in the future. -func populateEnvironmentVariables(ctx context.Context, pod *corev1.Pod, rm *manager.ResourceManager, recorder record.EventRecorder) error { +// PopulateEnvironmentVariables populates the environment of each container (and init container) in the specified pod. +func PopulateEnvironmentVariables(ctx context.Context, pod *corev1.Pod, rm *manager.ResourceManager, recorder record.EventRecorder) error { // Populate each init container's environment. for idx := range pod.Spec.InitContainers { diff --git a/node/env_internal_test.go b/internal/podutils/env_internal_test.go similarity index 97% rename from node/env_internal_test.go rename to internal/podutils/env_internal_test.go index bf472383b..d40632ee8 100644 --- a/node/env_internal_test.go +++ b/internal/podutils/env_internal_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package node +package podutils import ( "context" @@ -215,7 +215,7 @@ func TestPopulatePodWithInitContainersUsingEnv(t *testing.T) { } // Populate the pod's environment. - err := populateEnvironmentVariables(context.Background(), pod, rm, er) + err := PopulateEnvironmentVariables(context.Background(), pod, rm, er) assert.Check(t, err) // Make sure that all the containers' environments contain all the expected keys and values. @@ -375,7 +375,7 @@ func TestPopulatePodWithInitContainersUsingEnvWithFieldRef(t *testing.T) { } // Populate the pod's environment. - err := populateEnvironmentVariables(context.Background(), pod, rm, er) + err := PopulateEnvironmentVariables(context.Background(), pod, rm, er) assert.NilError(t, err) // Make sure that all the containers' environments contain all the expected keys and values. @@ -491,7 +491,7 @@ func TestPopulatePodWithInitContainersUsingEnvFrom(t *testing.T) { } // Populate the pod's environment. - err := populateEnvironmentVariables(context.Background(), pod, rm, er) + err := PopulateEnvironmentVariables(context.Background(), pod, rm, er) assert.Check(t, err) // Make sure that all the containers' environments contain all the expected keys and values. @@ -632,7 +632,7 @@ func TestEnvFromConfigMapAndSecretWithInvalidKeys(t *testing.T) { } // Populate the pods's environment. - err := populateEnvironmentVariables(context.Background(), pod, rm, er) + err := PopulateEnvironmentVariables(context.Background(), pod, rm, er) assert.Check(t, err) // Make sure that the container's environment has two variables (corresponding to the single valid key in both the configmap and the secret). @@ -703,7 +703,7 @@ func TestEnvOverridesEnvFrom(t *testing.T) { } // Populate the pods's environment. - err := populateEnvironmentVariables(context.Background(), pod, rm, er) + 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. @@ -780,7 +780,7 @@ func TestEnvFromInexistentConfigMaps(t *testing.T) { } // Populate the pods's environment. - err := populateEnvironmentVariables(context.Background(), pod, rm, er) + err := PopulateEnvironmentVariables(context.Background(), pod, rm, er) assert.Check(t, is.ErrorContains(err, "")) // Make sure that two events have been recorded with the correct reason and message. @@ -837,7 +837,7 @@ func TestEnvFromInexistentSecrets(t *testing.T) { } // Populate the pods's environment. - err := populateEnvironmentVariables(context.Background(), pod, rm, er) + err := PopulateEnvironmentVariables(context.Background(), pod, rm, er) assert.Check(t, is.ErrorContains(err, "")) // Make sure that two events have been recorded with the correct reason and message. @@ -877,7 +877,7 @@ func TestEnvReferencingInexistentConfigMapKey(t *testing.T) { }, Key: "key", // This scenario has been observed before https://github.com/virtual-kubelet/virtual-kubelet/issues/444#issuecomment-449611851. - // A nil value of optional means "mandatory", hence we should expect "populateEnvironmentVariables" to return an error. + // A nil value of optional means "mandatory", hence we should expect "PopulateEnvironmentVariables" to return an error. Optional: nil, }, }, @@ -890,7 +890,7 @@ func TestEnvReferencingInexistentConfigMapKey(t *testing.T) { } // Populate the pods's environment. - err := populateEnvironmentVariables(context.Background(), pod, rm, er) + err := PopulateEnvironmentVariables(context.Background(), pod, rm, er) assert.Check(t, is.ErrorContains(err, "")) // Make sure that two events have been recorded with the correct reason and message. @@ -927,7 +927,7 @@ func TestEnvReferencingInexistentSecretKey(t *testing.T) { }, Key: "key", // This scenario has been observed before https://github.com/virtual-kubelet/virtual-kubelet/issues/444#issuecomment-449611851. - // A nil value of optional means "mandatory", hence we should expect "populateEnvironmentVariables" to return an error. + // A nil value of optional means "mandatory", hence we should expect "PopulateEnvironmentVariables" to return an error. Optional: nil, }, }, @@ -940,7 +940,7 @@ func TestEnvReferencingInexistentSecretKey(t *testing.T) { } // Populate the pods's environment. - err := populateEnvironmentVariables(context.Background(), pod, rm, er) + err := PopulateEnvironmentVariables(context.Background(), pod, rm, er) assert.Check(t, is.ErrorContains(err, "")) // Make sure that two events have been recorded with the correct reason and message. @@ -1023,7 +1023,7 @@ func TestServiceEnvVar(t *testing.T) { for _, tc := range testCases { pod.Spec.EnableServiceLinks = tc.enableServiceLinks - err := populateEnvironmentVariables(context.Background(), pod, rm, er) + err := PopulateEnvironmentVariables(context.Background(), pod, rm, er) assert.NilError(t, err, "[%s]", tc.name) assert.Check(t, is.DeepEqual(pod.Spec.Containers[0].Env, tc.expectedEnvs, sortOpt)) } @@ -1066,7 +1066,7 @@ func TestComposingEnv(t *testing.T) { } // Populate the pods's environment. - err := populateEnvironmentVariables(context.Background(), pod, rm, er) + 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. diff --git a/node/pod.go b/node/pod.go index 7fa15f3af..3dc8d38fd 100644 --- a/node/pod.go +++ b/node/pod.go @@ -21,6 +21,7 @@ import ( "github.com/google/go-cmp/cmp" pkgerrors "github.com/pkg/errors" + "github.com/virtual-kubelet/virtual-kubelet/internal/podutils" "github.com/virtual-kubelet/virtual-kubelet/log" "github.com/virtual-kubelet/virtual-kubelet/trace" corev1 "k8s.io/api/core/v1" @@ -68,7 +69,7 @@ func (pc *PodController) createOrUpdatePod(ctx context.Context, pod *corev1.Pod) // We do this so we don't mutate the pod from the informer cache pod = pod.DeepCopy() - if err := populateEnvironmentVariables(ctx, pod, pc.resourceManager, pc.recorder); err != nil { + if err := podutils.PopulateEnvironmentVariables(ctx, pod, pc.resourceManager, pc.recorder); err != nil { span.SetStatus(err) return err } diff --git a/test/e2e/basic.go b/test/e2e/basic.go index c0f45c628..66efed094 100644 --- a/test/e2e/basic.go +++ b/test/e2e/basic.go @@ -6,7 +6,7 @@ import ( "testing" "time" - "github.com/virtual-kubelet/virtual-kubelet/node" + "github.com/virtual-kubelet/virtual-kubelet/internal/podutils" "gotest.tools/assert" v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -289,7 +289,7 @@ func (ts *EndToEndTestSuite) TestCreatePodWithOptionalInexistentSecrets(t *testi } // Wait for an event concerning the missing secret to be reported on the pod. - if err := f.WaitUntilPodEventWithReason(pod, node.ReasonOptionalSecretNotFound); err != nil { + if err := f.WaitUntilPodEventWithReason(pod, podutils.ReasonOptionalSecretNotFound); err != nil { t.Fatal(err) } @@ -320,7 +320,7 @@ func (ts *EndToEndTestSuite) TestCreatePodWithMandatoryInexistentSecrets(t *test }() // Wait for an event concerning the missing secret to be reported on the pod. - if err := f.WaitUntilPodEventWithReason(pod, node.ReasonMandatorySecretNotFound); err != nil { + if err := f.WaitUntilPodEventWithReason(pod, podutils.ReasonMandatorySecretNotFound); err != nil { t.Fatal(err) } @@ -356,7 +356,7 @@ func (ts *EndToEndTestSuite) TestCreatePodWithOptionalInexistentConfigMap(t *tes } // Wait for an event concerning the missing config map to be reported on the pod. - if err := f.WaitUntilPodEventWithReason(pod, node.ReasonOptionalConfigMapNotFound); err != nil { + if err := f.WaitUntilPodEventWithReason(pod, podutils.ReasonOptionalConfigMapNotFound); err != nil { t.Fatal(err) } @@ -387,7 +387,7 @@ func (ts *EndToEndTestSuite) TestCreatePodWithMandatoryInexistentConfigMap(t *te }() // Wait for an event concerning the missing config map to be reported on the pod. - if err := f.WaitUntilPodEventWithReason(pod, node.ReasonMandatoryConfigMapNotFound); err != nil { + if err := f.WaitUntilPodEventWithReason(pod, podutils.ReasonMandatoryConfigMapNotFound); err != nil { t.Fatal(err) }