Add h2c origin support#3
Conversation
Greptile SummaryThis PR adds
Confidence Score: 5/5Safe to merge; the h2c transport, validation, and config plumbing are all consistent with existing patterns and well-covered by tests. The core h2c changes are correct: validation fires at both parse time and service-start time, the .github/workflows/docker-publish.yml — the three
|
| Filename | Overview |
|---|---|
| ingress/origin_service.go | Core h2c implementation: newHTTPTransport now returns http.RoundTripper and branches on cfg.H2cOrigin to produce an *http2.Transport; validateHTTPOriginConfig/validateHTTPOriginScheme cover all service types; helloWorld.start pre-validates before delegating to httpService.start; dialer extraction is correctly shared between h2c and plain paths. |
| ingress/ingress.go | Adds validateHTTPOriginConfig(service, cfg) call inside validateIngress loop, catching h2c conflicts at YAML-parse time before any service is started. |
| ingress/origin_proxy.go | Adds a *http.Transport type assertion guard in SetOriginServerName so h2c's *http2.Transport is not subject to TLS SNI mutation. |
| ingress/config.go | Adds H2cOriginFlag, H2cOrigin bool to OriginRequestConfig, and wires setH2cOrigin / originRequestFromSingleRule / ConvertToRawOriginConfig consistently with the existing Http2Origin pattern. |
| config/configuration.go | Adds H2cOrigin *bool to raw OriginRequestConfig; also fixes os.Open → os.Stat in FileExists, defers file close correctly, and replaces s.Duration.Seconds() with s.Seconds() in JSON marshaling. |
| connection/quic_connection.go | Adds a process-wide quicTrailerDropWarningLogged atomic.Bool so the trailer-drop warning is emitted once, preventing log flooding for gRPC origins; logger is now threaded into httpResponseAdapter. |
| .github/workflows/docker-publish.yml | New release workflow for h2c-tagged builds; uses ${{ inputs.release_tag }} directly in bash run: blocks — a GitHub Actions expression-injection pattern that should use an env var intermediary instead. |
| .github/workflows/check.yaml | Refreshed CI: checkout now precedes setup-go (required for go-version-file: go.mod), Go version is derived from go.mod, unprivileged ICMP sysctl added for Linux runners. |
| datagramsession/manager_test.go | Test refactored to propagate errors via channels rather than calling require in goroutines, eliminating potential goroutine-leak on assertion failures; TestManagerCtxDoneCloseSessions renamed and tightened to assert closedByRemote: true. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Ingress Rule parsed] --> B{validateHTTPOriginConfig}
B -->|httpService| C{scheme?}
B -->|helloWorld| D[scheme = 'https']
B -->|unixSocketPath| E[scheme = service.scheme]
B -->|other| F[return nil]
C --> G[validateHTTPOriginScheme]
D --> G
E --> G
G --> H{H2cOrigin AND Http2Origin?}
H -->|yes| I[conflict error]
H -->|no| J{H2cOrigin AND scheme is https/wss?}
J -->|yes| K[scheme error]
J -->|no| L[valid config]
L --> M[newHTTPTransport]
M --> N{cfg.H2cOrigin?}
N -->|yes| O[return http2.Transport AllowHTTP: true ReadIdleTimeout: cfg.KeepAliveTimeout DialTLSContext: plain TCP dial]
N -->|no| P[return http.Transport ForceAttemptHTTP2: cfg.Http2Origin TLS cert pool loaded]
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
.github/workflows/docker-publish.yml:37-57
GitHub Actions expression injection: `${{ inputs.release_tag }}` is substituted directly into the shell script text before the runner executes it. A value containing `"`, `$(...)`, or backticks can escape the double-quoted assignment and execute arbitrary commands before the `v*-h2c*` guard runs. The safe pattern is to expose the input as an environment variable and reference it via `$VAR` inside the script — the shell then receives it as a literal value regardless of content. The same pattern appears in all three `vars` steps (`build-binaries`, `build-and-push`, `publish-release`).
```suggestion
env:
INPUT_RELEASE_TAG: ${{ inputs.release_tag }}
run: |
if [[ "${GITHUB_EVENT_NAME}" == "workflow_dispatch" ]]; then
raw_tag="${INPUT_RELEASE_TAG}"
else
raw_tag="${GITHUB_REF_NAME}"
fi
if [[ -z "${raw_tag}" || "${raw_tag}" != v*-h2c* ]]; then
echo "Invalid h2c release tag: ${raw_tag}" >&2
exit 1
fi
echo "raw_tag=${raw_tag}" >> "${GITHUB_OUTPUT}"
echo "release_version=${raw_tag#v}" >> "${GITHUB_OUTPUT}"
- name: Checkout
uses: actions/checkout@v6.0.2
with:
ref: ${{ steps.vars.outputs.raw_tag }}
- name: Install Go
```
Reviews (6): Last reviewed commit: "ci: add fork h2c release workflows" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func (o *httpService) SetOriginServerName(req *http.Request) { | ||
| o.transport.DialTLSContext = func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| conn, err := o.transport.DialContext(ctx, network, addr) | ||
| t, ok := o.transport.(*http.Transport) | ||
| if !ok { | ||
| // h2c transport doesn't use TLS, so SNI matching is not applicable | ||
| return | ||
| } | ||
| t.DialTLSContext = func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| conn, err := t.DialContext(ctx, network, addr) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return tls.Client(conn, &tls.Config{ | ||
| RootCAs: o.transport.TLSClientConfig.RootCAs, | ||
| InsecureSkipVerify: o.transport.TLSClientConfig.InsecureSkipVerify, // nolint: gosec | ||
| RootCAs: t.TLSClientConfig.RootCAs, | ||
| InsecureSkipVerify: t.TLSClientConfig.InsecureSkipVerify, // nolint: gosec | ||
| ServerName: req.Host, | ||
| }), nil | ||
| } |
There was a problem hiding this comment.
SetOriginServerName mutates a shared transport per-request — pre-existing data race
This code (touched in this PR to add the *http.Transport type guard) assigns to t.DialTLSContext on every request when matchSNIToHost is enabled. Because o.transport is a shared *http.Transport connection pool, concurrent requests will race writing different closures to the same field. This was present before the PR; the type-assertion guard added here is a good safety improvement, but the underlying race is still there for the *http.Transport case. Worth tracking for a follow-up fix.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ingress/origin_proxy.go
Line: 61-77
Comment:
**`SetOriginServerName` mutates a shared transport per-request — pre-existing data race**
This code (touched in this PR to add the `*http.Transport` type guard) assigns to `t.DialTLSContext` on every request when `matchSNIToHost` is enabled. Because `o.transport` is a shared `*http.Transport` connection pool, concurrent requests will race writing different closures to the same field. This was present before the PR; the type-assertion guard added here is a good safety improvement, but the underlying race is still there for the `*http.Transport` case. Worth tracking for a follow-up fix.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Acknowledged. This shared *http.Transport mutation predates h2c; this PR only adds the type guard so h2c's *http2.Transport does not enter TLS SNI mutation. Keeping the SetOriginServerName race as separate follow-up scope.
|
Addressed the h2c-specific Greptile findings in 39a80e0:
The |
| if cfg.H2cOrigin && scheme == "https" { | ||
| return fmt.Errorf("h2cOrigin is enabled but the origin uses https://; " + | ||
| "h2c is HTTP/2 over cleartext; use http:// or disable h2cOrigin") | ||
| } |
There was a problem hiding this comment.
validateHTTPOriginScheme checks for scheme == "https" but not "wss". A user who configures wss://localhost:8080 as an origin with h2cOrigin: true gets no validation error at parse or start time. At runtime, httpService.RoundTrip rewrites wss → https, so the http2.Transport passes "https" to its own DialTLSContext. Since our DialTLSContext always dials plain TCP (no TLS), the h2 preface is sent to a TLS-expecting server, which closes the connection immediately with a parse error — no useful diagnostic about the misconfiguration reaches the user.
| if cfg.H2cOrigin && scheme == "https" { | |
| return fmt.Errorf("h2cOrigin is enabled but the origin uses https://; " + | |
| "h2c is HTTP/2 over cleartext; use http:// or disable h2cOrigin") | |
| } | |
| if cfg.H2cOrigin && (scheme == "https" || scheme == "wss") { | |
| return fmt.Errorf("h2cOrigin is enabled but the origin uses %s://; " + | |
| "h2c is HTTP/2 over cleartext; use http:// (or ws://) or disable h2cOrigin", scheme) | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: ingress/origin_service.go
Line: 131-134
Comment:
`validateHTTPOriginScheme` checks for `scheme == "https"` but not `"wss"`. A user who configures `wss://localhost:8080` as an origin with `h2cOrigin: true` gets no validation error at parse or start time. At runtime, `httpService.RoundTrip` rewrites `wss` → `https`, so the `http2.Transport` passes `"https"` to its own `DialTLSContext`. Since our `DialTLSContext` always dials plain TCP (no TLS), the h2 preface is sent to a TLS-expecting server, which closes the connection immediately with a parse error — no useful diagnostic about the misconfiguration reaches the user.
```suggestion
if cfg.H2cOrigin && (scheme == "https" || scheme == "wss") {
return fmt.Errorf("h2cOrigin is enabled but the origin uses %s://; " +
"h2c is HTTP/2 over cleartext; use http:// (or ws://) or disable h2cOrigin", scheme)
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 2ef591a.
validateHTTPOriginScheme now rejects both https and wss, interpolates the actual %s:// scheme, and keeps ws:// allowed as cleartext. Added coverage in TestH2cOriginTransport and TestParseIngressRejectsInvalidH2cOriginConfig.
Validation run on 2ef591a9:
git diff --checkgo test -mod=vendor ./cryptogo test -mod=vendor ./config ./ingress ./connection- focused h2c CLI/ingress test run
- temp ingress smoke config confirms
h2cOrigin + wss://fails withwss://in the error HOME="$(mktemp -d)" make test lint
|
Follow-up on the Greptile summary proxy item: not implementable as written against this vendored API. |
mannie-exe
force-pushed
the
feat/h2c-origin
branch
from
f976168 to
85333a1
Compare
mannie-exe
force-pushed
the
feat/h2c-origin
branch
from
85333a1 to
cbaf898
Compare
mannie-exe
force-pushed
the
feat/h2c-origin
branch
from
cbaf898 to
553d836
Compare
1 participant
Summary
h2cOrigin/--h2c-originfor cleartext HTTP/2 originsh2cOrigin+http2Origincombinations at config/runtime boundariescontrib/h2c-origin-upstream-20260516Test plan
git diff --checkgo test -mod=vendor ./cryptogo test -mod=vendor ./config ./ingress ./connectiongo test -mod=vendor ./cmd/cloudflared/tunnel ./ingress -run 'TestTunnelH2cOriginFlagRegistered|TestH2cOriginFromCLI|TestH2cOriginTransport|TestUnixSocketH2cOriginConflict|TestParseIngressRejectsInvalidH2cOriginConfig' -count=1go run -mod=vendor ./cmd/cloudflared tunnel --help | rg "h2c-origin"go run -mod=vendor ./cmd/cloudflared tunnel --h2c-origin --helpcloudflared tunnel --config /tmp/h2c-valid.yml ingress validateequivalent viago runpassescloudflared tunnel --config /tmp/h2c-conflict.yml ingress validateequivalent viago runfails as expectedHOME="$(mktemp -d)" make test lint