From 3315fa6e0f74707a765593538c96d3611746e41b Mon Sep 17 00:00:00 2001 From: Miguel da Costa Martins Marcelino Date: Thu, 18 Jun 2026 10:56:53 +0000 Subject: [PATCH] TUN-10630: Fix precheck protocol override As it stands, cloudflared prechecks are not taking the `protocol` flag into consideration and is instead falling back to the default protocol, which is QUIC. Prechecks should report the protocol cloudflared will use, not the default protocol. --- cmd/cloudflared/tunnel/cmd.go | 7 +- prechecks/checker.go | 50 +++++++++++- prechecks/checker_test.go | 141 ++++++++++++++++++++++++++++++++++ prechecks/types.go | 9 +++ 4 files changed, 200 insertions(+), 7 deletions(-) diff --git a/cmd/cloudflared/tunnel/cmd.go b/cmd/cloudflared/tunnel/cmd.go index 9a1b47cf..f44922e4 100644 --- a/cmd/cloudflared/tunnel/cmd.go +++ b/cmd/cloudflared/tunnel/cmd.go @@ -537,9 +537,10 @@ func runPrechecks(c *cli.Context, log *zerolog.Logger, region string) { } cfg := prechecks.Config{ - Region: region, - IPVersion: ipVersion, - EdgeAddrs: c.StringSlice(cfdflags.Edge), + Region: region, + IPVersion: ipVersion, + EdgeAddrs: c.StringSlice(cfdflags.Edge), + ProtocolOverride: c.String(cfdflags.Protocol), } dialers := prechecks.RunDialers{ diff --git a/prechecks/checker.go b/prechecks/checker.go index 8d98f61f..58748f55 100644 --- a/prechecks/checker.go +++ b/prechecks/checker.go @@ -156,7 +156,7 @@ func Run(ctx context.Context, caCert string, cfg Config, log *zerolog.Logger, ru return Report{ RunID: runID, Results: append(dnsResults, results.Collect()...), - SuggestedProtocol: suggestProtocol(results.QUIC, results.HTTP2), + SuggestedProtocol: suggestProtocol(results.QUIC, results.HTTP2, cfg.ProtocolOverride), } } @@ -303,10 +303,52 @@ func severity(s Status) int { } } -// suggestProtocol recommends QUIC when all QUIC region probes passed, HTTP/2 -// when all HTTP/2 probes passed, and nil when neither transport works. +// parseProtocolOverride converts the raw --protocol flag string into a +// *connection.Protocol. It returns nil when the string is empty, "auto", or +// unrecognised, so the probe heuristic is used in those cases. "h2mux" is +// treated as HTTP/2 because both map to the same transport. +func parseProtocolOverride(flag string) *connection.Protocol { + switch flag { + case connection.QUIC.String(): + p := connection.QUIC + return &p + case connection.HTTP2.String(), "h2mux": + p := connection.HTTP2 + return &p + default: + // "auto", empty, or unknown — no override; let the heuristic decide. + return nil + } +} + +// suggestProtocol determines the protocol to report in the pre-check summary. +// +// When the caller has explicitly overridden the protocol via --protocol, that +// choice is honoured when its transport probes produced evidence and did not +// fail. +// +// When there is no override (auto-selection), precedence is QUIC, HTTP/2, +// and nil. A protocol is only suggested if all probes pass. +// // Any region failing means the transport is treated as failed (worst wins). -func suggestProtocol(quicResults, http2Results []CheckResult) *connection.Protocol { +func suggestProtocol(quicResults, http2Results []CheckResult, overrideFlag string) *connection.Protocol { + if override := parseProtocolOverride(overrideFlag); override != nil { + switch *override { + case connection.QUIC: + // Only report QUIC as the suggested protocol if its probes did not + // all fail — if they did, fall through to the heuristic so the + // summary can report a usable fallback or nil. + if len(quicResults) > 0 && worstStatus(quicResults) != Fail { + return new(connection.QUIC) + } + case connection.HTTP2: + // Same logic for an explicit HTTP/2 override. + if len(http2Results) > 0 && worstStatus(http2Results) != Fail { + return new(connection.HTTP2) + } + } + } + if len(quicResults) > 0 && worstStatus(quicResults) == Pass { quic := connection.QUIC return &quic diff --git a/prechecks/checker_test.go b/prechecks/checker_test.go index e13cdd95..4bd968aa 100644 --- a/prechecks/checker_test.go +++ b/prechecks/checker_test.go @@ -587,3 +587,144 @@ func TestRun_EdgeAddrs_UnresolvableAddr(t *testing.T) { assert.Nil(t, report.SuggestedProtocol) assert.True(t, report.hasHardFail()) } + +// --------------------------------------------------------------------------- +// Protocol override tests +// --------------------------------------------------------------------------- + +// TestRun_ProtocolOverride_HTTP2_BothPass verifies that when --protocol http2 +// is set and both transports are reachable, the summary reports HTTP/2 (not +// QUIC, which would otherwise win the heuristic). +func TestRun_ProtocolOverride_HTTP2_BothPass(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + + dns := mocks.NewMockDNSResolver(ctrl) + tcp := mocks.NewMockTCPDialer(ctrl) + quicD := mocks.NewMockQUICDialer(ctrl) + mgmt := mocks.NewMockManagementDialer(ctrl) + fakeQUICConn := newFakeQUICConn(ctrl) + + dns.EXPECT().Resolve(gomock.Any()).Return(twoRegionAddrs(), nil) + tcp.EXPECT().DialEdge(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nopConn{}, nil).AnyTimes() + quicD.EXPECT().DialQuic(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(fakeQUICConn, nil).AnyTimes() + mgmt.EXPECT().DialContext(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nopConn{}, nil) + + cfg := Config{ + Timeout: 2 * time.Second, + IPVersion: allregions.Auto, + ProtocolOverride: "http2", + } + report := Run(t.Context(), emptyCert, cfg, nopLogger(), + RunDialers{DNSResolver: dns, TCPDialer: tcp, QUICDialer: quicD, ManagementDialer: mgmt}) + + // Both transports pass, but the override must win — HTTP/2 is reported. + require.NotNil(t, report.SuggestedProtocol) + assert.Equal(t, connection.HTTP2, *report.SuggestedProtocol, + "override http2 should be reported even though QUIC probes also passed") + assert.False(t, report.hasHardFail()) +} + +// TestRun_ProtocolOverride_QUIC_BothPass verifies that when --protocol quic is +// set and both transports are reachable, the summary reports QUIC (same as the +// heuristic would choose, but driven by the override). +func TestRun_ProtocolOverride_QUIC_BothPass(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + + dns := mocks.NewMockDNSResolver(ctrl) + tcp := mocks.NewMockTCPDialer(ctrl) + quicD := mocks.NewMockQUICDialer(ctrl) + mgmt := mocks.NewMockManagementDialer(ctrl) + fakeQUICConn := newFakeQUICConn(ctrl) + + dns.EXPECT().Resolve(gomock.Any()).Return(twoRegionAddrs(), nil) + tcp.EXPECT().DialEdge(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nopConn{}, nil).AnyTimes() + quicD.EXPECT().DialQuic(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(fakeQUICConn, nil).AnyTimes() + mgmt.EXPECT().DialContext(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nopConn{}, nil) + + cfg := Config{ + Timeout: 2 * time.Second, + IPVersion: allregions.Auto, + ProtocolOverride: "quic", + } + report := Run(t.Context(), emptyCert, cfg, nopLogger(), + RunDialers{DNSResolver: dns, TCPDialer: tcp, QUICDialer: quicD, ManagementDialer: mgmt}) + + require.NotNil(t, report.SuggestedProtocol) + assert.Equal(t, connection.QUIC, *report.SuggestedProtocol) + assert.False(t, report.hasHardFail()) +} + +// TestRun_ProtocolOverride_HTTP2_QUICBlocked verifies that when --protocol http2 +// is set and QUIC is blocked, we still report HTTP/2 (not a fallback to the +// heuristic, since the overridden transport is healthy). +func TestRun_ProtocolOverride_HTTP2_QUICBlocked(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + + dns := mocks.NewMockDNSResolver(ctrl) + tcp := mocks.NewMockTCPDialer(ctrl) + quicD := mocks.NewMockQUICDialer(ctrl) + mgmt := mocks.NewMockManagementDialer(ctrl) + + dns.EXPECT().Resolve(gomock.Any()).Return(twoRegionAddrs(), nil) + tcp.EXPECT().DialEdge(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nopConn{}, nil).AnyTimes() + quicD.EXPECT().DialQuic(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, errors.New("blocked")).AnyTimes() + mgmt.EXPECT().DialContext(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nopConn{}, nil) + + cfg := Config{ + Timeout: 2 * time.Second, + IPVersion: allregions.Auto, + ProtocolOverride: "http2", + } + report := Run(t.Context(), emptyCert, cfg, nopLogger(), + RunDialers{DNSResolver: dns, TCPDialer: tcp, QUICDialer: quicD, ManagementDialer: mgmt}) + + require.NotNil(t, report.SuggestedProtocol) + assert.Equal(t, connection.HTTP2, *report.SuggestedProtocol) + assert.False(t, report.hasHardFail()) +} + +// TestRun_ProtocolOverride_HTTP2_BothBlocked verifies that when --protocol http2 +// is set but the HTTP/2 transport itself also fails (hard fail), the override +// falls through to the heuristic which returns nil — there is no usable protocol. +func TestRun_ProtocolOverride_HTTP2_BothBlocked(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + + dns := mocks.NewMockDNSResolver(ctrl) + tcp := mocks.NewMockTCPDialer(ctrl) + quicD := mocks.NewMockQUICDialer(ctrl) + mgmt := mocks.NewMockManagementDialer(ctrl) + + dns.EXPECT().Resolve(gomock.Any()).Return(twoRegionAddrs(), nil) + tcp.EXPECT().DialEdge(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, errors.New("blocked")).AnyTimes() + quicD.EXPECT().DialQuic(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, errors.New("blocked")).AnyTimes() + mgmt.EXPECT().DialContext(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nopConn{}, nil) + + cfg := Config{ + Timeout: 2 * time.Second, + IPVersion: allregions.Auto, + ProtocolOverride: "http2", + } + report := Run(t.Context(), emptyCert, cfg, nopLogger(), + RunDialers{DNSResolver: dns, TCPDialer: tcp, QUICDialer: quicD, ManagementDialer: mgmt}) + + // The overridden transport (HTTP/2) is blocked, so the override cannot be + // honoured and the hard-fail path reports no suggested protocol. + assert.Nil(t, report.SuggestedProtocol) + assert.True(t, report.hasHardFail()) +} diff --git a/prechecks/types.go b/prechecks/types.go index 54856b50..12aee712 100644 --- a/prechecks/types.go +++ b/prechecks/types.go @@ -126,4 +126,13 @@ type Config struct { // no SRV records to validate — and transport probes target each addr // individually, labeled with the original addr string. EdgeAddrs []string + + // ProtocolOverride is the raw --protocol flag value (e.g. "quic", + // "http2", "h2mux"). When non-empty and not "auto", the pre-checks still + // probe both transports for diagnostic completeness, but the reported + // SuggestedProtocol honours the override so that the summary message + // reflects what cloudflared will actually use — not what the probe + // heuristic would recommend on its own. Parsing happens inside the + // prechecks package. + ProtocolOverride string }