Review from @yujuhong

Signed-off-by: Jean Rouge <rougej+github@gmail.com>

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
 	}

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
 }

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

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