From 7c558fb7bbb71d5c19ca6bd3641b193c48981262 Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Sun, 22 Jul 2018 16:36:37 +0200 Subject: [PATCH] Remove kubelet-level docker shared pid flag The --docker-disable-shared-pid flag has been deprecated since 1.10 and has been superceded by ShareProcessNamespace in the pod API, which is scheduled for beta in 1.12. --- cmd/kubelet/app/options/container_runtime.go | 1 - cmd/kubelet/app/server.go | 2 +- pkg/kubelet/config/flags.go | 7 --- pkg/kubelet/dockershim/docker_service.go | 11 +--- pkg/kubelet/dockershim/helpers_linux.go | 2 +- pkg/kubelet/dockershim/security_context.go | 15 ++--- .../dockershim/security_context_test.go | 63 +++---------------- pkg/kubelet/kubelet.go | 3 +- 8 files changed, 16 insertions(+), 88 deletions(-) diff --git a/cmd/kubelet/app/options/container_runtime.go b/cmd/kubelet/app/options/container_runtime.go index 693994cc462..a811577d958 100644 --- a/cmd/kubelet/app/options/container_runtime.go +++ b/cmd/kubelet/app/options/container_runtime.go @@ -49,7 +49,6 @@ func NewContainerRuntimeOptions() *config.ContainerRuntimeOptions { RedirectContainerStreaming: false, DockerEndpoint: dockerEndpoint, DockershimRootDirectory: "/var/lib/dockershim", - DockerDisableSharedPID: true, PodSandboxImage: defaultPodSandboxImage, ImagePullProgressDeadline: metav1.Duration{Duration: 1 * time.Minute}, ExperimentalDockershim: false, diff --git a/cmd/kubelet/app/server.go b/cmd/kubelet/app/server.go index 18f76911240..37f72bc7b25 100644 --- a/cmd/kubelet/app/server.go +++ b/cmd/kubelet/app/server.go @@ -1161,7 +1161,7 @@ func RunDockershim(f *options.KubeletFlags, c *kubeletconfiginternal.KubeletConf // Standalone dockershim will always start the local streaming server. ds, err := dockershim.NewDockerService(dockerClientConfig, r.PodSandboxImage, streamingConfig, &pluginSettings, - f.RuntimeCgroups, c.CgroupDriver, r.DockershimRootDirectory, r.DockerDisableSharedPID, true /*startLocalStreamingServer*/) + f.RuntimeCgroups, c.CgroupDriver, r.DockershimRootDirectory, true /*startLocalStreamingServer*/) if err != nil { return err } diff --git a/pkg/kubelet/config/flags.go b/pkg/kubelet/config/flags.go index bedc919f6ef..4b5494e7866 100644 --- a/pkg/kubelet/config/flags.go +++ b/pkg/kubelet/config/flags.go @@ -48,11 +48,6 @@ type ContainerRuntimeOptions struct { DockershimRootDirectory string // Enable dockershim only mode. ExperimentalDockershim bool - // This flag, if set, disables use of a shared PID namespace for pods running in the docker CRI runtime. - // A shared PID namespace is the only option in non-docker runtimes and is required by the CRI. The ability to - // disable it for docker will be removed unless a compelling use case is discovered with widespread use. - // TODO: Remove once we no longer support disabling shared PID namespace (https://issues.k8s.io/41938) - DockerDisableSharedPID bool // PodSandboxImage is the image whose network/ipc namespaces // containers in each pod will use. PodSandboxImage string @@ -93,8 +88,6 @@ func (s *ContainerRuntimeOptions) AddFlags(fs *pflag.FlagSet) { fs.MarkHidden("experimental-dockershim") fs.StringVar(&s.DockershimRootDirectory, "experimental-dockershim-root-directory", s.DockershimRootDirectory, "Path to the dockershim root directory.") fs.MarkHidden("experimental-dockershim-root-directory") - fs.BoolVar(&s.DockerDisableSharedPID, "docker-disable-shared-pid", s.DockerDisableSharedPID, fmt.Sprintf("Setting this to false causes Kubernetes to create pods using a shared process namespace for containers in a pod when running with Docker 1.13.1 or higher. A future Kubernetes release will make this configurable instead in the API. %s", dockerOnlyWarning)) - fs.MarkDeprecated("docker-disable-shared-pid", "will be removed in a future release. This option will be replaced by PID namespace sharing that is configurable per-pod using the API. See https://features.k8s.io/495") fs.StringVar(&s.PodSandboxImage, "pod-infra-container-image", s.PodSandboxImage, fmt.Sprintf("The image whose network/ipc namespaces containers in each pod will use. %s", dockerOnlyWarning)) fs.StringVar(&s.DockerEndpoint, "docker-endpoint", s.DockerEndpoint, fmt.Sprintf("Use this for the docker endpoint to communicate with. %s", dockerOnlyWarning)) fs.DurationVar(&s.ImagePullProgressDeadline.Duration, "image-pull-progress-deadline", s.ImagePullProgressDeadline.Duration, fmt.Sprintf("If no pulling progress is made before this deadline, the image pulling will be cancelled. %s", dockerOnlyWarning)) diff --git a/pkg/kubelet/dockershim/docker_service.go b/pkg/kubelet/dockershim/docker_service.go index 8201ab941f2..d7fcc9232d1 100644 --- a/pkg/kubelet/dockershim/docker_service.go +++ b/pkg/kubelet/dockershim/docker_service.go @@ -187,9 +187,8 @@ func NewDockerClientFromConfig(config *ClientConfig) libdocker.Interface { } // NOTE: Anything passed to DockerService should be eventually handled in another way when we switch to running the shim as a different process. -func NewDockerService(config *ClientConfig, podSandboxImage string, streamingConfig *streaming.Config, - pluginSettings *NetworkPluginSettings, cgroupsName string, kubeCgroupDriver string, dockershimRootDir string, - disableSharedPID, startLocalStreamingServer bool) (DockerService, error) { +func NewDockerService(config *ClientConfig, podSandboxImage string, streamingConfig *streaming.Config, pluginSettings *NetworkPluginSettings, + cgroupsName string, kubeCgroupDriver string, dockershimRootDir string, startLocalStreamingServer bool) (DockerService, error) { client := NewDockerClientFromConfig(config) @@ -210,7 +209,6 @@ func NewDockerService(config *ClientConfig, podSandboxImage string, streamingCon }, containerManager: cm.NewContainerManager(cgroupsName, client), checkpointManager: checkpointManager, - disableSharedPID: disableSharedPID, startLocalStreamingServer: startLocalStreamingServer, networkReady: make(map[string]bool), } @@ -304,11 +302,6 @@ type dockerService struct { // version checking for some operations. Use this cache to avoid querying // the docker daemon every time we need to do such checks. versionCache *cache.ObjectCache - // This option provides an escape hatch to override the new default behavior for Docker under - // the CRI to use a shared PID namespace for all pods. It is temporary and will be removed. - // See proposals/pod-pid-namespace.md for details. - // TODO: Remove once the escape hatch is no longer used (https://issues.k8s.io/41938) - disableSharedPID bool // startLocalStreamingServer indicates whether dockershim should start a // streaming server on localhost. startLocalStreamingServer bool diff --git a/pkg/kubelet/dockershim/helpers_linux.go b/pkg/kubelet/dockershim/helpers_linux.go index 5d231ea82a2..a93403ac3b6 100644 --- a/pkg/kubelet/dockershim/helpers_linux.go +++ b/pkg/kubelet/dockershim/helpers_linux.go @@ -120,7 +120,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) } - modifyContainerPIDNamespaceOverrides(ds.disableSharedPID, apiVersion, createConfig.HostConfig, podSandboxID) + modifyContainerPIDNamespaceOverrides(apiVersion, createConfig.HostConfig, podSandboxID) } // Apply cgroupsParent derived from the sandbox config. diff --git a/pkg/kubelet/dockershim/security_context.go b/pkg/kubelet/dockershim/security_context.go index d060144cb59..343c3876480 100644 --- a/pkg/kubelet/dockershim/security_context.go +++ b/pkg/kubelet/dockershim/security_context.go @@ -200,20 +200,13 @@ func modifyHostOptionsForContainer(nsOpts *runtimeapi.NamespaceOption, podSandbo } } -// modifyPIDNamespaceOverrides implements two temporary overrides for the default PID namespace sharing for Docker: +// modifyPIDNamespaceOverrides implements a temporary override for the default PID namespace sharing for Docker: // 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 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)) +func modifyContainerPIDNamespaceOverrides(version *semver.Version, hc *dockercontainer.HostConfig, podSandboxID string) { + if version.LT(semver.Version{Major: 1, Minor: 26}) && strings.HasPrefix(string(hc.PidMode), "container:") { + hc.PidMode = "" } } diff --git a/pkg/kubelet/dockershim/security_context_test.go b/pkg/kubelet/dockershim/security_context_test.go index 4986f60be66..59876e72a22 100644 --- a/pkg/kubelet/dockershim/security_context_test.go +++ b/pkg/kubelet/dockershim/security_context_test.go @@ -362,90 +362,41 @@ func TestModifyContainerNamespaceOptions(t *testing.T) { func TestModifyContainerNamespacePIDOverride(t *testing.T) { cases := []struct { name string - disable bool version *semver.Version input, expected dockercontainer.PidMode }{ { - name: "mode:CONTAINER docker:NEW flag:UNSET", - disable: true, + name: "mode:CONTAINER docker:NEW", 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: "", - expected: "container:sandbox", - }, - { - name: "mode:CONTAINER docker:OLD flag:UNSET", - disable: true, + name: "mode:CONTAINER docker:OLD", version: &semver.Version{Major: 1, Minor: 25}, input: "", expected: "", }, { - name: "mode:CONTAINER docker:OLD flag:SET", - disable: false, - version: &semver.Version{Major: 1, Minor: 25}, - input: "", - expected: "", - }, - { - name: "mode:HOST docker:NEW flag:UNSET", - disable: true, + name: "mode:HOST docker:NEW", 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, + name: "mode:HOST docker:OLD", 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: "mode:POD docker:NEW flag:UNSET", - disable: true, + name: "mode:POD docker:NEW", 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: 1, Minor: 26}, - input: "container:sandbox", - expected: "container:sandbox", - }, - { - name: "mode:POD docker:OLD flag:UNSET", - disable: true, - version: &semver.Version{Major: 1, Minor: 25}, - input: "container:sandbox", - expected: "", - }, - { - name: "mode:POD docker:OLD flag:SET", - disable: false, + name: "mode:POD docker:OLD", version: &semver.Version{Major: 1, Minor: 25}, input: "container:sandbox", expected: "", @@ -453,7 +404,7 @@ func TestModifyContainerNamespacePIDOverride(t *testing.T) { } for _, tc := range cases { dockerCfg := &dockercontainer.HostConfig{PidMode: tc.input} - modifyContainerPIDNamespaceOverrides(tc.disable, tc.version, dockerCfg, "sandbox") + modifyContainerPIDNamespaceOverrides(tc.version, dockerCfg, "sandbox") assert.Equal(t, tc.expected, dockerCfg.PidMode, "[Test case %q]", tc.name) } } diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 8f0828b748a..baff274c1bf 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -602,8 +602,7 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration, // Create and start the CRI shim running as a grpc server. streamingConfig := getStreamingConfig(kubeCfg, kubeDeps, crOptions) ds, err := dockershim.NewDockerService(kubeDeps.DockerClientConfig, crOptions.PodSandboxImage, streamingConfig, - &pluginSettings, runtimeCgroups, kubeCfg.CgroupDriver, crOptions.DockershimRootDirectory, - crOptions.DockerDisableSharedPID, !crOptions.RedirectContainerStreaming) + &pluginSettings, runtimeCgroups, kubeCfg.CgroupDriver, crOptions.DockershimRootDirectory, !crOptions.RedirectContainerStreaming) if err != nil { return nil, err }