internal/dag: Fix comparison of include match conditions (#4931)

Previously includeMatchConditionsIdentical was just comparing adjacent
includes and comparing conditions element by element rather than as a
whole, ANDed together

Also don't exit processor early so other routes etc. are configured
to preserve traffic as much as possible. This behavior is more similar to
how other types of invalid match conditions are processed

Signed-off-by: Sunjay Bhatia <[email protected]>

changelogs/unreleased/4931-sunjayBhatia-minor.md

@@ -0,0 +1,10 @@
+## Fix handling of duplicate HTTPProxy Include Conditions
+
+Duplicate include conditions are now correctly identified and HTTPProxies are marked with the condition `IncludeError` and reason `DuplicateMatchConditions`.
+Previously the HTTPProxy processor was only comparing adjacent includes and comparing conditions element by element rather than as a whole, ANDed together.
+
+In addition, the previous behavior when duplicate Include Conditions were identified was to throw out all routes, including valid ones, on the offending HTTPProxy.
+Any referenced child HTTPProxies were marked as `Orphaned` as a result, even if they were included correctly.
+With this change, all valid Includes and Route rules are processed and programmed in the data plane, which is a difference in behavior from previous releases.
+An Include is deemed to be a duplicate if it has the exact same match Conditions as an Include that precedes it in the list.
+Only child HTTPProxies that are referenced by a duplicate Include and not in any other valid Include are marked as `Orphaned`

internal/dag/builder_test.go

@@ -6479,6 +6479,28 @@ func TestDAGInsert(t *testing.T) {
 				}},
 				Name:      "kuard",
 				Namespace: "default",
+			}, {
+				// This second include has a similar set of conditions with
+				// slight differences which should still ensure there is a
+				// route programmed.
+				Conditions: []contour_api_v1.MatchCondition{{
+					Header: &contour_api_v1.HeaderMatchCondition{
+						Name:    "x-request-id",
+						Present: true,
+					},
+				}, {
+					Header: &contour_api_v1.HeaderMatchCondition{
+						Name:       "x-timeout",
+						NotPresent: true,
+					},
+				}, {
+					Header: &contour_api_v1.HeaderMatchCondition{
+						Name:  "digest-auth",
+						Exact: "scott",
+					},
+				}},
+				Name:      "kuard",
+				Namespace: "default",
 			}},
 		},
 	}
@@ -9905,6 +9927,16 @@ func TestDAGInsert(t *testing.T) {
 								{Name: "digest-password", Value: "tiger", MatchType: "exact", Invert: true},
 							},
 							Clusters: clusters(service(s1)),
+						}, &Route{
+							PathMatchCondition: prefixString("/kuard"),
+							HeaderMatchConditions: []HeaderMatchCondition{
+								{Name: "x-request-id", MatchType: "present"},
+								{Name: "x-timeout", MatchType: "present", Invert: true},
+								{Name: "digest-auth", Value: "scott", MatchType: "exact"},
+								{Name: "e-tag", Value: "abcdef", MatchType: "contains"},
+								{Name: "digest-password", Value: "tiger", MatchType: "exact", Invert: true},
+							},
+							Clusters: clusters(service(s1)),
 						}),
 					),
 				},
@@ -10913,7 +10945,26 @@ func TestDAGInsert(t *testing.T) {
 			objs: []interface{}{
 				proxy108, proxy108a, proxy108b, s1, s12, s13,
 			},
-			want: listeners(),
+			want: listeners(
+				&Listener{
+					Name: HTTP_LISTENER_NAME,
+					Port: 80,
+					VirtualHosts: virtualhosts(
+						virtualhost("example.com",
+							// route on root proxy is served
+							prefixroute("/", service(s1)),
+							// route for first valid include is served
+							&Route{
+								PathMatchCondition: prefixString("/blog"),
+								HeaderMatchConditions: []HeaderMatchCondition{
+									{Name: "x-header", Value: "abc", MatchType: "contains"},
+								},
+								Clusters: clusters(service(s12)),
+							},
+						),
+					),
+				},
+			),
 		},
 		"insert proxy with tcp forward without TLS termination w/ passthrough": {
 			objs: []interface{}{

internal/dag/conditions.go

@@ -26,7 +26,7 @@ import (
 // prefix Condition.
 // pathMatchConditionsValid guarantees that if a prefix is present, it will start with a
 // / character, so we can simply concatenate.
-func mergePathMatchConditions(conds []contour_api_v1.MatchCondition) MatchCondition {
+func mergePathMatchConditions(conds []contour_api_v1.MatchCondition) *PrefixMatchCondition {
 	prefix := ""
 	for _, cond := range conds {
 		prefix += cond.Prefix

internal/dag/httpproxy_processor.go

@@ -30,7 +30,6 @@ import (
 	"github.com/projectcontour/contour/internal/k8s"
 	"github.com/projectcontour/contour/internal/status"
 	"github.com/projectcontour/contour/internal/timeout"
-	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -617,14 +616,8 @@ func (p *HTTPProxyProcessor) computeRoutes(
 	visited = append(visited, proxy)
 	var routes []*Route
 
-	// Check for duplicate conditions on the includes
-	if includeMatchConditionsIdentical(proxy.Spec.Includes) {
-		validCond.AddError(contour_api_v1.ConditionTypeIncludeError, "DuplicateMatchConditions",
-			"duplicate conditions defined on an include")
-		return nil
-	}
-
-	// Loop over and process all includes
+	// Loop over and process all includes, including checking for duplicate conditions.
+	seenConds := map[string][][]HeaderMatchCondition{}
 	for _, include := range proxy.Spec.Includes {
 		namespace := include.Namespace
 		if namespace == "" {
@@ -643,6 +636,13 @@ func (p *HTTPProxyProcessor) computeRoutes(
 			continue
 		}
 
+		// Check to see if we have any duplicate include conditions.
+		if includeMatchConditionsIdentical(include, seenConds) {
+			validCond.AddError(contour_api_v1.ConditionTypeIncludeError, "DuplicateMatchConditions",
+				"duplicate conditions defined on an include")
+			continue
+		}
+
 		includedProxy, ok := p.source.httpproxies[types.NamespacedName{Name: include.Name, Namespace: namespace}]
 		if !ok {
 			validCond.AddErrorf(contour_api_v1.ConditionTypeIncludeError, "IncludeNotFound",
@@ -1390,19 +1390,53 @@ func toStringSlice(hvs []contour_api_v1.CORSHeaderValue) []string {
 	return s
 }
 
-func includeMatchConditionsIdentical(includes []contour_api_v1.Include) bool {
-	j := 0
-	for i := 1; i < len(includes); i++ {
-		// Now compare each include's set of conditions
-		for _, cA := range includes[i].Conditions {
-			for _, cB := range includes[j].Conditions {
-				if (cA.Prefix == cB.Prefix) && equality.Semantic.DeepEqual(cA.Header, cB.Header) {
-					return true
-				}
+func includeMatchConditionsIdentical(include contour_api_v1.Include, seenConds map[string][][]HeaderMatchCondition) bool {
+	pathPrefix := mergePathMatchConditions(include.Conditions).Prefix
+
+	includeHeaderConds := mergeHeaderMatchConditions(include.Conditions)
+	sort.SliceStable(includeHeaderConds, func(i, j int) bool {
+		if includeHeaderConds[i].MatchType != includeHeaderConds[j].MatchType {
+			return includeHeaderConds[i].MatchType < includeHeaderConds[j].MatchType
+		}
+		if includeHeaderConds[i].Invert != includeHeaderConds[j].Invert {
+			return includeHeaderConds[i].Invert
+		}
+		if includeHeaderConds[i].Name != includeHeaderConds[j].Name {
+			return includeHeaderConds[i].Name < includeHeaderConds[j].Name
+		}
+		if includeHeaderConds[i].Value != includeHeaderConds[j].Value {
+			return includeHeaderConds[i].Value < includeHeaderConds[j].Value
+		}
+		return false
+	})
+
+	seenHeaderConds, pathSeen := seenConds[pathPrefix]
+	if !pathSeen {
+		seenConds[pathPrefix] = [][]HeaderMatchCondition{
+			includeHeaderConds,
+		}
+		return false
+	}
+
+	for _, headerConds := range seenHeaderConds {
+		if len(headerConds) != len(includeHeaderConds) {
+			seenConds[pathPrefix] = append(seenConds[pathPrefix], includeHeaderConds)
+			continue
+		}
+
+		headerCondsIdentical := true
+		for i := range headerConds {
+			if headerConds[i] != includeHeaderConds[i] {
+				seenConds[pathPrefix] = append(seenConds[pathPrefix], includeHeaderConds)
+				headerCondsIdentical = false
+				break
 			}
 		}
-		j++
+		if headerCondsIdentical {
+			return true
+		}
 	}
+
 	return false
 }