adding websocket support

[anavin-pub]

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

[google-cla]

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]

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)?

[anavin-pub]

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?

[ojarjur]

This method doesn't handle the case that the status in the open response is not http.StatusOK.

Additionally, the corresponding logic in the other response handler methods is duplicated.

I think we should move that to the handleFrontendRequest method so that we avoid duplicating it.

I'll leave a comment below for my suggestion on how to do that.

[ojarjur]

Three suggestions for this:

1. 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.
2. At the start of the nested block, perform the check that the status code from the response is 200.
3. 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]

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.

[anavin-pub]

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!

[anavin-pub]

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

[anavin-pub]

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.