websocket: migrate to gorilla/websocket

[stbenjam]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This change migrates away from golang.org/x/net/websocket, in favor
of github.com/gorilla/websocket. The main motivation for this change
is I would like to run the e2e tests through a squid proxy to reach
the remote cluster. Largely this just works, however the
golang.org/x/net/websocket package does not support this and these
tests fail for me when going through a proxy.

The docs for net/websocket
even point out that gorilla/websocket is more actively maintained and
supports more features.

gorilla/websocket is already available as well, so we're not introducing
any additional dependencies.

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

N/A

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

I'm using this squid configuration:

http_access allow all
http_port 3128
debug_options ALL,1 33,2 28,9
dns_v4_first on
coredump_dir /var/spool/squid

Launched as:

  sudo podman run -d --rm \
     --net host \
     --volume $PWD/squid.conf:/etc/squid/squid.conf \
     --name squid \
     docker.io/sameersbn/squid:3.5.27-2

When using a proxy with golang.org/x/net/websocket, the tests fail:

export HTTP_PROXY=http://10.N.N.N:3128
export HTTPS_PROXY=http://10.N.N.N:3128

$ ./_output/bin/ginkgo -p -focus ".*websocket.*"  ./test/e2e
[...]
Summarizing 4 Failures:

[Fail] [sig-node] Pods [It] should support remote command execution over websockets [NodeConformance] [Conformance] 
/home/stbenjam/go/src/github.com/kubernetes/kubernetes/vendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:113

[Fail] [sig-node] Pods [It] should support retrieving logs from the container over websockets [NodeConformance] [Conformance] 
/home/stbenjam/go/src/github.com/kubernetes/kubernetes/vendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:113

[Fail] [sig-cli] Kubectl Port forwarding With a server listening on 0.0.0.0 [It] should support forwarding over websockets 
/home/stbenjam/go/src/github.com/kubernetes/kubernetes/test/e2e/kubectl/portforward.go:469

[Fail] [sig-cli] Kubectl Port forwarding With a server listening on localhost [It] should support forwarding over websockets 
/home/stbenjam/go/src/github.com/kubernetes/kubernetes/test/e2e/kubectl/portforward.go:491

Ran 4 of 6433 Specs in 16.684 seconds
FAIL! -- 0 Passed | 4 Failed | 0 Pending | 6429 Skipped


Ginkgo ran 1 suite in 35.950365959s
Test Suite Failed

With this PR:

$ ./_output/bin/ginkgo -p -focus ".*websocket.*"  ./test/e2e
[...]
Ran 4 of 6433 Specs in 16.594 seconds
SUCCESS! -- 4 Passed | 0 Failed | 0 Pending | 6429 Skipped


Ginkgo ran 1 suite in 39.173493889s
Test Suite Passed

[k8s-ci-robot]

Hi @stbenjam. Thanks for your PR.

I'm waiting for a kubernetes 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.

[smarterclayton]

I'm generally in favor of this, will look.

[smarterclayton]

/ok-to-test

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: stbenjam
To complete the pull request process, please assign liggitt after the PR has been reviewed.
You can assign the PR to them by writing /assign @liggitt in a comment when ready.

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

[stbenjam]

@endocrimes Sure, rebased - thank you

[stbenjam]

/test pull-kubernetes-unit

Don't see an existing issue, but I can't imagine this is related to this PR:

 === Failed
=== FAIL: vendor/k8s.io/client-go/dynamic/dynamicinformer TestDynamicSharedInformerFactory/scenario_1:_test_if_adding_an_object_triggers_AddFunc (3.04s)
W0720 14:23:31.738665   87519 mutation_detector.go:53] Mutation detector is enabled, this will result in memory leakage.
    informer_test.go:245: tested informer haven't received an object, waited 3s
    --- FAIL: TestDynamicSharedInformerFactory/scenario_1:_test_if_adding_an_object_triggers_AddFunc (3.04s) 

[stbenjam]

/test pull-kubernetes-integration

[endocrimes]

/retest

[dims]

/test all
/assign @smarterclayton @liggitt

[k8s-ci-robot]

@stbenjam: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-node-kubelet-serial	f77db14	link	/test pull-kubernetes-node-kubelet-serial
pull-kubernetes-node-kubelet-serial-containerd	f77db14	link	/test pull-kubernetes-node-kubelet-serial-containerd
pull-kubernetes-integration	f77db14	link	/test pull-kubernetes-integration
pull-kubernetes-e2e-gce-ubuntu-containerd	f77db14	link	/test pull-kubernetes-e2e-gce-ubuntu-containerd

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[liggitt]

Thanks for the PR, but I'm -1 on switching from the golang.org dependency to this. We currently have a single transitive test-only dependency on that package via the etcd embedded test server. I'd rather see us drop that transitive dep and stop vendoring this, and pursue adding the proxy support to the golang.org/x/net impl

[stbenjam]

The net package admits it's not "as actively maintained" as gorilla/websockets: https://pkg.go.dev/golang.org/x/net/websocket

And it hasn't seen a commit in two years: https://github.com/golang/net/tree/master/websocket

golang/go#18152 admits gorilla/websocket is the library basically everyone uses:

It is not well maintained, and essentially everyone uses https://github.com/gorilla/websocket.

This was raised as a concern in #17244: because it is in the x/net repository, people attach greater value to it than it really ought to have.

[smarterclayton]

Yeah I kind of fall on @stbenjam's side here - I've seen that same perspective in the community w.r.t. net vs gorilla and "in the standard library" doesn't seem to be as strong of an argument as it should be.

[liggitt]

And it hasn't seen a commit in two years: https://github.com/golang/net/tree/master/websocket

fwiw, gorilla/websocket has only seen ~2 functional changes in the last two years (gorilla/websocket@873e67e), so I'm not sure it is significantly more actively developed

That said, that can also be seen as a sign of stability, and since gorilla/websocket has no dependencies outside the stdlib, it doesn't make our dependency story worse.

[liggitt]

golang.org/x/net/websocket is used in 10 test files and 3 non-test files... is the expectation that we would stop using it in all of those files?

[k8s-ci-robot]

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

[aojea]

it seems that gorilla is looking for maintainers https://groups.google.com/d/msgid/golang-nuts/40a9cc71-8fa3-44a6-a2c4-f031358b99ffn%40googlegroups.com?utm_medium=email&utm_source=footer

doesn't seem a good idea to migrate :/

Note: I don't expect this to be quick or easy - the websocket library, with 16k stars & 15k unique clones per week, has been looking for a new maintainer for 3.5+ years, and has yet to have anyone reliably stick. If I don't have any luck finding new maintainer(s) in the next 6 months or so, it's likely I'll mark these projects as in maintenance mode only and archive the repos.

[dims]

Is this PR still needed, please rebase if so (or we can close it?)

[stbenjam]

We can close it for now, especially in light of @aojea's comment

/close

[k8s-ci-robot]

@stbenjam: Closed this PR.

In response to this:

We can close it for now, especially in light of @aojea's comment

/close

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.