Skip to content

Conversation

@film42
Copy link
Contributor

@film42 film42 commented Sep 3, 2025

This adds support for http/2 cleartext (h2c) where prior knowledge is known about the server so an upgrade request (where potential memory consumption issues are not possible).

To quote this issues:

Go 1.24's net/http package includes support for HTTP/2 with prior knowledge. It does not include support for the deprecated protocol upgrade mechanism, and we have no plans to add such support.

Good news!

This change is useful for those who run rails apps behind Cloud Run (or AWS) where some limits may apply to apps (like max file size) unless you run http/2 and enable h2c in the cloud provider settings.

Snippet taken from: #72

Applied feedback from: #3

@film42
Copy link
Contributor Author

film42 commented Sep 3, 2025

I have this disabled by default, but I wonder if it's actually safe by default to leave enabled. I know #72 shared the blog post about h2c smuggling that can happen as a result of an h2c upgrade, but now that a protocol upgrade is deprecated, not implemented (and has no plans to be added), maybe it's safe to leave this enabled by default?

Example:

curl -vvv -i --http2-prior-knowledge http://localhost:4000/sessions/new

In the output you can see there is no upgrade since http2 is assumed.

Verbose Output
* Host localhost:4000 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0*   Trying [::1]:4000...
* Connected to localhost (::1) port 4000
* [HTTP/2] [1] OPENED stream for http://localhost:4000/sessions/new
* [HTTP/2] [1] [:method: GET]
* [HTTP/2] [1] [:scheme: http]
* [HTTP/2] [1] [:authority: localhost:4000]
* [HTTP/2] [1] [:path: /sessions/new]
* [HTTP/2] [1] [user-agent: curl/8.7.1]
* [HTTP/2] [1] [accept: */*]
> GET /sessions/new HTTP/2
> Host: localhost:4000
> User-Agent: curl/8.7.1
> Accept: */*
>
* Request completely sent off
< HTTP/2 404
< content-type: text/html; charset=UTF-8
< server-timing: render_partial.action_view;dur=0.67, render_collection.action_view;dur=4.60, render_template.action_view;dur=10.99, render_layout.action_view;dur=8.97
< vary: Accept-Encoding
< x-cache: miss
< x-request-id: 4841e945-8c91-4a10-a3a8-f7fbba54f359
< x-runtime: 0.054539
< x-web-console-mount-point: /__web_console
< x-web-console-session-id: 52411745e2db02badf1bae152343add9
< content-length: 116338
< date: Wed, 03 Sep 2025 19:59:42 GMT
<
{ [4096 bytes data]
HTTP/2 404
content-type: text/html; charset=UTF-8
server-timing: render_partial.action_view;dur=0.67, render_collection.action_view;dur=4.60, render_template.action_view;dur=10.99, render_layout.action_view;dur=8.97
vary: Accept-Encoding
x-cache: miss
x-request-id: 4841e945-8c91-4a10-a3a8-f7fbba54f359
x-runtime: 0.054539
x-web-console-mount-point: /__web_console
x-web-console-session-id: 52411745e2db02badf1bae152343add9
content-length: 116338
date: Wed, 03 Sep 2025 19:59:42 GMT

[ html begins ]

...

Unless there is a confident "yes" to enabling by default, I would prefer to have this released soon as I could really use it right away. No issues with leaving this disabled by default while we learn more.

@film42
Copy link
Contributor Author

film42 commented Sep 3, 2025

Sorry for a 3rd comment. Forgot to ping @kevinmcconnell for review.

This adds support for http/2 cleartext (h2c) where prior knowledge is
known about the server so an upgrade request (where potential memory
consumption issues are not possible).

To quote [this issues](golang/go#72039):

> Go 1.24's net/http package includes support for HTTP/2 with prior knowledge. It does not include support for the deprecated protocol upgrade mechanism, and we have no plans to add such support.

Good news!

This change is useful for those who run rails apps behind Cloud Run (or
AWS) where some limits may apply to apps (like max file size) unless you
run http/2 and enable h2c in the cloud provider settings.

Snippet taken from: basecamp#72

Applied feedback from: basecamp#3
"cache", cache,
"query", r.URL.RawQuery)
"query", r.URL.RawQuery,
"proto", r.Proto)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been testing this in cloud run to make sure everything works as expected and one item missing for me was seeing the protocol used for the request.

With this I was able to test the following:

  1. By default, cloud run will use http/1.1.
  2. With cloud run's Use HTTP/2 end-to-end enabled, it will use http/2 and fail without h2c enabled in thruster.
  3. With H2C_ENABLED=true DEBUG=true I see the "Enabling h2c" log, cloud run use http/2, and requests are proxied successfully.

Nice!

@le0pard
Copy link
Contributor

le0pard commented Sep 6, 2025

Thanks @film42 for doing this. I was also though to add this, but you already did greate job.

Lets add tests for new config variable in https://github.com/basecamp/thruster/blob/main/internal/config_test.go and add tests for h2c in https://github.com/basecamp/thruster/blob/main/internal/handler_test.go (this example generated by LLM, so may not work)

// TestHandler_H2C tests the handler with an H2C (HTTP/2 Cleartext) server.
// This ensures that the application handler is compatible with an H2C connection.
func TestHandler_H2C(t *testing.T) {
	// 1. Create the application handler we want to test.
	appHandler := NewHandler()

	// 2. Wrap the application handler with the h2c handler.
	h2cHandler := h2c.NewHandler(appHandler, &http2.Server{})

	// 3. Start a test server with the h2c handler.
	// httptest.NewServer handles creating a listener on a random free port
	// and shutting down the server cleanly after the test.
	server := httptest.NewServer(h2cHandler)
	defer server.Close()

	// 4. Create a special HTTP/2 client that can perform h2c (unencrypted HTTP/2).
	// We configure its transport to dial our test server directly without TLS.
	client := http.Client{
		Transport: &http2.Transport{
			// Allow non-encrypted HTTP/2 connections.
			AllowHTTP: true,
			// The DialTLS function is used to establish the connection.
			// We override it to dial our test server's plain TCP listener.
			DialTLS: func(network, addr string, cfg *tls.Config) (net.Conn, error) {
				return net.Dial(network, server.Listener.Addr().String())
			},
		},
	}

	// 5. Make a request to the /up endpoint using the h2c client.
	resp, err := client.Get(server.URL + "/up")
	if !assert.NoError(t, err, "request should not fail") {
		t.FailNow()
	}
	defer resp.Body.Close()

	// 6. Assert that the response was successful and delivered over HTTP/2.
	assert.Equal(t, http.StatusOK, resp.StatusCode, "should return status OK")
	assert.Equal(t, "HTTP/2.0", resp.Proto, "protocol should be HTTP/2.0")

	// 7. Assert the response body is correct.
	body, err := io.ReadAll(resp.Body)
	assert.NoError(t, err, "reading body should not fail")
	assert.Equal(t, "OK", string(body), "body should be OK")
}

If you need help with tests - just write here, I will try to help

P.S. I also using GCP Cloud Run and want to test it here too

In order to do this with public functions, we'll need to refactor the
way server is initialized. This isn't a hard thing to do, but I first
want feedback from the maintainers to see if there is appetite to do so.
@le0pard
Copy link
Contributor

le0pard commented Sep 6, 2025

Thanks @film42 .

Ping @kevinmcconnell . Please check this stuff, when you will have time. If it will be merged, it also will resolve #72 and #3

I will be first in queue to test this on production after thruster new version release. Thanks

@film42
Copy link
Contributor Author

film42 commented Sep 6, 2025

Thanks @le0pard . Since this is added at the server Protocols and not a handler, I pushed two commits and you can determine if you want to keep one or both; or if something else is requred.

  1. The first commit includes the config tests and adds a new internal/server_test.go which shows that the configuration flag is properly setting the correct protocols on the http server.
  2. The second commit adds an end-to-end h2c call client → thruster → upstream and back. I put this in a second commit because I'm bypassing a few of the public function calls. For example, I'm using net.Listen(":0") to get a random port for the test server. You can pass ":0" on the server's Addr, but then you can't retrieve the assigned port (to my knowledge). So, it's up to you if you want to keep this second commit as is, require a refactor to use public methods, or remove it all together.

Btw, I have two apps running in prod with test builds and they are both working great.

@film42
Copy link
Contributor Author

film42 commented Sep 11, 2025

Update here. I ended up disabling h2c for the time being because when action cable tries to connect, I see a 503 in google cloud. More to unpack here but this is a pretty big edge case that needs addressing.

@le0pard
Copy link
Contributor

le0pard commented Sep 11, 2025

Yep, that is why it is better disabled by default. So who needs - activate it

@film42
Copy link
Contributor Author

film42 commented Sep 11, 2025

Agreed. I have one app that doesn't use action cable and I will leave this enabled for that one. Action cable is the only "gotcha" I've come across so far.

@le0pard
Copy link
Contributor

le0pard commented Oct 10, 2025

Ping @kevinmcconnell , this should resolve also #3 🙇

@le0pard
Copy link
Contributor

le0pard commented Oct 18, 2025

https://github.com/le0pard/thruster_plus - resolved for now by fork (binstub need regenerated after replacement). Testing on staging env for now - looks like working

image
@kevinmcconnell
Copy link
Collaborator

Thanks for this @film42 & @le0pard. It looks great. Appreciate the thorough and tidy change!

(And apologies for the slow response on the PR; I have been heads-down with some other work but finally getting back to this now).

@kevinmcconnell kevinmcconnell merged commit 305ba54 into basecamp:main Oct 19, 2025
Eric-Guo added a commit to Eric-Guo/thrustOauth2idServer that referenced this pull request Oct 20, 2025
Eric-Guo added a commit to Eric-Guo/thrustOauth2idServer that referenced this pull request Oct 20, 2025
Eric-Guo added a commit to Eric-Guo/thrustOauth2idServer that referenced this pull request Oct 20, 2025
Eric-Guo added a commit to Eric-Guo/thrustOauth2idServer that referenced this pull request Oct 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants