internal/dag: Loosen restrictions on duplicate include detection

changelogs/unreleased/4931-sunjayBhatia-major.md

@@ -0,0 +1,71 @@
+## 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`
+
+### Caveat for empty or individual prefix path matches on `/`
+
+A caveat to the above, is that an empty list of include conditions or a set of conditions that only consist of the prefix match on `/` are not treated as duplicates.
+
+This special case has been added because many users rely on the behavior this enables and many Contour examples demonstrating inclusion actually use it.
+For example:
+
+```
+---
+apiVersion: projectcontour.io/v1
+kind: HTTPProxy
+metadata:
+  name: example
+spec:
+  virtualhost:
+    fqdn: foo-example.bar.com
+  includes:
+  - name: example-child1
+  - name: example-child2
+---
+apiVersion: projectcontour.io/v1
+kind: HTTPProxy
+metadata:
+  name: example-child1
+spec:
+  routes:
+    - conditions:
+      - prefix: /
+      services:
+      - name: s1
+        port: 80
+---
+apiVersion: projectcontour.io/v1
+kind: HTTPProxy
+metadata:
+  name: example-child2
+spec:
+  routes:
+  - conditions:
+    - prefix: /foo
+    services:
+    - name: s2
+      port: 80
+```
+
+While the include conditions are equivalent, the resulting routing rules when the child routes are taken into account are distinct.
+
+This special casing is a stop-gap for this release, to ensure we do not break user's configuration that is currently valid and working.
+
+### Future changes to inclusion and route duplicate detection
+
+Currently duplicate route conditions are not checked in an HTTPProxy include tree or within an individual HTTPProxy.
+This means that you can have routes listed later in the list of routes on an HTTPProxy silently override others.
+The same can happen if you have an include tree that generates duplicate routes based on the include conditions and route conditions.
+
+If you are relying on this behavior, changes will be coming in the next Contour release.
+
+We will be submitting a design document to address this as it will be a significant behavior change and encourage the community to weigh in.
+The current plan is to fully validate duplicate route match conditions as they are generated from the tree of includes and routes.
+There will likely be changes to status conditions set on HTTPRoutes to improve reporting such invalid configuration.

internal/dag/httpproxy_processor.go

@@ -1392,8 +1392,19 @@ func toStringSlice(hvs []contour_api_v1.CORSHeaderValue) []string {
 
 func includeMatchConditionsIdentical(include contour_api_v1.Include, seenConds map[string][][]HeaderMatchCondition) bool {
 	pathPrefix := mergePathMatchConditions(include.Conditions).Prefix
-
 	includeHeaderConds := mergeHeaderMatchConditions(include.Conditions)
+
+	// Note: this is stop-gap that we intend to change.
+	// This means that an empty set of conditions or a lone path prefix match on "/"
+	// are not considered duplicates. Previously, validation of
+	// includes was implemented in such a way that users could be relying on this
+	// behavior to set up their include tree.
+	// It is unlikely that there is much usage of duplicate non-default include
+	// conditions, so we think this special case is safe.
+	if pathPrefix == "/" && len(includeHeaderConds) == 0 {
+		return false
+	}
+
 	sort.SliceStable(includeHeaderConds, func(i, j int) bool {
 		if includeHeaderConds[i].MatchType != includeHeaderConds[j].MatchType {
 			return includeHeaderConds[i].MatchType < includeHeaderConds[j].MatchType

internal/dag/status_test.go

@@ -1602,6 +1602,79 @@ func TestDAGStatus(t *testing.T) {
 		},
 	})
 
+	proxyIncludeConditionsEmpty := &contour_api_v1.HTTPProxy{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "roots",
+			Name:      "example",
+		},
+		Spec: contour_api_v1.HTTPProxySpec{
+			VirtualHost: &contour_api_v1.VirtualHost{
+				Fqdn: "example.com",
+			},
+			Includes: []contour_api_v1.Include{{
+				Name:      "blogteama",
+				Namespace: "teama",
+			}, {
+				Name:      "blogteamb",
+				Namespace: "teamb",
+			}},
+		},
+	}
+
+	run(t, "empty include conditions", testcase{
+		objs: []interface{}{proxyIncludeConditionsEmpty, proxyValidBlogTeamA, proxyValidBlogTeamB, fixture.ServiceTeamAKuard, fixture.ServiceTeamBKuard},
+		want: map[types.NamespacedName]contour_api_v1.DetailedCondition{
+			{Name: proxyValidBlogTeamA.Name, Namespace: proxyValidBlogTeamA.Namespace}: fixture.NewValidCondition().
+				Valid(),
+			{Name: proxyValidBlogTeamB.Name, Namespace: proxyValidBlogTeamB.Namespace}: fixture.NewValidCondition().
+				Valid(),
+			{Name: proxyIncludeConditionsEmpty.Name,
+				Namespace: proxyIncludeConditionsEmpty.Namespace}: fixture.NewValidCondition().
+				Valid(),
+		},
+	})
+
+	proxyIncludeConditionsPrefixRoot := &contour_api_v1.HTTPProxy{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "roots",
+			Name:      "example",
+		},
+		Spec: contour_api_v1.HTTPProxySpec{
+			VirtualHost: &contour_api_v1.VirtualHost{
+				Fqdn: "example.com",
+			},
+			Includes: []contour_api_v1.Include{{
+				Name:      "blogteama",
+				Namespace: "teama",
+				Conditions: []contour_api_v1.MatchCondition{{
+					Prefix: "/",
+				}},
+			}, {
+				Name:      "blogteamb",
+				Namespace: "teamb",
+			}, {
+				Name:      "blogteamb",
+				Namespace: "teamb",
+				Conditions: []contour_api_v1.MatchCondition{{
+					Prefix: "/",
+				}},
+			}},
+		},
+	}
+
+	run(t, "multiple prefix / include conditions", testcase{
+		objs: []interface{}{proxyIncludeConditionsPrefixRoot, proxyValidBlogTeamA, proxyValidBlogTeamB, fixture.ServiceTeamAKuard, fixture.ServiceTeamBKuard},
+		want: map[types.NamespacedName]contour_api_v1.DetailedCondition{
+			{Name: proxyValidBlogTeamA.Name, Namespace: proxyValidBlogTeamA.Namespace}: fixture.NewValidCondition().
+				Valid(),
+			{Name: proxyValidBlogTeamB.Name, Namespace: proxyValidBlogTeamB.Namespace}: fixture.NewValidCondition().
+				Valid(),
+			{Name: proxyIncludeConditionsPrefixRoot.Name,
+				Namespace: proxyIncludeConditionsPrefixRoot.Namespace}: fixture.NewValidCondition().
+				Valid(),
+		},
+	})
+
 	proxyInvalidConflictingIncludeConditions := &contour_api_v1.HTTPProxy{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: "roots",