Merge pull request #59125 from verb/pid-annotation

pkg/kubelet/dockershim/helpers_linux.go

@@ -118,7 +118,7 @@ func (ds *dockerService) updateCreateConfig(
 		if err := applyContainerSecurityContext(lc, podSandboxID, createConfig.Config, createConfig.HostConfig, securityOptSep); err != nil {
 			return fmt.Errorf("failed to apply container security context for container %q: %v", config.Metadata.Name, err)
 		}
-		modifyPIDNamespaceOverrides(ds.disableSharedPID, apiVersion, createConfig.HostConfig)
+		modifyContainerPIDNamespaceOverrides(ds.disableSharedPID, apiVersion, createConfig.HostConfig, podSandboxID)
 	}
 
 	// Apply cgroupsParent derived from the sandbox config.

pkg/kubelet/dockershim/security_context.go

@@ -122,13 +122,16 @@ func modifyHostConfig(sc *runtimeapi.LinuxContainerSecurityContext, hostConfig *
 
 // modifySandboxNamespaceOptions apply namespace options for sandbox
 func modifySandboxNamespaceOptions(nsOpts *runtimeapi.NamespaceOption, hostConfig *dockercontainer.HostConfig, network *knetwork.PluginManager) {
+	// The sandbox's PID namespace is the one that's shared, so CONTAINER and POD are equivalent for it
 	modifyCommonNamespaceOptions(nsOpts, hostConfig)
 	modifyHostOptionsForSandbox(nsOpts, network, hostConfig)
 }
 
 // modifyContainerNamespaceOptions apply namespace options for container
 func modifyContainerNamespaceOptions(nsOpts *runtimeapi.NamespaceOption, podSandboxID string, hostConfig *dockercontainer.HostConfig) {
-	hostConfig.PidMode = dockercontainer.PidMode(fmt.Sprintf("container:%v", podSandboxID))
+	if nsOpts.GetPid() == runtimeapi.NamespaceMode_POD {
+		hostConfig.PidMode = dockercontainer.PidMode(fmt.Sprintf("container:%v", podSandboxID))
+	}
 	modifyCommonNamespaceOptions(nsOpts, hostConfig)
 	modifyHostOptionsForContainer(nsOpts, podSandboxID, hostConfig)
 }
@@ -181,14 +184,16 @@ func modifyHostOptionsForContainer(nsOpts *runtimeapi.NamespaceOption, podSandbo
 //     1. Docker engine prior to API Version 1.24 doesn't support attaching to another container's
 //        PID namespace, and it didn't stabilize until 1.26. This check can be removed when Kubernetes'
 //        minimum Docker version is at least 1.13.1 (API version 1.26).
-//     2. The administrator has overridden the default behavior by means of a kubelet flag. This is an
-//        "escape hatch" to return to previous behavior of isolated namespaces and should be removed once
-//        no longer needed.
-func modifyPIDNamespaceOverrides(disableSharedPID bool, version *semver.Version, hc *dockercontainer.HostConfig) {
-	if !strings.HasPrefix(string(hc.PidMode), "container:") {
-		return
-	}
-	if disableSharedPID || version.LT(semver.Version{Major: 1, Minor: 26}) {
-		hc.PidMode = ""
+//     2. The administrator can override the API behavior by using the deprecated --docker-disable-shared-pid=false
+//        flag. Until this flag is removed, this causes pods to use NamespaceMode_POD instead of
+//        NamespaceMode_CONTAINER regardless of pod configuration.
+// TODO(verb): remove entirely once these two conditions are satisfied
+func modifyContainerPIDNamespaceOverrides(disableSharedPID bool, version *semver.Version, hc *dockercontainer.HostConfig, podSandboxID string) {
+	if version.LT(semver.Version{Major: 1, Minor: 26}) {
+		if strings.HasPrefix(string(hc.PidMode), "container:") {
+			hc.PidMode = ""
+		}
+	} else if !disableSharedPID && hc.PidMode == "" {
+		hc.PidMode = dockercontainer.PidMode(fmt.Sprintf("container:%v", podSandboxID))
 	}
 }

pkg/kubelet/dockershim/security_context_test.go

@@ -328,51 +328,93 @@ func TestModifyContainerNamespacePIDOverride(t *testing.T) {
 		input, expected dockercontainer.PidMode
 	}{
 		{
-			name:     "SharedPID.Enable",
+			name:     "mode:CONTAINER docker:NEW flag:UNSET",
+			disable:  true,
+			version:  &semver.Version{Major: 1, Minor: 26},
+			input:    "",
+			expected: "",
+		},
+		{
+			name:     "mode:CONTAINER docker:NEW flag:SET",
 			disable:  false,
 			version:  &semver.Version{Major: 1, Minor: 26},
-			input:    "container:sandbox",
+			input:    "",
 			expected: "container:sandbox",
 		},
 		{
-			name:     "SharedPID.Disable",
+			name:     "mode:CONTAINER docker:OLD flag:UNSET",
 			disable:  true,
-			version:  &semver.Version{Major: 1, Minor: 26},
-			input:    "container:sandbox",
+			version:  &semver.Version{Major: 1, Minor: 25},
+			input:    "",
 			expected: "",
 		},
 		{
-			name:     "SharedPID.OldDocker",
+			name:     "mode:CONTAINER docker:OLD flag:SET",
 			disable:  false,
 			version:  &semver.Version{Major: 1, Minor: 25},
-			input:    "container:sandbox",
+			input:    "",
 			expected: "",
 		},
 		{
-			name:     "SharedPID.HostPid",
+			name:     "mode:HOST docker:NEW flag:UNSET",
+			disable:  true,
+			version:  &semver.Version{Major: 1, Minor: 26},
+			input:    "host",
+			expected: "host",
+		},
+		{
+			name:     "mode:HOST docker:NEW flag:SET",
+			disable:  false,
+			version:  &semver.Version{Major: 1, Minor: 26},
+			input:    "host",
+			expected: "host",
+		},
+		{
+			name:     "mode:HOST docker:OLD flag:UNSET",
 			disable:  true,
-			version:  &semver.Version{Major: 1, Minor: 27},
+			version:  &semver.Version{Major: 1, Minor: 25},
+			input:    "host",
+			expected: "host",
+		},
+		{
+			name:     "mode:HOST docker:OLD flag:SET",
+			disable:  false,
+			version:  &semver.Version{Major: 1, Minor: 25},
 			input:    "host",
 			expected: "host",
 		},
 		{
-			name:     "SharedPID.DistantFuture",
+			name:     "mode:POD docker:NEW flag:UNSET",
+			disable:  true,
+			version:  &semver.Version{Major: 1, Minor: 26},
+			input:    "container:sandbox",
+			expected: "container:sandbox",
+		},
+		{
+			name:     "mode:POD docker:NEW flag:SET",
 			disable:  false,
-			version:  &semver.Version{Major: 2, Minor: 10},
+			version:  &semver.Version{Major: 1, Minor: 26},
 			input:    "container:sandbox",
 			expected: "container:sandbox",
 		},
 		{
-			name:     "SharedPID.EmptyPidMode",
+			name:     "mode:POD docker:OLD flag:UNSET",
 			disable:  true,
 			version:  &semver.Version{Major: 1, Minor: 25},
-			input:    "",
+			input:    "container:sandbox",
+			expected: "",
+		},
+		{
+			name:     "mode:POD docker:OLD flag:SET",
+			disable:  false,
+			version:  &semver.Version{Major: 1, Minor: 25},
+			input:    "container:sandbox",
 			expected: "",
 		},
 	}
 	for _, tc := range cases {
 		dockerCfg := &dockercontainer.HostConfig{PidMode: tc.input}
-		modifyPIDNamespaceOverrides(tc.disable, tc.version, dockerCfg)
+		modifyContainerPIDNamespaceOverrides(tc.disable, tc.version, dockerCfg, "sandbox")
 		assert.Equal(t, tc.expected, dockerCfg.PidMode, "[Test case %q]", tc.name)
 	}
 }

pkg/kubelet/kuberuntime/BUILD

@@ -84,6 +84,7 @@ go_test(
     embed = [":go_default_library"],
     deps = [
         "//pkg/credentialprovider:go_default_library",
+        "//pkg/features:go_default_library",
         "//pkg/kubelet/apis/cri/runtime/v1alpha2:go_default_library",
         "//pkg/kubelet/apis/cri/testing:go_default_library",
         "//pkg/kubelet/container:go_default_library",
@@ -101,6 +102,7 @@ go_test(
         "//vendor/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/util/feature/testing:go_default_library",
         "//vendor/k8s.io/client-go/util/flowcontrol:go_default_library",
     ],
 )

pkg/kubelet/kuberuntime/helpers.go

@@ -294,10 +294,13 @@ func networkNamespaceForPod(pod *v1.Pod) runtimeapi.NamespaceMode {
 }
 
 func pidNamespaceForPod(pod *v1.Pod) runtimeapi.NamespaceMode {
-	if pod != nil && pod.Spec.HostPID {
-		return runtimeapi.NamespaceMode_NODE
+	if pod != nil {
+		if pod.Spec.HostPID {
+			return runtimeapi.NamespaceMode_NODE
+		}
+		// TODO(verb): set NamespaceMode_POD based on ShareProcessNamespace after #58716 is merged
 	}
-	// Note that PID does not default to the zero value
+	// Note that PID does not default to the zero value for v1.Pod
 	return runtimeapi.NamespaceMode_CONTAINER
 }
 

pkg/kubelet/kuberuntime/helpers_test.go

@@ -25,6 +25,9 @@ import (
 
 	"k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	utilfeaturetesting "k8s.io/apiserver/pkg/util/feature/testing"
+	"k8s.io/kubernetes/pkg/features"
 	runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
 	runtimetesting "k8s.io/kubernetes/pkg/kubelet/apis/cri/testing"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
@@ -307,6 +310,8 @@ func TestGetSeccompProfileFromAnnotations(t *testing.T) {
 }
 
 func TestNamespacesForPod(t *testing.T) {
+	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodShareProcessNamespace, true)()
+
 	for desc, test := range map[string]struct {
 		input    *v1.Pod
 		expected *runtimeapi.NamespaceOption
@@ -341,6 +346,7 @@ func TestNamespacesForPod(t *testing.T) {
 				Pid:     runtimeapi.NamespaceMode_NODE,
 			},
 		},
+		// TODO(verb): add test cases for ShareProcessNamespace true (after #58716 is merged)
 	} {
 		t.Logf("TestCase: %s", desc)
 		actual := namespacesForPod(test.input)