fix: auto detect upstream protocol by zirain · Pull Request #6792 · envoyproxy/gateway

zirain · 2025-08-15T04:57:56Z

This PR changed the default behavior when you enabled tls on EnvoyProxy but without alpn.
Before:

typedExtensionProtocolOptions unset, which means http1.

After:

AutoHttpConfig enabled, which means:

// Clusters configured with “AutoHttpConfig“ will use the highest available
// protocol; HTTP/2 if supported, otherwise HTTP/1.

codecov · 2025-08-15T05:06:44Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.05%. Comparing base (efc3d2c) to head (2938f7e).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6792      +/-   ##
==========================================
+ Coverage   71.02%   71.05%   +0.02%     
==========================================
  Files         227      227              
  Lines       40331    40350      +19     
==========================================
+ Hits        28645    28670      +25     
+ Misses       9992     9987       -5     
+ Partials     1694     1693       -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.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

internal/xds/translator/cluster.go

arkodg · 2025-09-19T22:15:43Z

hey @zirain can you also update the doc string in

gateway/api/v1alpha1/tls_types.go

Line 83 in 5ebc004

// +optional

that defaults to [http/1.1, h2]

and add a breaking changes note in the release notes

api/v1alpha1/tls_types.go

arkodg · 2025-09-22T22:39:43Z

test/e2e/tests/backend_tls_settings.go

-					MaxVersion: ptr.To(egv1a1.TLSv13),
+					MinVersion:    ptr.To(egv1a1.TLSv13),
+					MaxVersion:    ptr.To(egv1a1.TLSv13),
+					ALPNProtocols: []egv1a1.ALPNProtocol{"http/1.1"},


can we verify, http/1.1 is being used here ?

%UPSTREAM_PROTOCOL% will help here, need more work.

it's tested here:

gateway/test/e2e/testdata/expect/echo-service-tls-settings-res.yaml

Line 4 in a685667

negotiatedProtocol: "http/1.1"

arkodg · 2025-09-22T22:43:05Z

internal/xds/translator/cluster.go

 	for i, ds := range args.settings {
 		if ds.TLS != nil {
+			if !requiresAutoHTTPConfig && len(ds.TLS.ALPNProtocols) == 0 {


this means, if ALPN is set to h2, it won't actually connect over h2 (without setting applicationProtocol in the backend), is that right ?

if alpnProtocols = [h2], requiresAutoHTTPConfig is false, it should be same as the old logic which means use h2.
what I missed?

gateway/internal/gatewayapi/testdata/clienttrafficpolicy-for-tcp-listeners.out.yaml

Lines 253 to 254 in 5ebc004

alpnProtocols:

- h2

imo we should use this for the default case when tls is non nil

Priority - Use Client Protocol - Force H2 using Backend App Protocol or GRPCRoute ( isHTTP2) - TLS != nil (AutoTLS) - Default ( H1)

arkodg · 2025-09-23T16:56:40Z

internal/xds/translator/cluster.go

 				},
 			},
 		}
+	case requiresAutoHTTPConfig:


should this be above requiresHTTP1Options ?

Also above http2 options, if that's the case, right?

I don't think it should be above http2 options because of the priority is as following:

Priority - Use Client Protocol - Force H2 using Backend App Protocol or GRPCRoute ( isHTTP2) - TLS != nil (AutoTLS) - Default ( H1)

@arkodg previous priority didn't point out the case of explicit http1 settings.

requiresHTTP1Options := args.http1Settings != nil && (args.http1Settings.EnableTrailers || args.http1Settings.PreserveHeaderCase || args.http1Settings.HTTP10 != nil)

yeah, I think this field should be renamed to hasHTTP1Options , and Auto tls should take precedence over it (it will also consume those http1 values)

this PR won't be backported, let's discuss it in an seperated PR?

right now if a user sets http1options and http2options in BTP for all routes, they cannot use auto TLS to decide the right proto to use per backend, which is why im recommending moving case requiresAutoHTTPConfig above case requiresHTTP1Options

In that case, we might need more work about merge those options into auto.
That's why I proposal to do it in another PR, and also discuss guydc's comment.

sure follow up PR works

api/v1alpha1/tls_types.go

internal/xds/translator/cluster.go

guydc · 2025-09-23T22:02:55Z

internal/xds/translator/cluster.go

 				},
 			},
 		}
+	case requiresAutoHTTPConfig:


Also above http2 options, if that's the case, right?

Signed-off-by: zirain <zirain2009@gmail.com>

guydc · 2025-09-24T12:58:29Z

In another issue, we should clarify how this field works. For example, what happens when:

Using grpc route/h2 proto but setting backend ALPN to [http/1.1] ? Would it break? would it be ignored?
Using preserve client proto but setting backend ALPN to [not-a-real-proto]
etc.

I don't think that we can perform any sort of validation in this case to stop users from configuring bad values, since this value has meaning in the context of a specific backend, e.g. proxy has backends that grpc while others require auto and others preservation.

zirain requested a review from a team as a code owner August 15, 2025 04:57

arkodg reviewed Aug 15, 2025

View reviewed changes

internal/xds/translator/cluster.go Show resolved Hide resolved

guydc reviewed Aug 19, 2025

View reviewed changes

internal/xds/translator/cluster.go Outdated Show resolved Hide resolved

zirain force-pushed the alpn/h2 branch 2 times, most recently from 04c3d7c to 51bf619 Compare August 24, 2025 16:27

zirain requested review from arkodg and guydc August 26, 2025 12:36

arkodg reviewed Sep 19, 2025

View reviewed changes

internal/xds/translator/cluster.go Outdated Show resolved Hide resolved

arkodg reviewed Sep 19, 2025

View reviewed changes

internal/xds/translator/cluster.go Show resolved Hide resolved

zirain force-pushed the alpn/h2 branch from 51bf619 to 0272ac7 Compare September 20, 2025 01:21

zirain requested a review from arkodg September 20, 2025 02:40

arkodg reviewed Sep 22, 2025

View reviewed changes

api/v1alpha1/tls_types.go Outdated Show resolved Hide resolved

arkodg reviewed Sep 22, 2025

View reviewed changes

zirain force-pushed the alpn/h2 branch from b162d73 to 2e38c0b Compare September 23, 2025 02:52

arkodg reviewed Sep 23, 2025

View reviewed changes

guydc reviewed Sep 23, 2025

View reviewed changes

zirain added 8 commits September 24, 2025 06:37

alpn auto

ee9578b

Signed-off-by: zirain <zirain2009@gmail.com>

fix and add test

41b5a26

Signed-off-by: zirain <zirain2009@gmail.com>

address review comment

72ca7d1

Signed-off-by: zirain <zirain2009@gmail.com>

release notes

6ef79ab

Signed-off-by: zirain <zirain2009@gmail.com>

fix gen

2cbcc9e

Signed-off-by: zirain <zirain2009@gmail.com>

update crd

dc953ae

Signed-off-by: zirain <zirain2009@gmail.com>

fix

a21d5fb

Signed-off-by: zirain <zirain2009@gmail.com>

address guyc's comment

4869bd4

Signed-off-by: zirain <zirain2009@gmail.com>

zirain force-pushed the alpn/h2 branch from 2e38c0b to 4869bd4 Compare September 23, 2025 22:38

fix gen

2938f7e

Signed-off-by: zirain <zirain2009@gmail.com>

guydc approved these changes Sep 24, 2025

View reviewed changes

arkodg approved these changes Sep 25, 2025

View reviewed changes

arkodg merged commit e2abd7b into envoyproxy:main Sep 25, 2025
35 checks passed

zirain deleted the alpn/h2 branch September 25, 2025 00:41

zirain mentioned this pull request Sep 25, 2025

follow up discussion for auto detect upstream protocol #7057

Closed

arkodg mentioned this pull request Nov 5, 2025

BackendTrafficPolicy with ProxyProtocolV2 enabled with TLS Backend now fails to configure the envoy cluster. #7438

Closed

zirain mentioned this pull request Nov 6, 2025

fix auto http config with proxy protocol #7439

Merged

Inode1 mentioned this pull request Jan 11, 2026

Auto-detect upstream protocol breaks with multiple backends (HTTP + HTTPS) #7917

Closed

arkodg mentioned this pull request Jan 17, 2026

fix: do not set autoHTTPConfig when used mixed(HTTP + HTTPS) backends #7950

Merged

Conversation

zirain commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arkodg commented Sep 19, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guydc commented Sep 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zirain commented Aug 15, 2025 •

edited

Loading

codecov bot commented Aug 15, 2025 •

edited

Loading

arkodg Sep 22, 2025 •

edited

Loading