internal/dag: add more header condition validations

Detect additional invalid header condition scenarios and write them
to the proxy's status, specifically for:
- routes with both Exact and NotExact conditions with the same
value for the same header
- routes with both Contains and NotContains conditions with the
same value for the same header

Signed-off-by: Steve Kriss <krisss@vmware.com>

internal/dag/builder.go

@@ -645,9 +645,9 @@ func (b *Builder) computeRoutes(sw *ObjectStatusWriter, proxy *projcontour.HTTPP
 
 		conds := append(conditions, route.Conditions...)
 
-		// Look for duplicate exact match headers on this route
-		if !headerConditionsAreValid(conds) {
-			sw.SetInvalid("cannot specify duplicate header 'exact match' conditions in the same route")
+		// Look for invalid header conditions on this route
+		if err := headerConditionsValid(conds); err != nil {
+			sw.SetInvalid(err.Error())
 			return nil
 		}
 

internal/dag/conditions.go

@@ -111,22 +111,71 @@ func mergeHeaderConditions(conds []projcontour.Condition) []HeaderCondition {
 	return hc
 }
 
-func headerConditionsAreValid(conditions []projcontour.Condition) bool {
-	// Look for duplicate "exact match" headers on conditions
-	// if found, set error condition on HTTPProxy
-	encountered := map[string]bool{}
+// headerConditionsValid validates that the header conditions within a
+// slice of Conditions are valid. Specifically, it returns an error for
+// any of the following scenarios:
+//	- more than 1 'exact' condition for the same header
+//	- an 'exact' and a 'notexact' condition for the same header, with the same values
+//	- a 'contains' and a 'notcontains' condition for the same header, with the same values
+//
+// Note that there are additional, more complex scenarios that we could check for here. For
+// example, "exact: foo" and "notcontains: <any substring of foo>" are contradictory.
+func headerConditionsValid(conditions []projcontour.Condition) error {
+	seenConditions := map[projcontour.HeaderCondition]bool{}
+	headersWithExactMatch := map[string]bool{}
+
 	for _, v := range conditions {
 		if v.Header == nil {
 			continue
 		}
+
+		headerName := strings.ToLower(v.Header.Name)
 		switch {
 		case v.Header.Exact != "":
-			headerName := strings.ToLower(v.Header.Name)
-			if encountered[headerName] {
-				return false
+			// Look for duplicate "exact match" headers on conditions
+			if headersWithExactMatch[headerName] {
+				return errors.New("cannot specify duplicate header 'exact match' conditions in the same route")
+			}
+			headersWithExactMatch[headerName] = true
+
+			// look for a NotExact condition on the same header with the same value
+			if seenConditions[projcontour.HeaderCondition{
+				Name:     headerName,
+				NotExact: v.Header.Exact,
+			}] {
+				return errors.New("cannot specify contradictory 'exact' and 'notexact' conditions for the same route and header")
+			}
+		case v.Header.NotExact != "":
+			// look for an Exact condition on the same header with the same value
+			if seenConditions[projcontour.HeaderCondition{
+				Name:  headerName,
+				Exact: v.Header.NotExact,
+			}] {
+				return errors.New("cannot specify contradictory 'exact' and 'notexact' conditions for the same route and header")
+			}
+		case v.Header.Contains != "":
+			// look for a NotContains condition on the same header with the same value
+			if seenConditions[projcontour.HeaderCondition{
+				Name:        headerName,
+				NotContains: v.Header.Contains,
+			}] {
+				return errors.New("cannot specify contradictory 'contains' and 'notcontains' conditions for the same route and header")
+			}
+		case v.Header.NotContains != "":
+			// look for a Contains condition on the same header with the same value
+			if seenConditions[projcontour.HeaderCondition{
+				Name:     headerName,
+				Contains: v.Header.NotContains,
+			}] {
+				return errors.New("cannot specify contradictory 'contains' and 'notcontains' conditions for the same route and header")
 			}
-			encountered[headerName] = true
 		}
+
+		key := *v.Header
+		// use the lower-cased header name so comparisons are case-insensitive
+		key.Name = headerName
+		seenConditions[key] = true
 	}
-	return true
+
+	return nil
 }

internal/dag/conditions_test.go

@@ -317,11 +317,19 @@ func TestPrefixConditionsValid(t *testing.T) {
 func TestValidateHeaderConditions(t *testing.T) {
 	tests := map[string]struct {
 		conditions []projcontour.Condition
-		want       bool
+		wantErr    bool
 	}{
 		"empty condition list": {
 			conditions: nil,
-			want:       true,
+			wantErr:    false,
+		},
+		"prefix only": {
+			conditions: []projcontour.Condition{
+				{
+					Prefix: "/blog",
+				},
+			},
+			wantErr: false,
 		},
 		"valid conditions": {
 			conditions: []projcontour.Condition{
@@ -332,9 +340,27 @@ func TestValidateHeaderConditions(t *testing.T) {
 					},
 				},
 			},
-			want: true,
+			wantErr: false,
+		},
+		"prefix conditions + valid headers": {
+			conditions: []projcontour.Condition{
+				{
+					Prefix: "/blog",
+				}, {
+					Header: &projcontour.HeaderCondition{
+						Name:        "x-header",
+						NotContains: "abc",
+					},
+				}, {
+					Header: &projcontour.HeaderCondition{
+						Name:        "another-header",
+						NotContains: "123",
+					},
+				},
+			},
+			wantErr: false,
 		},
-		"invalid conditions": {
+		"multiple 'exact' conditions for the same header are invalid": {
 			conditions: []projcontour.Condition{
 				{
 					Header: &projcontour.HeaderCondition{
@@ -348,40 +374,132 @@ func TestValidateHeaderConditions(t *testing.T) {
 					},
 				},
 			},
-			want: false,
+			wantErr: true,
 		},
-		"prefix only": {
+		"multiple 'exact' conditions for different headers are valid": {
 			conditions: []projcontour.Condition{
 				{
-					Prefix: "/blog",
+					Header: &projcontour.HeaderCondition{
+						Name:  "x-header",
+						Exact: "abc",
+					},
+				}, {
+					Header: &projcontour.HeaderCondition{
+						Name:  "x-different-header",
+						Exact: "123",
+					},
 				},
 			},
-			want: true,
+			wantErr: false,
 		},
-		"prefix conditions + valid headers": {
+		"'exact' and 'notexact' conditions for the same header with the same value are invalid": {
 			conditions: []projcontour.Condition{
 				{
-					Prefix: "/blog",
+					Header: &projcontour.HeaderCondition{
+						Name:  "x-header",
+						Exact: "abc",
+					},
+				}, {
+					Header: &projcontour.HeaderCondition{
+						Name:     "x-header",
+						NotExact: "abc",
+					},
+				},
+			},
+			wantErr: true,
+		},
+		"'exact' and 'notexact' conditions for the same header with different values are valid": {
+			conditions: []projcontour.Condition{
+				{
+					Header: &projcontour.HeaderCondition{
+						Name:  "x-header",
+						Exact: "abc",
+					},
+				}, {
+					Header: &projcontour.HeaderCondition{
+						Name:     "x-header",
+						NotExact: "def",
+					},
+				},
+			},
+			wantErr: false,
+		},
+		"'exact' and 'notexact' conditions for different headers with the same value are valid": {
+			conditions: []projcontour.Condition{
+				{
+					Header: &projcontour.HeaderCondition{
+						Name:  "x-header",
+						Exact: "abc",
+					},
+				}, {
+					Header: &projcontour.HeaderCondition{
+						Name:     "x-another-header",
+						NotExact: "abc",
+					},
+				},
+			},
+			wantErr: false,
+		},
+		"'contains' and 'notcontains' conditions for the same header with the same value are invalid": {
+			conditions: []projcontour.Condition{
+				{
+					Header: &projcontour.HeaderCondition{
+						Name:     "x-header",
+						Contains: "abc",
+					},
 				}, {
 					Header: &projcontour.HeaderCondition{
 						Name:        "x-header",
 						NotContains: "abc",
 					},
+				},
+			},
+			wantErr: true,
+		},
+		"'contains' and 'notcontains' conditions for the same header with different values are valid": {
+			conditions: []projcontour.Condition{
+				{
+					Header: &projcontour.HeaderCondition{
+						Name:     "x-header",
+						Contains: "abc",
+					},
 				}, {
 					Header: &projcontour.HeaderCondition{
-						Name:        "another-header",
-						NotContains: "123",
+						Name:        "x-header",
+						NotContains: "def",
 					},
 				},
 			},
-			want: true,
+			wantErr: false,
+		},
+		"'contains' and 'notcontains' conditions for different headers with the same value are valid": {
+			conditions: []projcontour.Condition{
+				{
+					Header: &projcontour.HeaderCondition{
+						Name:     "x-header",
+						Contains: "abc",
+					},
+				}, {
+					Header: &projcontour.HeaderCondition{
+						Name:        "x-another-header",
+						NotContains: "abc",
+					},
+				},
+			},
+			wantErr: false,
 		},
 	}
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			got := headerConditionsAreValid(tc.conditions)
-			assert.Equal(t, tc.want, got)
+			gotErr := headerConditionsValid(tc.conditions)
+
+			if !tc.wantErr && gotErr != nil {
+				t.Fatalf("Expected no error, got (%v)", gotErr)
+			}
+			if tc.wantErr && gotErr == nil {
+				t.Fatalf("Expected error, got none")
+			}
 		})
 	}
 }

site/docs/master/httpproxy.md

@@ -1403,6 +1403,7 @@ Some examples of invalid configurations that Contour provides statuses for:
 - Root HTTPProxy does not specify fqdn.
 - Multiple prefixes cannot be specified on the same set of route conditions.
 - Multiple header conditions of type "exact match" with the same header key.
+- Contradictory header conditions on a route, e.g. a "contains" and "notcontains" condition for the same header and value.
 
  [1]: https://kubernetes.io/docs/concepts/services-networking/ingress/
  [2]: https://github.com/kubernetes/ingress-nginx/blob/master/docs/user-guide/nginx-configuration/annotations.md