From e7da8c3f3529ed1312129529115df2bb2bbbdece Mon Sep 17 00:00:00 2001
From: Samuel Lang <gh@lang-sam.de>
Date: Wed, 26 Aug 2020 11:27:05 +0200
Subject: [PATCH] Skipper: preserve Predicates

Current implementation did overwrite potentially existing Predicates.

We face the situation that we need to add further Predicates which we need to keep in order to have  a proper route setup
---
 pkg/router/skipper.go      | 22 ++++++++++++++++--
 pkg/router/skipper_test.go | 46 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/pkg/router/skipper.go b/pkg/router/skipper.go
index 6b5a8c7cd..aed495a9e 100644
--- a/pkg/router/skipper.go
+++ b/pkg/router/skipper.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"strings"
 
 	"github.com/google/go-cmp/cmp"
 	"go.uber.org/zap"
@@ -176,7 +177,8 @@ func (skp *SkipperRouter) SetRoutes(canary *flaggerv1.Canary, primaryWeight, can
 
 	// Disable the canary-ingress route after the canary process
 	if canaryWeight == 0 {
-		iClone.Annotations[skipperpredicateAnnotationKey] = canaryRouteDisable
+		// ensuring False() is at first place
+		iClone.Annotations[skipperpredicateAnnotationKey] = insertPredicate(iClone.Annotations[skipperpredicateAnnotationKey], canaryRouteDisable)
 	}
 
 	_, err = skp.kubeClient.NetworkingV1beta1().Ingresses(canary.Namespace).Update(
@@ -212,7 +214,7 @@ func (skp *SkipperRouter) makeAnnotations(annotations map[string]string, backend
 	}
 	annotations[skipperBackendWeightsAnnotationKey] = string(b)
 	// adding more weight to canary route solves traffic bypassing through apexIngress
-	annotations[skipperpredicateAnnotationKey] = canaryRouteWeight
+	annotations[skipperpredicateAnnotationKey] = insertPredicate(annotations[skipperpredicateAnnotationKey], canaryRouteWeight)
 
 	return annotations
 }
@@ -233,3 +235,19 @@ func (skp *SkipperRouter) backendWeights(annotation map[string]string) (backendW
 func (skp *SkipperRouter) getIngressNames(name string) (apexName, canaryName string) {
 	return name, fmt.Sprintf(canaryPatternf, name)
 }
+
+func insertPredicate(raw, insert string) string {
+	// ensuring it at first place
+	predicates := []string{insert}
+	for _, x := range strings.Split(raw, "&&") {
+		predicate := strings.TrimSpace(x)
+		// dropping conflicting predicates
+		if predicate == "" ||
+			predicate == canaryRouteWeight ||
+			predicate == canaryRouteDisable {
+			continue
+		}
+		predicates = append(predicates, predicate)
+	}
+	return strings.Join(predicates, " && ")
+}
diff --git a/pkg/router/skipper_test.go b/pkg/router/skipper_test.go
index e782f38e3..6301af57a 100644
--- a/pkg/router/skipper_test.go
+++ b/pkg/router/skipper_test.go
@@ -105,3 +105,49 @@ func TestSkipperRouter_GetSetRoutes(t *testing.T) {
 	}
 
 }
+
+func Test_insertPredicate(t *testing.T) {
+	tests := []struct {
+		name   string
+		raw    string
+		insert string
+		want   string
+	}{
+		{
+			name:   "a few Predicates lined up",
+			raw:    `Host(/^my-host-header\.example\.org$/) && Method("GET") && Path("/hello")`,
+			insert: "Weight(100)",
+			want:   `Weight(100) && Host(/^my-host-header\.example\.org$/) && Method("GET") && Path("/hello")`,
+		},
+		{
+			name:   "adds Predicate if none is set",
+			raw:    "",
+			insert: "Weight(100)",
+			want:   `Weight(100)`,
+		},
+		{
+			name:   "removes duplicated Predicate Weight(100)",
+			raw:    `Weight(100) && Host(/^my-host-header\.example\.org$/) && Method("GET") && Path("/hello")`,
+			insert: "Weight(100)",
+			want:   `Weight(100) && Host(/^my-host-header\.example\.org$/) && Method("GET") && Path("/hello")`,
+		},
+		{
+			name:   "removes duplicated Predicate False() and reorders them",
+			raw:    `Host(/^my-host-header\.example\.org$/) && Method("GET") && Path("/hello")&&False()`,
+			insert: "False()",
+			want:   `False() && Host(/^my-host-header\.example\.org$/) && Method("GET") && Path("/hello")`,
+		},
+		{
+			name:   "removes conflicting Predicate False()",
+			raw:    `Host(/^my-host-header\.example\.org$/) &&  False() && Method("GET") && Path("/hello")`,
+			insert: "Weight(100)",
+			want:   `Weight(100) && Host(/^my-host-header\.example\.org$/) && Method("GET") && Path("/hello")`,
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, insertPredicate(tt.raw, tt.insert))
+		})
+	}
+}