feat(interceptor): add HTTP/2 h2c support for cleartext connections #1394
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| package http | ||
|
|
||
| import ( | ||
| "context" | ||
| "crypto/tls" | ||
| "fmt" | ||
| "io" | ||
| "net" | ||
| "net/http" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "golang.org/x/net/http2" | ||
| ) | ||
|
|
||
| func TestHTTP2H2CSupport(t *testing.T) { | ||
| // Create a simple handler that reports the protocol version | ||
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| fmt.Fprintf(w, "Protocol: %s", r.Proto) | ||
| }) | ||
|
|
||
| // Start server in background | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| serverAddr := "127.0.0.1:18888" | ||
|
Member
There was a problem hiding this comment. I think we should not expect a specific port on a system to be free for us to use during a test. I know that the httptest package can pick a selected port, not sure if we can use it here, but maybe there is another way to get a port thats always free to use. |
||
| serverErrors := make(chan error, 1) | ||
|
|
||
| go func() { | ||
| if err := ServeContext(ctx, serverAddr, handler, nil); err != nil && err != http.ErrServerClosed { | ||
| serverErrors <- err | ||
| } | ||
| }() | ||
|
|
||
| // Wait for server to start | ||
| time.Sleep(500 * time.Millisecond) | ||
|
Comment on lines
+35
to
+36
There was a problem hiding this comment. Using a hardcoded sleep to wait for server startup is flaky and can cause intermittent test failures. Consider using a retry loop with a timeout to check if the server is accepting connections, or implement a readiness signal from the server goroutine. |
||
|
|
||
| // Check for server startup errors | ||
| select { | ||
| case err := <-serverErrors: | ||
| t.Fatalf("Server failed to start: %v", err) | ||
| default: | ||
| } | ||
|
|
||
| t.Run("HTTP/1.1 should work", func(t *testing.T) { | ||
| client := &http.Client{ | ||
| Timeout: 5 * time.Second, | ||
| } | ||
|
|
||
| resp, err := client.Get(fmt.Sprintf("http://%s/test", serverAddr)) | ||
| if err != nil { | ||
| t.Fatalf("HTTP/1.1 request failed: %v", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| body, _ := io.ReadAll(resp.Body) | ||
|
There was a problem hiding this comment. The test ignores errors from ReadAll. While this is in a test and the error is unlikely, it's better to be consistent with error handling. Consider checking the error or using an explicit underscore to indicate intentional ignoring. |
||
| t.Logf("HTTP/1.1 Response: %s", string(body)) | ||
|
|
||
| if resp.Proto != "HTTP/1.1" { | ||
| t.Errorf("Expected HTTP/1.1, got %s", resp.Proto) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("HTTP/2 h2c should work", func(t *testing.T) { | ||
| // Create HTTP/2 client with prior knowledge (h2c) | ||
| client := &http.Client{ | ||
| Transport: &http2.Transport{ | ||
| AllowHTTP: true, | ||
| DialTLS: func(network, addr string, cfg *tls.Config) (net.Conn, error) { | ||
| // Use standard dialer for h2c (cleartext HTTP/2) | ||
| return net.Dial(network, addr) | ||
| }, | ||
|
Comment on lines
+69
to
+72
Member
There was a problem hiding this comment. Is this actually needed/used during an non HTTPS request? |
||
| }, | ||
| Timeout: 5 * time.Second, | ||
| } | ||
|
|
||
| resp, err := client.Get(fmt.Sprintf("http://%s/test-h2", serverAddr)) | ||
| if err != nil { | ||
| t.Fatalf("HTTP/2 h2c request failed: %v", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| body, _ := io.ReadAll(resp.Body) | ||
|
There was a problem hiding this comment. The test ignores errors from ReadAll. While this is in a test and the error is unlikely, it's better to be consistent with error handling. Consider checking the error or using an explicit underscore to indicate intentional ignoring. |
||
| t.Logf("HTTP/2 Response: %s", string(body)) | ||
|
|
||
| if resp.Proto != "HTTP/2.0" { | ||
| t.Errorf("Expected HTTP/2.0, got %s", resp.Proto) | ||
| } | ||
| }) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,10 +5,19 @@ import ( | |
| "crypto/tls" | ||
| "net/http" | ||
|
|
||
| "golang.org/x/net/http2" | ||
| "golang.org/x/net/http2/h2c" | ||
|
|
||
| "github.com/kedacore/http-add-on/pkg/util" | ||
| ) | ||
|
|
||
| func ServeContext(ctx context.Context, addr string, hdl http.Handler, tlsConfig *tls.Config) error { | ||
| // For non-TLS connections, wrap handler with h2c to support HTTP/2 cleartext | ||
| if tlsConfig == nil { | ||
| h2s := &http2.Server{} | ||
| hdl = h2c.NewHandler(hdl, h2s) | ||
| } | ||
|
Comment on lines
+16
to
+19
Member
There was a problem hiding this comment. golang has HTTP2 support for server and clients in its standard library, they added it quite recently: https://go.dev/doc/go1.24#nethttppkgnethttp I would prefer using that over the extended standard library as they already plan to deprecate the |
||
|
|
||
| srv := &http.Server{ | ||
| Handler: hdl, | ||
| Addr: addr, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should have e2e tests covering the different scenarios here (client uses http1/h2c, user application supports http1/h2c/both) as unit tests would not cover all cases.