Figure out a plan for https://github.com/gorilla/websocket

[dprotaso]

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

[dprotaso]

/triage accepted

[dprotaso]

Possible alternative - https://github.com/gobwas/ws

[Gekko0114]

Hi @dprotaso ,
Can I work on this if you don't plan to do it?

[dprotaso]

@Gekko0114 sure

[Gekko0114]

Thanks!
/assign

[Gekko0114]

We have to fix knative/pkg as well.
knative/pkg#2727

[skonto]

@Gekko0114 @dprotaso pls check: #13932 (comment)

[dprotaso]

From: @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.

I'm not really married to any implementation - in theory I'd prefer the go stdlib just support and maintain websocket as a first party https://pkg.go.dev/golang.org/x/net/websocket

Oddly what's funny that page links out to https://pkg.go.dev/nhooyr.io/websocket

Also sorta random is the lack of WebSocket support over HTTP/2 golang/go#32763 (comment) & golang/go#49918

[skonto]

I'm not really married to any implementation - in theory I'd prefer the go stdlib just support

In the past they referenced gorilla in their docs. Now just check updates for https://github.com/nhooyr/websocket does not seem that active either. I am not in favor of anything, however before we do any change pls consider the info for the reboot.

[dprotaso]

Yeah nhooyr doesn't seem active - which is unfortunate given security focused PRs nhooyr/websocket#360

[dprotaso]

The other bit https://github.com/gobwas/ws mentions in their README.md

This implementation of RFC6455 passes Autobahn Test Suite and currently has about 78% coverage.

I wonder what kind of coverage gorilla/websocket has

[dprotaso]

When things are decided between RedHat and the gorilla folks do you mind following up on this issue

/assign @skonto @AlexVulaj

[knative-prow]

@dprotaso: GitHub didn't allow me to assign the following users: AlexVulaj.

Note that only knative members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

When things are decided between RedHat and the gorilla folks do you mind following up on this issue

/assign @skonto @AlexVulaj

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]

@Gekko0114 thanks for the help in the meantime

[cristaloleg]

The other bit gobwas/ws mentions in their README.md

This implementation of RFC6455 passes Autobahn Test Suite and currently has about 78% coverage.

I wonder what kind of coverage gorilla/websocket has

Hey, gobwa/ws maitainer here. I have plans to looks at autobahn tests in June, so coverage should increase. No official deadline for now.

[andig]

Worth mentioning https://github.com/nhooyr/websocket too

[ReToCode]

@dprotaso might be off-topic, but why do we use Websockets between Activator <> Autoscaler in the first place? Could that not just be GRPC?

[skonto]

The project is back alive: gorilla/websocket@931041c.

[skonto]

@ReToCode We changed the stats format for QP <-> Autoscaler long ago to be pb:
#8396, #8556. Making more efficient than using json etc.
We use websockets with a binary format for the Activator <-> Autoscaler exchange and use http based scraping for Autoscaler <-> QP (qp sets "application/protobuf" in the response to indicate that it sends pb data).
Probably we could have grpc/http2 there assuming it performs better but not sure about it for this use case. @dprotaso could add more.

[dprotaso]

Probably we could have grpc/http2 there assuming it performs better but not sure about it for this use case.

Yeah prior to making these changes we'd want to use the new perf tests to see what gains we get. Plus we now have to play a 'migration' path if we do make changes

From the historical point of view I don't recall why we went with websockets+json - maybe it was easier to start with.

[dprotaso]

Since we're already using the latest version of https://github.com/gorilla/websocket I'm going to close this out

I think churning on websocket libraries won't be particularly useful - but rather work towards doing something more efficient. But we should open a separate issue for that.