Deep equality helper should not mutate state

pkg/controller/deployment/rollback.go

@@ -73,7 +73,11 @@ func (dc *DeploymentController) rollback(d *extensions.Deployment, rsList []*ext
 // cleans up the rollback spec so subsequent requeues of the deployment won't end up in here.
 func (dc *DeploymentController) rollbackToTemplate(d *extensions.Deployment, rs *extensions.ReplicaSet) (bool, error) {
 	performedRollback := false
-	if !deploymentutil.EqualIgnoreHash(d.Spec.Template, rs.Spec.Template) {
+	isEqual, err := deploymentutil.EqualIgnoreHash(&d.Spec.Template, &rs.Spec.Template)
+	if err != nil {
+		return false, err
+	}
+	if !isEqual {
 		glog.V(4).Infof("Rolling back deployment %q to template spec %+v", d.Name, rs.Spec.Template.Spec)
 		deploymentutil.SetFromReplicaSetTemplate(d, rs.Spec.Template)
 		// set RS (the old RS we'll rolling back to) annotations back to the deployment;

pkg/controller/deployment/util/deployment_util.go

@@ -640,29 +640,42 @@ func ListPods(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet
 // We ignore pod-template-hash because the hash result would be different upon podTemplateSpec API changes
 // (e.g. the addition of a new field will cause the hash code to change)
 // Note that we assume input podTemplateSpecs contain non-empty labels
-func EqualIgnoreHash(template1, template2 v1.PodTemplateSpec) bool {
+func EqualIgnoreHash(template1, template2 *v1.PodTemplateSpec) (bool, error) {
+	cp, err := api.Scheme.DeepCopy(template1)
+	if err != nil {
+		return false, err
+	}
+	t1Copy := cp.(*v1.PodTemplateSpec)
+	cp, err = api.Scheme.DeepCopy(template2)
+	if err != nil {
+		return false, err
+	}
+	t2Copy := cp.(*v1.PodTemplateSpec)
 	// First, compare template.Labels (ignoring hash)
-	labels1, labels2 := template1.Labels, template2.Labels
+	labels1, labels2 := t1Copy.Labels, t2Copy.Labels
 	if len(labels1) > len(labels2) {
 		labels1, labels2 = labels2, labels1
 	}
 	// We make sure len(labels2) >= len(labels1)
 	for k, v := range labels2 {
 		if labels1[k] != v && k != extensions.DefaultDeploymentUniqueLabelKey {
-			return false
+			return false, nil
 		}
 	}
 	// Then, compare the templates without comparing their labels
-	template1.Labels, template2.Labels = nil, nil
-	return apiequality.Semantic.DeepEqual(template1, template2)
+	t1Copy.Labels, t2Copy.Labels = nil, nil
+	return apiequality.Semantic.DeepEqual(t1Copy, t2Copy), nil
 }
 
 // FindNewReplicaSet returns the new RS this given deployment targets (the one with the same pod template).
 func FindNewReplicaSet(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet) (*extensions.ReplicaSet, error) {
-	newRSTemplate := GetNewReplicaSetTemplate(deployment)
 	sort.Sort(controller.ReplicaSetsByCreationTimestamp(rsList))
 	for i := range rsList {
-		if EqualIgnoreHash(rsList[i].Spec.Template, newRSTemplate) {
+		equal, err := EqualIgnoreHash(&rsList[i].Spec.Template, &deployment.Spec.Template)
+		if err != nil {
+			return nil, err
+		}
+		if equal {
 			// In rare cases, such as after cluster upgrades, Deployment may end up with
 			// having more than one new ReplicaSets that have the same template as its template,
 			// see https://github.com/kubernetes/kubernetes/issues/40415

pkg/controller/deployment/util/deployment_util_test.go

@@ -31,7 +31,6 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	core "k8s.io/client-go/testing"
-	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/v1"
 	extensions "k8s.io/kubernetes/pkg/apis/extensions/v1beta1"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/clientset/fake"
@@ -444,29 +443,22 @@ func TestEqualIgnoreHash(t *testing.T) {
 
 	for _, test := range tests {
 		runTest := func(t1, t2 *v1.PodTemplateSpec, reversed bool) {
-			// Set up
-			t1Copy, err := api.Scheme.DeepCopy(t1)
-			if err != nil {
-				t.Errorf("Failed setting up the test: %v", err)
-			}
-			t2Copy, err := api.Scheme.DeepCopy(t2)
-			if err != nil {
-				t.Errorf("Failed setting up the test: %v", err)
-			}
 			reverseString := ""
 			if reversed {
 				reverseString = " (reverse order)"
 			}
 			// Run
-			equal := EqualIgnoreHash(*t1, *t2)
+			equal, err := EqualIgnoreHash(t1, t2)
+			if err != nil {
+				t.Errorf("%s: unexpected error: %v", err, test.test)
+				return
+			}
 			if equal != test.expected {
-				t.Errorf("In test case %q%s, expected %v", test.test, reverseString, test.expected)
+				t.Errorf("%q%s: expected %v", test.test, reverseString, test.expected)
+				return
 			}
 			if t1.Labels == nil || t2.Labels == nil {
-				t.Errorf("In test case %q%s, unexpected labels becomes nil", test.test, reverseString)
-			}
-			if !reflect.DeepEqual(t1, t1Copy) || !reflect.DeepEqual(t2, t2Copy) {
-				t.Errorf("In test case %q%s, unexpected input template modified", test.test, reverseString)
+				t.Errorf("%q%s: unexpected labels becomes nil", test.test, reverseString)
 			}
 		}
 		runTest(&test.former, &test.latter, false)