Fall back to http1 on failed http2 probe

[linkvt]

Fixes #15432 Fixes #10962

Release Note

Fall back to HTTP1 on failed HTTP2 health probes (e.g. on connection error or non-readiness)

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

• HTTP/2 probe fallback logic: Replaces the deprecated HTTP upgrade mechanism (OPTIONS with Connection: Upgrade, HTTP2-Settings) with direct H2C GET requests. Falls back to HTTP/1.1 if H2C probe fails or returns non-ready status
• Simplified transport handling: Removes version-spoofing transport wrapper in favor of protocol hints via req.ProtoMajor
• Test updates: Rewrites hellohttp2 test service to use standard library HTTP/2 server

Additional Change (Could be Separate PR)

Also includes support for overriding the queue-proxy image via the queue.sidecar.serving.knative.dev/image annotation on KService specs. I found this very useful during my tests, as...

• I could ko apply a single file
• get the update of both service and queue-pro in one revision
• could compare the behavior of different kservices with different queue-proxy images (relevant for the next section)

Replacement of golang.org/x/net/http2 with stdlib

I looked into the replacement of golang.org/x/net/http2 (as h2c and http2 support exists in stdlib since 1.24) and golang.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:

A client that knows that a server supports HTTP/2 can establish a TCP connection and send the connection preface (Section 3.4) followed by HTTP/2 frames. Servers can identify these connections by the presence of the connection preface. This only affects the establishment of HTTP/2 connections over cleartext TCP; HTTP/2 connections over TLS MUST use protocol negotiation in TLS [TLS-ALPN].

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 Revision reconciliation 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!

[codecov[bot]]

Codecov Report

:x: Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 80.25%. Comparing base (06034f2) to head (f95df2c).

Files with missing lines	Patch %	Lines
pkg/queue/health/probe.go	89.47%	2 Missing :warning:

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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

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

[dprotaso]

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.

[dprotaso]

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.

Is there a way to do this over multiple releases?

[dprotaso]

I think we should scope this to just the queue proxy => user container as per this issue https://github.com/knative/serving/issues/15432

and for now ignore activator => queue proxy interaction given that we control both ends of that hop. Thus ignore issue https://github.com/knative/serving/issues/10962

[linkvt]

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.

I understand the reasoning but don't think that we're currently that consistent about that. We're currently able to set e.g. the ingress class or the certificate class through an annotation.

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: https://github.com/knative/serving/issues/16043#issuecomment-3237893462 . 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, ...

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.

Is there a way to do this over multiple releases?

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 think we should scope this to just the queue proxy => user container as per this issue #15432

and for now ignore activator => queue proxy interaction given that we control both ends of that hop. Thus ignore issue #10962

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 Upgrade stuff is not needed anymore which means that we also don't have to fix #10962 anymore. You mention ignore activator => queue proxy interaction but the HTTP2 Upgrade stuff here is only about the queue proxy probe, there is no relation to the activator I think or am I missing something?

Besides that, thanks for the review!

Edit: did a rebase due to conflict in go.mod

[linkvt]

/retest

[dprotaso]

FYI - I was playing with using the new go1.24 features of not needing the h2c package. See: https://github.com/knative/pkg/pull/3298

Though when I test it out it's failing on Kourier and I could use some help there - https://github.com/knative/serving/pull/16280

[dprotaso]

@linkvt can we drop the queue proxy annotations from this PR

[knative-prow[bot]]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: linkvt Once this PR has been reviewed and has the lgtm label, please assign dsimansk for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[linkvt]

@dprotaso sure, done 👍