Canary new annotation

internal/ingress/annotations/canary/main.go

@@ -17,8 +17,12 @@ limitations under the License.
 package canary
 
 import (
+	"regexp"
+	"strings"
+
 	extensions "k8s.io/api/extensions/v1beta1"
 
+	yaml "gopkg.in/yaml.v2"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
 	"k8s.io/ingress-nginx/internal/ingress/errors"
 	"k8s.io/ingress-nginx/internal/ingress/resolver"
@@ -28,12 +32,31 @@ type canary struct {
 	r resolver.Resolver
 }
 
+var (
+	headerRegexp = regexp.MustCompile(`^[a-zA-Z\d\-_]+$`)
+)
+
+// Policy public struct for parsing `canary-by-policy` YAML data
+// struct fields have to be public in order for yaml.Unmarshal to populate them correctly
+type Policy struct {
+	Header struct {
+		Name   string   `yaml:"name"`
+		Values []string `yaml:"values,flow"`
+	}
+	Cookie struct {
+		Name   string   `yaml:"name"`
+		Values []string `yaml:"values,flow"`
+	}
+	Weigth int `yaml:"weight"`
+}
+
 // Config returns the configuration rules for setting up the Canary
 type Config struct {
-	Enabled bool
-	Weight  int
-	Header  string
-	Cookie  string
+	Header       string
+	HeaderValues []string
+	Cookie       string
+	CookieValues []string
+	Weight       int
 }
 
 // NewParser parses the ingress for canary related annotations
@@ -46,30 +69,46 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation {
 func (c canary) Parse(ing *extensions.Ingress) (interface{}, error) {
 	config := &Config{}
 	var err error
+	var predicate string
 
-	config.Enabled, err = parser.GetBoolAnnotation("canary", ing)
+	predicate, err = parser.GetStringAnnotation("canary-by-policy", ing)
 	if err != nil {
-		config.Enabled = false
+		return config, nil
 	}
-
-	config.Weight, err = parser.GetIntAnnotation("canary-weight", ing)
+	policy := Policy{}
+	err = yaml.Unmarshal([]byte(predicate), &policy)
 	if err != nil {
-		config.Weight = 0
+		return nil, errors.NewInvalidAnnotationConfiguration("canary-by-policy", "bad YAML policy")
 	}
-
-	config.Header, err = parser.GetStringAnnotation("canary-by-header", ing)
-	if err != nil {
-		config.Header = ""
+	if policy.Header.Name != "" {
+		if len(policy.Header.Values) == 0 || !headerRegexp.MatchString(policy.Header.Name) {
+			return nil, errors.NewInvalidAnnotationConfiguration("canary-by-policy", "invalid canary header policy")
+		}
+		for _, v := range policy.Header.Values {
+			if v == "" {
+				return nil, errors.NewInvalidAnnotationConfiguration("canary-by-policy", "empty header value")
+			}
+		}
+		// convert to nginx syntax
+		config.Header = "http_" + strings.Replace(strings.ToLower(policy.Header.Name), "-", "_", -1)
+		config.HeaderValues = policy.Header.Values
 	}
-
-	config.Cookie, err = parser.GetStringAnnotation("canary-by-cookie", ing)
-	if err != nil {
-		config.Cookie = ""
+	if policy.Cookie.Name != "" {
+		if len(policy.Cookie.Values) == 0 || !headerRegexp.MatchString(policy.Cookie.Name) {
+			return nil, errors.NewInvalidAnnotationConfiguration("canary-by-policy", "invalid canary cookie policy")
+		}
+		for _, v := range policy.Cookie.Values {
+			if v == "" {
+				return nil, errors.NewInvalidAnnotationConfiguration("canary-by-policy", "empty cookie value")
+			}
+		}
+		// convert to nginx syntax
+		config.Cookie = "cookie_" + strings.Replace(strings.ToLower(policy.Cookie.Name), "-", "_", -1)
+		config.CookieValues = policy.Cookie.Values
 	}
-
-	if !config.Enabled && (config.Weight > 0 || len(config.Header) > 0 || len(config.Cookie) > 0) {
-		return nil, errors.NewInvalidAnnotationConfiguration("canary", "configured but not enabled")
+	if policy.Weigth < 0 || policy.Weigth > 100 {
+		return nil, errors.NewInvalidAnnotationConfiguration("canary-by-policy", "invalid weight policy")
 	}
-
+	config.Weight = policy.Weigth
 	return config, nil
 }

internal/ingress/annotations/canary/main_test.go

@@ -17,15 +17,16 @@ limitations under the License.
 package canary
 
 import (
+	"testing"
+
+	yaml "gopkg.in/yaml.v2"
 	api "k8s.io/api/core/v1"
 	extensions "k8s.io/api/extensions/v1beta1"
 	metaV1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
-	"testing"
 
 	"k8s.io/ingress-nginx/internal/ingress/resolver"
-	"strconv"
 )
 
 func buildIngress() *extensions.Ingress {
@@ -70,57 +71,140 @@ func TestAnnotations(t *testing.T) {
 	ing.SetAnnotations(data)
 
 	tests := []struct {
-		title         string
-		canaryEnabled bool
-		canaryWeight  int
-		canaryHeader  string
-		canaryCookie  string
-		expErr        bool
+		title              string
+		canaryWeight       int
+		canaryHeader       string
+		canaryNginxHeader  string
+		canaryHeaderValues []string
+		canaryCookie       string
+		canaryNginxCookie  string
+		canaryCookieValues []string
+		expErr             bool
 	}{
-		{"canary disabled and no weight", false, 0, "", "", false},
-		{"canary disabled and weight", false, 20, "", "", true},
-		{"canary disabled and header", false, 0, "X-Canary", "", true},
-		{"canary disabled and cookie", false, 0, "", "canary_enabled", true},
-		{"canary enabled and weight", true, 20, "", "", false},
-		{"canary enabled and no weight", true, 0, "", "", false},
-		{"canary enabled by header", true, 20, "X-Canary", "", false},
-		{"canary enabled by cookie", true, 20, "", "canary_enabled", false},
+		{"canary with valid weight", 10, "", "", nil, "", "", nil, false},
+		{"canary with negative weight", -5, "", "", nil, "", "", nil, true},
+		{"canary with invalid weight", 110, "", "", nil, "", "", nil, true},
+		{"canary with valid header", 0, "X-Canary", "http_x_canary", []string{"value1", "value2"}, "", "", nil, false},
+		{"canary with header and empty value", 0, "X-Canary", "", []string{"value1", ""}, "", "", nil, true},
+		{"canary with header and no values", 0, "X-Canary", "", []string{}, "", "", nil, true},
+		{"canary with valid cookie", 0, "", "", nil, "canary_enabled", "cookie_canary_enabled", []string{"allow", "do_canary"}, false},
+		{"canary with cookie and empty value", 0, "", "", nil, "canary_enabled", "", []string{"", "allow"}, true},
+		{"canary with cookie and no values", 0, "", "", nil, "canary_enabled", "", []string{}, true},
 	}
 
 	for _, test := range tests {
-		data[parser.GetAnnotationWithPrefix("canary")] = strconv.FormatBool(test.canaryEnabled)
-		data[parser.GetAnnotationWithPrefix("canary-weight")] = strconv.Itoa(test.canaryWeight)
-		data[parser.GetAnnotationWithPrefix("canary-by-header")] = test.canaryHeader
-		data[parser.GetAnnotationWithPrefix("canary-by-cookie")] = test.canaryCookie
+		info := Policy{
+			Header: struct {
+				Name   string   `yaml:"name"`
+				Values []string `yaml:"values,flow"`
+			}{
+				Name:   test.canaryHeader,
+				Values: test.canaryHeaderValues,
+			},
+			Cookie: struct {
+				Name   string   `yaml:"name"`
+				Values []string `yaml:"values,flow"`
+			}{
+				Name:   test.canaryCookie,
+				Values: test.canaryCookieValues,
+			},
+			Weigth: test.canaryWeight,
+		}
+		bytes, err := yaml.Marshal(info)
+		data[parser.GetAnnotationWithPrefix("canary-by-policy")] = string(bytes)
 
 		i, err := NewParser(&resolver.Mock{}).Parse(ing)
 		if test.expErr {
 			if err == nil {
 				t.Errorf("%v: expected error but returned nil", test.title)
 			}
-
 			continue
 		} else {
 			if err != nil {
 				t.Errorf("%v: expected nil but returned error %v", test.title, err)
 			}
 		}
-
 		canaryConfig, ok := i.(*Config)
 		if !ok {
 			t.Errorf("%v: expected an External type", test.title)
 		}
-		if canaryConfig.Enabled != test.canaryEnabled {
-			t.Errorf("%v: expected \"%v\", but \"%v\" was returned", test.title, test.canaryEnabled, canaryConfig.Enabled)
-		}
 		if canaryConfig.Weight != test.canaryWeight {
 			t.Errorf("%v: expected \"%v\", but \"%v\" was returned", test.title, test.canaryWeight, canaryConfig.Weight)
 		}
-		if canaryConfig.Header != test.canaryHeader {
+		if canaryConfig.Header != test.canaryNginxHeader {
 			t.Errorf("%v: expected \"%v\", but \"%v\" was returned", test.title, test.canaryHeader, canaryConfig.Header)
 		}
-		if canaryConfig.Cookie != test.canaryCookie {
+		if canaryConfig.Cookie != test.canaryNginxCookie {
 			t.Errorf("%v: expected \"%v\", but \"%v\" was returned", test.title, test.canaryCookie, canaryConfig.Cookie)
 		}
+		if !test.expErr {
+			if !equals(canaryConfig.HeaderValues, test.canaryHeaderValues) {
+				t.Errorf("%v: expected \"%v\", but \"%v\" was returned", test.title, test.canaryHeaderValues, canaryConfig.HeaderValues)
+			}
+			if !equals(canaryConfig.CookieValues, test.canaryCookieValues) {
+				t.Errorf("%v: expected \"%v\", but \"%v\" was returned", test.title, test.canaryCookieValues, canaryConfig.CookieValues)
+			}
+		}
+	}
+}
+
+// equals is duplicated in test_equals() since the right place for the method is not found yet
+func equals(s1, s2 []string) bool {
+	if len(s1) != len(s2) {
+		return false
+	}
+
+	for i, s := range s1 {
+		if s != s2[i] {
+			return false
+		}
+	}
+	return true
+}
+
+func TestBadAnnotationFormat(t *testing.T) {
+	ing := buildIngress()
+
+	data := map[string]string{}
+	ing.SetAnnotations(data)
+
+	tests := []struct {
+		title  string
+		policy string
+	}{
+		{
+			title: "invalid field type",
+			policy: `
+weight: text
+`},
+		{
+			title: "tabulation indent",
+			policy: `
+header:
+	Name: abc
+`},
+		{
+			title: "invalid indentation",
+			policy: `header:
+ name:
+  values:
+   - v1
+`},
+		{
+			title: "invalid structure",
+			policy: `
+cookie:
+ name: canary_enabled
+ values:
+  - WrongField: X
+`},
+	}
+
+	for _, test := range tests {
+		data[parser.GetAnnotationWithPrefix("canary-by-policy")] = test.policy
+		_, err := NewParser(&resolver.Mock{}).Parse(ing)
+		if err == nil || err.Error() != "the annotation canary-by-policy does not contain a valid configuration: bad YAML policy" {
+			t.Errorf("%v: expected error but returned nil", test.title)
+		}
 	}
 }

internal/ingress/controller/controller.go

@@ -690,9 +690,11 @@ func (n *NGINXController) createUpstreams(data []*ingress.Ingress, du *ingress.B
 			if anns.Canary.Enabled {
 				upstreams[defBackend].NoServer = true
 				upstreams[defBackend].TrafficShapingPolicy = ingress.TrafficShapingPolicy{
-					Weight: anns.Canary.Weight,
-					Header: anns.Canary.Header,
-					Cookie: anns.Canary.Cookie,
+					Weight:       anns.Canary.Weight,
+					Header:       anns.Canary.Header,
+					HeaderValues: anns.Canary.HeaderValues,
+					Cookie:       anns.Canary.Cookie,
+					CookieValues: anns.Canary.CookieValues,
 				}
 			}
 
@@ -757,9 +759,11 @@ func (n *NGINXController) createUpstreams(data []*ingress.Ingress, du *ingress.B
 				if anns.Canary.Enabled {
 					upstreams[name].NoServer = true
 					upstreams[name].TrafficShapingPolicy = ingress.TrafficShapingPolicy{
-						Weight: anns.Canary.Weight,
-						Header: anns.Canary.Header,
-						Cookie: anns.Canary.Cookie,
+						Weight:       anns.Canary.Weight,
+						Header:       anns.Canary.Header,
+						HeaderValues: anns.Canary.HeaderValues,
+						Cookie:       anns.Canary.Cookie,
+						CookieValues: anns.Canary.CookieValues,
 					}
 				}
 

internal/ingress/types.go

@@ -116,10 +116,14 @@ type TrafficShapingPolicy struct {
 	// e.g. Weight 20 means 20% of traffic will be redirected to the backend and 80% will remain
 	// with the other backend. 0 weight will not send any traffic to this backend
 	Weight int `json:"weight"`
-	// Header on which to redirect requests to this backend
+	// Header key in ngnix on which to redirect requests to this backend
 	Header string `json:"header"`
-	// Cookie on which to redirect requests to this backend
+	// HeaderValues array of header values to match in order to redirect requests to this backend
+	HeaderValues []string `json:"headerValues,omitempty"`
+	// Cookie key in nginx on which to redirect requests to this backend
 	Cookie string `json:"cookie"`
+	// CookieValues array of cookie values to match in order to redirect requests to this backend
+	CookieValues []string `json:"cookieValues,omitempty"`
 }
 
 // HashInclude defines if a field should be used or not to calculate the hash

internal/ingress/types_equals.go

@@ -293,10 +293,30 @@ func (tsp1 TrafficShapingPolicy) Equal(tsp2 TrafficShapingPolicy) bool {
 	if tsp1.Header != tsp2.Header {
 		return false
 	}
+	if equals(tsp1.HeaderValues, tsp2.HeaderValues) {
+		return false
+	}
 	if tsp1.Cookie != tsp2.Cookie {
 		return false
 	}
+	if equals(tsp1.CookieValues, tsp2.CookieValues) {
+		return false
+	}
+
+	return true
+}
 
+// equals compares in natural order; same values in different order yield 'false'
+func equals(s1, s2 []string) bool {
+	if len(s1) != len(s2) {
+		return false
+	}
+
+	for i, s := range s1 {
+		if s != s2[i] {
+			return false
+		}
+	}
 	return true
 }