Fix race condition for consuming podIP via downward API.

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 = dm.determineContainerIP(pod.Namespace, pod.Name, inspectResult)
-				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()
-				}
-			}
 		}
 	} 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

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)
+		}
+	}
+}

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)
 	})

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{
+		env := []api.EnvVar{
-			ObjectMeta: api.ObjectMeta{
+			{
-				Name:   podName,
+				Name: "POD_NAME",
-				Labels: map[string]string{"name": podName},
+				ValueFrom: &api.EnvVarSource{
-			},
+					FieldRef: &api.ObjectFieldSelector{
-			Spec: api.PodSpec{
+						APIVersion: "v1",
-				Containers: []api.Container{
+						FieldPath:  "metadata.name",
-					{
+					},
-						Name:    "dapi-container",
+				},
-						Image:   "gcr.io/google_containers/busybox",
+			},
-						Command: []string{"sh", "-c", "env"},
+			{
-						Env: []api.EnvVar{
+				Name: "POD_NAMESPACE",
-							{
+				ValueFrom: &api.EnvVarSource{
-								Name: "POD_NAME",
+					FieldRef: &api.ObjectFieldSelector{
-								ValueFrom: &api.EnvVarSource{
+						APIVersion: "v1",
-									FieldRef: &api.ObjectFieldSelector{
+						FieldPath:  "metadata.namespace",
-										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)
+}

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),

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

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,
 		)

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)

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
+// 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.  It retrieves
+// for all of the containers in the podSpec to move into the 'Success' status, and tests
-// the exact container log and searches for lines of expected output.
+// the specified container log against the given expected output using a substring matcher.
-func testContainerOutputInNamespace(scenarioName string, c *client.Client, pod *api.Pod, containerIndex int, expectedOutput []string, ns string) {
+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)
 	}
 }