Omit nil or empty field when calculating hash value

Author: dixudx

pkg/kubelet/container/BUILD

@@ -46,6 +46,7 @@ go_test(
     name = "go_default_test",
     srcs = [
         "cache_test.go",
+        "container_hash_test.go",
         "helpers_test.go",
         "ref_test.go",
         "runtime_cache_test.go",

pkg/kubelet/container/container_hash_test.go

@@ -0,0 +1,89 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package container
+
+import (
+	"encoding/json"
+	"testing"
+
+	"k8s.io/api/core/v1"
+)
+
+var (
+	sampleContainer = `
+{
+  "name": "test_container",
+  "image": "foo/image:v1",
+  "command": [
+    "/bin/testcmd"
+  ],
+  "args": [
+    "/bin/sh",
+    "-c",
+    "echo abc"
+  ],
+  "ports": [
+    {
+      "containerPort": 8001
+    }
+  ],
+  "env": [
+    {
+      "name": "ENV_FOO",
+      "value": "bar"
+    },
+    {
+      "name": "ENV_BAR",
+      "valueFrom": {
+        "secretKeyRef": {
+          "name": "foo",
+          "key": "bar",
+          "optional": true
+        }
+      }
+    }
+  ],
+  "resources": {
+    "limits": {
+      "foo": "1G"
+    },
+    "requests": {
+      "foo": "500M"
+    }
+  }
+}
+`
+
+	sampleV115HashValue = uint64(0x311670a)
+	sampleV116HashValue = sampleV115HashValue
+)
+
+func TestConsistentHashContainer(t *testing.T) {
+	container := &v1.Container{}
+	if err := json.Unmarshal([]byte(sampleContainer), container); err != nil {
+		t.Error(err)
+	}
+
+	currentHash := HashContainer(container)
+	if currentHash != sampleV116HashValue {
+		t.Errorf("mismatched hash value with v1.16")
+	}
+
+	if currentHash != sampleV115HashValue {
+		t.Errorf("mismatched hash value with v1.15")
+	}
+}

pkg/kubelet/container/helpers.go

@@ -17,6 +17,7 @@ limitations under the License.
 package container
 
 import (
+	"encoding/json"
 	"fmt"
 	"hash/fnv"
 	"strings"
@@ -92,9 +93,13 @@ func ShouldContainerBeRestarted(container *v1.Container, pod *v1.Pod, podStatus
 
 // HashContainer returns the hash of the container. It is used to compare
 // the running container with its desired spec.
+// Note: remember to update hashValues in container_hash_test.go as well.
 func HashContainer(container *v1.Container) uint64 {
 	hash := fnv.New32a()
-	hashutil.DeepHashObject(hash, *container)
+	// Omit nil or empty field when calculating hash value
+	// Please see https://github.com/kubernetes/kubernetes/issues/53644
+	containerJson, _ := json.Marshal(container)
+	hashutil.DeepHashObject(hash, containerJson)
 	return uint64(hash.Sum32())
 }
 

pkg/kubelet/container/helpers_test.go

@@ -574,3 +574,37 @@ func TestMakePortMappings(t *testing.T) {
 		assert.Equal(t, tt.expectedPortMappings, actual, "[%d]", i)
 	}
 }
+
+func TestHashContainer(t *testing.T) {
+	testCases := []struct {
+		name          string
+		image         string
+		args          []string
+		containerPort int32
+		expectedHash  uint64
+	}{
+		{
+			name:  "test_container",
+			image: "foo/image:v1",
+			args: []string{
+				"/bin/sh",
+				"-c",
+				"echo abc",
+			},
+			containerPort: int32(8001),
+			expectedHash:  uint64(0x3c42280f),
+		},
+	}
+
+	for _, tc := range testCases {
+		container := v1.Container{
+			Name:  tc.name,
+			Image: tc.image,
+			Args:  tc.args,
+			Ports: []v1.ContainerPort{{ContainerPort: tc.containerPort}},
+		}
+
+		hashVal := HashContainer(&container)
+		assert.Equal(t, tc.expectedHash, hashVal, "the hash value here should not be changed.")
+	}
+}