From 7d02ea9bb742d0bd0728d644399fb36cdbabc6c3 Mon Sep 17 00:00:00 2001 From: Andy Goldstein Date: Tue, 13 Oct 2015 15:51:37 -0400 Subject: [PATCH] Fix race condition for consuming podIP via downward API. --- pkg/kubelet/dockertools/manager.go | 64 ++++++++++++---- pkg/kubelet/dockertools/manager_test.go | 58 +++++++++++++++ test/e2e/docker_containers.go | 8 +- test/e2e/downward_api.go | 97 ++++++++++++++++--------- test/e2e/downwardapi_volume.go | 2 +- test/e2e/framework.go | 7 +- test/e2e/host_path.go | 4 +- test/e2e/secrets.go | 2 +- test/e2e/util.go | 31 ++++++-- 9 files changed, 212 insertions(+), 61 deletions(-) diff --git a/pkg/kubelet/dockertools/manager.go b/pkg/kubelet/dockertools/manager.go index 1305c5cd038..94e1126d7f5 100644 --- a/pkg/kubelet/dockertools/manager.go +++ b/pkg/kubelet/dockertools/manager.go @@ -311,16 +311,59 @@ type containerStatusResult struct { err error } +const podIPDownwardAPISelector = "status.podIP" + +// podDependsOnIP returns whether any containers in a pod depend on using the pod IP via +// the downward API. +func podDependsOnPodIP(pod *api.Pod) bool { + for _, container := range pod.Spec.Containers { + for _, env := range container.Env { + if env.ValueFrom != nil && + env.ValueFrom.FieldRef != nil && + env.ValueFrom.FieldRef.FieldPath == podIPDownwardAPISelector { + return true + } + } + } + + return false +} + +// determineContainerIP determines the IP address of the given container. It is expected +// that the container passed is the infrastructure container of a pod and the responsibility +// of the caller to ensure that the correct container is passed. +func (dm *DockerManager) determineContainerIP(podNamespace, podName string, container *docker.Container) string { + result := "" + + if container.NetworkSettings != nil { + result = container.NetworkSettings.IPAddress + } + + if dm.networkPlugin.Name() != network.DefaultPluginName { + netStatus, err := dm.networkPlugin.Status(podNamespace, podName, kubetypes.DockerID(container.ID)) + if err != nil { + glog.Errorf("NetworkPlugin %s failed on the status hook for pod '%s' - %v", dm.networkPlugin.Name(), podName, err) + } else if netStatus != nil { + result = netStatus.IP.String() + } + } + + return result +} + func (dm *DockerManager) inspectContainer(dockerID, containerName, tPath string, pod *api.Pod) *containerStatusResult { result := containerStatusResult{api.ContainerStatus{}, "", nil} inspectResult, err := dm.client.InspectContainer(dockerID) - if err != nil { result.err = err return &result } + // NOTE (pmorie): this is a seriously fishy if statement. A nil result from + // InspectContainer seems like it should should always be paired with a + // non-nil error in the result of InspectContainer. if inspectResult == nil { + glog.Errorf("Received a nil result from InspectContainer without receiving an error for container ID %v", dockerID) // Why did we not get an error? return &result } @@ -338,18 +381,7 @@ func (dm *DockerManager) inspectContainer(dockerID, containerName, tPath string, StartedAt: unversioned.NewTime(inspectResult.State.StartedAt), } if containerName == PodInfraContainerName { - if inspectResult.NetworkSettings != nil { - result.ip = inspectResult.NetworkSettings.IPAddress - } - // override the above if a network plugin exists - if dm.networkPlugin.Name() != network.DefaultPluginName { - netStatus, err := dm.networkPlugin.Status(pod.Namespace, pod.Name, kubetypes.DockerID(dockerID)) - if err != nil { - glog.Errorf("NetworkPlugin %s failed on the status hook for pod '%s' - %v", dm.networkPlugin.Name(), pod.Name, err) - } else if netStatus != nil { - result.ip = netStatus.IP.String() - } - } + result.ip = dm.determineContainerIP(pod.Namespace, pod.Name, inspectResult) } } else if !inspectResult.State.FinishedAt.IsZero() || inspectResult.State.ExitCode != 0 { // When a container fails to start State.ExitCode is non-zero, FinishedAt and StartedAt are both zero @@ -1859,6 +1891,12 @@ func (dm *DockerManager) SyncPod(pod *api.Pod, runningPod kubecontainer.Pod, pod if err = hairpin.SetUpContainer(podInfraContainer.State.Pid, "eth0"); err != nil { glog.Warningf("Hairpin setup failed for pod %q: %v", podFullName, err) } + if podDependsOnPodIP(pod) { + // Find the pod IP after starting the infra container in order to expose + // it safely via the downward API without a race. + pod.Status.PodIP = dm.determineContainerIP(pod.Name, pod.Namespace, podInfraContainer) + } + } // Start everything diff --git a/pkg/kubelet/dockertools/manager_test.go b/pkg/kubelet/dockertools/manager_test.go index a52efda40c6..adc5509b5fb 100644 --- a/pkg/kubelet/dockertools/manager_test.go +++ b/pkg/kubelet/dockertools/manager_test.go @@ -2059,3 +2059,61 @@ func TestGetIPCMode(t *testing.T) { t.Errorf("expected host ipc mode for pod but got %v", ipcMode) } } + +func TestPodDependsOnPodIP(t *testing.T) { + tests := []struct { + name string + expected bool + env api.EnvVar + }{ + { + name: "depends on pod IP", + expected: true, + env: api.EnvVar{ + Name: "POD_IP", + ValueFrom: &api.EnvVarSource{ + FieldRef: &api.ObjectFieldSelector{ + APIVersion: testapi.Default.Version(), + FieldPath: "status.podIP", + }, + }, + }, + }, + { + name: "literal value", + expected: false, + env: api.EnvVar{ + Name: "SOME_VAR", + Value: "foo", + }, + }, + { + name: "other downward api field", + expected: false, + env: api.EnvVar{ + Name: "POD_NAME", + ValueFrom: &api.EnvVarSource{ + FieldRef: &api.ObjectFieldSelector{ + APIVersion: testapi.Default.Version(), + FieldPath: "metadata.name", + }, + }, + }, + }, + } + + for _, tc := range tests { + pod := &api.Pod{ + Spec: api.PodSpec{ + Containers: []api.Container{ + {Env: []api.EnvVar{tc.env}}, + }, + }, + } + + result := podDependsOnPodIP(pod) + if e, a := tc.expected, result; e != a { + t.Errorf("%v: Unexpected result; expected %v, got %v", tc.name, e, a) + } + } +} diff --git a/test/e2e/docker_containers.go b/test/e2e/docker_containers.go index 753ab7178e6..c8f72b08a0c 100644 --- a/test/e2e/docker_containers.go +++ b/test/e2e/docker_containers.go @@ -47,7 +47,7 @@ var _ = Describe("Docker Containers", func() { }) It("should use the image defaults if command and args are blank [Conformance]", func() { - testContainerOutputInNamespace("use defaults", c, entrypointTestPod(), 0, []string{ + testContainerOutput("use defaults", c, entrypointTestPod(), 0, []string{ "[/ep default arguments]", }, ns) }) @@ -56,7 +56,7 @@ var _ = Describe("Docker Containers", func() { pod := entrypointTestPod() pod.Spec.Containers[0].Args = []string{"override", "arguments"} - testContainerOutputInNamespace("override arguments", c, pod, 0, []string{ + testContainerOutput("override arguments", c, pod, 0, []string{ "[/ep override arguments]", }, ns) }) @@ -67,7 +67,7 @@ var _ = Describe("Docker Containers", func() { pod := entrypointTestPod() pod.Spec.Containers[0].Command = []string{"/ep-2"} - testContainerOutputInNamespace("override command", c, pod, 0, []string{ + testContainerOutput("override command", c, pod, 0, []string{ "[/ep-2]", }, ns) }) @@ -77,7 +77,7 @@ var _ = Describe("Docker Containers", func() { pod.Spec.Containers[0].Command = []string{"/ep-2"} pod.Spec.Containers[0].Args = []string{"override", "arguments"} - testContainerOutputInNamespace("override all", c, pod, 0, []string{ + testContainerOutput("override all", c, pod, 0, []string{ "[/ep-2 override arguments]", }, ns) }) diff --git a/test/e2e/downward_api.go b/test/e2e/downward_api.go index 5e362356ed0..73fde0804ba 100644 --- a/test/e2e/downward_api.go +++ b/test/e2e/downward_api.go @@ -30,46 +30,75 @@ var _ = Describe("Downward API", func() { It("should provide pod name and namespace as env vars [Conformance]", func() { podName := "downward-api-" + string(util.NewUUID()) - pod := &api.Pod{ - ObjectMeta: api.ObjectMeta{ - Name: podName, - Labels: map[string]string{"name": podName}, - }, - Spec: api.PodSpec{ - Containers: []api.Container{ - { - Name: "dapi-container", - Image: "gcr.io/google_containers/busybox", - Command: []string{"sh", "-c", "env"}, - Env: []api.EnvVar{ - { - Name: "POD_NAME", - ValueFrom: &api.EnvVarSource{ - FieldRef: &api.ObjectFieldSelector{ - APIVersion: "v1", - FieldPath: "metadata.name", - }, - }, - }, - { - Name: "POD_NAMESPACE", - ValueFrom: &api.EnvVarSource{ - FieldRef: &api.ObjectFieldSelector{ - APIVersion: "v1", - FieldPath: "metadata.namespace", - }, - }, - }, - }, + env := []api.EnvVar{ + { + Name: "POD_NAME", + ValueFrom: &api.EnvVarSource{ + FieldRef: &api.ObjectFieldSelector{ + APIVersion: "v1", + FieldPath: "metadata.name", + }, + }, + }, + { + Name: "POD_NAMESPACE", + ValueFrom: &api.EnvVarSource{ + FieldRef: &api.ObjectFieldSelector{ + APIVersion: "v1", + FieldPath: "metadata.namespace", }, }, - RestartPolicy: api.RestartPolicyNever, }, } - framework.TestContainerOutput("downward api env vars", pod, 0, []string{ + expectations := []string{ fmt.Sprintf("POD_NAME=%v", podName), fmt.Sprintf("POD_NAMESPACE=%v", framework.Namespace.Name), - }) + } + + testDownwardAPI(framework, podName, env, expectations) + }) + + It("should provide pod IP as an env var", func() { + podName := "downward-api-" + string(util.NewUUID()) + env := []api.EnvVar{ + { + Name: "POD_IP", + ValueFrom: &api.EnvVarSource{ + FieldRef: &api.ObjectFieldSelector{ + APIVersion: "v1", + FieldPath: "status.podIP", + }, + }, + }, + } + + expectations := []string{ + "POD_IP=(?:\\d+)\\.(?:\\d+)\\.(?:\\d+)\\.(?:\\d+)", + } + + testDownwardAPI(framework, podName, env, expectations) }) }) + +func testDownwardAPI(framework *Framework, podName string, env []api.EnvVar, expectations []string) { + pod := &api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: podName, + Labels: map[string]string{"name": podName}, + }, + Spec: api.PodSpec{ + Containers: []api.Container{ + { + Name: "dapi-container", + Image: "gcr.io/google_containers/busybox", + Command: []string{"sh", "-c", "env"}, + Env: env, + }, + }, + RestartPolicy: api.RestartPolicyNever, + }, + } + + framework.TestContainerOutputRegexp("downward api env vars", pod, 0, expectations) +} diff --git a/test/e2e/downwardapi_volume.go b/test/e2e/downwardapi_volume.go index c850de1a84e..7e0ff66160c 100644 --- a/test/e2e/downwardapi_volume.go +++ b/test/e2e/downwardapi_volume.go @@ -86,7 +86,7 @@ var _ = Describe("Downward API volume", func() { RestartPolicy: api.RestartPolicyNever, }, } - testContainerOutputInNamespace("downward API volume plugin", f.Client, pod, 0, []string{ + testContainerOutput("downward API volume plugin", f.Client, pod, 0, []string{ fmt.Sprintf("cluster=\"rack10\"\n"), fmt.Sprintf("builder=\"john-doe\"\n"), fmt.Sprintf("%s\n", podName), diff --git a/test/e2e/framework.go b/test/e2e/framework.go index 8dcfc9c9335..c768a860253 100644 --- a/test/e2e/framework.go +++ b/test/e2e/framework.go @@ -121,7 +121,12 @@ func (f *Framework) WaitForPodRunning(podName string) error { // Runs the given pod and verifies that the output of exact container matches the desired output. func (f *Framework) TestContainerOutput(scenarioName string, pod *api.Pod, containerIndex int, expectedOutput []string) { - testContainerOutputInNamespace(scenarioName, f.Client, pod, containerIndex, expectedOutput, f.Namespace.Name) + testContainerOutput(scenarioName, f.Client, pod, containerIndex, expectedOutput, f.Namespace.Name) +} + +// Runs the given pod and verifies that the output of exact container matches the desired regexps. +func (f *Framework) TestContainerOutputRegexp(scenarioName string, pod *api.Pod, containerIndex int, expectedOutput []string) { + testContainerOutputRegexp(scenarioName, f.Client, pod, containerIndex, expectedOutput, f.Namespace.Name) } // WaitForAnEndpoint waits for at least one endpoint to become available in the diff --git a/test/e2e/host_path.go b/test/e2e/host_path.go index 19b0dba1a09..9db3e9c682f 100644 --- a/test/e2e/host_path.go +++ b/test/e2e/host_path.go @@ -70,7 +70,7 @@ var _ = Describe("hostPath", func() { fmt.Sprintf("--fs_type=%v", volumePath), fmt.Sprintf("--file_mode=%v", volumePath), } - testContainerOutputInNamespace("hostPath mode", c, pod, 0, []string{ + testContainerOutput("hostPath mode", c, pod, 0, []string{ "mode of file \"/test-volume\": dtrwxrwxrwx", // we expect the sticky bit (mode flag t) to be set for the dir }, namespace.Name) @@ -96,7 +96,7 @@ var _ = Describe("hostPath", func() { } //Read the content of the file with the second container to //verify volumes being shared properly among continers within the pod. - testContainerOutputInNamespace("hostPath r/w", c, pod, 1, []string{ + testContainerOutput("hostPath r/w", c, pod, 1, []string{ "content of file \"/test-volume/test-file\": mount-tester new file", }, namespace.Name, ) diff --git a/test/e2e/secrets.go b/test/e2e/secrets.go index 96097a78f99..b67e172e07a 100644 --- a/test/e2e/secrets.go +++ b/test/e2e/secrets.go @@ -92,7 +92,7 @@ var _ = Describe("Secrets", func() { }, } - testContainerOutputInNamespace("consume secrets", f.Client, pod, 0, []string{ + testContainerOutput("consume secrets", f.Client, pod, 0, []string{ "content of file \"/etc/secret-volume/data-1\": value-1", "mode of file \"/etc/secret-volume/data-1\": -r--r--r--", }, f.Namespace.Name) diff --git a/test/e2e/util.go b/test/e2e/util.go index 8c1c98b7472..a2caaffeff7 100644 --- a/test/e2e/util.go +++ b/test/e2e/util.go @@ -55,6 +55,8 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + + gomegatypes "github.com/onsi/gomega/types" ) const ( @@ -1080,10 +1082,29 @@ func tryKill(cmd *exec.Cmd) { } } -// testContainerOutputInNamespace runs the given pod in the given namespace and waits -// for all of the containers in the podSpec to move into the 'Success' status. It retrieves -// the exact container log and searches for lines of expected output. -func testContainerOutputInNamespace(scenarioName string, c *client.Client, pod *api.Pod, containerIndex int, expectedOutput []string, ns string) { +// testContainerOutput runs the given pod in the given namespace and waits +// for all of the containers in the podSpec to move into the 'Success' status, and tests +// the specified container log against the given expected output using a substring matcher. +func testContainerOutput(scenarioName string, c *client.Client, pod *api.Pod, containerIndex int, expectedOutput []string, ns string) { + testContainerOutputMatcher(scenarioName, c, pod, containerIndex, expectedOutput, ns, ContainSubstring) +} + +// testContainerOutputRegexp runs the given pod in the given namespace and waits +// for all of the containers in the podSpec to move into the 'Success' status, and tests +// the specified container log against the given expected output using a regexp matcher. +func testContainerOutputRegexp(scenarioName string, c *client.Client, pod *api.Pod, containerIndex int, expectedOutput []string, ns string) { + testContainerOutputMatcher(scenarioName, c, pod, containerIndex, expectedOutput, ns, MatchRegexp) +} + +// testContainerOutputMatcher runs the given pod in the given namespace and waits +// for all of the containers in the podSpec to move into the 'Success' status, and tests +// the specified container log against the given expected output using the given matcher. +func testContainerOutputMatcher(scenarioName string, + c *client.Client, + pod *api.Pod, + containerIndex int, + expectedOutput []string, ns string, + matcher func(string, ...interface{}) gomegatypes.GomegaMatcher) { By(fmt.Sprintf("Creating a pod to test %v", scenarioName)) defer c.Pods(ns).Delete(pod.Name, api.NewDeleteOptions(0)) @@ -1139,7 +1160,7 @@ func testContainerOutputInNamespace(scenarioName string, c *client.Client, pod * } for _, m := range expectedOutput { - Expect(string(logs)).To(ContainSubstring(m), "%q in container output", m) + Expect(string(logs)).To(matcher(m), "%q in container output", m) } }