Set docker opt separator correctly for SELinux options

pkg/kubelet/dockershim/security_context.go

@@ -24,6 +24,7 @@ import (
 
 	"k8s.io/kubernetes/pkg/api/v1"
 	runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
+	"k8s.io/kubernetes/pkg/kubelet/dockertools"
 	"k8s.io/kubernetes/pkg/kubelet/dockertools/securitycontext"
 	"k8s.io/kubernetes/pkg/kubelet/network"
 )
@@ -103,6 +104,7 @@ func modifyHostConfig(sc *runtimeapi.LinuxContainerSecurityContext, hostConfig *
 				Type:  sc.SelinuxOptions.Type,
 				Level: sc.SelinuxOptions.Level,
 			},
+			dockertools.SecurityOptSeparatorNew,
 		)
 	}
 }

pkg/kubelet/dockershim/security_context_test.go

@@ -82,10 +82,10 @@ func TestModifyHostConfig(t *testing.T) {
 	}
 	setSELinuxHC := &dockercontainer.HostConfig{
 		SecurityOpt: []string{
-			fmt.Sprintf("%s:%s", securitycontext.DockerLabelUser, "user"),
-			fmt.Sprintf("%s:%s", securitycontext.DockerLabelRole, "role"),
-			fmt.Sprintf("%s:%s", securitycontext.DockerLabelType, "type"),
-			fmt.Sprintf("%s:%s", securitycontext.DockerLabelLevel, "level"),
+			fmt.Sprintf("%s:%s", securitycontext.DockerLabelUser('='), "user"),
+			fmt.Sprintf("%s:%s", securitycontext.DockerLabelRole('='), "role"),
+			fmt.Sprintf("%s:%s", securitycontext.DockerLabelType('='), "type"),
+			fmt.Sprintf("%s:%s", securitycontext.DockerLabelLevel('='), "level"),
 		},
 	}
 
@@ -181,10 +181,10 @@ func TestModifyHostConfigAndNamespaceOptionsForContainer(t *testing.T) {
 	}
 	setSELinuxHC := &dockercontainer.HostConfig{
 		SecurityOpt: []string{
-			fmt.Sprintf("%s:%s", securitycontext.DockerLabelUser, "user"),
-			fmt.Sprintf("%s:%s", securitycontext.DockerLabelRole, "role"),
-			fmt.Sprintf("%s:%s", securitycontext.DockerLabelType, "type"),
-			fmt.Sprintf("%s:%s", securitycontext.DockerLabelLevel, "level"),
+			fmt.Sprintf("%s:%s", securitycontext.DockerLabelUser('='), "user"),
+			fmt.Sprintf("%s:%s", securitycontext.DockerLabelRole('='), "role"),
+			fmt.Sprintf("%s:%s", securitycontext.DockerLabelType('='), "type"),
+			fmt.Sprintf("%s:%s", securitycontext.DockerLabelLevel('='), "level"),
 		},
 		IpcMode:     dockercontainer.IpcMode(sandboxNSMode),
 		NetworkMode: dockercontainer.NetworkMode(sandboxNSMode),
@@ -351,10 +351,10 @@ func fullValidHostConfig() *dockercontainer.HostConfig {
 		CapAdd:     []string{"addCapA", "addCapB"},
 		CapDrop:    []string{"dropCapA", "dropCapB"},
 		SecurityOpt: []string{
-			fmt.Sprintf("%s:%s", securitycontext.DockerLabelUser, "user"),
-			fmt.Sprintf("%s:%s", securitycontext.DockerLabelRole, "role"),
-			fmt.Sprintf("%s:%s", securitycontext.DockerLabelType, "type"),
-			fmt.Sprintf("%s:%s", securitycontext.DockerLabelLevel, "level"),
+			fmt.Sprintf("%s:%s", securitycontext.DockerLabelUser('='), "user"),
+			fmt.Sprintf("%s:%s", securitycontext.DockerLabelRole('='), "role"),
+			fmt.Sprintf("%s:%s", securitycontext.DockerLabelType('='), "type"),
+			fmt.Sprintf("%s:%s", securitycontext.DockerLabelLevel('='), "level"),
 		},
 	}
 }

pkg/kubelet/dockertools/BUILD

@@ -129,6 +129,7 @@ go_test(
         "//vendor:github.com/golang/mock/gomock",
         "//vendor:github.com/google/cadvisor/info/v1",
         "//vendor:github.com/stretchr/testify/assert",
+        "//vendor:github.com/stretchr/testify/require",
         "//vendor:k8s.io/apimachinery/pkg/api/equality",
         "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1",
         "//vendor:k8s.io/apimachinery/pkg/runtime",

pkg/kubelet/dockertools/docker_manager.go

@@ -108,6 +108,11 @@ const (
 
 	// The expiration time of version cache.
 	versionCacheTTL = 60 * time.Second
+
+	// Docker changed the API for specifying options in v1.11
+	SecurityOptSeparatorChangeVersion = "1.23" // Corresponds to docker 1.11.x
+	SecurityOptSeparatorOld           = ':'
+	SecurityOptSeparatorNew           = '='
 )
 
 var (
@@ -631,10 +636,11 @@ func (dm *DockerManager) runContainer(
 	if err != nil {
 		return kubecontainer.ContainerID{}, err
 	}
-	fmtSecurityOpts, err := dm.fmtDockerOpts(securityOpts)
+	optSeparator, err := dm.getDockerOptSeparator()
 	if err != nil {
 		return kubecontainer.ContainerID{}, err
 	}
+	fmtSecurityOpts := FmtDockerOpts(securityOpts, optSeparator)
 
 	// Pod information is recorded on the container as labels to preserve it in the event the pod is deleted
 	// while the Kubelet is down and there is no information available to recover the pod.
@@ -813,7 +819,7 @@ func (dm *DockerManager) runContainer(
 	glog.V(3).Infof("Container %v/%v/%v: setting entrypoint \"%v\" and command \"%v\"", pod.Namespace, pod.Name, container.Name, dockerOpts.Config.Entrypoint, dockerOpts.Config.Cmd)
 
 	supplementalGids := dm.runtimeHelper.GetExtraSupplementalGroupsForPod(pod)
-	securityContextProvider := dockersecurity.NewSimpleSecurityContextProvider()
+	securityContextProvider := dockersecurity.NewSimpleSecurityContextProvider(optSeparator)
 	securityContextProvider.ModifyContainerConfig(pod, container, dockerOpts.Config)
 	securityContextProvider.ModifyHostConfig(pod, container, dockerOpts.HostConfig, supplementalGids)
 	createResp, err := dm.client.CreateContainer(dockerOpts)
@@ -1157,31 +1163,23 @@ func (dm *DockerManager) checkVersionCompatibility() error {
 	return nil
 }
 
-func (dm *DockerManager) fmtDockerOpts(opts []dockerOpt) ([]string, error) {
-	version, err := dm.APIVersion()
-	if err != nil {
-		return nil, err
-	}
-
-	const (
-		// Docker changed the API for specifying options in v1.11
-		optSeparatorChangeVersion = "1.23" // Corresponds to docker 1.11.x
-		optSeparatorOld           = ':'
-		optSeparatorNew           = '='
-	)
-
-	sep := optSeparatorNew
-	if result, err := version.Compare(optSeparatorChangeVersion); err != nil {
-		return nil, fmt.Errorf("error parsing docker API version: %v", err)
+func (dm *DockerManager) getDockerOptSeparator() (rune, error) {
+	sep := SecurityOptSeparatorNew
+	if result, err := dm.checkDockerAPIVersion(SecurityOptSeparatorChangeVersion); err != nil {
+		return sep, err
 	} else if result < 0 {
-		sep = optSeparatorOld
+		sep = SecurityOptSeparatorOld
 	}
+	return sep, nil
+}
 
+// FmtDockerOpts formats the docker security options using the given separator.
+func FmtDockerOpts(opts []dockerOpt, sep rune) []string {
 	fmtOpts := make([]string, len(opts))
 	for i, opt := range opts {
 		fmtOpts[i] = fmt.Sprintf("%s%c%s", opt.key, sep, opt.value)
 	}
-	return fmtOpts, nil
+	return fmtOpts
 }
 
 type dockerOpt struct {

pkg/kubelet/dockertools/docker_manager_linux_test.go

@@ -89,8 +89,7 @@ func TestGetSecurityOpts(t *testing.T) {
 	for i, test := range tests {
 		securityOpts, err := dm.getSecurityOpts(test.pod, containerName)
 		assert.NoError(t, err, "TestCase[%d]: %s", i, test.msg)
-		opts, err := dm.fmtDockerOpts(securityOpts)
-		assert.NoError(t, err, "TestCase[%d]: %s", i, test.msg)
+		opts := FmtDockerOpts(securityOpts, '=')
 		assert.Len(t, opts, len(test.expectedOpts), "TestCase[%d]: %s", i, test.msg)
 		for _, opt := range test.expectedOpts {
 			assert.Contains(t, opts, opt, "TestCase[%d]: %s", i, test.msg)

pkg/kubelet/dockertools/docker_manager_test.go

@@ -37,6 +37,7 @@ import (
 	"github.com/golang/mock/gomock"
 	cadvisorapi "github.com/google/cadvisor/info/v1"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -1644,27 +1645,29 @@ func verifySyncResults(t *testing.T, expectedResults []*kubecontainer.SyncResult
 		}
 	}
 }
-
-func TestSecurityOptsOperator(t *testing.T) {
+func TestGetDockerOptSeparator(t *testing.T) {
 	dm110, _ := newTestDockerManagerWithVersion("1.10.1", "1.22")
 	dm111, _ := newTestDockerManagerWithVersion("1.11.0", "1.23")
 
+	sep, err := dm110.getDockerOptSeparator()
+	require.NoError(t, err, "error getting docker opt separator for 1.10.1")
+	assert.Equal(t, SecurityOptSeparatorOld, sep, "security opt separator for docker 1.10")
+
+	sep, err = dm111.getDockerOptSeparator()
+	require.NoError(t, err, "error getting docker opt separator for 1.11.1")
+	assert.Equal(t, SecurityOptSeparatorNew, sep, "security opt separator for docker 1.11")
+}
+
+func TestFmtDockerOpts(t *testing.T) {
 	secOpts := []dockerOpt{{"seccomp", "unconfined", ""}}
-	opts, err := dm110.fmtDockerOpts(secOpts)
-	if err != nil {
-		t.Fatalf("error getting security opts for Docker 1.10: %v", err)
-	}
-	if expected := []string{"seccomp:unconfined"}; len(opts) != 1 || opts[0] != expected[0] {
-		t.Fatalf("security opts for Docker 1.10: expected %v, got: %v", expected, opts)
-	}
 
-	opts, err = dm111.fmtDockerOpts(secOpts)
-	if err != nil {
-		t.Fatalf("error getting security opts for Docker 1.11: %v", err)
-	}
-	if expected := []string{"seccomp=unconfined"}; len(opts) != 1 || opts[0] != expected[0] {
-		t.Fatalf("security opts for Docker 1.11: expected %v, got: %v", expected, opts)
-	}
+	opts := FmtDockerOpts(secOpts, ':')
+	assert.Len(t, opts, 1)
+	assert.Contains(t, opts, "seccomp:unconfined", "Docker 1.10")
+
+	opts = FmtDockerOpts(secOpts, '=')
+	assert.Len(t, opts, 1)
+	assert.Contains(t, opts, "seccomp=unconfined", "Docker 1.11")
 }
 
 func TestCheckVersionCompatibility(t *testing.T) {

pkg/kubelet/dockertools/securitycontext/BUILD

@@ -15,6 +15,7 @@ go_library(
         "fake.go",
         "provider.go",
         "types.go",
+        "util.go",
     ],
     tags = ["automanaged"],
     deps = [

pkg/kubelet/dockertools/securitycontext/provider.go

@@ -29,12 +29,14 @@ import (
 )
 
 // NewSimpleSecurityContextProvider creates a new SimpleSecurityContextProvider.
-func NewSimpleSecurityContextProvider() SecurityContextProvider {
-	return SimpleSecurityContextProvider{}
+func NewSimpleSecurityContextProvider(securityOptSeparator rune) SecurityContextProvider {
+	return SimpleSecurityContextProvider{securityOptSeparator}
 }
 
 // SimpleSecurityContextProvider is the default implementation of a SecurityContextProvider.
-type SimpleSecurityContextProvider struct{}
+type SimpleSecurityContextProvider struct {
+	securityOptSeparator rune
+}
 
 // ModifyContainerConfig is called before the Docker createContainer call.
 // The security context provider can make changes to the Config with which
@@ -93,25 +95,37 @@ func (p SimpleSecurityContextProvider) ModifyHostConfig(pod *v1.Pod, container *
 	}
 
 	if effectiveSC.SELinuxOptions != nil {
-		hostConfig.SecurityOpt = ModifySecurityOptions(hostConfig.SecurityOpt, effectiveSC.SELinuxOptions)
+		hostConfig.SecurityOpt = ModifySecurityOptions(hostConfig.SecurityOpt, effectiveSC.SELinuxOptions, p.securityOptSeparator)
 	}
 }
 
-// ModifySecurityOptions adds SELinux options to config.
-func ModifySecurityOptions(config []string, selinuxOpts *v1.SELinuxOptions) []string {
-	config = modifySecurityOption(config, DockerLabelUser, selinuxOpts.User)
-	config = modifySecurityOption(config, DockerLabelRole, selinuxOpts.Role)
-	config = modifySecurityOption(config, DockerLabelType, selinuxOpts.Type)
-	config = modifySecurityOption(config, DockerLabelLevel, selinuxOpts.Level)
+// ModifySecurityOptions adds SELinux options to config using the given
+// separator.
+func ModifySecurityOptions(config []string, selinuxOpts *v1.SELinuxOptions, separator rune) []string {
+	// Note, strictly speaking, we are actually mutating the values of these
+	// keys, rather than formatting name and value into a string.  Docker re-
+	// uses the same option name multiple times (it's just 'label') with
+	// different values which are themselves key-value pairs.  For example,
+	// the SELinux type is represented by the security opt:
+	//
+	//   label<separator>type:<selinux_type>
+	//
+	// In Docker API versions before 1.23, the separator was the `:` rune; in
+	// API version 1.23 it changed to the `=` rune.
+	config = modifySecurityOption(config, DockerLabelUser(separator), selinuxOpts.User)
+	config = modifySecurityOption(config, DockerLabelRole(separator), selinuxOpts.Role)
+	config = modifySecurityOption(config, DockerLabelType(separator), selinuxOpts.Type)
+	config = modifySecurityOption(config, DockerLabelLevel(separator), selinuxOpts.Level)
 
 	return config
 }
 
-// modifySecurityOption adds the security option of name to the config array with value in the form
-// of name:value
+// modifySecurityOption adds the security option of name to the config array
+// with value in the form of name:value.
 func modifySecurityOption(config []string, name, value string) []string {
 	if len(value) > 0 {
 		config = append(config, fmt.Sprintf("%s:%s", name, value))
 	}
+
 	return config
 }

pkg/kubelet/dockertools/securitycontext/provider_test.go

@@ -74,7 +74,7 @@ func TestModifyContainerConfig(t *testing.T) {
 		},
 	}
 
-	provider := NewSimpleSecurityContextProvider()
+	provider := NewSimpleSecurityContextProvider('=')
 	dummyContainer := &v1.Container{}
 	for _, tc := range cases {
 		pod := &v1.Pod{Spec: v1.PodSpec{SecurityContext: tc.podSc}}
@@ -104,10 +104,10 @@ func TestModifyHostConfig(t *testing.T) {
 
 	setSELinuxHC := &dockercontainer.HostConfig{}
 	setSELinuxHC.SecurityOpt = []string{
-		fmt.Sprintf("%s:%s", DockerLabelUser, "user"),
-		fmt.Sprintf("%s:%s", DockerLabelRole, "role"),
-		fmt.Sprintf("%s:%s", DockerLabelType, "type"),
-		fmt.Sprintf("%s:%s", DockerLabelLevel, "level"),
+		fmt.Sprintf("%s:%s", DockerLabelUser(':'), "user"),
+		fmt.Sprintf("%s:%s", DockerLabelRole(':'), "role"),
+		fmt.Sprintf("%s:%s", DockerLabelType(':'), "type"),
+		fmt.Sprintf("%s:%s", DockerLabelLevel(':'), "level"),
 	}
 
 	// seLinuxLabelsSC := fullValidSecurityContext()
@@ -158,7 +158,7 @@ func TestModifyHostConfig(t *testing.T) {
 		},
 	}
 
-	provider := NewSimpleSecurityContextProvider()
+	provider := NewSimpleSecurityContextProvider(':')
 	dummyContainer := &v1.Container{}
 
 	for _, tc := range cases {
@@ -228,7 +228,7 @@ func TestModifyHostConfigPodSecurityContext(t *testing.T) {
 		},
 	}
 
-	provider := NewSimpleSecurityContextProvider()
+	provider := NewSimpleSecurityContextProvider(':')
 	dummyContainer := &v1.Container{}
 	dummyContainer.SecurityContext = fullValidSecurityContext()
 	dummyPod := &v1.Pod{
@@ -325,10 +325,10 @@ func fullValidHostConfig() *dockercontainer.HostConfig {
 		CapAdd:     []string{"addCapA", "addCapB"},
 		CapDrop:    []string{"dropCapA", "dropCapB"},
 		SecurityOpt: []string{
-			fmt.Sprintf("%s:%s", DockerLabelUser, "user"),
-			fmt.Sprintf("%s:%s", DockerLabelRole, "role"),
-			fmt.Sprintf("%s:%s", DockerLabelType, "type"),
-			fmt.Sprintf("%s:%s", DockerLabelLevel, "level"),
+			fmt.Sprintf("%s:%s", DockerLabelUser(':'), "user"),
+			fmt.Sprintf("%s:%s", DockerLabelRole(':'), "role"),
+			fmt.Sprintf("%s:%s", DockerLabelType(':'), "type"),
+			fmt.Sprintf("%s:%s", DockerLabelLevel(':'), "level"),
 		},
 	}
 }

pkg/kubelet/dockertools/securitycontext/types.go

@@ -39,11 +39,3 @@ type SecurityContextProvider interface {
 	// - supplementalGids: additional supplemental GIDs associated with the pod's volumes
 	ModifyHostConfig(pod *v1.Pod, container *v1.Container, hostConfig *dockercontainer.HostConfig, supplementalGids []int64)
 }
-
-const (
-	DockerLabelUser    string = "label:user"
-	DockerLabelRole    string = "label:role"
-	DockerLabelType    string = "label:type"
-	DockerLabelLevel   string = "label:level"
-	DockerLabelDisable string = "label:disable"
-)

pkg/kubelet/dockertools/securitycontext/util.go

+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package securitycontext
+
+import (
+	"fmt"
+)
+
+// DockerLabelUser returns the fragment of a Docker security opt that
+// describes the SELinux user. Note that strictly speaking this is not
+// actually the name of the security opt, but a fragment of the whole key-
+// value pair necessary to set the opt.
+func DockerLabelUser(separator rune) string {
+	return fmt.Sprintf("label%cuser", separator)
+}
+
+// DockerLabelRole returns the fragment of a Docker security opt that
+// describes the SELinux role. Note that strictly speaking this is not
+// actually the name of the security opt, but a fragment of the whole key-
+// value pair necessary to set the opt.
+func DockerLabelRole(separator rune) string {
+	return fmt.Sprintf("label%crole", separator)
+}
+
+// DockerLabelType returns the fragment of a Docker security opt that
+// describes the SELinux type. Note that strictly speaking this is not
+// actually the name of the security opt, but a fragment of the whole key-
+// value pair necessary to set the opt.
+func DockerLabelType(separator rune) string {
+	return fmt.Sprintf("label%ctype", separator)
+}
+
+// DockerLabelLevel returns the fragment of a Docker security opt that
+// describes the SELinux level. Note that strictly speaking this is not
+// actually the name of the security opt, but a fragment of the whole key-
+// value pair necessary to set the opt.
+func DockerLabelLevel(separator rune) string {
+	return fmt.Sprintf("label%clevel", separator)
+}
+
+// DockerLaelDisable returns the Docker security opt that disables SELinux for
+// the container.
+func DockerLabelDisable(separator rune) string {
+	return fmt.Sprintf("label%cdisable", separator)
+}