Merge pull request #52052 from joelsmith/master

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Clean up kublet secret and configmap unit test

**What this PR does / why we need it**:

These changes are clean-up items to fix confusing code encountered while investigating #52043. No actual bugs are fixed here (except, maybe, correcting unit tests that had actual/expected swapped).

A summary of the changes, as listed in the commit:

* Expected value comes before actual value in assert.Equal()
* Use `assert.Equal()` instead of `assert.True()` when possible
* Add a unit test that verifies no-op pod updates to the `secret_manager` and the `configmap_manager`
* Add a clarifying comment about why it's good to seemingly delete a secret on updates.
* Fix (for now, non-buggy) variable shadowing issue

**Special notes for your reviewer**:

N/A

**Release note**:
```release-note
NONE
```

pkg/kubelet/configmap/configmap_manager.go

@@ -282,6 +282,11 @@ func (c *cachingConfigMapManager) RegisterPod(pod *v1.Pod) {
 	c.registeredPods[key] = pod
 	if prev != nil {
 		for name := range getConfigMapNames(prev) {
+			// On an update, the .Add() call above will have re-incremented the
+			// ref count of any existing items, so any configmaps that are in both
+			// names and prev need to have their ref counts decremented. Any that
+			// are only in prev need to be completely removed. This unconditional
+			// call takes care of both cases.
 			c.configMapStore.Delete(prev.Namespace, name)
 		}
 	}

pkg/kubelet/configmap/configmap_manager_test.go

@@ -299,11 +299,11 @@ type configMapsToAttach struct {
 	volumes                []string
 }
 
-func podWithConfigMaps(ns, name string, toAttach configMapsToAttach) *v1.Pod {
+func podWithConfigMaps(ns, podName string, toAttach configMapsToAttach) *v1.Pod {
 	pod := &v1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: ns,
-			Name:      name,
+			Name:      podName,
 		},
 		Spec: v1.PodSpec{},
 	}
@@ -454,6 +454,17 @@ func TestCacheRefcounts(t *testing.T) {
 	manager.RegisterPod(podWithConfigMaps("ns1", "other-name", s2))
 	manager.UnregisterPod(podWithConfigMaps("ns1", "other-name", s2))
 
+	s5 := configMapsToAttach{
+		containerEnvConfigMaps: []envConfigMaps{
+			{envVarNames: []string{"s7"}},
+			{envFromNames: []string{"s70"}},
+		},
+	}
+
+	// Check the no-op update scenario
+	manager.RegisterPod(podWithConfigMaps("ns1", "noop-pod", s5))
+	manager.RegisterPod(podWithConfigMaps("ns1", "noop-pod", s5))
+
 	refs := func(ns, name string) int {
 		store.lock.Lock()
 		defer store.lock.Unlock()
@@ -463,17 +474,18 @@ func TestCacheRefcounts(t *testing.T) {
 		}
 		return item.refCount
 	}
-	assert.Equal(t, refs("ns1", "s1"), 1)
+	assert.Equal(t, 1, refs("ns1", "s1"))
-	assert.Equal(t, refs("ns1", "s10"), 1)
+	assert.Equal(t, 1, refs("ns1", "s10"))
-	assert.Equal(t, refs("ns1", "s2"), 1)
+	assert.Equal(t, 1, refs("ns1", "s2"))
-	assert.Equal(t, refs("ns1", "s3"), 3)
+	assert.Equal(t, 3, refs("ns1", "s3"))
-	assert.Equal(t, refs("ns1", "s30"), 2)
+	assert.Equal(t, 2, refs("ns1", "s30"))
-	assert.Equal(t, refs("ns1", "s4"), 2)
+	assert.Equal(t, 2, refs("ns1", "s4"))
-	assert.Equal(t, refs("ns1", "s5"), 4)
+	assert.Equal(t, 4, refs("ns1", "s5"))
-	assert.Equal(t, refs("ns1", "s50"), 2)
+	assert.Equal(t, 2, refs("ns1", "s50"))
-	assert.Equal(t, refs("ns1", "s6"), 0)
+	assert.Equal(t, 0, refs("ns1", "s6"))
-	assert.Equal(t, refs("ns1", "s60"), 0)
+	assert.Equal(t, 0, refs("ns1", "s60"))
-	assert.Equal(t, refs("ns1", "s7"), 0)
+	assert.Equal(t, 1, refs("ns1", "s7"))
+	assert.Equal(t, 1, refs("ns1", "s70"))
 }
 
 func TestCachingConfigMapManager(t *testing.T) {

pkg/kubelet/secret/secret_manager.go

@@ -282,6 +282,11 @@ func (c *cachingSecretManager) RegisterPod(pod *v1.Pod) {
 	c.registeredPods[key] = pod
 	if prev != nil {
 		for name := range getSecretNames(prev) {
+			// On an update, the .Add() call above will have re-incremented the
+			// ref count of any existing secrets, so any secrets that are in both
+			// names and prev need to have their ref counts decremented. Any that
+			// are only in prev need to be completely removed. This unconditional
+			// call takes care of both cases.
 			c.secretStore.Delete(prev.Namespace, name)
 		}
 	}

pkg/kubelet/secret/secret_manager_test.go

@@ -299,11 +299,11 @@ type secretsToAttach struct {
 	containerEnvSecrets  []envSecrets
 }
 
-func podWithSecrets(ns, name string, toAttach secretsToAttach) *v1.Pod {
+func podWithSecrets(ns, podName string, toAttach secretsToAttach) *v1.Pod {
 	pod := &v1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: ns,
-			Name:      name,
+			Name:      podName,
 		},
 		Spec: v1.PodSpec{},
 	}
@@ -453,27 +453,38 @@ func TestCacheRefcounts(t *testing.T) {
 	manager.RegisterPod(podWithSecrets("ns1", "other-name", s2))
 	manager.UnregisterPod(podWithSecrets("ns1", "other-name", s2))
 
+	s5 := secretsToAttach{
+		containerEnvSecrets: []envSecrets{
+			{envVarNames: []string{"s7"}},
+			{envFromNames: []string{"s70"}},
+		},
+	}
+	// Check the no-op update scenario
+	manager.RegisterPod(podWithSecrets("ns1", "noop-pod", s5))
+	manager.RegisterPod(podWithSecrets("ns1", "noop-pod", s5))
+
 	// Now we have: 3 pods with s1, 2 pods with s2 and 2 pods with s3, 0 pods with s4.
-	verify := func(ns, name string, count int) bool {
+	refs := func(ns, name string) int {
 		store.lock.Lock()
 		defer store.lock.Unlock()
 		item, ok := store.items[objectKey{ns, name}]
 		if !ok {
-			return count == 0
+			return 0
 		}
-		return item.refCount == count
+		return item.refCount
 	}
-	assert.True(t, verify("ns1", "s1", 3))
+	assert.Equal(t, 3, refs("ns1", "s1"))
-	assert.True(t, verify("ns1", "s10", 1))
+	assert.Equal(t, 1, refs("ns1", "s10"))
-	assert.True(t, verify("ns1", "s2", 3))
+	assert.Equal(t, 3, refs("ns1", "s2"))
-	assert.True(t, verify("ns1", "s3", 3))
+	assert.Equal(t, 3, refs("ns1", "s3"))
-	assert.True(t, verify("ns1", "s30", 2))
+	assert.Equal(t, 2, refs("ns1", "s30"))
-	assert.True(t, verify("ns1", "s4", 2))
+	assert.Equal(t, 2, refs("ns1", "s4"))
-	assert.True(t, verify("ns1", "s5", 4))
+	assert.Equal(t, 4, refs("ns1", "s5"))
-	assert.True(t, verify("ns1", "s50", 2))
+	assert.Equal(t, 2, refs("ns1", "s50"))
-	assert.True(t, verify("ns1", "s6", 0))
+	assert.Equal(t, 0, refs("ns1", "s6"))
-	assert.True(t, verify("ns1", "s60", 0))
+	assert.Equal(t, 0, refs("ns1", "s60"))
-	assert.True(t, verify("ns1", "s7", 0))
+	assert.Equal(t, 1, refs("ns1", "s7"))
+	assert.Equal(t, 1, refs("ns1", "s70"))
 }
 
 func TestCachingSecretManager(t *testing.T) {