From 3f5675880d29b203d577149a257fa5fa0f2b5397 Mon Sep 17 00:00:00 2001 From: Jean Rouge Date: Mon, 4 Feb 2019 16:42:52 -0800 Subject: [PATCH 1/6] Kubelet changes for Windows GMSA support This patch comprises the kubelet changes outlined in the GMSA KEP (https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20181221-windows-group-managed-service-accounts-for-container-identity.md) to add GMSA support to Windows workloads. More precisely, it includes the logic proposed in the KEP to resolve which GMSA spec should be applied to which containers, and changes `dockershim` to copy the relevant GMSA credential specs to Windows registry values prior to creating the container, passing them down to docker itself, and finally removing the values from the registry afterwards; both these changes need to be activated with the `WindowsGMSA` feature gate. Includes unit tests. Signed-off-by: Jean Rouge --- pkg/features/kube_features.go | 6 + pkg/kubelet/dockershim/BUILD | 9 + pkg/kubelet/dockershim/docker_container.go | 9 + .../docker_container_unsupported.go | 39 +++ .../dockershim/docker_container_windows.go | 180 ++++++++++++++ .../docker_container_windows_test.go | 233 ++++++++++++++++++ pkg/kubelet/dockershim/naming.go | 1 + pkg/kubelet/kuberuntime/BUILD | 1 + .../kuberuntime_container_windows.go | 43 ++++ .../kuberuntime_container_windows_test.go | 82 ++++++ 10 files changed, 603 insertions(+) create mode 100644 pkg/kubelet/dockershim/docker_container_unsupported.go create mode 100644 pkg/kubelet/dockershim/docker_container_windows.go create mode 100644 pkg/kubelet/dockershim/docker_container_windows_test.go create mode 100644 pkg/kubelet/kuberuntime/kuberuntime_container_windows_test.go diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index a3b523a8d77..d6827246908 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -404,6 +404,12 @@ const ( // // Enables the AWS EBS in-tree driver to AWS EBS CSI Driver migration feature. CSIMigrationAWS utilfeature.Feature = "CSIMigrationAWS" + + // owner: @wk8 + // alpha: v1.14 + // + // Enables GMSA support for Windows workloads. + WindowsGMSA utilfeature.Feature = "WindowsGMSA" ) func init() { diff --git a/pkg/kubelet/dockershim/BUILD b/pkg/kubelet/dockershim/BUILD index 5915ed02124..4cc4e520794 100644 --- a/pkg/kubelet/dockershim/BUILD +++ b/pkg/kubelet/dockershim/BUILD @@ -7,6 +7,8 @@ go_library( "doc.go", "docker_checkpoint.go", "docker_container.go", + "docker_container_unsupported.go", + "docker_container_windows.go", "docker_image.go", "docker_image_linux.go", "docker_image_unsupported.go", @@ -71,8 +73,11 @@ go_library( "//vendor/k8s.io/utils/exec:go_default_library", ] + select({ "@io_bazel_rules_go//go/platform:windows": [ + "//pkg/features:go_default_library", "//pkg/kubelet/apis:go_default_library", "//pkg/kubelet/winstats:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", + "//vendor/golang.org/x/sys/windows/registry:go_default_library", ], "//conditions:default": [], }), @@ -84,6 +89,7 @@ go_test( "convert_test.go", "docker_checkpoint_test.go", "docker_container_test.go", + "docker_container_windows_test.go", "docker_image_test.go", "docker_sandbox_test.go", "docker_service_test.go", @@ -118,6 +124,9 @@ go_test( "@io_bazel_rules_go//go/platform:linux": [ "//staging/src/k8s.io/api/core/v1:go_default_library", ], + "@io_bazel_rules_go//go/platform:windows": [ + "//vendor/golang.org/x/sys/windows/registry:go_default_library", + ], "//conditions:default": [], }), ) diff --git a/pkg/kubelet/dockershim/docker_container.go b/pkg/kubelet/dockershim/docker_container.go index 3c6b9b48497..47908d3fc40 100644 --- a/pkg/kubelet/dockershim/docker_container.go +++ b/pkg/kubelet/dockershim/docker_container.go @@ -162,11 +162,20 @@ func (ds *dockerService) CreateContainer(_ context.Context, r *runtimeapi.Create hc.SecurityOpt = append(hc.SecurityOpt, securityOpts...) + cleanupInfo, err := ds.applyPlatformSpecificDockerConfig(r, &createConfig) + if err != nil { + return nil, err + } + createResp, err := ds.client.CreateContainer(createConfig) if err != nil { createResp, err = recoverFromCreationConflictIfNeeded(ds.client, createConfig, err) } + if err = ds.performPlatformSpecificContainerCreationCleanup(cleanupInfo); err != nil { + return nil, err + } + if createResp != nil { return &runtimeapi.CreateContainerResponse{ContainerId: createResp.ID}, nil } diff --git a/pkg/kubelet/dockershim/docker_container_unsupported.go b/pkg/kubelet/dockershim/docker_container_unsupported.go new file mode 100644 index 00000000000..38e7c9fdbac --- /dev/null +++ b/pkg/kubelet/dockershim/docker_container_unsupported.go @@ -0,0 +1,39 @@ +// +build !windows + +/* +Copyright 2019 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 dockershim + +import ( + dockertypes "github.com/docker/docker/api/types" + runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" +) + +type containerCreationCleanupInfo struct{} + +// applyPlatformSpecificDockerConfig applies platform-specific configurations to a dockertypes.ContainerCreateConfig struct. +// The containerCreationCleanupInfo struct it returns will be passed as is to performPlatformSpecificContainerCreationCleanup +// after the container has been created. +func (ds *dockerService) applyPlatformSpecificDockerConfig(*runtimeapi.CreateContainerRequest, *dockertypes.ContainerCreateConfig) (*containerCreationCleanupInfo, error) { + return nil, nil +} + +// performPlatformSpecificContainerCreationCleanup is responsible for doing any platform-specific cleanup +// after a container creation. +func (ds *dockerService) performPlatformSpecificContainerCreationCleanup(*containerCreationCleanupInfo) error { + return nil +} diff --git a/pkg/kubelet/dockershim/docker_container_windows.go b/pkg/kubelet/dockershim/docker_container_windows.go new file mode 100644 index 00000000000..e102edbbbbe --- /dev/null +++ b/pkg/kubelet/dockershim/docker_container_windows.go @@ -0,0 +1,180 @@ +// +build windows + +/* +Copyright 2019 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 dockershim + +import ( + "crypto/rand" + "crypto/sha256" + "encoding/hex" + "fmt" + "io" + + "golang.org/x/sys/windows/registry" + + dockertypes "github.com/docker/docker/api/types" + dockercontainer "github.com/docker/docker/api/types/container" + + utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/klog" + kubefeatures "k8s.io/kubernetes/pkg/features" + runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" + "k8s.io/kubernetes/pkg/kubelet/kuberuntime" +) + +type containerCreationCleanupInfo struct { + gMSARegistryValueName string +} + +// applyPlatformSpecificDockerConfig applies platform-specific configurations to a dockertypes.ContainerCreateConfig struct. +// The containerCreationCleanupInfo struct it returns will be passed as is to performPlatformSpecificContainerCreationCleanup +// after the container has been created. +func (ds *dockerService) applyPlatformSpecificDockerConfig(request *runtimeapi.CreateContainerRequest, createConfig *dockertypes.ContainerCreateConfig) (*containerCreationCleanupInfo, error) { + cleanupInfo := &containerCreationCleanupInfo{} + + if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.WindowsGMSA) { + if err := applyGMSAConfig(request, createConfig, cleanupInfo); err != nil { + return nil, err + } + } + + return cleanupInfo, nil +} + +// applyGMSAConfig looks at the kuberuntime.GMSASpecContainerAnnotationKey container annotation; if present, +// it copies its contents to a unique registry value, and sets a SecurityOpt on the config pointing to that registry value. +// We use registry values instead of files since their location cannot change - as opposed to credential spec files, +// whose location could potentially change down the line, or even be unknown (eg if docker is not installed on the +// C: drive) +// When docker supports passing a credential spec's contents directly, we should switch to using that +// as it will avoid cluttering the registry. +func applyGMSAConfig(request *runtimeapi.CreateContainerRequest, createConfig *dockertypes.ContainerCreateConfig, cleanupInfo *containerCreationCleanupInfo) error { + config := request.GetConfig() + credSpec := config.Annotations[kuberuntime.GMSASpecContainerAnnotationKey] + if credSpec == "" { + return nil + } + + valueName, err := copyGMSACredSpecToRegistryValue(credSpec, makeContainerName(request.GetSandboxConfig(), config)) + if err != nil { + return err + } + + if createConfig.HostConfig == nil { + createConfig.HostConfig = &dockercontainer.HostConfig{} + } + + createConfig.HostConfig.SecurityOpt = append(createConfig.HostConfig.SecurityOpt, "credentialspec=registry://"+valueName) + cleanupInfo.gMSARegistryValueName = valueName + + return nil +} + +const ( + registryNamePrefix = "k8s-cred-spec-" + // same as https://github.com/moby/moby/blob/93d994e29c9cc8d81f1b0477e28d705fa7e2cd72/daemon/oci_windows.go#L23 + credentialSpecRegistryLocation = `SOFTWARE\Microsoft\Windows NT\CurrentVersion\Virtualization\Containers\CredentialSpecs` +) + +// useful to allow mocking the registry in tests +type registryKey interface { + SetStringValue(name, value string) error + DeleteValue(name string) error + Close() error +} + +var registryCreateKeyFunc = func(baseKey registry.Key, path string, access uint32) (registryKey, bool, error) { + return registry.CreateKey(baseKey, path, access) +} + +// and same for random +var randomReader = rand.Reader + +// copyGMSACredSpecToRegistryKey copies the credential specs to a unique registry value, and returns its name. +// To avoid leaking registry keys over the life of the node, we generate a unique name for that value, and clean +// it up after creating the container. +func copyGMSACredSpecToRegistryValue(credSpec string, dockerContainerName string) (string, error) { + valueName, err := gMSARegistryValueName(credSpec, dockerContainerName) + if err != nil { + return "", err + } + + // write to the registry + key, _, err := registryCreateKeyFunc(registry.LOCAL_MACHINE, credentialSpecRegistryLocation, registry.SET_VALUE) + if err != nil { + return "", fmt.Errorf("unable to open registry key %s: %v", credentialSpecRegistryLocation, err) + } + defer key.Close() + if err = key.SetStringValue(valueName, credSpec); err != nil { + return "", fmt.Errorf("unable to write into registry value %s/%s: %v", credentialSpecRegistryLocation, valueName, err) + } + + return valueName, nil +} + +// gMSARegistryValueName computes the name of the registry value where to store the GMSA cred spec contents. +// The value's name is computed by concatenating the docker container's name (guaranteed to be unique over the +// container's lifetime), the value itself, and an additional 64 random bytes. +func gMSARegistryValueName(inputs ...string) (string, error) { + hasher := sha256.New() + for _, s := range inputs { + // according to the doc, that can never return an error + io.WriteString(hasher, s) + } + randBytes := make([]byte, 64) + if _, err := randomReader.Read(randBytes); err != nil { + return "", fmt.Errorf("unable to generate random string: %v", err) + } + hasher.Write(randBytes) + + return registryNamePrefix + hex.EncodeToString(hasher.Sum(nil)), nil +} + +// performPlatformSpecificContainerCreationCleanup is responsible for doing any platform-specific cleanup +// after a container creation. +func (ds *dockerService) performPlatformSpecificContainerCreationCleanup(cleanupInfo *containerCreationCleanupInfo) error { + if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.WindowsGMSA) { + // this is best effort, we don't bubble errors upstream as failing to remove the GMSA registry keys shouldn't + // prevent k8s from working correctly, and the leaked registry keys are not a major concern anyway: + // they don't contain any secret, and they're sufficiently random to prevent collisions with + // future ones + if err := removeGMSARegistryValue(cleanupInfo); err != nil { + klog.Warningf("won't remove GMSA cred spec registry value: %v", err) + } + } + + return nil +} + +// removeGMSARegistryValue removes the registry value containing the GMSA cred spec for this container, if any. +func removeGMSARegistryValue(cleanupInfo *containerCreationCleanupInfo) error { + if cleanupInfo.gMSARegistryValueName == "" { + return nil + } + + key, _, err := registryCreateKeyFunc(registry.LOCAL_MACHINE, credentialSpecRegistryLocation, registry.SET_VALUE) + if err != nil { + return fmt.Errorf("unable to open registry key %s: %v", credentialSpecRegistryLocation, err) + } + defer key.Close() + if err = key.DeleteValue(cleanupInfo.gMSARegistryValueName); err != nil { + return fmt.Errorf("unable to remove registry value %s/%s: %v", credentialSpecRegistryLocation, cleanupInfo.gMSARegistryValueName, err) + } + + return nil +} diff --git a/pkg/kubelet/dockershim/docker_container_windows_test.go b/pkg/kubelet/dockershim/docker_container_windows_test.go new file mode 100644 index 00000000000..e4f553a7076 --- /dev/null +++ b/pkg/kubelet/dockershim/docker_container_windows_test.go @@ -0,0 +1,233 @@ +/* +Copyright 2019 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 dockershim + +import ( + "bytes" + "fmt" + "regexp" + "strings" + "testing" + + dockertypes "github.com/docker/docker/api/types" + "github.com/stretchr/testify/assert" + "golang.org/x/sys/windows/registry" + runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" +) + +type dummyRegistryKey struct { + setStringError error + stringValues [][]string + deleteValueError error + deletedValueNames []string + closed bool +} + +func (k *dummyRegistryKey) SetStringValue(name, value string) error { + k.stringValues = append(k.stringValues, []string{name, value}) + return k.setStringError +} + +func (k *dummyRegistryKey) DeleteValue(name string) error { + k.deletedValueNames = append(k.deletedValueNames, name) + return k.deleteValueError +} + +func (k *dummyRegistryKey) Close() error { + k.closed = true + return nil +} + +func TestApplyGMSAConfig(t *testing.T) { + dummyCredSpec := "test cred spec contents" + randomBytes := []byte{85, 205, 157, 137, 41, 50, 187, 175, 242, 115, 92, 212, 181, 70, 56, 20, 172, 17, 100, 178, 19, 42, 217, 177, 240, 37, 127, 123, 53, 250, 61, 157, 11, 41, 69, 160, 117, 163, 51, 118, 53, 86, 167, 111, 137, 78, 195, 229, 50, 144, 178, 209, 66, 107, 144, 165, 184, 92, 10, 17, 229, 163, 194, 12} + expectedHash := "8975ef53024af213c1aca6dfc6e2e48f42c3a984a79e67b140627b8d96007c2a" + expectedValueName := "k8s-cred-spec-" + expectedHash + + sandboxConfig := &runtimeapi.PodSandboxConfig{ + Metadata: &runtimeapi.PodSandboxMetadata{ + Namespace: "namespace", + Uid: "uid", + }, + } + + containerMeta := &runtimeapi.ContainerMetadata{ + Name: "container_name", + Attempt: 12, + } + + requestWithoutGMSAAnnotation := &runtimeapi.CreateContainerRequest{ + Config: &runtimeapi.ContainerConfig{Metadata: containerMeta}, + SandboxConfig: sandboxConfig, + } + + requestWithGMSAAnnotation := &runtimeapi.CreateContainerRequest{ + Config: &runtimeapi.ContainerConfig{ + Metadata: containerMeta, + Annotations: map[string]string{"container.alpha.windows.kubernetes.io/gmsa-credential-spec": dummyCredSpec}, + }, + SandboxConfig: sandboxConfig, + } + + t.Run("happy path", func(t *testing.T) { + key := &dummyRegistryKey{} + defer setRegistryCreateKeyFunc(t, key)() + defer setRandomReader(randomBytes)() + + createConfig := &dockertypes.ContainerCreateConfig{} + cleanupInfo := &containerCreationCleanupInfo{} + err := applyGMSAConfig(requestWithGMSAAnnotation, createConfig, cleanupInfo) + + assert.Nil(t, err) + + // the registry key should have been properly created + assert.Equal(t, 1, len(key.stringValues)) + assert.Equal(t, []string{expectedValueName, dummyCredSpec}, key.stringValues[0]) + assert.True(t, key.closed) + + // the create config's security opt should have been populated + assert.Equal(t, createConfig.HostConfig.SecurityOpt, []string{"credentialspec=registry://" + expectedValueName}) + + // and the name of that value should have been saved to the cleanup info + assert.Equal(t, expectedValueName, cleanupInfo.gMSARegistryValueName) + }) + t.Run("happy path with a truly random string", func(t *testing.T) { + defer setRegistryCreateKeyFunc(t, &dummyRegistryKey{})() + + createConfig := &dockertypes.ContainerCreateConfig{} + cleanupInfo := &containerCreationCleanupInfo{} + err := applyGMSAConfig(requestWithGMSAAnnotation, createConfig, cleanupInfo) + + assert.Nil(t, err) + + secOpt := createConfig.HostConfig.SecurityOpt[0] + + expectedPrefix := "credentialspec=registry://k8s-cred-spec-" + assert.Equal(t, expectedPrefix, secOpt[:len(expectedPrefix)]) + + hash := secOpt[len(expectedPrefix):] + hexRegex, _ := regexp.Compile("^[0-9a-f]{64}$") + assert.True(t, hexRegex.MatchString(hash)) + assert.NotEqual(t, expectedHash, hash) + + assert.Equal(t, "k8s-cred-spec-"+hash, cleanupInfo.gMSARegistryValueName) + }) + t.Run("if there's an error opening the registry key", func(t *testing.T) { + defer setRegistryCreateKeyFunc(t, &dummyRegistryKey{}, fmt.Errorf("dummy error"))() + + err := applyGMSAConfig(requestWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCreationCleanupInfo{}) + + assert.NotNil(t, err) + assert.True(t, strings.Contains(err.Error(), "unable to open registry key")) + }) + t.Run("if there's an error writing the registry key", func(t *testing.T) { + key := &dummyRegistryKey{} + key.setStringError = fmt.Errorf("dummy error") + defer setRegistryCreateKeyFunc(t, key)() + + err := applyGMSAConfig(requestWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCreationCleanupInfo{}) + + assert.NotNil(t, err) + assert.True(t, strings.Contains(err.Error(), "unable to write into registry value")) + assert.True(t, key.closed) + }) + t.Run("if there is no GMSA annotation", func(t *testing.T) { + createConfig := &dockertypes.ContainerCreateConfig{} + + err := applyGMSAConfig(requestWithoutGMSAAnnotation, createConfig, &containerCreationCleanupInfo{}) + + assert.Nil(t, err) + assert.Nil(t, createConfig.HostConfig) + }) +} + +func TestRemoveGMSARegistryValue(t *testing.T) { + emptyCleanupInfo := &containerCreationCleanupInfo{} + + valueName := "k8s-cred-spec-8975ef53024af213c1aca6dfc6e2e48f42c3a984a79e67b140627b8d96007c2a" + cleanupInfoWithValue := &containerCreationCleanupInfo{gMSARegistryValueName: valueName} + + t.Run("it does remove the registry value", func(t *testing.T) { + key := &dummyRegistryKey{} + defer setRegistryCreateKeyFunc(t, key)() + + err := removeGMSARegistryValue(cleanupInfoWithValue) + + assert.Nil(t, err) + + // the registry key should have been properly deleted + assert.Equal(t, 1, len(key.deletedValueNames)) + assert.Equal(t, []string{valueName}, key.deletedValueNames) + assert.True(t, key.closed) + }) + t.Run("if there's an error opening the registry key", func(t *testing.T) { + defer setRegistryCreateKeyFunc(t, &dummyRegistryKey{}, fmt.Errorf("dummy error"))() + + err := removeGMSARegistryValue(cleanupInfoWithValue) + + assert.NotNil(t, err) + assert.True(t, strings.Contains(err.Error(), "unable to open registry key")) + }) + t.Run("if there's an error writing the registry key", func(t *testing.T) { + key := &dummyRegistryKey{} + key.deleteValueError = fmt.Errorf("dummy error") + defer setRegistryCreateKeyFunc(t, key)() + + err := removeGMSARegistryValue(cleanupInfoWithValue) + + assert.NotNil(t, err) + assert.True(t, strings.Contains(err.Error(), "unable to remove registry value")) + assert.True(t, key.closed) + }) + t.Run("if there's no registry value to be removed", func(t *testing.T) { + err := removeGMSARegistryValue(emptyCleanupInfo) + + assert.Nil(t, err) + }) +} + +// setRegistryCreateKeyFunc replaces the registryCreateKeyFunc package variable, and returns a function +// to be called to revert the change when done with testing. +func setRegistryCreateKeyFunc(t *testing.T, key *dummyRegistryKey, err ...error) func() { + previousRegistryCreateKeyFunc := registryCreateKeyFunc + + registryCreateKeyFunc = func(baseKey registry.Key, path string, access uint32) (registryKey, bool, error) { + // this should always be called with exactly the same arguments + assert.Equal(t, registry.LOCAL_MACHINE, baseKey) + assert.Equal(t, credentialSpecRegistryLocation, path) + assert.Equal(t, uint32(registry.SET_VALUE), access) + + if len(err) > 0 { + return nil, false, err[0] + } + return key, false, nil + } + + return func() { + registryCreateKeyFunc = previousRegistryCreateKeyFunc + } +} + +// setRandomReader replaces the randomReader package variable with a dummy reader that returns the provided +// byte slice, and returns a function to be called to revert the change when done with testing. +func setRandomReader(b []byte) func() { + previousRandomReader := randomReader + randomReader = bytes.NewReader(b) + return func() { + randomReader = previousRandomReader + } +} diff --git a/pkg/kubelet/dockershim/naming.go b/pkg/kubelet/dockershim/naming.go index a2a97c2da4e..cb2bc467b1b 100644 --- a/pkg/kubelet/dockershim/naming.go +++ b/pkg/kubelet/dockershim/naming.go @@ -66,6 +66,7 @@ func makeSandboxName(s *runtimeapi.PodSandboxConfig) string { }, nameDelimiter) } +// makeContainerName generates a container name that's guaranteed to be unique on its host. func makeContainerName(s *runtimeapi.PodSandboxConfig, c *runtimeapi.ContainerConfig) string { return strings.Join([]string{ kubePrefix, // 0 diff --git a/pkg/kubelet/kuberuntime/BUILD b/pkg/kubelet/kuberuntime/BUILD index 12c16e7e548..c7eec262375 100644 --- a/pkg/kubelet/kuberuntime/BUILD +++ b/pkg/kubelet/kuberuntime/BUILD @@ -89,6 +89,7 @@ go_test( "instrumented_services_test.go", "kuberuntime_container_linux_test.go", "kuberuntime_container_test.go", + "kuberuntime_container_windows_test.go", "kuberuntime_gc_test.go", "kuberuntime_image_test.go", "kuberuntime_manager_test.go", diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container_windows.go b/pkg/kubelet/kuberuntime/kuberuntime_container_windows.go index 77165e81b21..da606c3cfbf 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_container_windows.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_container_windows.go @@ -23,6 +23,8 @@ import ( "github.com/docker/docker/pkg/sysinfo" "k8s.io/api/core/v1" + utilfeature "k8s.io/apiserver/pkg/util/feature" + kubefeatures "k8s.io/kubernetes/pkg/features" kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis" runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" "k8s.io/kubernetes/pkg/securitycontext" @@ -35,6 +37,10 @@ func (m *kubeGenericRuntimeManager) applyPlatformSpecificContainerConfig(config return err } + if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.WindowsGMSA) { + determineEffectiveSecurityContext(config, container, pod) + } + config.Windows = windowsConfig return nil } @@ -97,3 +103,40 @@ func (m *kubeGenericRuntimeManager) generateWindowsContainerConfig(container *v1 return wc, nil } + +const ( + // GMSASpecContainerAnnotationKey is the container annotation where we store the contents of the GMSA credential spec to use. + GMSASpecContainerAnnotationKey = "container.alpha.windows.kubernetes.io/gmsa-credential-spec" + // gMSAContainerSpecPodAnnotationKeySuffix is the suffix of the pod annotation where the GMSA webhook admission controller + // stores the contents of the GMSA credential spec for a given container (the full annotation being the container's name + // with this suffix appended). + gMSAContainerSpecPodAnnotationKeySuffix = "." + GMSASpecContainerAnnotationKey + // gMSAPodSpecPodAnnotationKey is the pod annotation where the GMSA webhook admission controller stores the contents of the GMSA + // credential spec to use for containers that do not have their own specific GMSA cred spec set via a + // gMSAContainerSpecPodAnnotationKeySuffix annotation as explained above + gMSAPodSpecPodAnnotationKey = "pod.alpha.windows.kubernetes.io/gmsa-credential-spec" +) + +// determineEffectiveSecurityContext determines the effective GMSA credential spec and, if any, copies it to the container's +// GMSASpecContainerAnnotationKey annotation. +func determineEffectiveSecurityContext(config *runtimeapi.ContainerConfig, container *v1.Container, pod *v1.Pod) { + var containerCredSpec string + + containerGMSAPodAnnotation := container.Name + gMSAContainerSpecPodAnnotationKeySuffix + if pod.Annotations[containerGMSAPodAnnotation] != "" { + containerCredSpec = pod.Annotations[containerGMSAPodAnnotation] + } else if pod.Annotations[gMSAPodSpecPodAnnotationKey] != "" { + containerCredSpec = pod.Annotations[gMSAPodSpecPodAnnotationKey] + } + + if containerCredSpec != "" { + if config.Annotations == nil { + config.Annotations = make(map[string]string) + } + config.Annotations[GMSASpecContainerAnnotationKey] = containerCredSpec + } else { + // the annotation shouldn't be present, but let's err on the side of caution: + // it should only be set here and nowhere else + delete(config.Annotations, GMSASpecContainerAnnotationKey) + } +} diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container_windows_test.go b/pkg/kubelet/kuberuntime/kuberuntime_container_windows_test.go new file mode 100644 index 00000000000..e10b43eca9b --- /dev/null +++ b/pkg/kubelet/kuberuntime/kuberuntime_container_windows_test.go @@ -0,0 +1,82 @@ +/* +Copyright 2019 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 kuberuntime + +import ( + "github.com/stretchr/testify/assert" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" +) + +func TestDetermineEffectiveSecurityContext(t *testing.T) { + containerName := "container_name" + container := &corev1.Container{Name: containerName} + dummyCredSpec := "test cred spec contents" + + buildPod := func(annotations map[string]string) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: annotations, + }, + } + } + + t.Run("when there's a specific GMSA for that pod, and no pod-wide GMSA", func(t *testing.T) { + containerConfig := &runtimeapi.ContainerConfig{} + + pod := buildPod(map[string]string{ + "container_name.container.alpha.windows.kubernetes.io/gmsa-credential-spec": dummyCredSpec, + }) + + determineEffectiveSecurityContext(containerConfig, container, pod) + + assert.Equal(t, dummyCredSpec, containerConfig.Annotations["container.alpha.windows.kubernetes.io/gmsa-credential-spec"]) + }) + t.Run("when there's a specific GMSA for that pod, and a pod-wide GMSA", func(t *testing.T) { + containerConfig := &runtimeapi.ContainerConfig{} + + pod := buildPod(map[string]string{ + "container_name.container.alpha.windows.kubernetes.io/gmsa-credential-spec": dummyCredSpec, + "pod.alpha.windows.kubernetes.io/gmsa-credential-spec": "should be ignored", + }) + + determineEffectiveSecurityContext(containerConfig, container, pod) + + assert.Equal(t, dummyCredSpec, containerConfig.Annotations["container.alpha.windows.kubernetes.io/gmsa-credential-spec"]) + }) + t.Run("when there's no specific GMSA for that pod, and a pod-wide GMSA", func(t *testing.T) { + containerConfig := &runtimeapi.ContainerConfig{} + + pod := buildPod(map[string]string{ + "pod.alpha.windows.kubernetes.io/gmsa-credential-spec": dummyCredSpec, + }) + + determineEffectiveSecurityContext(containerConfig, container, pod) + + assert.Equal(t, dummyCredSpec, containerConfig.Annotations["container.alpha.windows.kubernetes.io/gmsa-credential-spec"]) + }) + t.Run("when there's no specific GMSA for that pod, and no pod-wide GMSA", func(t *testing.T) { + containerConfig := &runtimeapi.ContainerConfig{} + + determineEffectiveSecurityContext(containerConfig, container, &corev1.Pod{}) + + assert.Nil(t, containerConfig.Annotations) + }) +} From c4806186d4ac8a778d45f300f39bae270fbc0e0a Mon Sep 17 00:00:00 2001 From: Jean Rouge Date: Tue, 5 Feb 2019 15:25:04 -0800 Subject: [PATCH 2/6] Review comments * value names are now purely random * cleaning up leaked registry keys at Kubelet init * fixing a small bug masking create errors Signed-off-by: Jean Rouge --- pkg/kubelet/dockershim/docker_container.go | 12 +- .../docker_container_unsupported.go | 7 + .../dockershim/docker_container_windows.go | 86 ++++--- .../docker_container_windows_test.go | 215 ++++++++++++------ pkg/kubelet/dockershim/docker_service.go | 12 + pkg/kubelet/dockershim/naming.go | 1 - .../kuberuntime_container_windows_test.go | 8 +- 7 files changed, 235 insertions(+), 106 deletions(-) diff --git a/pkg/kubelet/dockershim/docker_container.go b/pkg/kubelet/dockershim/docker_container.go index 47908d3fc40..acabd69eba5 100644 --- a/pkg/kubelet/dockershim/docker_container.go +++ b/pkg/kubelet/dockershim/docker_container.go @@ -167,19 +167,23 @@ func (ds *dockerService) CreateContainer(_ context.Context, r *runtimeapi.Create return nil, err } - createResp, err := ds.client.CreateContainer(createConfig) - if err != nil { - createResp, err = recoverFromCreationConflictIfNeeded(ds.client, createConfig, err) + createResp, createErr := ds.client.CreateContainer(createConfig) + if createErr != nil { + createResp, createErr = recoverFromCreationConflictIfNeeded(ds.client, createConfig, createErr) } if err = ds.performPlatformSpecificContainerCreationCleanup(cleanupInfo); err != nil { + if createErr != nil { + // that one is more important to surface + return nil, createErr + } return nil, err } if createResp != nil { return &runtimeapi.CreateContainerResponse{ContainerId: createResp.ID}, nil } - return nil, err + return nil, createErr } // getContainerLogPath returns the container log path specified by kubelet and the real diff --git a/pkg/kubelet/dockershim/docker_container_unsupported.go b/pkg/kubelet/dockershim/docker_container_unsupported.go index 38e7c9fdbac..11ec38b6335 100644 --- a/pkg/kubelet/dockershim/docker_container_unsupported.go +++ b/pkg/kubelet/dockershim/docker_container_unsupported.go @@ -37,3 +37,10 @@ func (ds *dockerService) applyPlatformSpecificDockerConfig(*runtimeapi.CreateCon func (ds *dockerService) performPlatformSpecificContainerCreationCleanup(*containerCreationCleanupInfo) error { return nil } + +// platformSpecificContainerCreationKubeletInitCleanup is called when the kubelet +// is starting, and is meant to clean up any cruft left by previous runs; +// errors are simply logged, but don't prevent the kubelet from starting. +func (ds *dockerService) platformSpecificContainerCreationInitCleanup() (errors []error) { + return +} diff --git a/pkg/kubelet/dockershim/docker_container_windows.go b/pkg/kubelet/dockershim/docker_container_windows.go index e102edbbbbe..fdbf6fcc6fb 100644 --- a/pkg/kubelet/dockershim/docker_container_windows.go +++ b/pkg/kubelet/dockershim/docker_container_windows.go @@ -20,10 +20,9 @@ package dockershim import ( "crypto/rand" - "crypto/sha256" "encoding/hex" "fmt" - "io" + "regexp" "golang.org/x/sys/windows/registry" @@ -48,7 +47,7 @@ func (ds *dockerService) applyPlatformSpecificDockerConfig(request *runtimeapi.C cleanupInfo := &containerCreationCleanupInfo{} if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.WindowsGMSA) { - if err := applyGMSAConfig(request, createConfig, cleanupInfo); err != nil { + if err := applyGMSAConfig(request.GetConfig(), createConfig, cleanupInfo); err != nil { return nil, err } } @@ -63,14 +62,13 @@ func (ds *dockerService) applyPlatformSpecificDockerConfig(request *runtimeapi.C // C: drive) // When docker supports passing a credential spec's contents directly, we should switch to using that // as it will avoid cluttering the registry. -func applyGMSAConfig(request *runtimeapi.CreateContainerRequest, createConfig *dockertypes.ContainerCreateConfig, cleanupInfo *containerCreationCleanupInfo) error { - config := request.GetConfig() +func applyGMSAConfig(config *runtimeapi.ContainerConfig, createConfig *dockertypes.ContainerCreateConfig, cleanupInfo *containerCreationCleanupInfo) error { credSpec := config.Annotations[kuberuntime.GMSASpecContainerAnnotationKey] if credSpec == "" { return nil } - valueName, err := copyGMSACredSpecToRegistryValue(credSpec, makeContainerName(request.GetSandboxConfig(), config)) + valueName, err := copyGMSACredSpecToRegistryValue(credSpec) if err != nil { return err } @@ -86,15 +84,19 @@ func applyGMSAConfig(request *runtimeapi.CreateContainerRequest, createConfig *d } const ( - registryNamePrefix = "k8s-cred-spec-" // same as https://github.com/moby/moby/blob/93d994e29c9cc8d81f1b0477e28d705fa7e2cd72/daemon/oci_windows.go#L23 credentialSpecRegistryLocation = `SOFTWARE\Microsoft\Windows NT\CurrentVersion\Virtualization\Containers\CredentialSpecs` + // the prefix for the registry values we write GMSA cred specs to + gMSARegistryValueNamePrefix = "k8s-cred-spec-" + // the number of random bytes to generate suffixes for registry value names + gMSARegistryValueNameSuffixRandomBytes = 40 ) // useful to allow mocking the registry in tests type registryKey interface { SetStringValue(name, value string) error DeleteValue(name string) error + ReadValueNames(n int) ([]string, error) Close() error } @@ -106,10 +108,8 @@ var registryCreateKeyFunc = func(baseKey registry.Key, path string, access uint3 var randomReader = rand.Reader // copyGMSACredSpecToRegistryKey copies the credential specs to a unique registry value, and returns its name. -// To avoid leaking registry keys over the life of the node, we generate a unique name for that value, and clean -// it up after creating the container. -func copyGMSACredSpecToRegistryValue(credSpec string, dockerContainerName string) (string, error) { - valueName, err := gMSARegistryValueName(credSpec, dockerContainerName) +func copyGMSACredSpecToRegistryValue(credSpec string) (string, error) { + valueName, err := gMSARegistryValueName() if err != nil { return "", err } @@ -128,21 +128,18 @@ func copyGMSACredSpecToRegistryValue(credSpec string, dockerContainerName string } // gMSARegistryValueName computes the name of the registry value where to store the GMSA cred spec contents. -// The value's name is computed by concatenating the docker container's name (guaranteed to be unique over the -// container's lifetime), the value itself, and an additional 64 random bytes. -func gMSARegistryValueName(inputs ...string) (string, error) { - hasher := sha256.New() - for _, s := range inputs { - // according to the doc, that can never return an error - io.WriteString(hasher, s) - } - randBytes := make([]byte, 64) - if _, err := randomReader.Read(randBytes); err != nil { - return "", fmt.Errorf("unable to generate random string: %v", err) - } - hasher.Write(randBytes) +// The value's name is purely random. +func gMSARegistryValueName() (string, error) { + randBytes := make([]byte, gMSARegistryValueNameSuffixRandomBytes) - return registryNamePrefix + hex.EncodeToString(hasher.Sum(nil)), nil + if n, err := randomReader.Read(randBytes); err != nil || n != gMSARegistryValueNameSuffixRandomBytes { + if err == nil { + err = fmt.Errorf("only got %v random bytes, expected %v", n, len(randBytes)) + } + return "", fmt.Errorf("unable to generate random registry value name: %v", err) + } + + return gMSARegistryValueNamePrefix + hex.EncodeToString(randBytes), nil } // performPlatformSpecificContainerCreationCleanup is responsible for doing any platform-specific cleanup @@ -161,9 +158,8 @@ func (ds *dockerService) performPlatformSpecificContainerCreationCleanup(cleanup return nil } -// removeGMSARegistryValue removes the registry value containing the GMSA cred spec for this container, if any. func removeGMSARegistryValue(cleanupInfo *containerCreationCleanupInfo) error { - if cleanupInfo.gMSARegistryValueName == "" { + if cleanupInfo == nil || cleanupInfo.gMSARegistryValueName == "" { return nil } @@ -178,3 +174,39 @@ func removeGMSARegistryValue(cleanupInfo *containerCreationCleanupInfo) error { return nil } + +// platformSpecificContainerCreationKubeletInitCleanup is called when the kubelet +// is starting, and is meant to clean up any cruft left by previous runs; +// errors are simply logged, but don't prevent the kubelet from starting. +func (ds *dockerService) platformSpecificContainerCreationInitCleanup() (errors []error) { + if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.WindowsGMSA) { + errors = removeAllGMSARegistryValues() + } + return +} + +// This is the regex used to detect gMSA cred spec registry values. +var gMSARegistryValueNamesRegex = regexp.MustCompile(fmt.Sprintf("^%s[0-9a-f]{%d}$", gMSARegistryValueNamePrefix, 2*gMSARegistryValueNameSuffixRandomBytes)) + +func removeAllGMSARegistryValues() (errors []error) { + key, _, err := registryCreateKeyFunc(registry.LOCAL_MACHINE, credentialSpecRegistryLocation, registry.SET_VALUE) + if err != nil { + return []error{fmt.Errorf("unable to open registry key %s: %v", credentialSpecRegistryLocation, err)} + } + defer key.Close() + + valueNames, err := key.ReadValueNames(0) + if err != nil { + return []error{fmt.Errorf("unable to list values under registry key %s: %v", credentialSpecRegistryLocation, err)} + } + + for _, valueName := range valueNames { + if gMSARegistryValueNamesRegex.MatchString(valueName) { + if err = key.DeleteValue(valueName); err != nil { + errors = append(errors, fmt.Errorf("unable to remove registry value %s/%s: %v", credentialSpecRegistryLocation, valueName, err)) + } + } + } + + return +} diff --git a/pkg/kubelet/dockershim/docker_container_windows_test.go b/pkg/kubelet/dockershim/docker_container_windows_test.go index e4f553a7076..bf321c1eb62 100644 --- a/pkg/kubelet/dockershim/docker_container_windows_test.go +++ b/pkg/kubelet/dockershim/docker_container_windows_test.go @@ -30,21 +30,35 @@ import ( ) type dummyRegistryKey struct { - setStringError error - stringValues [][]string - deleteValueError error - deletedValueNames []string - closed bool + setStringValueError error + setStringValueArgs [][]string + + deleteValueFunc func(name string) error + deleteValueArgs []string + + readValueNamesError error + readValueNamesReturn []string + readValueNamesArgs []int + + closed bool } func (k *dummyRegistryKey) SetStringValue(name, value string) error { - k.stringValues = append(k.stringValues, []string{name, value}) - return k.setStringError + k.setStringValueArgs = append(k.setStringValueArgs, []string{name, value}) + return k.setStringValueError } func (k *dummyRegistryKey) DeleteValue(name string) error { - k.deletedValueNames = append(k.deletedValueNames, name) - return k.deleteValueError + k.deleteValueArgs = append(k.deleteValueArgs, name) + if k.deleteValueFunc == nil { + return nil + } + return k.deleteValueFunc(name) +} + +func (k *dummyRegistryKey) ReadValueNames(n int) ([]string, error) { + k.readValueNamesArgs = append(k.readValueNamesArgs, n) + return k.readValueNamesReturn, k.readValueNamesError } func (k *dummyRegistryKey) Close() error { @@ -54,33 +68,12 @@ func (k *dummyRegistryKey) Close() error { func TestApplyGMSAConfig(t *testing.T) { dummyCredSpec := "test cred spec contents" - randomBytes := []byte{85, 205, 157, 137, 41, 50, 187, 175, 242, 115, 92, 212, 181, 70, 56, 20, 172, 17, 100, 178, 19, 42, 217, 177, 240, 37, 127, 123, 53, 250, 61, 157, 11, 41, 69, 160, 117, 163, 51, 118, 53, 86, 167, 111, 137, 78, 195, 229, 50, 144, 178, 209, 66, 107, 144, 165, 184, 92, 10, 17, 229, 163, 194, 12} - expectedHash := "8975ef53024af213c1aca6dfc6e2e48f42c3a984a79e67b140627b8d96007c2a" - expectedValueName := "k8s-cred-spec-" + expectedHash + randomBytes := []byte{0x19, 0x0, 0x25, 0x45, 0x18, 0x52, 0x9e, 0x2a, 0x3d, 0xed, 0xb8, 0x5c, 0xde, 0xc0, 0x3c, 0xe2, 0x70, 0x55, 0x96, 0x47, 0x45, 0x9a, 0xb5, 0x31, 0xf0, 0x7a, 0xf5, 0xeb, 0x1c, 0x54, 0x95, 0xfd, 0xa7, 0x9, 0x43, 0x5c, 0xe8, 0x2a, 0xb8, 0x9c} + expectedHex := "1900254518529e2a3dedb85cdec03ce270559647459ab531f07af5eb1c5495fda709435ce82ab89c" + expectedValueName := "k8s-cred-spec-" + expectedHex - sandboxConfig := &runtimeapi.PodSandboxConfig{ - Metadata: &runtimeapi.PodSandboxMetadata{ - Namespace: "namespace", - Uid: "uid", - }, - } - - containerMeta := &runtimeapi.ContainerMetadata{ - Name: "container_name", - Attempt: 12, - } - - requestWithoutGMSAAnnotation := &runtimeapi.CreateContainerRequest{ - Config: &runtimeapi.ContainerConfig{Metadata: containerMeta}, - SandboxConfig: sandboxConfig, - } - - requestWithGMSAAnnotation := &runtimeapi.CreateContainerRequest{ - Config: &runtimeapi.ContainerConfig{ - Metadata: containerMeta, - Annotations: map[string]string{"container.alpha.windows.kubernetes.io/gmsa-credential-spec": dummyCredSpec}, - }, - SandboxConfig: sandboxConfig, + containerConfigWithGMSAAnnotation := &runtimeapi.ContainerConfig{ + Annotations: map[string]string{"container.alpha.windows.kubernetes.io/gmsa-credential-spec": dummyCredSpec}, } t.Run("happy path", func(t *testing.T) { @@ -90,17 +83,20 @@ func TestApplyGMSAConfig(t *testing.T) { createConfig := &dockertypes.ContainerCreateConfig{} cleanupInfo := &containerCreationCleanupInfo{} - err := applyGMSAConfig(requestWithGMSAAnnotation, createConfig, cleanupInfo) + err := applyGMSAConfig(containerConfigWithGMSAAnnotation, createConfig, cleanupInfo) assert.Nil(t, err) // the registry key should have been properly created - assert.Equal(t, 1, len(key.stringValues)) - assert.Equal(t, []string{expectedValueName, dummyCredSpec}, key.stringValues[0]) + if assert.Equal(t, 1, len(key.setStringValueArgs)) { + assert.Equal(t, []string{expectedValueName, dummyCredSpec}, key.setStringValueArgs[0]) + } assert.True(t, key.closed) // the create config's security opt should have been populated - assert.Equal(t, createConfig.HostConfig.SecurityOpt, []string{"credentialspec=registry://" + expectedValueName}) + if assert.NotNil(t, createConfig.HostConfig) { + assert.Equal(t, createConfig.HostConfig.SecurityOpt, []string{"credentialspec=registry://" + expectedValueName}) + } // and the name of that value should have been saved to the cleanup info assert.Equal(t, expectedValueName, cleanupInfo.gMSARegistryValueName) @@ -110,45 +106,58 @@ func TestApplyGMSAConfig(t *testing.T) { createConfig := &dockertypes.ContainerCreateConfig{} cleanupInfo := &containerCreationCleanupInfo{} - err := applyGMSAConfig(requestWithGMSAAnnotation, createConfig, cleanupInfo) + err := applyGMSAConfig(containerConfigWithGMSAAnnotation, createConfig, cleanupInfo) assert.Nil(t, err) - secOpt := createConfig.HostConfig.SecurityOpt[0] + if assert.NotNil(t, createConfig.HostConfig) && assert.Equal(t, 1, len(createConfig.HostConfig.SecurityOpt)) { + secOpt := createConfig.HostConfig.SecurityOpt[0] - expectedPrefix := "credentialspec=registry://k8s-cred-spec-" - assert.Equal(t, expectedPrefix, secOpt[:len(expectedPrefix)]) + expectedPrefix := "credentialspec=registry://k8s-cred-spec-" + assert.Equal(t, expectedPrefix, secOpt[:len(expectedPrefix)]) - hash := secOpt[len(expectedPrefix):] - hexRegex, _ := regexp.Compile("^[0-9a-f]{64}$") - assert.True(t, hexRegex.MatchString(hash)) - assert.NotEqual(t, expectedHash, hash) + hex := secOpt[len(expectedPrefix):] + hexRegex := regexp.MustCompile("^[0-9a-f]{80}$") + assert.True(t, hexRegex.MatchString(hex)) + assert.NotEqual(t, expectedHex, hex) - assert.Equal(t, "k8s-cred-spec-"+hash, cleanupInfo.gMSARegistryValueName) + assert.Equal(t, "k8s-cred-spec-"+hex, cleanupInfo.gMSARegistryValueName) + } + }) + t.Run("when there's an error generating the random value name", func(t *testing.T) { + defer setRandomReader([]byte{})() + + err := applyGMSAConfig(containerConfigWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCreationCleanupInfo{}) + + if assert.NotNil(t, err) { + assert.True(t, strings.Contains(err.Error(), "unable to generate random registry value name")) + } }) t.Run("if there's an error opening the registry key", func(t *testing.T) { defer setRegistryCreateKeyFunc(t, &dummyRegistryKey{}, fmt.Errorf("dummy error"))() - err := applyGMSAConfig(requestWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCreationCleanupInfo{}) + err := applyGMSAConfig(containerConfigWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCreationCleanupInfo{}) - assert.NotNil(t, err) - assert.True(t, strings.Contains(err.Error(), "unable to open registry key")) + if assert.NotNil(t, err) { + assert.True(t, strings.Contains(err.Error(), "unable to open registry key")) + } }) - t.Run("if there's an error writing the registry key", func(t *testing.T) { + t.Run("if there's an error writing to the registry key", func(t *testing.T) { key := &dummyRegistryKey{} - key.setStringError = fmt.Errorf("dummy error") + key.setStringValueError = fmt.Errorf("dummy error") defer setRegistryCreateKeyFunc(t, key)() - err := applyGMSAConfig(requestWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCreationCleanupInfo{}) + err := applyGMSAConfig(containerConfigWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCreationCleanupInfo{}) - assert.NotNil(t, err) - assert.True(t, strings.Contains(err.Error(), "unable to write into registry value")) + if assert.NotNil(t, err) { + assert.True(t, strings.Contains(err.Error(), "unable to write into registry value")) + } assert.True(t, key.closed) }) t.Run("if there is no GMSA annotation", func(t *testing.T) { createConfig := &dockertypes.ContainerCreateConfig{} - err := applyGMSAConfig(requestWithoutGMSAAnnotation, createConfig, &containerCreationCleanupInfo{}) + err := applyGMSAConfig(&runtimeapi.ContainerConfig{}, createConfig, &containerCreationCleanupInfo{}) assert.Nil(t, err) assert.Nil(t, createConfig.HostConfig) @@ -156,9 +165,7 @@ func TestApplyGMSAConfig(t *testing.T) { } func TestRemoveGMSARegistryValue(t *testing.T) { - emptyCleanupInfo := &containerCreationCleanupInfo{} - - valueName := "k8s-cred-spec-8975ef53024af213c1aca6dfc6e2e48f42c3a984a79e67b140627b8d96007c2a" + valueName := "k8s-cred-spec-1900254518529e2a3dedb85cdec03ce270559647459ab531f07af5eb1c5495fda709435ce82ab89c" cleanupInfoWithValue := &containerCreationCleanupInfo{gMSARegistryValueName: valueName} t.Run("it does remove the registry value", func(t *testing.T) { @@ -170,8 +177,9 @@ func TestRemoveGMSARegistryValue(t *testing.T) { assert.Nil(t, err) // the registry key should have been properly deleted - assert.Equal(t, 1, len(key.deletedValueNames)) - assert.Equal(t, []string{valueName}, key.deletedValueNames) + if assert.Equal(t, 1, len(key.deleteValueArgs)) { + assert.Equal(t, []string{valueName}, key.deleteValueArgs) + } assert.True(t, key.closed) }) t.Run("if there's an error opening the registry key", func(t *testing.T) { @@ -179,24 +187,91 @@ func TestRemoveGMSARegistryValue(t *testing.T) { err := removeGMSARegistryValue(cleanupInfoWithValue) - assert.NotNil(t, err) - assert.True(t, strings.Contains(err.Error(), "unable to open registry key")) + if assert.NotNil(t, err) { + assert.True(t, strings.Contains(err.Error(), "unable to open registry key")) + } }) - t.Run("if there's an error writing the registry key", func(t *testing.T) { + t.Run("if there's an error deleting from the registry key", func(t *testing.T) { key := &dummyRegistryKey{} - key.deleteValueError = fmt.Errorf("dummy error") + key.deleteValueFunc = func(name string) error { return fmt.Errorf("dummy error") } defer setRegistryCreateKeyFunc(t, key)() err := removeGMSARegistryValue(cleanupInfoWithValue) - assert.NotNil(t, err) - assert.True(t, strings.Contains(err.Error(), "unable to remove registry value")) + if assert.NotNil(t, err) { + assert.True(t, strings.Contains(err.Error(), "unable to remove registry value")) + } assert.True(t, key.closed) }) - t.Run("if there's no registry value to be removed", func(t *testing.T) { - err := removeGMSARegistryValue(emptyCleanupInfo) + t.Run("if there's no registry value to be removed, it does nothing", func(t *testing.T) { + key := &dummyRegistryKey{} + defer setRegistryCreateKeyFunc(t, key)() + + err := removeGMSARegistryValue(&containerCreationCleanupInfo{}) assert.Nil(t, err) + assert.Equal(t, 0, len(key.deleteValueArgs)) + }) +} + +func TestRemoveAllGMSARegistryValues(t *testing.T) { + cred1 := "k8s-cred-spec-1900254518529e2a3dedb85cdec03ce270559647459ab531f07af5eb1c5495fda709435ce82ab89c" + cred2 := "k8s-cred-spec-8891436007c795a904fdf77b5348e94305e4c48c5f01c47e7f65e980dc7edda85f112715891d65fd" + cred3 := "k8s-cred-spec-2f11f1c9e4f8182fe13caa708bd42b2098c8eefc489d6cc98806c058ccbe4cb3703b9ade61ce59a1" + cred4 := "k8s-cred-spec-dc532f189598a8220a1e538f79081eee979f94fbdbf8d37e36959485dee57157c03742d691e1fae2" + + t.Run("it removes the keys matching the k8s creds pattern", func(t *testing.T) { + key := &dummyRegistryKey{readValueNamesReturn: []string{cred1, "other_creds", cred2}} + defer setRegistryCreateKeyFunc(t, key)() + + errors := removeAllGMSARegistryValues() + + assert.Equal(t, 0, len(errors)) + assert.Equal(t, []string{cred1, cred2}, key.deleteValueArgs) + assert.Equal(t, []int{0}, key.readValueNamesArgs) + assert.True(t, key.closed) + }) + t.Run("it ignores errors and does a best effort at removing all k8s creds", func(t *testing.T) { + key := &dummyRegistryKey{ + readValueNamesReturn: []string{cred1, cred2, cred3, cred4}, + deleteValueFunc: func(name string) error { + if name == cred1 || name == cred3 { + return fmt.Errorf("dummy error") + } + return nil + }, + } + defer setRegistryCreateKeyFunc(t, key)() + + errors := removeAllGMSARegistryValues() + + assert.Equal(t, 2, len(errors)) + for _, err := range errors { + assert.True(t, strings.Contains(err.Error(), "unable to remove registry value")) + } + assert.Equal(t, []string{cred1, cred2, cred3, cred4}, key.deleteValueArgs) + assert.Equal(t, []int{0}, key.readValueNamesArgs) + assert.True(t, key.closed) + }) + t.Run("if there's an error opening the registry key", func(t *testing.T) { + defer setRegistryCreateKeyFunc(t, &dummyRegistryKey{}, fmt.Errorf("dummy error"))() + + errors := removeAllGMSARegistryValues() + + if assert.Equal(t, 1, len(errors)) { + assert.True(t, strings.Contains(errors[0].Error(), "unable to open registry key")) + } + }) + t.Run("if it's unable to list the registry values", func(t *testing.T) { + key := &dummyRegistryKey{readValueNamesError: fmt.Errorf("dummy error")} + defer setRegistryCreateKeyFunc(t, key)() + + errors := removeAllGMSARegistryValues() + + if assert.Equal(t, 1, len(errors)) { + assert.True(t, strings.Contains(errors[0].Error(), "unable to list values under registry key")) + } + assert.True(t, key.closed) }) } diff --git a/pkg/kubelet/dockershim/docker_service.go b/pkg/kubelet/dockershim/docker_service.go index 97f6543c4bb..7fa50e5f7a6 100644 --- a/pkg/kubelet/dockershim/docker_service.go +++ b/pkg/kubelet/dockershim/docker_service.go @@ -394,6 +394,8 @@ func (ds *dockerService) GetPodPortMappings(podSandboxID string) ([]*hostport.Po // Start initializes and starts components in dockerService. func (ds *dockerService) Start() error { + ds.initCleanup() + // Initialize the legacy cleanup flag. if ds.startLocalStreamingServer { go func() { @@ -405,6 +407,16 @@ func (ds *dockerService) Start() error { return ds.containerManager.Start() } +// initCleanup is responsible for cleaning up any crufts left by previous +// runs. If there are any errros, it simply logs them. +func (ds *dockerService) initCleanup() { + errors := ds.platformSpecificContainerCreationInitCleanup() + + for _, err := range errors { + klog.Warningf("initialization error: %v", err) + } +} + // Status returns the status of the runtime. func (ds *dockerService) Status(_ context.Context, r *runtimeapi.StatusRequest) (*runtimeapi.StatusResponse, error) { runtimeReady := &runtimeapi.RuntimeCondition{ diff --git a/pkg/kubelet/dockershim/naming.go b/pkg/kubelet/dockershim/naming.go index cb2bc467b1b..a2a97c2da4e 100644 --- a/pkg/kubelet/dockershim/naming.go +++ b/pkg/kubelet/dockershim/naming.go @@ -66,7 +66,6 @@ func makeSandboxName(s *runtimeapi.PodSandboxConfig) string { }, nameDelimiter) } -// makeContainerName generates a container name that's guaranteed to be unique on its host. func makeContainerName(s *runtimeapi.PodSandboxConfig, c *runtimeapi.ContainerConfig) string { return strings.Join([]string{ kubePrefix, // 0 diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container_windows_test.go b/pkg/kubelet/kuberuntime/kuberuntime_container_windows_test.go index e10b43eca9b..b438e44acdd 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_container_windows_test.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_container_windows_test.go @@ -38,7 +38,7 @@ func TestDetermineEffectiveSecurityContext(t *testing.T) { } } - t.Run("when there's a specific GMSA for that pod, and no pod-wide GMSA", func(t *testing.T) { + t.Run("when there's a specific GMSA for that container, and no pod-wide GMSA", func(t *testing.T) { containerConfig := &runtimeapi.ContainerConfig{} pod := buildPod(map[string]string{ @@ -49,7 +49,7 @@ func TestDetermineEffectiveSecurityContext(t *testing.T) { assert.Equal(t, dummyCredSpec, containerConfig.Annotations["container.alpha.windows.kubernetes.io/gmsa-credential-spec"]) }) - t.Run("when there's a specific GMSA for that pod, and a pod-wide GMSA", func(t *testing.T) { + t.Run("when there's a specific GMSA for that container, and a pod-wide GMSA", func(t *testing.T) { containerConfig := &runtimeapi.ContainerConfig{} pod := buildPod(map[string]string{ @@ -61,7 +61,7 @@ func TestDetermineEffectiveSecurityContext(t *testing.T) { assert.Equal(t, dummyCredSpec, containerConfig.Annotations["container.alpha.windows.kubernetes.io/gmsa-credential-spec"]) }) - t.Run("when there's no specific GMSA for that pod, and a pod-wide GMSA", func(t *testing.T) { + t.Run("when there's no specific GMSA for that container, and a pod-wide GMSA", func(t *testing.T) { containerConfig := &runtimeapi.ContainerConfig{} pod := buildPod(map[string]string{ @@ -72,7 +72,7 @@ func TestDetermineEffectiveSecurityContext(t *testing.T) { assert.Equal(t, dummyCredSpec, containerConfig.Annotations["container.alpha.windows.kubernetes.io/gmsa-credential-spec"]) }) - t.Run("when there's no specific GMSA for that pod, and no pod-wide GMSA", func(t *testing.T) { + t.Run("when there's no specific GMSA for that container, and no pod-wide GMSA", func(t *testing.T) { containerConfig := &runtimeapi.ContainerConfig{} determineEffectiveSecurityContext(containerConfig, container, &corev1.Pod{}) From b1ea622359c6c41d0d11e0ba8f43503da293f1c8 Mon Sep 17 00:00:00 2001 From: Jean Rouge Date: Sat, 16 Feb 2019 07:40:05 -0800 Subject: [PATCH 3/6] Review from @yujuhong Signed-off-by: Jean Rouge --- pkg/kubelet/dockershim/docker_container.go | 16 ++-- .../docker_container_unsupported.go | 14 ++-- .../dockershim/docker_container_windows.go | 76 +++++++++++-------- .../docker_container_windows_test.go | 30 ++++---- 4 files changed, 72 insertions(+), 64 deletions(-) diff --git a/pkg/kubelet/dockershim/docker_container.go b/pkg/kubelet/dockershim/docker_container.go index acabd69eba5..51156483f2c 100644 --- a/pkg/kubelet/dockershim/docker_container.go +++ b/pkg/kubelet/dockershim/docker_container.go @@ -114,8 +114,9 @@ func (ds *dockerService) CreateContainer(_ context.Context, r *runtimeapi.Create if iSpec := config.GetImage(); iSpec != nil { image = iSpec.Image } + containerName := makeContainerName(sandboxConfig, config) createConfig := dockertypes.ContainerCreateConfig{ - Name: makeContainerName(sandboxConfig, config), + Name: containerName, Config: &dockercontainer.Config{ // TODO: set User. Entrypoint: dockerstrslice.StrSlice(config.Command), @@ -166,20 +167,17 @@ func (ds *dockerService) CreateContainer(_ context.Context, r *runtimeapi.Create if err != nil { return nil, err } + defer func() { + for _, err := range ds.performPlatformSpecificContainerCreationCleanup(cleanupInfo) { + klog.Warningf("error when cleaning up after container %v's creation: %v", containerName, err) + } + }() createResp, createErr := ds.client.CreateContainer(createConfig) if createErr != nil { createResp, createErr = recoverFromCreationConflictIfNeeded(ds.client, createConfig, createErr) } - if err = ds.performPlatformSpecificContainerCreationCleanup(cleanupInfo); err != nil { - if createErr != nil { - // that one is more important to surface - return nil, createErr - } - return nil, err - } - if createResp != nil { return &runtimeapi.CreateContainerResponse{ContainerId: createResp.ID}, nil } diff --git a/pkg/kubelet/dockershim/docker_container_unsupported.go b/pkg/kubelet/dockershim/docker_container_unsupported.go index 11ec38b6335..10dea828f14 100644 --- a/pkg/kubelet/dockershim/docker_container_unsupported.go +++ b/pkg/kubelet/dockershim/docker_container_unsupported.go @@ -33,14 +33,16 @@ func (ds *dockerService) applyPlatformSpecificDockerConfig(*runtimeapi.CreateCon } // performPlatformSpecificContainerCreationCleanup is responsible for doing any platform-specific cleanup -// after a container creation. -func (ds *dockerService) performPlatformSpecificContainerCreationCleanup(*containerCreationCleanupInfo) error { - return nil +// after a container creation. Any errors it returns are simply logged, but do not fail the container +// creation. +func (ds *dockerService) performPlatformSpecificContainerCreationCleanup(cleanupInfo *containerCreationCleanupInfo) (errors []error) { + return } -// platformSpecificContainerCreationKubeletInitCleanup is called when the kubelet -// is starting, and is meant to clean up any cruft left by previous runs; -// errors are simply logged, but don't prevent the kubelet from starting. +// platformSpecificContainerCreationInitCleanup is called when dockershim +// is starting, and is meant to clean up any cruft left by previous runs +// creating containers. +// Errors are simply logged, but don't prevent dockershim from starting. func (ds *dockerService) platformSpecificContainerCreationInitCleanup() (errors []error) { return } diff --git a/pkg/kubelet/dockershim/docker_container_windows.go b/pkg/kubelet/dockershim/docker_container_windows.go index fdbf6fcc6fb..5a3df0ac0dc 100644 --- a/pkg/kubelet/dockershim/docker_container_windows.go +++ b/pkg/kubelet/dockershim/docker_container_windows.go @@ -30,7 +30,6 @@ import ( dockercontainer "github.com/docker/docker/api/types/container" utilfeature "k8s.io/apiserver/pkg/util/feature" - "k8s.io/klog" kubefeatures "k8s.io/kubernetes/pkg/features" runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" "k8s.io/kubernetes/pkg/kubelet/kuberuntime" @@ -92,7 +91,9 @@ const ( gMSARegistryValueNameSuffixRandomBytes = 40 ) -// useful to allow mocking the registry in tests +// registryKey is an interface wrapper around `registry.Key`, +// listing only the methods we care about here. +// It's mainly useful to easily allow mocking the registry in tests. type registryKey interface { SetStringValue(name, value string) error DeleteValue(name string) error @@ -104,9 +105,14 @@ var registryCreateKeyFunc = func(baseKey registry.Key, path string, access uint3 return registry.CreateKey(baseKey, path, access) } -// and same for random +// randomReader is only meant to ever be overriden for testing purposes, +// same idea as for `registryKey` above var randomReader = rand.Reader +// gMSARegistryValueNamesRegex is the regex used to detect gMSA cred spec +// registry values in `removeAllGMSARegistryValues` below. +var gMSARegistryValueNamesRegex = regexp.MustCompile(fmt.Sprintf("^%s[0-9a-f]{%d}$", gMSARegistryValueNamePrefix, 2*gMSARegistryValueNameSuffixRandomBytes)) + // copyGMSACredSpecToRegistryKey copies the credential specs to a unique registry value, and returns its name. func copyGMSACredSpecToRegistryValue(credSpec string) (string, error) { valueName, err := gMSARegistryValueName() @@ -117,45 +123,53 @@ func copyGMSACredSpecToRegistryValue(credSpec string) (string, error) { // write to the registry key, _, err := registryCreateKeyFunc(registry.LOCAL_MACHINE, credentialSpecRegistryLocation, registry.SET_VALUE) if err != nil { - return "", fmt.Errorf("unable to open registry key %s: %v", credentialSpecRegistryLocation, err) + return "", fmt.Errorf("unable to open registry key %q: %v", credentialSpecRegistryLocation, err) } defer key.Close() if err = key.SetStringValue(valueName, credSpec); err != nil { - return "", fmt.Errorf("unable to write into registry value %s/%s: %v", credentialSpecRegistryLocation, valueName, err) + return "", fmt.Errorf("unable to write into registry value %q/%q: %v", credentialSpecRegistryLocation, valueName, err) } return valueName, nil } // gMSARegistryValueName computes the name of the registry value where to store the GMSA cred spec contents. -// The value's name is purely random. +// The value's name is a purely random suffix appended to `gMSARegistryValueNamePrefix`. func gMSARegistryValueName() (string, error) { - randBytes := make([]byte, gMSARegistryValueNameSuffixRandomBytes) + randomSuffix, err := randomString(gMSARegistryValueNameSuffixRandomBytes) - if n, err := randomReader.Read(randBytes); err != nil || n != gMSARegistryValueNameSuffixRandomBytes { - if err == nil { - err = fmt.Errorf("only got %v random bytes, expected %v", n, len(randBytes)) - } - return "", fmt.Errorf("unable to generate random registry value name: %v", err) + if err != nil { + return "", fmt.Errorf("error when generating gMSA registry value name: %v", err) } - return gMSARegistryValueNamePrefix + hex.EncodeToString(randBytes), nil + return gMSARegistryValueNamePrefix + randomSuffix, nil +} + +// randomString returns a random hex string. +func randomString(length int) (string, error) { + randBytes := make([]byte, length) + + if n, err := randomReader.Read(randBytes); err != nil || n != length { + if err == nil { + err = fmt.Errorf("only got %v random bytes, expected %v", n, length) + } + return "", fmt.Errorf("unable to generate random string: %v", err) + } + + return hex.EncodeToString(randBytes), nil } // performPlatformSpecificContainerCreationCleanup is responsible for doing any platform-specific cleanup -// after a container creation. -func (ds *dockerService) performPlatformSpecificContainerCreationCleanup(cleanupInfo *containerCreationCleanupInfo) error { +// after a container creation. Any errors it returns are simply logged, but do not fail the container +// creation. +func (ds *dockerService) performPlatformSpecificContainerCreationCleanup(cleanupInfo *containerCreationCleanupInfo) (errors []error) { if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.WindowsGMSA) { - // this is best effort, we don't bubble errors upstream as failing to remove the GMSA registry keys shouldn't - // prevent k8s from working correctly, and the leaked registry keys are not a major concern anyway: - // they don't contain any secret, and they're sufficiently random to prevent collisions with - // future ones if err := removeGMSARegistryValue(cleanupInfo); err != nil { - klog.Warningf("won't remove GMSA cred spec registry value: %v", err) + errors = append(errors, err) } } - return nil + return } func removeGMSARegistryValue(cleanupInfo *containerCreationCleanupInfo) error { @@ -165,19 +179,20 @@ func removeGMSARegistryValue(cleanupInfo *containerCreationCleanupInfo) error { key, _, err := registryCreateKeyFunc(registry.LOCAL_MACHINE, credentialSpecRegistryLocation, registry.SET_VALUE) if err != nil { - return fmt.Errorf("unable to open registry key %s: %v", credentialSpecRegistryLocation, err) + return fmt.Errorf("unable to open registry key %q: %v", credentialSpecRegistryLocation, err) } defer key.Close() if err = key.DeleteValue(cleanupInfo.gMSARegistryValueName); err != nil { - return fmt.Errorf("unable to remove registry value %s/%s: %v", credentialSpecRegistryLocation, cleanupInfo.gMSARegistryValueName, err) + return fmt.Errorf("unable to remove registry value %q/%q: %v", credentialSpecRegistryLocation, cleanupInfo.gMSARegistryValueName, err) } return nil } -// platformSpecificContainerCreationKubeletInitCleanup is called when the kubelet -// is starting, and is meant to clean up any cruft left by previous runs; -// errors are simply logged, but don't prevent the kubelet from starting. +// platformSpecificContainerCreationInitCleanup is called when dockershim +// is starting, and is meant to clean up any cruft left by previous runs +// creating containers. +// Errors are simply logged, but don't prevent dockershim from starting. func (ds *dockerService) platformSpecificContainerCreationInitCleanup() (errors []error) { if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.WindowsGMSA) { errors = removeAllGMSARegistryValues() @@ -185,25 +200,22 @@ func (ds *dockerService) platformSpecificContainerCreationInitCleanup() (errors return } -// This is the regex used to detect gMSA cred spec registry values. -var gMSARegistryValueNamesRegex = regexp.MustCompile(fmt.Sprintf("^%s[0-9a-f]{%d}$", gMSARegistryValueNamePrefix, 2*gMSARegistryValueNameSuffixRandomBytes)) - func removeAllGMSARegistryValues() (errors []error) { key, _, err := registryCreateKeyFunc(registry.LOCAL_MACHINE, credentialSpecRegistryLocation, registry.SET_VALUE) if err != nil { - return []error{fmt.Errorf("unable to open registry key %s: %v", credentialSpecRegistryLocation, err)} + return []error{fmt.Errorf("unable to open registry key %q: %v", credentialSpecRegistryLocation, err)} } defer key.Close() valueNames, err := key.ReadValueNames(0) if err != nil { - return []error{fmt.Errorf("unable to list values under registry key %s: %v", credentialSpecRegistryLocation, err)} + return []error{fmt.Errorf("unable to list values under registry key %q: %v", credentialSpecRegistryLocation, err)} } for _, valueName := range valueNames { if gMSARegistryValueNamesRegex.MatchString(valueName) { if err = key.DeleteValue(valueName); err != nil { - errors = append(errors, fmt.Errorf("unable to remove registry value %s/%s: %v", credentialSpecRegistryLocation, valueName, err)) + errors = append(errors, fmt.Errorf("unable to remove registry value %q/%q: %v", credentialSpecRegistryLocation, valueName, err)) } } } diff --git a/pkg/kubelet/dockershim/docker_container_windows_test.go b/pkg/kubelet/dockershim/docker_container_windows_test.go index bf321c1eb62..21441509df3 100644 --- a/pkg/kubelet/dockershim/docker_container_windows_test.go +++ b/pkg/kubelet/dockershim/docker_container_windows_test.go @@ -20,11 +20,11 @@ import ( "bytes" "fmt" "regexp" - "strings" "testing" dockertypes "github.com/docker/docker/api/types" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "golang.org/x/sys/windows/registry" runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" ) @@ -129,18 +129,16 @@ func TestApplyGMSAConfig(t *testing.T) { err := applyGMSAConfig(containerConfigWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCreationCleanupInfo{}) - if assert.NotNil(t, err) { - assert.True(t, strings.Contains(err.Error(), "unable to generate random registry value name")) - } + require.NotNil(t, err) + assert.Contains(t, err.Error(), "error when generating gMSA registry value name: unable to generate random string") }) t.Run("if there's an error opening the registry key", func(t *testing.T) { defer setRegistryCreateKeyFunc(t, &dummyRegistryKey{}, fmt.Errorf("dummy error"))() err := applyGMSAConfig(containerConfigWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCreationCleanupInfo{}) - if assert.NotNil(t, err) { - assert.True(t, strings.Contains(err.Error(), "unable to open registry key")) - } + require.NotNil(t, err) + assert.Contains(t, err.Error(), "unable to open registry key") }) t.Run("if there's an error writing to the registry key", func(t *testing.T) { key := &dummyRegistryKey{} @@ -150,7 +148,7 @@ func TestApplyGMSAConfig(t *testing.T) { err := applyGMSAConfig(containerConfigWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCreationCleanupInfo{}) if assert.NotNil(t, err) { - assert.True(t, strings.Contains(err.Error(), "unable to write into registry value")) + assert.Contains(t, err.Error(), "unable to write into registry value") } assert.True(t, key.closed) }) @@ -187,9 +185,8 @@ func TestRemoveGMSARegistryValue(t *testing.T) { err := removeGMSARegistryValue(cleanupInfoWithValue) - if assert.NotNil(t, err) { - assert.True(t, strings.Contains(err.Error(), "unable to open registry key")) - } + require.NotNil(t, err) + assert.Contains(t, err.Error(), "unable to open registry key") }) t.Run("if there's an error deleting from the registry key", func(t *testing.T) { key := &dummyRegistryKey{} @@ -199,7 +196,7 @@ func TestRemoveGMSARegistryValue(t *testing.T) { err := removeGMSARegistryValue(cleanupInfoWithValue) if assert.NotNil(t, err) { - assert.True(t, strings.Contains(err.Error(), "unable to remove registry value")) + assert.Contains(t, err.Error(), "unable to remove registry value") } assert.True(t, key.closed) }) @@ -247,7 +244,7 @@ func TestRemoveAllGMSARegistryValues(t *testing.T) { assert.Equal(t, 2, len(errors)) for _, err := range errors { - assert.True(t, strings.Contains(err.Error(), "unable to remove registry value")) + assert.Contains(t, err.Error(), "unable to remove registry value") } assert.Equal(t, []string{cred1, cred2, cred3, cred4}, key.deleteValueArgs) assert.Equal(t, []int{0}, key.readValueNamesArgs) @@ -258,9 +255,8 @@ func TestRemoveAllGMSARegistryValues(t *testing.T) { errors := removeAllGMSARegistryValues() - if assert.Equal(t, 1, len(errors)) { - assert.True(t, strings.Contains(errors[0].Error(), "unable to open registry key")) - } + require.Equal(t, 1, len(errors)) + assert.Contains(t, errors[0].Error(), "unable to open registry key") }) t.Run("if it's unable to list the registry values", func(t *testing.T) { key := &dummyRegistryKey{readValueNamesError: fmt.Errorf("dummy error")} @@ -269,7 +265,7 @@ func TestRemoveAllGMSARegistryValues(t *testing.T) { errors := removeAllGMSARegistryValues() if assert.Equal(t, 1, len(errors)) { - assert.True(t, strings.Contains(errors[0].Error(), "unable to list values under registry key")) + assert.Contains(t, errors[0].Error(), "unable to list values under registry key") } assert.True(t, key.closed) }) From a09031dbbdaebd954ed491b2665b51f6542d7378 Mon Sep 17 00:00:00 2001 From: Jean Rouge Date: Tue, 19 Feb 2019 13:10:23 -0800 Subject: [PATCH 4/6] Typo in comment Signed-off-by: Jean Rouge --- pkg/kubelet/dockershim/docker_container_windows.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kubelet/dockershim/docker_container_windows.go b/pkg/kubelet/dockershim/docker_container_windows.go index 5a3df0ac0dc..aba74a65071 100644 --- a/pkg/kubelet/dockershim/docker_container_windows.go +++ b/pkg/kubelet/dockershim/docker_container_windows.go @@ -105,7 +105,7 @@ var registryCreateKeyFunc = func(baseKey registry.Key, path string, access uint3 return registry.CreateKey(baseKey, path, access) } -// randomReader is only meant to ever be overriden for testing purposes, +// randomReader is only meant to ever be overridden for testing purposes, // same idea as for `registryKey` above var randomReader = rand.Reader From f1bdfa93f90259f83496367f046c6d3164844e1f Mon Sep 17 00:00:00 2001 From: Jean Rouge Date: Mon, 25 Feb 2019 10:59:23 -0800 Subject: [PATCH 5/6] Review comments Signed-off-by: Jean Rouge --- .../dockershim/docker_container_windows.go | 22 ++++++------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/pkg/kubelet/dockershim/docker_container_windows.go b/pkg/kubelet/dockershim/docker_container_windows.go index aba74a65071..768f0c1d499 100644 --- a/pkg/kubelet/dockershim/docker_container_windows.go +++ b/pkg/kubelet/dockershim/docker_container_windows.go @@ -29,8 +29,6 @@ import ( dockertypes "github.com/docker/docker/api/types" dockercontainer "github.com/docker/docker/api/types/container" - utilfeature "k8s.io/apiserver/pkg/util/feature" - kubefeatures "k8s.io/kubernetes/pkg/features" runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" "k8s.io/kubernetes/pkg/kubelet/kuberuntime" ) @@ -45,10 +43,8 @@ type containerCreationCleanupInfo struct { func (ds *dockerService) applyPlatformSpecificDockerConfig(request *runtimeapi.CreateContainerRequest, createConfig *dockertypes.ContainerCreateConfig) (*containerCreationCleanupInfo, error) { cleanupInfo := &containerCreationCleanupInfo{} - if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.WindowsGMSA) { - if err := applyGMSAConfig(request.GetConfig(), createConfig, cleanupInfo); err != nil { - return nil, err - } + if err := applyGMSAConfig(request.GetConfig(), createConfig, cleanupInfo); err != nil { + return nil, err } return cleanupInfo, nil @@ -60,7 +56,8 @@ func (ds *dockerService) applyPlatformSpecificDockerConfig(request *runtimeapi.C // whose location could potentially change down the line, or even be unknown (eg if docker is not installed on the // C: drive) // When docker supports passing a credential spec's contents directly, we should switch to using that -// as it will avoid cluttering the registry. +// as it will avoid cluttering the registry - there is a moby PR out for this: +// https://github.com/moby/moby/pull/38777 func applyGMSAConfig(config *runtimeapi.ContainerConfig, createConfig *dockertypes.ContainerCreateConfig, cleanupInfo *containerCreationCleanupInfo) error { credSpec := config.Annotations[kuberuntime.GMSASpecContainerAnnotationKey] if credSpec == "" { @@ -163,10 +160,8 @@ func randomString(length int) (string, error) { // after a container creation. Any errors it returns are simply logged, but do not fail the container // creation. func (ds *dockerService) performPlatformSpecificContainerCreationCleanup(cleanupInfo *containerCreationCleanupInfo) (errors []error) { - if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.WindowsGMSA) { - if err := removeGMSARegistryValue(cleanupInfo); err != nil { - errors = append(errors, err) - } + if err := removeGMSARegistryValue(cleanupInfo); err != nil { + errors = append(errors, err) } return @@ -194,10 +189,7 @@ func removeGMSARegistryValue(cleanupInfo *containerCreationCleanupInfo) error { // creating containers. // Errors are simply logged, but don't prevent dockershim from starting. func (ds *dockerService) platformSpecificContainerCreationInitCleanup() (errors []error) { - if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.WindowsGMSA) { - errors = removeAllGMSARegistryValues() - } - return + return removeAllGMSARegistryValues() } func removeAllGMSARegistryValues() (errors []error) { From 0d392ffcef42ccf4f975f97e6a5e65a9ea99fa11 Mon Sep 17 00:00:00 2001 From: Jean Rouge Date: Tue, 26 Feb 2019 03:02:09 +0000 Subject: [PATCH 6/6] Udpated Bazel files Signed-off-by: Jean Rouge --- pkg/kubelet/dockershim/BUILD | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/kubelet/dockershim/BUILD b/pkg/kubelet/dockershim/BUILD index 4cc4e520794..c3f65639942 100644 --- a/pkg/kubelet/dockershim/BUILD +++ b/pkg/kubelet/dockershim/BUILD @@ -73,10 +73,8 @@ go_library( "//vendor/k8s.io/utils/exec:go_default_library", ] + select({ "@io_bazel_rules_go//go/platform:windows": [ - "//pkg/features:go_default_library", "//pkg/kubelet/apis:go_default_library", "//pkg/kubelet/winstats:go_default_library", - "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//vendor/golang.org/x/sys/windows/registry:go_default_library", ], "//conditions:default": [],