Skip to content

Replace gorilla/websocket with coder/websocket#1633

Open
Akram-saj wants to merge 1 commit into
kedacore:mainfrom
Akram-saj:replace-gorilla-websocket
Open

Replace gorilla/websocket with coder/websocket#1633
Akram-saj wants to merge 1 commit into
kedacore:mainfrom
Akram-saj:replace-gorilla-websocket

Conversation

@Akram-saj
Copy link
Copy Markdown

@Akram-saj Akram-saj commented May 13, 2026

  • Updated go.mod to use github.com/coder/websocket v1.8.12
  • Migrated test/helpers/traffic.go to use coder/websocket API
  • Updated test/e2e/default/websocket_test.go to use new API
  • Added context support for WebSocket operations
  • Improved close handling with proper status codes

Fixes #1617

Copilot AI review requested due to automatic review settings May 13, 2026 12:51
@Akram-saj Akram-saj requested a review from a team as a code owner May 13, 2026 12:51
@keda-automation keda-automation requested a review from a team May 13, 2026 12:51
@snyk-io
Copy link
Copy Markdown

snyk-io Bot commented May 13, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR migrates the project’s WebSocket usage in the e2e test harness from github.com/gorilla/websocket to github.com/coder/websocket, aiming to improve context-aware operations and close handling while addressing concerns about the gorilla/websocket dependency.

Changes:

  • Switched WebSocket dial/read/write in e2e helpers/tests to github.com/coder/websocket.
  • Introduced context-based dialing and updated close handling to send a normal close status.
  • Updated module dependencies to add github.com/coder/websocket and adjust WebSocket-related requirements.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

File Description
test/helpers/traffic.go Updates WebSocket dialing helper to use coder/websocket and adds timeout context + close status.
test/e2e/default/websocket_test.go Updates the e2e WebSocket echo test to use coder/websocket Read/Write APIs.
go.mod Adds coder/websocket and keeps gorilla/websocket listed as indirect.
go.sum Adds checksums for coder/websocket.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/helpers/traffic.go Outdated
Comment thread go.mod
@Akram-saj Akram-saj force-pushed the replace-gorilla-websocket branch from 99c1ec9 to 0b37059 Compare May 13, 2026 13:18
Comment thread test/helpers/traffic.go Outdated
Copy link
Copy Markdown
Contributor

@Fedosin Fedosin left a comment

Choose a reason for hiding this comment

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

Nit (low priority): In coder/websocket, the response body from Dial is already consumed by the library — you never need to close resp.Body yourself. This line is harmless but misleading, as it implies the caller has responsibility for the body. Consider removing it to avoid confusion for future readers.

(test/helpers/traffic.go, line 157: _ = resp.Body.Close())

@Akram-saj Akram-saj force-pushed the replace-gorilla-websocket branch from 0b37059 to c4a39c3 Compare May 14, 2026 06:54
@keda-automation keda-automation requested a review from a team May 14, 2026 06:54
Copy link
Copy Markdown
Member

@linkvt linkvt left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

Copy link
Copy Markdown
Member

@linkvt linkvt left a comment

Choose a reason for hiding this comment

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

Woups, seems that the tests dont work anymore, please make sure that published code was also tested as this just causes more work for us now going back and forth.

@keda-automation keda-automation requested a review from a team May 15, 2026 12:45
@Akram-saj Akram-saj force-pushed the replace-gorilla-websocket branch from 8ce79e6 to ca0db32 Compare May 15, 2026 16:40
Copy link
Copy Markdown
Member

@linkvt linkvt left a comment

Choose a reason for hiding this comment

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

Thanks for updating the PR, tests still seem to be failing.

You can run the tests locally as documented here: https://github.com/kedacore/http-add-on/blob/main/docs/developing.md#running-e2e-tests
Let me know in case the docs don't work for you!

@Fedosin
Copy link
Copy Markdown
Contributor

Fedosin commented May 18, 2026

The e2e test failure is caused by a difference in how gorilla/websocket and coder/websocket handle the Host header.

Root cause: coder/websocket explicitly ignores the Host key in DialOptions.HTTPHeader and always derives the Host header from the dial URL. Because the URL is ws://<proxy-address>/echo, the proxy receives the raw proxy address as the Host instead of the virtual hostname (e.g. e2e-websocket-XXXXX.test). The interceptor has no route for that host, so it returns 404.

In contrast, gorilla/websocket promoted a Host entry in the header map to Request.Host, so the old code worked.

Fix: put the virtual hostname in the URL and use a custom HTTPClient transport to steer the TCP connection to the proxy address:

func (f *Framework) WebSocketDial(host, path string) *websocket.Conn {
	f.t.Helper()

	ctx, cancel := context.WithTimeout(f.ctx, 45*time.Second)
	defer cancel()

	wsURL := url.URL{Scheme: "ws", Host: host, Path: path}
	conn, _, err := websocket.Dial(ctx, wsURL.String(), &websocket.DialOptions{
		HTTPClient: &http.Client{
			Transport: &http.Transport{
				DialContext: func(ctx context.Context, network, _ string) (net.Conn, error) {
					return (&net.Dialer{Timeout: 10 * time.Second}).DialContext(ctx, network, f.proxyAddr)
				},
			},
		},
	})
	if err != nil {
		f.t.Fatalf("WebSocket dial to %s%s failed: %v", host, path, err)
	}
	f.t.Cleanup(func() { _ = conn.Close(websocket.StatusNormalClosure, "") })

	return conn
}

This way the Host header is correctly set from the URL, while the underlying connection still goes through the interceptor proxy.

@keda-automation keda-automation requested a review from a team May 19, 2026 05:18
@Akram-saj
Copy link
Copy Markdown
Author

Thanks Fedosin and Vincent for your inputs. am pushing new changes.

@Akram-saj Akram-saj force-pushed the replace-gorilla-websocket branch 2 times, most recently from 2e36e99 to 9a63696 Compare May 19, 2026 12:10
- Updated go.mod to use github.com/coder/websocket v1.8.12
- Migrated test/helpers/traffic.go to use coder/websocket API
- Updated test/e2e/default/websocket_test.go to use new API
- Added context support for WebSocket operations
- Improved close handling with proper status codes

Fixes kedacore#1617

Signed-off-by: S. Akram Jaaveed <akramjaaveed.aj@gmail.com>
@Akram-saj Akram-saj force-pushed the replace-gorilla-websocket branch from 9a63696 to c28da96 Compare May 19, 2026 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate replacing gorilla/websocket dependency

4 participants