diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 83db1dda63..b2e4bd14d3 100644 --- a/pkg/features/kube_features.go +++ b/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: diff --git a/pkg/kubelet/dockershim/docker_container.go b/pkg/kubelet/dockershim/docker_container.go index 51156483f2..054600a267 100644 --- a/pkg/kubelet/dockershim/docker_container.go +++ b/pkg/kubelet/dockershim/docker_container.go @@ -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) + } +} diff --git a/pkg/kubelet/dockershim/docker_container_unsupported.go b/pkg/kubelet/dockershim/docker_container_unsupported.go index 10dea828f1..1e0ce8c71f 100644 --- a/pkg/kubelet/dockershim/docker_container_unsupported.go +++ b/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 } diff --git a/pkg/kubelet/dockershim/docker_container_windows.go b/pkg/kubelet/dockershim/docker_container_windows.go index 768f0c1d49..993c3ab9de 100644 --- a/pkg/kubelet/dockershim/docker_container_windows.go +++ b/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,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() } diff --git a/pkg/kubelet/dockershim/docker_container_windows_test.go b/pkg/kubelet/dockershim/docker_container_windows_test.go index 21441509df..00208c3253 100644 --- a/pkg/kubelet/dockershim/docker_container_windows_test.go +++ b/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,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)) diff --git a/pkg/kubelet/dockershim/docker_service.go b/pkg/kubelet/dockershim/docker_service.go index bf487ef216..1a9534ba22 100644 --- a/pkg/kubelet/dockershim/docker_service.go +++ b/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)