Merge pull request #64862 from feiskyer/win-cni

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Revert #64189: Fix Windows CNI for the sandbox case

**What this PR does / why we need it**:

This reverts PR #64189, which breaks DNS for Windows containers.

Refer https://github.com/kubernetes/kubernetes/pull/64189#issuecomment-395248704

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #64861

**Special notes for your reviewer**:

**Release note**:

```release-note
NONE
```

cc @madhanrm @PatrickLang @alinbalutoiu @dineshgovindasamy

pkg/kubelet/dockershim/docker_sandbox.go

@@ -412,9 +412,8 @@ func (ds *dockerService) PodSandboxStatus(ctx context.Context, req *runtimeapi.P
 
 	var IP string
 	// TODO: Remove this when sandbox is available on windows
-	// Currently windows supports both sandbox and non-sandbox cases.
 	// This is a workaround for windows, where sandbox is not in use, and pod IP is determined through containers belonging to the Pod.
-	if IP = ds.determinePodIPBySandboxID(podSandboxID, r); IP == "" {
+	if IP = ds.determinePodIPBySandboxID(podSandboxID); IP == "" {
 		IP = ds.getIP(podSandboxID, r)
 	}
 

pkg/kubelet/dockershim/helpers_linux.go

@@ -136,7 +136,7 @@ func (ds *dockerService) updateCreateConfig(
 	return nil
 }
 
-func (ds *dockerService) determinePodIPBySandboxID(uid string, sandbox *dockertypes.ContainerJSON) string {
+func (ds *dockerService) determinePodIPBySandboxID(uid string) string {
 	return ""
 }
 

pkg/kubelet/dockershim/helpers_unsupported.go

@@ -45,7 +45,7 @@ func (ds *dockerService) updateCreateConfig(
 	return nil
 }
 
-func (ds *dockerService) determinePodIPBySandboxID(uid string, sandbox *dockertypes.ContainerJSON) string {
+func (ds *dockerService) determinePodIPBySandboxID(uid string) string {
 	glog.Warningf("determinePodIPBySandboxID is unsupported in this build")
 	return ""
 }

pkg/kubelet/dockershim/helpers_windows.go

@@ -97,28 +97,7 @@ func applyWindowsContainerSecurityContext(wsc *runtimeapi.WindowsContainerSecuri
 	}
 }
 
-func (ds *dockerService) determinePodIPBySandboxID(sandboxID string, sandbox *dockertypes.ContainerJSON) string {
+func (ds *dockerService) determinePodIPBySandboxID(sandboxID string) string {
-	// Versions and feature support
-	// ============================
-	// Windows version >= Windows Server, Version 1709, Supports both sandbox and non-sandbox case
-	// Windows version == Windows Server 2016 Support only non-sandbox case
-	// Windows version < Windows Server 2016 is Not Supported
-
-	// Sandbox support in Windows mandates CNI Plugin.
-	// Presence of CONTAINER_NETWORK flag is considered as non-Sandbox cases here
-	// Hyper-V isolated containers are also considered as non-Sandbox cases
-
-	// Todo: Add a kernel version check for more validation
-
-	// Hyper-V only supports one container per Pod yet and the container will have a different
-	// IP address from sandbox. Retrieve the IP from the containers as this is a non-Sandbox case.
-	// TODO(feiskyer): remove this workaround after Hyper-V supports multiple containers per Pod.
-	if networkMode := os.Getenv("CONTAINER_NETWORK"); networkMode == "" && sandbox.HostConfig.Isolation != kubeletapis.HypervIsolationValue {
-		// Sandbox case, fetch the IP from the sandbox container.
-		return ds.getIP(sandboxID, sandbox)
-	}
-
-	// Non-Sandbox case, fetch the IP from the containers within the Pod.
 	opts := dockertypes.ContainerListOptions{
 		All:     true,
 		Filters: dockerfilters.NewArgs(),
@@ -138,9 +117,50 @@ func (ds *dockerService) determinePodIPBySandboxID(sandboxID string, sandbox *do
 			continue
 		}
 
+		// Versions and feature support
+		// ============================
+		// Windows version == Windows Server, Version 1709,, Supports both sandbox and non-sandbox case
+		// Windows version == Windows Server 2016   Support only non-sandbox case
+		// Windows version < Windows Server 2016 is Not Supported
+
+		// Sandbox support in Windows mandates CNI Plugin.
+		// Presence of CONTAINER_NETWORK flag is considered as non-Sandbox cases here
+
+		// Todo: Add a kernel version check for more validation
+
+		if networkMode := os.Getenv("CONTAINER_NETWORK"); networkMode == "" {
+			// On Windows, every container that is created in a Sandbox, needs to invoke CNI plugin again for adding the Network,
+			// with the shared container name as NetNS info,
+			// This is passed down to the platform to replicate some necessary information to the new container
+
+			//
+			// This place is chosen as a hack for now, since ds.getIP would end up calling CNI's addToNetwork
+			// That is why addToNetwork is required to be idempotent
+
+			// Instead of relying on this call, an explicit call to addToNetwork should be
+			// done immediately after ContainerCreation, in case of Windows only. TBD Issue # to handle this
+
+			if r.HostConfig.Isolation == kubeletapis.HypervIsolationValue {
+				// Hyper-V only supports one container per Pod yet and the container will have a different
+				// IP address from sandbox. Return the first non-sandbox container IP as POD IP.
+				// TODO(feiskyer): remove this workaround after Hyper-V supports multiple containers per Pod.
 				if containerIP := ds.getIP(c.ID, r); containerIP != "" {
 					return containerIP
 				}
+			} else {
+				// Do not return any IP, so that we would continue and get the IP of the Sandbox.
+				// Windows 1709 and 1803 doesn't have the Namespace support, so getIP() is called
+				// to replicate the DNS registry key to the Workload container (IP/Gateway/MAC is
+				// set separately than DNS).
+				// TODO(feiskyer): remove this workaround after Namespace is supported in Windows RS5.
+				ds.getIP(sandboxID, r)
+			}
+		} else {
+			// ds.getIP will call the CNI plugin to fetch the IP
+			if containerIP := ds.getIP(c.ID, r); containerIP != "" {
+				return containerIP
+			}
+		}
 	}
 
 	return ""