-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fall back to http1 on failed http2 probe #16205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fall back to http1 on failed http2 probe #16205
Conversation
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16205 +/- ##
==========================================
+ Coverage 80.05% 80.18% +0.12%
==========================================
Files 214 214
Lines 13281 13275 -6
==========================================
+ Hits 10632 10644 +12
+ Misses 2291 2271 -20
- Partials 358 360 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
386f8c6 to
9e6b6fa
Compare
|
|
||
| // Returns a transport that uses HTTP/2 if it's known to be supported, and otherwise | ||
| // spoofs the request & response versions to HTTP/1.1. | ||
| func autoDowngradingTransport(opt HTTPProbeConfigOptions) http.RoundTripper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not come up with a reason for why this autoDowngradingTransport would be needed at all, maybe it was required some time ago but doesn't seem to be the case today.
| t := http.DefaultTransport.(*http.Transport).Clone() | ||
| t.TLSClientConfig.InsecureSkipVerify = true | ||
| return t | ||
| }() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This transport was only used during HTTP2 detection, as we dont support https/h2 probes IMO not really needed.
9e6b6fa to
0c5f9df
Compare
0c5f9df to
c52f12c
Compare
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!