Fix duplicate ACME challenge paths across ingress rules

[linkvt]

Fixes #16258

When a Service has traffic tags, the Route controller was adding ALL ACME challenge paths to EVERY external ingress rule. This caused each challenge path to appear multiple times, leading to routing conflicts.

For example, with tags "blue" and "green", the challenge for blue-app.example.com would incorrectly appear in both the blue and green ingress rules. This happened because when iterating over traffic rules, each ACME challenge was added to EVERY rule, not just the matching one.

This also seems to trigger a bug in kourier causing our kourier-stable tests to fail since quite a while.
Edit: The bug has been fixed in the meanwhile, testgrid is now green for kourier-stable.

Solution

Create a dedicated ingress rule for each ACME challenge domain. Each rule contains only that challenge's path, preventing duplicates.

This approach was chosen because:

• No complex matching logic needed to determine which ACME paths belong to which traffic rule when merging ACME rules into existing traffic rules
• ACME challenge lifecycle is independent of traffic routing and hosts for traffic are often not equal to ACME challenge hosts
• Multiple rules with the same domain but different paths is supported by the Ingress spec so we're not breaking anything (same for route and domain mappings)

The same domain can now appear in both a traffic rule (for application requests) and an ACME rule (for certificate challenges).

Drawback: The list of rules is not as pretty anymore as when the default route of a public endpoint was grouped with its ACME challenge, but this is AFAIK only cosmetic.

Changes

• Removed logic that added ACME paths to traffic rules
• Added makeACMEChallengeRules() to create one dedicated rule per ACME challenge
• Updated tests to expect separate ACME rules instead of combined paths
• MakeACMEIngressPaths() remains unchanged for backward compatibility with DomainMapping

Before/After Example

Before: Three external traffic rules, each containing ALL three ACME challenge paths (9 total path entries for 3 challenges):

rules:
  # Default external rule - has ALL 3 ACME paths
  - hosts: [tls-test.tls-repro.127.0.0.1.sslip.io, blue-tls-test..., green-tls-test...]
    paths:
      - path: /.well-known/acme-challenge/ylYCkx...  # blue challenge
      - path: /.well-known/acme-challenge/Y_L7A1...  # green challenge
      - path: /.well-known/acme-challenge/IZTj12...  # default challenge
      - splits: [tls-test-00001]  # traffic

  # Blue external rule - has ALL 3 ACME paths again
  - hosts: [blue-tls-test.tls-repro.127.0.0.1.sslip.io, tls-test..., green-tls-test...]
    paths:
      - path: /.well-known/acme-challenge/ylYCkx...  # blue challenge (duplicate!)
      - path: /.well-known/acme-challenge/Y_L7A1...  # green challenge (duplicate!)
      - path: /.well-known/acme-challenge/IZTj12...  # default challenge (duplicate!)
      - splits: [tls-test-00001]  # traffic

  # Green external rule - has ALL 3 ACME paths again
  - hosts: [green-tls-test.tls-repro.127.0.0.1.sslip.io, tls-test..., blue-tls-test...]
    paths:
      - path: /.well-known/acme-challenge/ylYCkx...  # blue challenge (duplicate!)
      - path: /.well-known/acme-challenge/Y_L7A1...  # green challenge (duplicate!)
      - path: /.well-known/acme-challenge/IZTj12...  # default challenge (duplicate!)
      - splits: [tls-test-00001]  # traffic

After: Three external traffic rules without ACME paths, plus three dedicated ACME rules (3 total path entries for 3 challenges):

rules:
  # Default external rule - no ACME paths
  - hosts: [tls-test.tls-repro.127.0.0.1.sslip.io]
    paths:
      - splits: [tls-test-00001]  # traffic only

  # Blue external rule - no ACME paths
  - hosts: [blue-tls-test.tls-repro.127.0.0.1.sslip.io]
    paths:
      - splits: [tls-test-00001]  # traffic only

  # Green external rule - no ACME paths
  - hosts: [green-tls-test.tls-repro.127.0.0.1.sslip.io]
    paths:
      - splits: [tls-test-00001]  # traffic only

  # Dedicated blue ACME rule
  - hosts: [blue-tls-test.tls-repro.127.0.0.1.sslip.io]
    paths:
      - path: /.well-known/acme-challenge/ylYCkx...

  # Dedicated green ACME rule
  - hosts: [green-tls-test.tls-repro.127.0.0.1.sslip.io]
    paths:
      - path: /.well-known/acme-challenge/Y_L7A1...

  # Dedicated default ACME rule
  - hosts: [tls-test.tls-repro.127.0.0.1.sslip.io]
    paths:
      - path: /.well-known/acme-challenge/IZTj12...

Release Note

Fixed duplicate ACME challenge paths when Services with traffic tags use HTTP-01 challenges for TLS certificates.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.11%. Comparing base (b7a8837) to head (d6a585e).
⚠️ Report is 31 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16259      +/-   ##
==========================================
+ Coverage   80.05%   80.11%   +0.05%     
==========================================
  Files         215      215              
  Lines       13327    13331       +4     
==========================================
+ Hits        10669    10680      +11     
+ Misses       2299     2293       -6     
+ Partials      359      358       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[linkvt]

/retest
/kind bug

[dprotaso]

/test all

[dprotaso]

/test https

[dprotaso]

Changes look good.

/lgtm
/approve

/hold Running https tests

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, linkvt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dprotaso]

/test istio-latest-mesh-tls
/test kourier-stable-tls
/test contour-tls

[linkvt]

/retest

[dprotaso]

Ok we have HTTP01 tests but it's not running now

https://github.com/knative/serving/blob/ce76741c2d665829503dc623fdb3f46b59000c44/test/e2e-external-domain-tls-tests.sh#L200C9-L200C45

Looks like it runs on the periodic test? I'm guessing because running it for every PR is too much

https://github.com/knative/infra/blob/8046977ff7a6243ee088176926902be667b77fb6/prow/jobs_config/knative/serving.yaml#L141C74-L141C110

Let's merge this and @linkvt can you track those periodic tests for failures

/hold cancel

[linkvt]

Interesting, the tests I fixed with related PRs in the net-kourier repo a few days ago are failing again: https://testgrid.k8s.io/r/knative-own-testgrid/serving#kourier-stable

I will wait for another failure before looking into it.

Edit: Never mind, was able to identify the issue in net-kourier. envoy uses the first matching path, if / is before the /.well-known/... ACME stuff it's always ignored.
net-kourier PR will be done in a few minutes and appear below as a reference.

[linkvt]

There is now another additional test failure 🥲
Will look into it next week, still confident that all PRs were fixing actual issues which I could reproduce locally, but I guess there is always more...

Seems like contour also has a problem: https://testgrid.k8s.io/r/knative-own-testgrid/serving#contour-latest

Edit: kourier wasn't updated yet so my fix wasn't used in the test, need to check kourier again after: #16299