WIP: use gobwas/ws instead of gorilla/websocket

[Gekko0114]

Fixes #13824
Related to knative/pkg#2730

Proposed Changes

https://github.com/gorilla/websocket is deprecated and unmaintained.
We should switch to something that is and ensure everything works as expected (ie. draining in queue proxy etc).

• replaced gorilla/websocket with gobwas/ws

• 🧹 Update or clean up current behavior
  /kind deprecation

Release Note

use `gobwas/ws` instead of `gorilla/websocket`

[knative-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Gekko0114
Once this PR has been reviewed and has the lgtm label, please assign pradnyavmw for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[knative-prow]

Welcome @Gekko0114! It looks like this is your first PR to knative/serving 🎉

[knative-prow]

Hi @Gekko0114. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Gekko0114]

Hi @dprotaso,

I've created a WIP in the downstream serving repo to test the changes I've made.
Could you have a look?
Thanks,

[Gekko0114]

/retest

[Gekko0114]

/retest

[dprotaso]

Looks like the websocket tests are failing

[codecov]

Codecov Report

Patch coverage: 61.53% and project coverage change: -0.02 ⚠️

Comparison is base (ea2a6c8) 86.25% compared to head (48c6162) 86.24%.

❗ Current head 48c6162 differs from pull request most recent head 0ec26f6. Consider uploading reports for the commit 0ec26f6 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13932      +/-   ##
==========================================
- Coverage   86.25%   86.24%   -0.02%     
==========================================
  Files         199      199              
  Lines       14749    14753       +4     
==========================================
+ Hits        12722    12723       +1     
- Misses       1726     1728       +2     
- Partials      301      302       +1     

Impacted Files	Coverage Δ
pkg/activator/stat_reporter.go	41.66% <0.00%> (ø)
pkg/autoscaler/statserver/server.go	76.59% <62.50%> (-1.51%)	⬇️
pkg/autoscaler/statforwarder/processor.go	94.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Gekko0114]

Yes, the websocket tests in e2e testing are failing. The Dialer in gobwas/ws does not have a Proxy field, which I think is causing the problem, but I am not sure. If you have any suggestions, please let me know.

[skonto]

@Gekko0114 I run the PR locally. So it seems you dont need the proxy stuff (websocket_test.go). However you need to have the host passed.
Otherwise you get :

    websocket.go:70: Connecting using websocket: url=ws://10.98.128.178:80/?delay=2, host=web-socket-with-timeout-does-not-exceed-t-ryusmvxm.serving-tests.example.com
Connecting using websocket: url=ws://10.98.128.178:80/?delay=2, host=web-socket-with-timeout-does-not-exceed-t-ryusmvxm.serving-tests.example.com

With c, resp, err := dialer.Dial(u.String(), http.Header{"Host": {domain}}) //, http.Header{"Host": {domain}})
tests pass:

$ go test -v -tags=e2e -count=1 ./test/e2e -run ^TestWebSocketWithTimeout$
...
t-with-timeout-does-not-exceed-t-lepqcrbf] Reconcile succeeded
    stream.go:305: I 11:12:57.247 controller-5bb8f4cf98-nt2bm [knative.dev.serving.pkg.reconciler.service.Reconciler] [serving-tests/web-socket-with-timeout-does-not-exceed-t-lepqcrbf] Reconcile succeeded
--- PASS: TestWebSocketWithTimeout (8.30s)
    --- PASS: TestWebSocketWithTimeout/does_not_exceed_timeout_seconds (8.30s)
PASS
ok  	knative.dev/serving/test/e2e	8.327s

[skonto]

So here is why the new framework does not work from the client side. The old framework uses direct http calls
that are being upgraded and it sets correctly the host header. So at the envoy side you should see:

[2023-05-08T14:20:07.071Z] "GET /?delay=2 HTTP/1.1" 101 DC 22 18 2005 - "-" "-" "58d25238-8bb8-417a-89eb-4b3c4d32adf2" "web-socket-with-timeout-ws-does-not-excee-xnsxlsss.serving-tests.example.com" "10.244.0.6:8012"

The new framework though since it uses tcp directly and upgrades later by editing the tcp con stuff directly
it only supports stuff without proxying anything. It means that if I set:

		header := http.Header{}
		header.Set("Host", domain)
...
		dialer := &ws.Dialer{
			Timeout: 45 * time.Second,
			OnStatusError: func(status int, reason []byte, resp io.Reader) {
				var b bytes.Buffer
				io.Copy(&b, resp)
				t.Logf("HTTP connection failed: %v Response=%+v. ResponseBody=%q", string(reason), status, b.String())
			},
			Header: ws.HandshakeHeaderHTTP(header),
		}

I get:

[2023-05-08T12:12:49.346Z] "GET /?delay=2 HTTP/1.1" 404 NR 0 0 0 - "-" "-" "ebcd34aa-8fa4-42a2-b9f0-106c484ce2f6" "10.98.128.178:80,web-socket-with-timeout-ws-does-not-excee-hbeckqih.serving-tests.example.com" "-"

Hosts are merged. I managed to make it work by commenting this line out:

serving/vendor/github.com/gobwas/ws/http.go

Line 302 in 2dd57a9

httpWriteHeader(bw, headerHost, host)

and setting the header manually as above:

    stream.go:305: I 14:20:09.215 controller-5bb8f4cf98-nt2bm [knative.dev.serving.pkg.reconciler.route.Reconciler] [serving-tests/web-socket-with-timeout-ws-does-not-excee-xnsxlsss] Reconcile succeeded
--- PASS: TestWebSocketWithTimeoutWS (7.25s)
    --- PASS: TestWebSocketWithTimeoutWS/does_not_exceed_timeout_seconds (7.24s)
PASS
ok  	knative.dev/serving/test/e2e	7.269s

This is not to be used directly of course for our tests it is a proof of concept.

[Gekko0114]

@skonto ,

I see. Thank you so much for your investigation!

This is not to be used directly of course for our tests it is a proof of concept.

So, we should fix this issue on gobwas/ws first, right?

[skonto]

Yes could you try start the discussion or do a PR there to get things going? What I would propose is that if user sets the host header the library should pass it during connection upgrades and still use the url for direct connections to some target (in this case it is the ingress ip that is being target). I am also a bit concerned about this inflexibility of the new lib.

cc @nak3 @dprotaso may have more to add here.

[Gekko0114]

@skonto,
Sure. I will create an issue. Maybe I will create PR as well.
Also I have a bit concern that this lib doesn't seem to be updated so often.

[AlexVulaj]

Hey all - sorry to jump in here as I've been informed of this work by @skonto . I'm not sure if this changes your plans for this work at all, but a group I've started at Red Hat is currently in active discussions with the original maintainers of the Gorilla Web Toolkit to adopt the project and reboot it. While there are no hard timelines at this time, feel free to contact me if you have any questions or would like to know more.

[Gekko0114]

Hi @AlexVulaj
Thank you for the update.
Does this mean that we no longer need to replace gorilla/websocket with gobwas/ws?
Personally, I am fine with it since I found gorilla/websocket is a great library (also bobwas/ws is great).
If so, I will stop working on this project. Please keep me informed if there are any further updates regarding this matter. Thanks!

[Gekko0114]

FYI
I created an issue about gobwas/wss bug.
gobwas/ws#170

[AlexVulaj]

@Gekko0114 Yes, it should be the case that you won't need to replace it; however, I will caution that I have no promises on an official timeline of exactly when that reboot will occur. What I can say is that there is interest from both parties (the Gorilla maintainers as well as our group of RH engineers) to move forward. Let me know if I can answer anything else!

[knative-prow-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dprotaso]

Closing this for now - seems like RedHat might become owners of gorilla/websocket - so we'll pause this for now

#13824 (comment)