Bugfix Spec.DeepHash() to yield consistent values.

cmd/varnish/vcl/vcl.go

@@ -120,9 +120,16 @@ func (spec Spec) DeepHash() uint64 {
 	for _, rule := range spec.Rules {
 		rule.hash(hash)
 	}
-	for k, v := range spec.AllServices {
-		hash.Write([]byte(k))
-		v.hash(hash)
+	svcs := make([]string, len(spec.AllServices))
+	i := 0
+	for k := range spec.AllServices {
+		svcs[i] = k
+		i++
+	}
+	sort.Strings(svcs)
+	for _, svc := range svcs {
+		hash.Write([]byte(svc))
+		spec.AllServices[svc].hash(hash)
 	}
 	return hash.Sum64()
 }

cmd/varnish/vcl/vcl_test.go

@@ -32,6 +32,7 @@ import (
 	"bytes"
 	"io/ioutil"
 	"path/filepath"
+	"reflect"
 	"testing"
 )
 
@@ -67,6 +68,42 @@ var coffeeSvc = Service{
 	},
 }
 
+var cafeSpec = Spec{
+	DefaultService: Service{},
+	Rules: []Rule{{
+		Host: "cafe.example.com",
+		PathMap: map[string]Service{
+			"/tea":    teaSvc,
+			"/coffee": coffeeSvc,
+		},
+	}},
+	AllServices: map[string]Service{
+		"tea-svc":    teaSvc,
+		"coffee-svc": coffeeSvc,
+	},
+}
+
+func TestTemplate(t *testing.T) {
+	var buf bytes.Buffer
+	if err := Tmpl.Execute(&buf, cafeSpec); err != nil {
+		t.Error("Execute():", err)
+	}
+
+	goldpath := filepath.Join("testdata", "ingressrule.golden")
+	gold, err := ioutil.ReadFile(goldpath)
+	if err != nil {
+		t.Fatalf("Error reading %s: %v", goldpath, err)
+	}
+	if !bytes.Equal(buf.Bytes(), gold) {
+		t.Errorf("Generated VCL for IngressSpec does not match gold "+
+			"file: %s", goldpath)
+		if testing.Verbose() {
+			t.Log("Generated VCL:", string(buf.Bytes()))
+			t.Log(goldpath, ":", string(gold))
+		}
+	}
+}
+
 var coffeeSvc3 = Service{
 	Name: "coffee-svc",
 	Addresses: []Address{
@@ -85,63 +122,96 @@ var coffeeSvc3 = Service{
 	},
 }
 
-var cafeSpec = Spec{
+var cafeSpec2 = Spec{
 	DefaultService: Service{},
 	Rules: []Rule{{
 		Host: "cafe.example.com",
 		PathMap: map[string]Service{
 			"/tea":    teaSvc,
-			"/coffee": coffeeSvc,
+			"/coffee": coffeeSvc3,
 		},
 	}},
 	AllServices: map[string]Service{
 		"tea-svc":    teaSvc,
-		"coffee-svc": coffeeSvc,
+		"coffee-svc": coffeeSvc3,
 	},
 }
 
-var cafeSpec2 = Spec{
+func TestDeepHash(t *testing.T) {
+	if cafeSpec.DeepHash() == cafeSpec2.DeepHash() {
+		t.Errorf("DeepHash(): Distinct specs have equal hashes")
+		if testing.Verbose() {
+			t.Logf("spec1: %+v", cafeSpec)
+			t.Logf("spec2: %+v", cafeSpec2)
+			t.Logf("hash: %0x", cafeSpec.DeepHash())
+		}
+	}
+}
+
+var teaSvcShuf = Service{
+	Name: "tea-svc",
+	Addresses: []Address{
+		{
+			IP:   "192.0.2.3",
+			Port: 80,
+		},
+		{
+			IP:   "192.0.2.1",
+			Port: 80,
+		},
+		{
+			IP:   "192.0.2.2",
+			Port: 80,
+		},
+	},
+}
+
+var coffeeSvcShuf = Service{
+	Name: "coffee-svc",
+	Addresses: []Address{
+		{
+			IP:   "192.0.2.5",
+			Port: 80,
+		},
+		{
+			IP:   "192.0.2.4",
+			Port: 80,
+		},
+	},
+}
+
+var cafeSpecShuf = Spec{
 	DefaultService: Service{},
 	Rules: []Rule{{
 		Host: "cafe.example.com",
 		PathMap: map[string]Service{
-			"/tea":    teaSvc,
-			"/coffee": coffeeSvc3,
+			"/coffee": coffeeSvcShuf,
+			"/tea":    teaSvcShuf,
 		},
 	}},
 	AllServices: map[string]Service{
-		"tea-svc":    teaSvc,
-		"coffee-svc": coffeeSvc3,
+		"coffee-svc": coffeeSvcShuf,
+		"tea-svc":    teaSvcShuf,
 	},
 }
 
-func TestTemplate(t *testing.T) {
-	var buf bytes.Buffer
-	if err := Tmpl.Execute(&buf, cafeSpec); err != nil {
-		t.Error("Execute():", err)
-	}
-
-	goldpath := filepath.Join("testdata","ingressrule.golden")
-	gold, err := ioutil.ReadFile(goldpath)
-	if err != nil {
-		t.Fatalf("Error reading %s: %v", goldpath, err)
-	}
-	if !bytes.Equal(buf.Bytes(), gold) {
-		t.Errorf("Generated VCL for IngressSpec does not match gold "+
-			"file: %s", goldpath)
+func TestCanoncial(t *testing.T) {
+	canonCafe := cafeSpec.Canonical()
+	canonShuf := cafeSpecShuf.Canonical()
+	if !reflect.DeepEqual(canonCafe, canonShuf) {
+		t.Error("Canonical(): Equivalent VCL specs not deeply equal")
 		if testing.Verbose() {
-			t.Log("Generated VCL:", string(buf.Bytes()))
-			t.Log(goldpath, ":", string(gold))
+			t.Log("Canonical cafe:", canonCafe)
+			t.Log("Canonical shuffled cafe:", canonShuf)
 		}
 	}
-}
-
-func TestDeepHash(t *testing.T) {
-	t.Logf("cafeSpec.DeepHash(): %0x", cafeSpec.DeepHash())
-	t.Logf("cafeSpec2.DeepHash() %0x", cafeSpec2.DeepHash())
-	if cafeSpec.DeepHash() == cafeSpec2.DeepHash() {
-		t.Errorf("Distinct specs with different hashes: spec1=%+v "+
-			"spec2=%+v hash=%0x", cafeSpec, cafeSpec2,
-			cafeSpec.DeepHash())
+	if canonCafe.DeepHash() != canonShuf.DeepHash() {
+		t.Error("Canonical(): Unequal hashes for equivalent specs")
+		if testing.Verbose() {
+			t.Logf("spec1 canonical: %+v", canonCafe)
+			t.Logf("spec1 hash: %0x", canonCafe.DeepHash())
+			t.Logf("spec2 canonical: %+v", canonShuf)
+			t.Logf("spec2 hash: %0x", canonShuf.DeepHash())
+		}
 	}
 }