adding websocket support by anavin-pub · Pull Request #151 · google/inverting-proxy

anavin-pub · 2025-06-26T06:36:13Z

websocket requests are translated into HTTP requests between proxy and agent
client-proxy and agent-server are connected with websockets

google-cla · 2025-06-26T06:36:18Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

ojarjur

Thank you so much for putting in the time and effort to put this together.

Sorry for the long delay in responding; there was just too much other things going on and making it hard to make time for a proper review.

Overall this looks good but I do have some comments for how to improve this.

In particular, I think there is one potential scenario that could lead to the server code panicking. I left details in the comments.

ojarjur · 2025-06-27T17:08:07Z

server/server.go

 	"sync"
 	"time"

+	"github.com/coder/websocket"


The coder/websocket library looks like a nice option.

Unfortunately, we're already using gorilla on the agent side to recreate the connection, so this means we'll have two completely separate libraries that do the same thing.

That's not a blocker for this PR in any way; it's just something that seems unfortunate.

Do you have any idea how hard it would be to converge on a single library (either coder or gorilla)?

Would you be ok with me raising another PR to convert the agent part of the websocket code to coder? I had a high level look at agent/websockets code, and it seems to be doable. I am more familiar with the coder library than gorilla :)

Would you have any preference for coder or gorilla?

The agent is heavily used by lots of people, so I would not want to change it just for the sake of switching to a different (but equivalent library).

So, our options are either:

Have two separate libraries that we use in the different binaries, or

Make both use gorilla

I prefer the second, but I'm actually fine with either. Having two separate libraries is better than not having websocket support in the server, and it's not a "one-way door"... we can always convert the server to using gorilla later as a clean-up task.

So, again, this should not block this specific PR.

Instead, I was just wondering if you knew ahead of time what that future clean-up task (which might not ever actually happen) would look like.

Thanks, understood. Once this PR is complete, I'll try and start with the clean-up task. Currently, I don't have much experience with gorilla so I'm not able to estimate the effort.

server/server.go

ojarjur · 2025-10-22T00:51:42Z

server/server.go

+		return fmt.Errorf("timeout waiting for the response to %q", id)
 	case resp := <-pending.respChan:
+		// websocket shim endpoint handling
+		switch resp.Request.URL.Path {


Three suggestions for this:

Wrap the calls to the response handlers inside of a check that the ws variable is not nil; that way we don't risk a panic if an inbound request comes in with the same URL path.

At the start of the nested block, perform the check that the status code from the response is 200.

Only the open and poll handlers actually do anything with the response (other than checking the status code), so their handlers can be dropped.

E.G.

if ws != nil { // websocket shim endpoint handling if resp.StatusCode != http.StatusOK { respBody, err := io.ReadAll(resp.Body) if err != nil { return fmt.Errorf("%v: http status code is %v, error reading response body", r.URL.Path, resp.StatusCode) } return fmt.Errorf("%v: http status code %v, response: %v", r.URL.Path, resp.StatusCode, string(respBody)) } switch r.URL.Path { case shimPath + "/open": return websocketShimResponseHandlerOpen(resp, ws) case shimPath + "/poll": return websocketShimResponseHandlerPoll(resp, ws) } return nil }

ojarjur · 2025-10-22T01:14:39Z

server/server.go

+	ws := newWsSessionHelper()
+
+	// websocket: shimPath/open
+	req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("http://:%v%v/open", port, shimPath), nil)


I think the URL for these shim requests has to be based off of the incoming (websocket upgrade) request in order to maintain consistency and make it easier to properly configure the backend server.

Do you mean the shimPath?

In the current setup, it seems that the shimPath needs to be communicated both to the server and the agent before starting up either of the processes. (because it is a command line parameter to both of them and both values need to be same)

We might have to do some changes to both the agent and the server if we want to communicate the shimPath between the agent and server within the websocket upgrade request.

Please let me know your thoughts!

I don't remember what I was originally thinking, and reading over the comment now it doesn't make sense. You're right that the shimPath has to be configured in both places so they match.

However, there is an issue in the code.

The body of the request can't be nil. It has to have the URL of the websocket-upgrade request in it:

inverting-proxy/agent/websockets/shim.go

Line 380 in d9a3e67

targetURL, err := url.Parse(string(body))

Basically, this server is terminating the websocket connection and then the proxy agent has to recreate it to the backend.

In order for the proxy-agent to be able to recreate the websocket connection, it has to know the URL path for that outbound connection.

I've added the origin URL as body now, please have a look.

anavin-pub · 2025-10-27T08:54:36Z

Thanks for reviewing, let me review your comments and get back!

* websocket requests are translated into HTTP requests between proxy and agent * client-proxy and agent-server are connected with websockets

anavin-pub · 2026-01-22T11:29:15Z

Hi @ojarjur
Sorry for the delay in response, I couldn't work on this towards the end of last year.

I have made some progress, also had a few questions, please have a look.

ojarjur

Thank you again for all of your work here!

ojarjur · 2026-01-29T17:59:58Z

go.sum

 github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=
 github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
 github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc=
+github.com/coder/websocket v1.8.13 h1:f3QZdXy7uGVz+4uCJy2nTZyM0yTBj8yANEHhqlXZ9FE=


This looks off (it seemingly duplicates the following line and is a different format than the rest of the lines in this file)...

Do you know what is happening here?

It seems hashes of both the websocket package and it's go.mod file are being saved in the go.sum file. I do see other packages where go.mod file hash is being logged too.

Also, We might want to bump the minimum supported version of go for this package (excluding this PR too)
I get syntax errors when I try to compile the server with go version 1.18.

agent/utils/utils.go:528:28: undefined: strings.CutPrefix

Locally, I'm using 1.24.0 for testing.

I would be OK with updating the minimum Go version, but just for the server package; not the others (agent and app).

Please correct if I'm wrong but with the current setup (there being a single go.mod file for the whole repository) we cannot specify different versions for subfolders (like server/ and agent/) right?

Should I just leave it as-is?

ojarjur · 2026-01-29T18:00:51Z

.gitignore

 */#*#
 */.#*
 .gcloudignore
+temp


I think there should still be a trailing newline at the end of this file.

ojarjur · 2026-01-30T01:37:05Z

server/server.go

+	ws := newWsSessionHelper()
+
+	// websocket: shimPath/open
+	req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("http://:%v%v/open", port, shimPath), nil)


I don't remember what I was originally thinking, and reading over the comment now it doesn't make sense. You're right that the shimPath has to be configured in both places so they match.

However, there is an issue in the code.

The body of the request can't be nil. It has to have the URL of the websocket-upgrade request in it:

inverting-proxy/agent/websockets/shim.go

Line 380 in d9a3e67

targetURL, err := url.Parse(string(body))

Basically, this server is terminating the websocket connection and then the proxy agent has to recreate it to the backend.

In order for the proxy-agent to be able to recreate the websocket connection, it has to know the URL path for that outbound connection.

anavin-pub · 2026-01-31T08:19:10Z

Please let me know if I need to compress all the commits here into one before merging, will do so once you're ok with all the changes. Thanks!

ojarjur

Again; thank you so much for your work here. I really appreciate the time and effort you have put into this.

I do have a few more comments, but overall this is a good improvement.

ojarjur · 2026-02-06T02:20:40Z

server/server.go

+	if resp.StatusCode != http.StatusOK {
+		respBody, err := io.ReadAll(resp.Body)
+		if err != nil {
+			return fmt.Errorf("%v/open: http status code %v, error reading response body", shimPath, resp.StatusCode)
+		}
+		return fmt.Errorf("%v/open: http status code %v, response: %v", shimPath, resp.StatusCode, string(respBody))
 	}


This check has already been performed before this method is called (on lines 312 to 318).

It can be removed from here.

ojarjur · 2026-02-06T02:21:12Z

server/server.go

+	if resp.StatusCode != http.StatusOK {
+		respBody, err := io.ReadAll(resp.Body)
+		if err != nil {
+			return fmt.Errorf("%v/poll: http status code %v, error reading response body", shimPath, resp.StatusCode)
+		}
+		return fmt.Errorf("%v/poll: http status code %v, response: %v", shimPath, resp.StatusCode, string(respBody))
+	}


Similarly to the above; this check was already performed before this method was called and can be removed from this method.

ojarjur · 2026-02-06T02:23:28Z

server/server.go

+	ws := newWsSessionHelper()
+
+	// websocket: shimPath/open
+	req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("http://:%v%v/open", port, shimPath), bytes.NewBuffer([]byte("ws://"+r.Host+r.URL.RequestURI())))


It looks like the hostname part of the request URL is missing (it says "http://:..."

ojarjur · 2026-02-06T02:25:39Z

server/server.go

+		if err != nil {
+			log.Printf("Failed to encoded data in JSON format: %v", err)
+			return
+		}


Similarly here the request URL seems to be missing the hostname (and there are similar occurrences below).

I'd recommend moving this logic out into a helper method (e.g. something like newShimRequest(...) so that it's only done in one place and if there are any issues we only have to fix it in one place.

ojarjur · 2026-02-06T02:27:29Z

go.sum

 github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=
 github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
 github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc=
+github.com/coder/websocket v1.8.13 h1:f3QZdXy7uGVz+4uCJy2nTZyM0yTBj8yANEHhqlXZ9FE=


I would be OK with updating the minimum Go version, but just for the server package; not the others (agent and app).

anavin-pub mentioned this pull request Jun 26, 2025

Websocket shim is not getting injected to the response if accept encoding header is present #144

Open

anavin-pub closed this Jul 4, 2025

anavin-pub reopened this Jul 4, 2025

gogasca requested a review from ojarjur July 15, 2025 00:00

ojarjur reviewed Oct 22, 2025

View reviewed changes

anavin-pub force-pushed the shim-pr branch 2 times, most recently from c461996 to e5d2e43 Compare October 27, 2025 08:54

anavin-pub force-pushed the shim-pr branch 2 times, most recently from b793c1e to 5103a0e Compare January 22, 2026 11:05

anavin-pub added 2 commits January 22, 2026 16:36

adding websocket support

b900922

* websocket requests are translated into HTTP requests between proxy and agent * client-proxy and agent-server are connected with websockets

fix: open handler status code, cleanup of handlers

88a0950

anavin-pub force-pushed the shim-pr branch from 5103a0e to 88a0950 Compare January 22, 2026 11:06

fix: add ws to parameter for /data and /close

65e2eba

ojarjur reviewed Jan 30, 2026

View reviewed changes

anavin-pub added 2 commits January 31, 2026 13:05

fix: add URL when websocket open

b895ab3

newline for .gitignore file

3f752a9

ojarjur reviewed Feb 6, 2026

View reviewed changes

fix: removed duplicated code, created shim helper function

b4edb8a

Conversation

anavin-pub commented Jun 26, 2025

Uh oh!

google-cla bot commented Jun 26, 2025

Uh oh!

ojarjur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anavin-pub commented Oct 27, 2025

Uh oh!

anavin-pub commented Jan 22, 2026

Uh oh!

ojarjur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anavin-pub Jan 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anavin-pub commented Jan 31, 2026

Uh oh!

ojarjur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anavin-pub Jan 31, 2026 •

edited

Loading