Fix DomainMappings when internal-encryption is enabled

[KauzClay]

Changes

• 🐛 Fix bug
• Set proto to h2c when ports named http2 use port 80, even when internal encryption is on.
• the previous fix in Internal Encryption fixes #860 expected the K-Original-Host header to be present in the appendheaders section. However, it turns out that isn't added to the endpoint probe. This means that the ep would use h2 on port 80 when it was for DomainMappings.
• Instead of adding the k-original-host header to the endpoint probes, or only checking for the rewriteHost field, I think the solution I'm proposing in the PR is simpler, and gets at the root of the problem: traffic to port 80 shouldn't try to be encrypted.
• See this discussion for more context: Internal Encryption fixes #860 (comment)

/kind bug

Fixes #862

Release Note

Fixes issue where DomainMappings do not become ready when Internal Encryption is enabled.

Docs

N/A

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KauzClay

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [KauzClay]

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

[codecov]

Codecov Report

Merging #878 (340ab10) into main (60d558f) will increase coverage by 0.15%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #878      +/-   ##
==========================================
+ Coverage   93.89%   94.04%   +0.15%     
==========================================
  Files           7        7              
  Lines         802      789      -13     
==========================================
- Hits          753      742      -11     
+ Misses         28       27       -1     
+ Partials       21       20       -1     

Impacted Files	Coverage Δ
pkg/reconciler/contour/contour.go	84.61% <0.00%> (-1.71%)	⬇️
pkg/reconciler/contour/resources/httpproxy.go	97.82% <100.00%> (+2.35%)	⬆️

[KauzClay]

/assign @dprotaso

[dprotaso]

traffic to port 80 shouldn't try to be encrypted.

Right now the queue proxy will spin up two http servers (TLS & non-TLS) - but long term I don't think we should continue to spin up the non-TLS server when internal encryption is on.

So I wonder if the right thing to do here is to make sure we start probing the TLS ports as a solution vs. continuing to probe on an unencrypted port 80

[KauzClay]

traffic to port 80 shouldn't try to be encrypted.

Right now the queue proxy will spin up two http servers (TLS & non-TLS) - but long term I don't think we should continue to spin up the non-TLS server when internal encryption is on.

So I wonder if the right thing to do here is to make sure we start probing the TLS ports as a solution vs. continuing to probe on an unencrypted port 80

@dprotaso Maybe I misunderstood when I was poking at it, but the trouble I've run into is that when using a domain mapping, the service that gets probed is that top level service that points to the envoy. So if we update that service to have a 443 entry, then we also need the envoy to listen on 443. And then that starts creeping into the territory of adding TLS for internal routes.

My cluster is all messed up right now, but once I fix it I'll try to get a better example of what I'm trying to say

All that to say, I didn't know how to make a change do the right thing without spilling into a substantial change. And my goal for this PR was just to get the current broken implementation into a workable state.

[dprotaso]

The service that gets probed is that top level service that points to the envoy.
...
So if we update that service to have a 443 entry, then we also need the envoy to listen on 443. And then that starts creeping into the territory of adding TLS for internal routes.

When internal TLS is turned on everything goes through the activator so I believe that would be the target of the probing. I believe TLS for internal routes should 'just work'

And my goal for this PR was just to get the current broken implementation into a workable state.

We do want all the hops encrypted though so that would included traffic from external proxy => internal proxy. I'm flexble on the path we take to get there so let me know what you think.

[dprotaso]

Sorry the target of the probing = what responds to the probe

…

On Tue, Mar 14, 2023 at 19:21 Dave Protasowski ***@***.***> wrote: The service that gets probed is that top level service that points to the envoy. ... So if we update that service to have a 443 entry, then we also need the envoy to listen on 443. And then that starts creeping into the territory of adding TLS for internal routes. When internal TLS is turned on everything goes through the activator so I believe that would be the target of the probing. I believe TLS for internal routes should 'just work' And my goal for this PR was just to get the current broken implementation into a workable state. We do want all the hops encrypted though so that would included traffic from external proxy => internal proxy. I'm flexble on the path we take to get there so let me know what you think. — Reply to this email directly, view it on GitHub <#878 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAERARMZLR7LSJVS2OVLPTW4D4PTANCNFSM6AAAAAAVF5DOD4> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[KauzClay]

kinda left this one hanging, but, I think the work to add https to clusterLocal routes here (knative-extensions/net-certmanager#538) might help in this scenario.

As that moves forward, I assume there will be net-* changes required too. I'll see if I can do the net-contour one, and maybe that will help move this one along.

[knative-prow-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[github-actions]

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.