Change step hashing function to derive hash from json representation …

…of steps (#108)
argoproj · Jun 14, 2019 · 6dfc53e · 6dfc53e
1 parent 2448571
commit 6dfc53e
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 5 deletions.
diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go
@@ -1,6 +1,7 @@
 package conditions
 
 import (
+	"encoding/json"
 	"fmt"
 	"hash/fnv"
 	"math"
@@ -236,12 +237,17 @@ func RolloutComplete(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatu
 // ComputeStepHash returns a hash value calculated from the Rollout's steps. The hash will
 // be safe encoded to avoid bad words.
 func ComputeStepHash(rollout *v1alpha1.Rollout) string {
-	rolloutStepHasher := fnv.New32a()
-	if rollout.Spec.Strategy.BlueGreenStrategy != nil {
+	if rollout.Spec.Strategy.BlueGreenStrategy != nil || rollout.Spec.Strategy.CanaryStrategy == nil {
 		return ""
 	}
-	if rollout.Spec.Strategy.CanaryStrategy != nil {
-		hashutil.DeepHashObject(rolloutStepHasher, rollout.Spec.Strategy.CanaryStrategy.Steps)
+	rolloutStepHasher := fnv.New32a()
+	stepsBytes, err := json.Marshal(rollout.Spec.Strategy.CanaryStrategy.Steps)
+	if err != nil {
+		panic(err)
+	}
+	_, err = rolloutStepHasher.Write(stepsBytes)
+	if err != nil {
+		panic(err)
 	}
 	return rand.SafeEncodeString(fmt.Sprint(rolloutStepHasher.Sum32()))
 }

diff --git a/utils/conditions/conditions_test.go b/utils/conditions/conditions_test.go
@@ -763,6 +763,9 @@ func TestComputeGenerationHash(t *testing.T) {
 	assert.NotEqual(t, baseline, roPausedHash)
 }
 
+// TestComputeStableStepHash verifies we generate different hashes for various step definitions.
+// Also verifies we do not unintentionally break our ComputeStepHash function somehow (e.g. by
+// modifying types or change libraries)
 func TestComputeStepHash(t *testing.T) {
 	ro := &v1alpha1.Rollout{
 		Spec: v1alpha1.RolloutSpec{
@@ -788,23 +791,26 @@ func TestComputeStepHash(t *testing.T) {
 		},
 	}
 	roWithDiffStepsHash := ComputeStepHash(roWithDiffSteps)
+	assert.Equal(t, "79c9b9f6bf", roWithDiffStepsHash)
 
 	roWithSameSteps := ro.DeepCopy()
 	roWithSameSteps.Status.CurrentPodHash = "Test"
 	roWithSameSteps.Spec.Replicas = pointer.Int32Ptr(1)
 	roWithSameStepsHash := ComputeStepHash(roWithSameSteps)
+	assert.Equal(t, "6b9b86fbd5", roWithSameStepsHash)
 
 	roNoSteps := ro.DeepCopy()
 	roNoSteps.Spec.Strategy.CanaryStrategy.Steps = nil
 	roNoStepsHash := ComputeStepHash(roNoSteps)
+	assert.Equal(t, "5ffbfbbd64", roNoStepsHash)
 
 	roBlueGreen := ro.DeepCopy()
 	roBlueGreen.Spec.Strategy.CanaryStrategy = nil
 	roBlueGreen.Spec.Strategy.BlueGreenStrategy = &v1alpha1.BlueGreenStrategy{}
 	roBlueGreenHash := ComputeStepHash(roBlueGreen)
+	assert.Equal(t, "", roBlueGreenHash)
 
 	assert.NotEqual(t, baseline, roWithDiffStepsHash)
 	assert.Equal(t, baseline, roWithSameStepsHash)
 	assert.NotEqual(t, baseline, roNoStepsHash)
-	assert.Equal(t, "", roBlueGreenHash)
 }