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.
2026-06-23 04:10:20 +00:00 · 2026-06-18 10:56:53 +00:00
parent ad11e67340
commit 3315fa6e0f
4 changed files with 200 additions and 7 deletions
@@ -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{
@@ -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
@@ -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())
+}
@@ -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
 }