Fall back to http1 on failed http2 probe #16205
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16205 +/- ##
==========================================
+ Coverage 80.05% 80.25% +0.20%
==========================================
Files 215 215
Lines 13320 13303 -17
==========================================
+ Hits 10663 10676 +13
+ Misses 2295 2265 -30
Partials 362 362 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
386f8c6 to
9e6b6fa
Compare
0c5f9df to
c52f12c
Compare
|
I think we should remove the queue proxy image annotation change. I don't think that's something that we want users being able to manipulate given that's an operator concern. |
Is there a way to do this over multiple releases? |
I understand the reasoning but don't think that we're currently that consistent about that. Besides that it's quite hard to debug issues in queue-proxy as we directly affect all KServices, also considering the following recent comment of you where a service-local annotation would allow minimally invasive debugging: #16043 (comment) . This would maybe simplify debugging similar issues in production environments without affecting any other production workload. Let me know what you prefer, discard, keep, separate PR, ...
I think so but at first I would need to make it work at all with the std library implementation. Let me know if we want to work on this, I can create an issue for a PoC to migrate to the stdlib implementation in the activator and the queue-proxy.
I don't think that we should continue sending these old HTTP2 Upgrade headers as they are already deprecated since more than 3 years as per RFC9113. As the new code is using a compliant way to establish an h2c connection the Besides that, thanks for the review! Edit: did a rebase due to conflict in go.mod |
c52f12c to
ed40414
Compare
ed40414 to
483f15b
Compare
| } | ||
| return resp, err | ||
| // Set the protocol hint for the auto-selecting prober transport | ||
| r.ProtoMajor = protoMajor |
There was a problem hiding this comment.
Does this work? The godoc says:
// For client requests, these fields are ignored.
There was a problem hiding this comment.
ok - i see we inspect this here: https://github.com/knative/pkg/blob/e853b1d1d6bbd46b172b2a3b3b301c0854061408/network/transports.go#L42
There was a problem hiding this comment.
Agree that all of this is a bit confusing, but in this case I'm just preserving the existing behavior from old line 101.
483f15b to
cfd8e81
Compare
|
/retest |
|
FYI - I was playing with using the new go1.24 features of not needing the h2c package. See: knative/pkg#3298 Though when I test it out it's failing on Kourier and I could use some help there - #16280 |
|
@linkvt can we drop the queue proxy annotations from this PR |
cfd8e81 to
f95df2c
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: linkvt The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@dprotaso sure, done 👍 |
|
PR needs rebase. DetailsInstructions 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-sigs/prow repository. |
Fixes #15432
Fixes #10962
Release Note
Summary
Improves queue-proxy HTTP/2 probing reliability by switching from upgrade-based detection to direct H2C probes with fallback to HTTP/1.1.
Changes
Additional Change (Could be Separate PR)
Also includes support for overriding the queue-proxy image via the
queue.sidecar.serving.knative.dev/imageannotation on KService specs.I found this very useful during my tests, as...
ko applya single fileReplacement of
golang.org/x/net/http2with stdlibI looked into the replacement of
golang.org/x/net/http2(as h2c and http2 support exists in stdlib since 1.24) andgolang.org/x/net/http2/h2c(proposal for deprecation exists) in pkg but didn't include it in this PR as it was unexpectedly a huge topic.Switching to stdlib is not as easy, as the http2.Client sets up the http2 connection in a non standard way sending an HTTP2 Preface despite using the TLS connection:
This means that during a knative upgrade pod updates of queue-proxy before the activator would cause issues, as activator would send the preface the go stdlib http2 implementation in queue-proxy does not handle. We might be able to ignore such requests but I didn't test it yet.
The second task would be to setup H2 connections the standard way: via TLS ALPN.
But: how do we know in queue-proxy and the activator whether we actually want to use HTTP2?
The queue-proxy has currently no knowledge (besides it using the HTTP2 port 8013) and could rely on the probe to the user-container to figure this out and only afterwards accept HTTP2 connections.
We could derive this info during the
Revisionreconciliation but that would oppose removing the port naming restriction (see #4283).The activator could potentially add the h2 protocol in the transport so that proxy connections using it would attempt h2 .
But how does queue-proxy behave? Always accept h2 (defined in the TLS Config of the server) without knowing whether the service supports h2?
Then there is also h2c (http2 cleartext) - how are things negotiated in this case?
I have more questions than answers right now but fortunately a solution of that isn't required to fix the issue referenced above.
Thanks for the feedback!