HTTPProxy include validation recently became too tight

[ecordell]

What steps did you take and what happened:
Prior to #4931, this worked:

apiVersion: projectcontour.io/v1
kind: HTTPProxy
metadata:
  name: frontdoor
  namespace: example
spec:
  includes:
  - name: serviceA
    namespace: example
  - name: serviceB
    namespace: example
  virtualhost:
    fqdn: frontdoor.example.com
---
apiVersion: projectcontour.io/v1
kind: HTTPProxy
  name: serviceA
  namespace: example
spec:
  routes:
  - conditions:
    - prefix: /graphql
    services:
    - name: serviceA
      port: 5000
---
apiVersion: projectcontour.io/v1
kind: HTTPProxy
metadata:
  name: serviceB
  namespace: example
spec:
  routes:
  - conditions:
    - prefix: ""
    services:
    - name: serviceB
      port: 3000

But after the recent change, the top-level HTTPProxy now reports:

  conditions:
  - errors:
    - message: duplicate conditions defined on an include
      reason: DuplicateMatchConditions
      status: "True"
      type: IncludeError
    lastTransitionTime: "2023-01-26T12:09:47Z"
    message: At least one error present, see Errors for details
    observedGeneration: 1
    reason: ErrorPresent
    status: "False"
    type: Valid

Technically, the includes do have equal match conditions (nothing) - but they resolve to different fully resolved paths once the delegated HTTPProxy's paths is considered.

What did you expect to happen:

I expected this to be a valid config - from previous versions of contour, I know that this configuration used to create the envoy configuration that I expected (both services were routed properly). It seems like it reduces the value of the includes feature if I have to replicate the paths on the top-level object.

Environment:

• Contour version: v1.24.0-rc.1

[github-actions]

Hey @ecordell! Thanks for opening your first issue. We appreciate your contribution and welcome you to our community! We are glad to have you here and to have your input on Contour. You can also join us on our mailing list and in our channel in the Kubernetes Slack Workspace

[sunjayBhatia]

hm, I would argue the previous behavior was not intended and the logic for checking duplicate includes was incorrect so that HTTPProxy should never have been valid

but given this has been around for a long time, people may be relying on it, and we don't want to suddenly break users with currently working config, we can think about how we might rework this change

[ecordell]

Would it make more sense to validate the "merged" conditions for each route, rather than the conditions on the includes?

i.e. in this case it would verify: / and /graphql (and pass because they're different paths)

[sunjayBhatia]

that may work, was looking at detecting the duplicate includes/routes on only the leaf Routes, however @skriss brought up a good point while we were chatting that the UX for how to report duplicates is now much more complicated for owners of child proxies:

if there is a duplicate match condition through the include tree + routes, where does the status go to report this to the user?

If there isn't actually an include error on the parent, does it really make sense to put it there? how would the owner of the child route get to know the error is there?

if it goes on the leaf HTTPProxy, the owner of that included resource may not know the layout of the include tree and not be able to fix the issue

with the existing change, any conflicts are headed off at the root which makes it easier

[sunjayBhatia]

current plan is to go with a bit of a stop-gap solution, see here: #5017

the next contour release after 1.24 should have more correct validation in general, we will submit a design for that so the community can review

[sunjayBhatia]

@ecordell take a look at the latest main image once it is pushed, to see if it works for your uses, which we think are possibly a common one (includes w/o any conditions)

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all Issues.

This bot triages Issues according to the following rules:

• After 60d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the Issue is closed

You can:

• Mark this Issue as fresh by commenting
• Close this Issue
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all Issues.

This bot triages Issues according to the following rules:

• After 60d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the Issue is closed

You can:

• Mark this Issue as fresh by commenting
• Close this Issue
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack