Fixing a small bug with GMSA support

A previous PR (kubernetes#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 <[email protected]>

Author: wk8

pkg/features/kube_features.go

@@ -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:

pkg/kubelet/dockershim/docker_container.go

@@ -167,20 +167,25 @@ 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 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)
+	}
+}

pkg/kubelet/dockershim/docker_container_unsupported.go

@@ -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
 }

pkg/kubelet/dockershim/docker_container_windows.go

@@ -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,18 +160,23 @@ 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)
 	}
 
 	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()
 }
 

pkg/kubelet/dockershim/docker_container_windows_test.go

@@ -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,15 +127,15 @@ 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")
 	})
 	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))

pkg/kubelet/dockershim/docker_service.go

@@ -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)