github
/
k3s
mirror of https://github.com/k3s-io/k3s
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Fixing a small bug with GMSA support 

A previous PR (https://github.com/kubernetes/kubernetes/pull/73726)
added GMSA support to the dockershim. Unfortunately, there was a
bug in there: the registry keys used to pass the cred specs down
to Docker were being cleaned up too early, right after the containers'
creation - before Docker would ever try to read them, when trying to
actually start the container.

This patch fixes this.

An e2e test is also provided in a separate PR.

Signed-off-by: Jean Rouge <rougej+github@gmail.com>
pull/564/head
Jean Rouge 2019-02-28 10:09:02 -08:00
parent da5fd93e93
commit 1908a0ce04
6 changed files with 92 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
pkg/features/kube_features.go
View File

@ -480,6 +480,7 @@ var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureS
ProcMountType: {Default: false, PreRelease: utilfeature.Alpha},
TTLAfterFinished: {Default: false, PreRelease: utilfeature.Alpha},
KubeletPodResources: {Default: false, PreRelease: utilfeature.Alpha},
WindowsGMSA: {Default: false, PreRelease: utilfeature.Alpha},
// inherited features from generic apiserver, relisted here to get a conflict if it is changed
// unintentionally on either side:

38
pkg/kubelet/dockershim/docker_container.go
View File

@ -167,11 +167,6 @@ 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 {
@ -179,8 +174,18 @@ func (ds *dockerService) CreateContainer(_ context.Context, r *runtimeapi.Create
}
if createResp != nil {
return &runtimeapi.CreateContainerResponse{ContainerId: createResp.ID}, nil
containerID := createResp.ID
if cleanupInfo != nil {
// save it for when the container starts or gets removed
ds.containerCleanupInfos[containerID] = cleanupInfo
}
return &runtimeapi.CreateContainerResponse{ContainerId: containerID}, nil
}
// the creation failed, let's clean up right away
ds.performPlatformSpecificContainerCleanupAndLogErrors(containerName, cleanupInfo)
return nil, createErr
}
@ -267,6 +272,8 @@ func (ds *dockerService) StartContainer(_ context.Context, r *runtimeapi.StartCo
return nil, fmt.Errorf("failed to start container %q: %v", r.ContainerId, err)
}
ds.performPlatformSpecificContainerForContainer(r.ContainerId)
return &runtimeapi.StartContainerResponse{}, nil
}
@ -281,6 +288,8 @@ func (ds *dockerService) StopContainer(_ context.Context, r *runtimeapi.StopCont
// RemoveContainer removes the container.
func (ds *dockerService) RemoveContainer(_ context.Context, r *runtimeapi.RemoveContainerRequest) (*runtimeapi.RemoveContainerResponse, error) {
ds.performPlatformSpecificContainerForContainer(r.ContainerId)
// Ideally, log lifecycle should be independent of container lifecycle.
// However, docker will remove container log after container is removed,
// we can't prevent that now, so we also clean up the symlink here.
@ -438,3 +447,20 @@ func (ds *dockerService) UpdateContainerResources(_ context.Context, r *runtimea
}
return &runtimeapi.UpdateContainerResourcesResponse{}, nil
}
func (ds *dockerService) performPlatformSpecificContainerForContainer(containerID string) {
if cleanupInfo, present := ds.containerCleanupInfos[containerID]; present {
ds.performPlatformSpecificContainerCleanupAndLogErrors(containerID, cleanupInfo)
delete(ds.containerCleanupInfos, containerID)
}
}
func (ds *dockerService) performPlatformSpecificContainerCleanupAndLogErrors(containerNameOrID string, cleanupInfo *containerCleanupInfo) {
if cleanupInfo == nil {
return
}
for _, err := range ds.performPlatformSpecificContainerCleanup(cleanupInfo) {
klog.Warningf("error when cleaning up after container %q: %v", containerNameOrID, err)
}
}

29
pkg/kubelet/dockershim/docker_container_unsupported.go
View File

@ -23,26 +23,35 @@ import (
runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
)
type containerCreationCleanupInfo struct{}
type containerCleanupInfo 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) {
// The containerCleanupInfo struct it returns will be passed as is to performPlatformSpecificContainerCleanup
// after either:
// * the container creation has failed
// * the container has been successfully started
// * the container has been removed
// whichever happens first.
func (ds *dockerService) applyPlatformSpecificDockerConfig(*runtimeapi.CreateContainerRequest, *dockertypes.ContainerCreateConfig) (*containerCleanupInfo, error) {
return nil, nil
}
// performPlatformSpecificContainerCreationCleanup is responsible for doing any platform-specific cleanup
// 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) {
// performPlatformSpecificContainerCleanup is responsible for doing any platform-specific cleanup
// after either:
// * the container creation has failed
// * the container has been successfully started
// * the container has been removed
// whichever happens first.
// Any errors it returns are simply logged, but do not prevent the container from being started or
// removed.
func (ds *dockerService) performPlatformSpecificContainerCleanup(cleanupInfo *containerCleanupInfo) (errors []error) {
return
}
// platformSpecificContainerCreationInitCleanup is called when dockershim
// platformSpecificContainerInitCleanup 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) {
func (ds *dockerService) platformSpecificContainerInitCleanup() (errors []error) {
return
}

35
pkg/kubelet/dockershim/docker_container_windows.go
View File

@ -33,15 +33,19 @@ import (
"k8s.io/kubernetes/pkg/kubelet/kuberuntime"
)
type containerCreationCleanupInfo struct {
type containerCleanupInfo 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{}
// The containerCleanupInfo struct it returns will be passed as is to performPlatformSpecificContainerCleanup
// after either:
// * the container creation has failed
// * the container has been successfully started
// * the container has been removed
// whichever happens first.
func (ds *dockerService) applyPlatformSpecificDockerConfig(request *runtimeapi.CreateContainerRequest, createConfig *dockertypes.ContainerCreateConfig) (*containerCleanupInfo, error) {
cleanupInfo := &containerCleanupInfo{}
if err := applyGMSAConfig(request.GetConfig(), createConfig, cleanupInfo); err != nil {
return nil, err
@ -58,7 +62,7 @@ func (ds *dockerService) applyPlatformSpecificDockerConfig(request *runtimeapi.C
// When docker supports passing a credential spec's contents directly, we should switch to using that
// 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 {
func applyGMSAConfig(config *runtimeapi.ContainerConfig, createConfig *dockertypes.ContainerCreateConfig, cleanupInfo *containerCleanupInfo) error {
credSpec := config.Annotations[kuberuntime.GMSASpecContainerAnnotationKey]
if credSpec == "" {
return nil
@ -156,10 +160,15 @@ func randomString(length int) (string, error) {
return hex.EncodeToString(randBytes), nil
}
// performPlatformSpecificContainerCreationCleanup is responsible for doing any platform-specific cleanup
// 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) {
// performPlatformSpecificContainerCleanup is responsible for doing any platform-specific cleanup
// after either:
// * the container creation has failed
// * the container has been successfully started
// * the container has been removed
// whichever happens first.
// Any errors it returns are simply logged, but do not prevent the container from being started or
// removed.
func (ds *dockerService) performPlatformSpecificContainerCleanup(cleanupInfo *containerCleanupInfo) (errors []error) {
if err := removeGMSARegistryValue(cleanupInfo); err != nil {
errors = append(errors, err)
}
@ -167,7 +176,7 @@ func (ds *dockerService) performPlatformSpecificContainerCreationCleanup(cleanup
return
}
func removeGMSARegistryValue(cleanupInfo *containerCreationCleanupInfo) error {
func removeGMSARegistryValue(cleanupInfo *containerCleanupInfo) error {
if cleanupInfo == nil || cleanupInfo.gMSARegistryValueName == "" {
return nil
}
@ -184,11 +193,11 @@ func removeGMSARegistryValue(cleanupInfo *containerCreationCleanupInfo) error {
return nil
}
// platformSpecificContainerCreationInitCleanup is called when dockershim
// platformSpecificContainerInitCleanup 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) {
func (ds *dockerService) platformSpecificContainerInitCleanup() (errors []error) {
return removeAllGMSARegistryValues()
}

16
pkg/kubelet/dockershim/docker_container_windows_test.go
View File

@ -82,7 +82,7 @@ func TestApplyGMSAConfig(t *testing.T) {
defer setRandomReader(randomBytes)()
createConfig := &dockertypes.ContainerCreateConfig{}
cleanupInfo := &containerCreationCleanupInfo{}
cleanupInfo := &containerCleanupInfo{}
err := applyGMSAConfig(containerConfigWithGMSAAnnotation, createConfig, cleanupInfo)
assert.Nil(t, err)
@ -105,7 +105,7 @@ func TestApplyGMSAConfig(t *testing.T) {
defer setRegistryCreateKeyFunc(t, &dummyRegistryKey{})()
createConfig := &dockertypes.ContainerCreateConfig{}
cleanupInfo := &containerCreationCleanupInfo{}
cleanupInfo := &containerCleanupInfo{}
err := applyGMSAConfig(containerConfigWithGMSAAnnotation, createConfig, cleanupInfo)
assert.Nil(t, err)
@ -127,7 +127,7 @@ func TestApplyGMSAConfig(t *testing.T) {
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{})
err := applyGMSAConfig(containerConfigWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCleanupInfo{})
require.NotNil(t, err)
assert.Contains(t, err.Error(), "error when generating gMSA registry value name: unable to generate random string")
@ -135,7 +135,7 @@ func TestApplyGMSAConfig(t *testing.T) {
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{})
err := applyGMSAConfig(containerConfigWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCleanupInfo{})
require.NotNil(t, err)
assert.Contains(t, err.Error(), "unable to open registry key")
@ -145,7 +145,7 @@ func TestApplyGMSAConfig(t *testing.T) {
key.setStringValueError = fmt.Errorf("dummy error")
defer setRegistryCreateKeyFunc(t, key)()
err := applyGMSAConfig(containerConfigWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCreationCleanupInfo{})
err := applyGMSAConfig(containerConfigWithGMSAAnnotation, &dockertypes.ContainerCreateConfig{}, &containerCleanupInfo{})
if assert.NotNil(t, err) {
assert.Contains(t, err.Error(), "unable to write into registry value")
@ -155,7 +155,7 @@ func TestApplyGMSAConfig(t *testing.T) {
t.Run("if there is no GMSA annotation", func(t *testing.T) {
createConfig := &dockertypes.ContainerCreateConfig{}
err := applyGMSAConfig(&runtimeapi.ContainerConfig{}, createConfig, &containerCreationCleanupInfo{})
err := applyGMSAConfig(&runtimeapi.ContainerConfig{}, createConfig, &containerCleanupInfo{})
assert.Nil(t, err)
assert.Nil(t, createConfig.HostConfig)
@ -164,7 +164,7 @@ func TestApplyGMSAConfig(t *testing.T) {
func TestRemoveGMSARegistryValue(t *testing.T) {
valueName := "k8s-cred-spec-1900254518529e2a3dedb85cdec03ce270559647459ab531f07af5eb1c5495fda709435ce82ab89c"
cleanupInfoWithValue := &containerCreationCleanupInfo{gMSARegistryValueName: valueName}
cleanupInfoWithValue := &containerCleanupInfo{gMSARegistryValueName: valueName}
t.Run("it does remove the registry value", func(t *testing.T) {
key := &dummyRegistryKey{}
@ -204,7 +204,7 @@ func TestRemoveGMSARegistryValue(t *testing.T) {
key := &dummyRegistryKey{}
defer setRegistryCreateKeyFunc(t, key)()
err := removeGMSARegistryValue(&containerCreationCleanupInfo{})
err := removeGMSARegistryValue(&containerCleanupInfo{})
assert.Nil(t, err)
assert.Equal(t, 0, len(key.deleteValueArgs))

12
pkg/kubelet/dockershim/docker_service.go
View File

@ -155,7 +155,7 @@ type dockerNetworkHost struct {
*portMappingGetter
}
var internalLabelKeys []string = []string{containerTypeLabelKey, containerLogPathLabelKey, sandboxIDLabelKey}
var internalLabelKeys = []string{containerTypeLabelKey, containerLogPathLabelKey, sandboxIDLabelKey}
// ClientConfig is parameters used to initialize docker client
type ClientConfig struct {
@ -186,6 +186,7 @@ func NewDockerClientFromConfig(config *ClientConfig) libdocker.Interface {
return nil
}
// NewDockerService creates a new `DockerService` struct.
// NOTE: Anything passed to DockerService should be eventually handled in another way when we switch to running the shim as a different process.
func NewDockerService(config *ClientConfig, podSandboxImage string, streamingConfig *streaming.Config, pluginSettings *NetworkPluginSettings,
cgroupsName string, kubeCgroupDriver string, dockershimRootDir string, startLocalStreamingServer bool) (DockerService, error) {
@ -211,6 +212,7 @@ func NewDockerService(config *ClientConfig, podSandboxImage string, streamingCon
checkpointManager: checkpointManager,
startLocalStreamingServer: startLocalStreamingServer,
networkReady: make(map[string]bool),
containerCleanupInfos: make(map[string]*containerCleanupInfo),
}
// check docker version compatibility.
@ -305,6 +307,12 @@ type dockerService struct {
// startLocalStreamingServer indicates whether dockershim should start a
// streaming server on localhost.
startLocalStreamingServer bool
// containerCleanupInfos maps container IDs to the `containerCleanupInfo` structs
// needed to clean up after containers have been started or removed.
// (see `applyPlatformSpecificDockerConfig` and `performPlatformSpecificContainerCleanup`
// methods for more info).
containerCleanupInfos map[string]*containerCleanupInfo
}
// TODO: handle context.
@ -411,7 +419,7 @@ func (ds *dockerService) Start() error {
// 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()
errors := ds.platformSpecificContainerInitCleanup()
for _, err := range errors {
klog.Warningf("initialization error: %v", err)