Fix spec changes detection

.circleci/config.yml

@@ -35,9 +35,6 @@ jobs:
       - run:
           name: Verify code gen
           command: make test-codegen
-      - run:
-          name: Release notes
-          command: make release-notes
       - save_cache:
           key: go-mod-v3-{{ checksum "go.sum" }}
           paths:
@@ -71,6 +68,7 @@ jobs:
       - restore_cache:
           keys:
             - go-mod-v3-{{ checksum "go.sum" }}
+      - run: make release-notes
       - run: test/goreleaser.sh
 
   e2e-istio-testing:

Makefile

@@ -108,8 +108,7 @@ release-set: fmt version-set helm-package
 
 release-notes:
 	cd /tmp && GH_REL_URL="https://github.com/buchanae/github-release-notes/releases/download/0.2.0/github-release-notes-linux-amd64-0.2.0.tar.gz" && \
-    curl -sSL $${GH_REL_URL} | tar xz && sudo mv github-release-notes /usr/local/bin/ && \
-	github-release-notes -org weaveworks -repo flagger -since-latest-release
+    curl -sSL $${GH_REL_URL} | tar xz && sudo mv github-release-notes /usr/local/bin/
 
 reset-test:
 	kubectl delete -f ./artifacts/namespaces

go.mod

@@ -4,11 +4,11 @@ go 1.13
 
 require (
 	github.com/Masterminds/semver/v3 v3.0.3
+	github.com/davecgh/go-spew v1.1.1
 	github.com/golang/groupcache v0.0.0-20190129154638-5b532d6fd5ef // indirect
 	github.com/google/go-cmp v0.3.0
 	github.com/googleapis/gnostic v0.2.0 // indirect
 	github.com/imdario/mergo v0.3.7 // indirect
-	github.com/mitchellh/hashstructure v1.0.0
 	github.com/pkg/errors v0.8.1
 	github.com/prometheus/client_golang v1.0.0
 	go.uber.org/atomic v1.3.2 // indirect

go.sum

@@ -119,8 +119,6 @@ github.com/mailru/easyjson v0.0.0-20190626092158-b2ccc519800e/go.mod h1:C1wdFJiN
 github.com/mailru/easyjson v0.7.0/go.mod h1:KAzv3t3aY1NaHWoQz1+4F1ccyAH66Jk7yos7ldAVICs=
 github.com/matttproud/golang_protobuf_extensions v1.0.1 h1:4hp9jkHxhMHkqkrB3Ix0jegS5sx/RkqARlsWZ6pIwiU=
 github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0=
-github.com/mitchellh/hashstructure v1.0.0 h1:ZkRJX1CyOoTkar7p/mLS5TZU4nJ1Rn/F8u9dGS02Q3Y=
-github.com/mitchellh/hashstructure v1.0.0/go.mod h1:QjSHrPWS+BGUVBYkbTZWEnOh3G1DutKwClXU/ABz6AQ=
 github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=
 github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg=
 github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=

pkg/canary/deployment_controller_test.go

@@ -1,9 +1,11 @@
 package canary
 
 import (
-	"k8s.io/apimachinery/pkg/api/errors"
 	"testing"
 
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1"
@@ -39,29 +41,6 @@ func TestCanaryDeployer_Sync(t *testing.T) {
 	}
 }
 
-func TestCanaryDeployer_IsNewSpec(t *testing.T) {
-	mocks := newFixture()
-	err := mocks.deployer.Initialize(mocks.canary, true)
-	if err != nil {
-		t.Fatal(err.Error())
-	}
-
-	dep2 := newTestDeploymentV2()
-	_, err = mocks.kubeClient.AppsV1().Deployments("default").Update(dep2)
-	if err != nil {
-		t.Fatal(err.Error())
-	}
-
-	isNew, err := mocks.deployer.HasTargetChanged(mocks.canary)
-	if err != nil {
-		t.Fatal(err.Error())
-	}
-
-	if !isNew {
-		t.Errorf("Got %v wanted %v", isNew, true)
-	}
-}
-
 func TestCanaryDeployer_Promote(t *testing.T) {
 	mocks := newFixture()
 	err := mocks.deployer.Initialize(mocks.canary, true)
@@ -272,3 +251,128 @@ func TestCanaryDeployer_NoConfigTracking(t *testing.T) {
 		t.Errorf("Got config name %v wanted %v", configName, "podinfo-config-vol")
 	}
 }
+
+func TestCanaryDeployer_HasTargetChanged(t *testing.T) {
+	mocks := newFixture()
+	err := mocks.deployer.Initialize(mocks.canary, true)
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+
+	// save last applied hash
+	canary, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get("podinfo", metav1.GetOptions{})
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+	err = mocks.deployer.SyncStatus(canary, flaggerv1.CanaryStatus{Phase: flaggerv1.CanaryPhaseInitializing})
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+
+	// save last promoted hash
+	canary, err = mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get("podinfo", metav1.GetOptions{})
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+	err = mocks.deployer.SetStatusPhase(canary, flaggerv1.CanaryPhaseInitialized)
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+
+	dep, err := mocks.kubeClient.AppsV1().Deployments("default").Get("podinfo", metav1.GetOptions{})
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+
+	depClone := dep.DeepCopy()
+	depClone.Spec.Template.Spec.Containers[0].Resources = corev1.ResourceRequirements{
+		Requests: corev1.ResourceList{
+			corev1.ResourceCPU: *resource.NewQuantity(100, resource.DecimalExponent),
+		},
+	}
+
+	// update pod spec
+	_, err = mocks.kubeClient.AppsV1().Deployments("default").Update(depClone)
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+
+	canary, err = mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get("podinfo", metav1.GetOptions{})
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+
+	// detect change in last applied spec
+	isNew, err := mocks.deployer.HasTargetChanged(canary)
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+	if !isNew {
+		t.Errorf("Got %v wanted %v", isNew, true)
+	}
+
+	// save hash
+	err = mocks.deployer.SyncStatus(canary, flaggerv1.CanaryStatus{Phase: flaggerv1.CanaryPhaseProgressing})
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+
+	dep, err = mocks.kubeClient.AppsV1().Deployments("default").Get("podinfo", metav1.GetOptions{})
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+
+	depClone = dep.DeepCopy()
+	depClone.Spec.Template.Spec.Containers[0].Resources = corev1.ResourceRequirements{
+		Requests: corev1.ResourceList{
+			corev1.ResourceCPU: *resource.NewQuantity(1000, resource.DecimalExponent),
+		},
+	}
+
+	// update pod spec
+	_, err = mocks.kubeClient.AppsV1().Deployments("default").Update(depClone)
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+
+	canary, err = mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get("podinfo", metav1.GetOptions{})
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+
+	// ignore change as hash should be the same with last promoted
+	isNew, err = mocks.deployer.HasTargetChanged(canary)
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+	if isNew {
+		t.Errorf("Got %v wanted %v", isNew, false)
+	}
+
+	depClone = dep.DeepCopy()
+	depClone.Spec.Template.Spec.Containers[0].Resources = corev1.ResourceRequirements{
+		Requests: corev1.ResourceList{
+			corev1.ResourceCPU: *resource.NewQuantity(600, resource.DecimalExponent),
+		},
+	}
+
+	// update pod spec
+	_, err = mocks.kubeClient.AppsV1().Deployments("default").Update(depClone)
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+
+	canary, err = mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get("podinfo", metav1.GetOptions{})
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+
+	// detect change
+	isNew, err = mocks.deployer.HasTargetChanged(canary)
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+	if !isNew {
+		t.Errorf("Got %v wanted %v", isNew, true)
+	}
+}

pkg/canary/fixture.go

@@ -6,6 +6,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	hpav2 "k8s.io/api/autoscaling/v2beta1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
@@ -244,6 +245,11 @@ func newTestDeployment() *appsv1.Deployment {
 								"./podinfo",
 								"--port=9898",
 							},
+							Resources: corev1.ResourceRequirements{
+								Requests: corev1.ResourceList{
+									corev1.ResourceCPU: *resource.NewQuantity(1000, resource.DecimalExponent),
+								},
+							},
 							Args:       nil,
 							WorkingDir: "",
 							Ports: []corev1.ContainerPort{

pkg/canary/spec.go

@@ -2,29 +2,48 @@ package canary
 
 import (
 	"fmt"
+	"hash/fnv"
+
+	"github.com/davecgh/go-spew/spew"
+	"k8s.io/apimachinery/pkg/util/rand"
 
-	"github.com/mitchellh/hashstructure"
 	flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1"
 )
 
+// hasSpecChanged computes the hash of the spec and compares it with the
+// last applied spec, if the last applied hash is different but not equal
+// to last promoted one the it returns true
 func hasSpecChanged(cd *flaggerv1.Canary, spec interface{}) (bool, error) {
 	if cd.Status.LastAppliedSpec == "" {
 		return true, nil
 	}
 
-	newHash, err := hashstructure.Hash(spec, nil)
-	if err != nil {
-		return false, fmt.Errorf("hash error %v", err)
-	}
+	newHash := computeHash(spec)
 
 	// do not trigger a canary deployment on manual rollback
-	if cd.Status.LastPromotedSpec == fmt.Sprintf("%d", newHash) {
+	if cd.Status.LastPromotedSpec == newHash {
 		return false, nil
 	}
 
-	if cd.Status.LastAppliedSpec != fmt.Sprintf("%d", newHash) {
+	if cd.Status.LastAppliedSpec != newHash {
 		return true, nil
 	}
 
 	return false, nil
 }
+
+// computeHash returns a hash value calculated from a spec using the spew library
+// which follows pointers and prints actual values of the nested objects
+// ensuring the hash does not change when a pointer changes.
+func computeHash(spec interface{}) string {
+	hasher := fnv.New32a()
+	printer := spew.ConfigState{
+		Indent:         " ",
+		SortKeys:       true,
+		DisableMethods: true,
+		SpewKeys:       true,
+	}
+	printer.Fprintf(hasher, "%#v", spec)
+
+	return rand.SafeEncodeString(fmt.Sprint(hasher.Sum32()))
+}

pkg/canary/status.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/mitchellh/hashstructure"
 	ex "github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -36,13 +35,10 @@ func (c *DeploymentController) SyncStatus(cd *flaggerv1.Canary, status flaggerv1
 }
 
 func syncCanaryStatus(flaggerClient clientset.Interface, cd *flaggerv1.Canary, status flaggerv1.CanaryStatus, canaryResource interface{}, setAll func(cdCopy *flaggerv1.Canary)) error {
-	hash, err := hashstructure.Hash(canaryResource, nil)
-	if err != nil {
-		return ex.Wrap(err, "SyncStatus hash error")
-	}
+	hash := computeHash(canaryResource)
 
 	firstTry := true
-	err = retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) {
+	err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) {
 		var selErr error
 		if !firstTry {
 			cd, selErr = flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Get(cd.GetName(), metav1.GetOptions{})
@@ -56,7 +52,7 @@ func syncCanaryStatus(flaggerClient clientset.Interface, cd *flaggerv1.Canary, s
 		cdCopy.Status.CanaryWeight = status.CanaryWeight
 		cdCopy.Status.FailedChecks = status.FailedChecks
 		cdCopy.Status.Iterations = status.Iterations
-		cdCopy.Status.LastAppliedSpec = fmt.Sprintf("%d", hash)
+		cdCopy.Status.LastAppliedSpec = hash
 		cdCopy.Status.LastTransitionTime = metav1.Now()
 		setAll(cdCopy)
 

test/goreleaser.sh

@@ -3,8 +3,6 @@ set -e
 
 TAR_FILE="/tmp/goreleaser.tar.gz"
 RELEASES_URL="https://github.com/goreleaser/goreleaser/releases"
-GH_REL_DIR="github-release-notes-linux-amd64-0.2.0"
-GH_REL_URL="https://github.com/buchanae/github-release-notes/releases/download/0.2.0/${GH_REL_DIR}.tar.gz"
 test -z "$TMPDIR" && TMPDIR="$(mktemp -d)"
 
 last_version() {
@@ -23,8 +21,6 @@ download() {
   rm -f "$TAR_FILE"
   curl -s -L -o "$TAR_FILE" \
     "$RELEASES_URL/download/$VERSION/goreleaser_$(uname -s)_$(uname -m).tar.gz"
-
-  curl -sSL ${GH_REL_URL} | tar xz && sudo mv ${GH_REL_DIR}/github-release-notes /usr/local/bin/ && rm -rf ${GH_REL_DIR}
 }
 
 download