starred/headscale
1
0
Fork 0
mirror of https://github.com/juanfont/headscale.git synced 2026-01-11 20:00:28 +01:00
Code Issues 98 Packages Projects Releases 10 Wiki Activity

[Bug] TestDERPServerWebsocketScenario is broken #855

New Issue
Closed
opened 2025-12-29 02:24:53 +01:00 by adam · 9 comments
adam commented 2025-12-29 02:24:53 +01:00
Owner
Copy Link

Originally created by @kradalby on GitHub (Nov 17, 2024).

TestDERPServerWebsocketScenario is broken after websockets was removed from all clients but JS one: 020cacbe70

From #2046:

@enoperm:

Okay... at glance I'm not sure how to fix it without either a custom client (compiled with GOOS=js) or custom client builds, both of which I believe we'd prefer to avoid, since the point is to ensure we are compatible with the official client, right?

I agree that it is something to in general avoid, I am also not sure if you can run the JS client outside of a browser? Maybe it can be ran with NodeJS in a container? It looks like it can be forced into a Go client with a build flag?
I must admit I am not stoked about having to maintain that kind of container to test this, but maybe it can be done for only this test. I dont really want to remove the feature, but if it is untestable that might be a step to go for if it breaks in the future, it can be left until someone notices, but we are not maintaining features that cant be tested.

On the other hand, I specifically put work into being able to detect this exact scenario (behaviour of the client-under-test changing) in spite of the interface (env var). Seeing it turn red, it feels like it was worth the effort. :)

I agree, that is good, now we just need to make sure we continue to have tests.

Originally created by @kradalby on GitHub (Nov 17, 2024). TestDERPServerWebsocketScenario is broken after websockets was removed from all clients but JS one: https://github.com/tailscale/tailscale/commit/020cacbe702463f14a5d2d5427819c491c7e6578 From #2046: @enoperm: > Okay... at glance I'm not sure how to fix it without either a custom client (compiled with `GOOS=js`) or custom client builds, both of which I believe we'd prefer to avoid, since the point is to ensure we are compatible with the official client, right? > I agree that it is something to in general avoid, I am also not sure if you can run the JS client outside of a browser? Maybe it can be ran with NodeJS in a container? It looks like it can be forced into a Go client with a build flag? I must admit I am not stoked about having to maintain that kind of container to test this, but maybe it can be done for only this test. I dont really want to remove the feature, but if it is untestable that might be a step to go for _if it breaks in the future_, it can be left until someone notices, but we are not maintaining features that cant be tested. > On the other hand, I _specifically_ put work into being able to detect this exact scenario (behaviour of the client-under-test changing) in spite of the interface (env var). Seeing it turn red, it feels like it was worth the effort. :) I agree, that is good, now we just need to make sure we continue to have tests.
adam added the bug label 2025-12-29 02:24:53 +01:00
adam closed this issue 2025-12-29 02:24:53 +01:00
adam commented 2025-12-29 02:24:54 +01:00
Author
Owner
Copy Link

@enoperm commented on GitHub (Nov 17, 2024):

I have not looked into it much yet, but from what I have read so far, it should be possible to build the official client code with a flag to restore websocket support. Clients built with the flag still need to be controlled through the same variable as before - or rather, the referenced commit does not include changes to the functionality itself.
Whether the flag and the behaviour it unlocks is intended for removal in the future, I do not know. If Tailscale ends up not using it as a part of their own test suite, it would make sense for them to consider it dead code.

I think the last time I checked, the full client only ran in the browser, although I have ended up not experimenting much with the full Tailscale client, and instead forked off the tsconnect code to use it as a library instead.

If my memory serves correctly, it only took small patches to build the client with GOARCH=wasip1, but it had no means of opening a control socket. I think that's probably the most important blocker for that route.

I could try carving out the most relevant parts of one of my pet projects (the one that resulted in the websocket support MR), but running that would also require a browser, so I'd prefer to avoid this direction.

If testing the feature in itself is enough, perhaps I could just whip up a standalone DERP client. I have toyed with one before, although I have not tried to force websocket usage at that time.

@enoperm commented on GitHub (Nov 17, 2024): I have not looked into it much yet, but from what I have read so far, it should be possible to build the official client code with a flag to restore websocket support. Clients built with the flag still need to be controlled through the same variable as before - or rather, the referenced commit does not include changes to the functionality itself. Whether the flag and the behaviour it unlocks is intended for removal in the future, I do not know. If Tailscale ends up not using it as a part of their own test suite, it would make sense for them to consider it dead code. I *think* the last time I checked, the full client only ran in the browser, although I have ended up not experimenting much with the full Tailscale client, and instead forked off the `tsconnect` code to use it as a library instead. If my memory serves correctly, it only took small patches to build the client with `GOARCH=wasip1`, but it had no means of opening a control socket. I think that's probably the most important blocker for that route. I could try carving out the most relevant parts of one of my pet projects (the one that resulted in the websocket support MR), but running that would also require a browser, so I'd prefer to avoid this direction. If testing the *feature* in itself is enough, perhaps I could just whip up a standalone DERP client. I have toyed with one before, although I have not tried to force websocket usage at that time.
adam commented 2025-12-29 02:24:54 +01:00
Author
Owner
Copy Link

@kradalby commented on GitHub (Nov 17, 2024):

I have not looked into it much yet, but from what I have read so far, it should be possible to build the official client code with a flag to restore websocket support. Clients built with the flag still need to be controlled through the same variable as before - or rather, the referenced commit does not include changes to the functionality itself.

It does sound like building a client from HEAD for that particular test with the build flag pulling in websockets is the least custom part, so that would be preferable from a maintenance point of view.

Pass the build flag as an envvar to the HEAD build path in the integration test would make sense to me.

@kradalby commented on GitHub (Nov 17, 2024): > I have not looked into it much yet, but from what I have read so far, it should be possible to build the official client code with a flag to restore websocket support. Clients built with the flag still need to be controlled through the same variable as before - or rather, the referenced commit does not include changes to the functionality itself. It does sound like building a client from HEAD for that particular test with the build flag pulling in websockets is the least custom part, so that would be preferable from a maintenance point of view. Pass the build flag as an envvar to the HEAD build path in the integration test would make sense to me.
adam commented 2025-12-29 02:24:54 +01:00
Author
Owner
Copy Link

@enoperm commented on GitHub (Nov 17, 2024):

Short term, definitely. It does have the potential to yield another ticket about these tests down the road should Tailscale remove the flag.

@enoperm commented on GitHub (Nov 17, 2024): Short term, definitely. It does have the potential to yield another ticket about these tests down the road should Tailscale remove the flag.
adam commented 2025-12-29 02:24:54 +01:00
Author
Owner
Copy Link

@ArcticLampyrid commented on GitHub (Nov 18, 2024):

How about conducting tests on a real WebAssembly client in a browser? This can be done using Selenium or something similar.

Of course, this approach will be resource-intensive.

@ArcticLampyrid commented on GitHub (Nov 18, 2024): How about conducting tests on a real WebAssembly client in a browser? This can be done using Selenium or something similar. Of course, this approach will be resource-intensive.
adam commented 2025-12-29 02:24:55 +01:00
Author
Owner
Copy Link

@enoperm commented on GitHub (Nov 18, 2024):

I'll dig into what can be done with a WASM build in/outside a browser later, but first, I say let's tackle it with a custom build, as @kradalby suggested - that'll fix the testcases for everyone else. I raised my concerns, but I don't want to block a working solution while searching for a better one.

@enoperm commented on GitHub (Nov 18, 2024): I'll dig into what can be done with a WASM build in/outside a browser later, but first, I say let's tackle it with a custom build, as @kradalby suggested - that'll fix the testcases for everyone else. I raised my concerns, but I don't want to block a working solution while searching for a better one.
adam commented 2025-12-29 02:24:55 +01:00
Author
Owner
Copy Link

@kradalby commented on GitHub (Nov 18, 2024):

I dont really want to have to maintain browser tests, I will say a custom built client is already towards past what I want to deal with.

@kradalby commented on GitHub (Nov 18, 2024): I dont really want to have to maintain browser tests, I will say a custom built client is already towards past what I want to deal with.
adam commented 2025-12-29 02:24:55 +01:00
Author
Owner
Copy Link

@enoperm commented on GitHub (Nov 18, 2024):

I would rather avoid pulling in a browser as well.

I had some time to look at it, and fixing it for HEAD-version test instances is trivial, since the existing code already builds the current version locally. Other versions of the client are a bit more of a problem, because currently, the corresponding images are pulled from Docker Hub, and from what I can see there are no builds compiled with -tags ts_debug_websockets.

So, assuming we do want to go down the route of custom-built clients while keeping the test matrix, we either have to build other versions of Tailscale locally in addition to HEAD (we can still fallback to Docker Hub for images that predate the build flag to save time), or have to provide prebuilt images with the expected build flags to avoid the extra resource usage and build time on every single test execution.

In the latter case, the images need to be built, and stored somewhere (I'd guess preferably in a CI/CD pipeline, somewhere, which means someone would have to set it all up), the clients used in tests may lag behind official releases while someone or something triggers that pipeline (I don't think that's a major issue for this project), and we'd potentially be missing out on updates to older tags if we are not careful about the implementation. I looked through the Tailscale changelog and I have found at least one example of a security update that was released for an older version.

I suspect neither of us wants to put that much effort into supporting a singe test case, for a feature that (since it was never available in released version of the built-in DERP server) currently has no users.

With that in mind, how about restricting that testcase to only running against HEAD? It'd still be running off the official client code, the feature would still get tested, and considering how rarely their git HEAD overlaps with an officially released client, the client does not seem any less realistic than it was before.

As a plan B, the official derper binary is also 100% compatible with websockets, so just removing the test and documenting that for this specific feature users may want to use that is also a possibility.

@enoperm commented on GitHub (Nov 18, 2024): I would rather avoid pulling in a browser as well. I had some time to look at it, and fixing it for `HEAD`-version test instances is trivial, since the existing code already builds the current version locally. Other versions of the client are a bit more of a problem, because currently, the corresponding images are pulled from Docker Hub, and from what I can see there are no builds compiled with `-tags ts_debug_websockets`. So, assuming we do want to go down the route of custom-built clients while keeping the test matrix, we either have to build other versions of Tailscale locally in addition to `HEAD` (we can still fallback to Docker Hub for images that predate the build flag to save time), or have to provide prebuilt images with the expected build flags to avoid the extra resource usage and build time on *every single test execution*. In the latter case, the images need to be built, and stored somewhere (I'd guess preferably in a CI/CD pipeline, somewhere, which means someone would have to set it all up), the clients used in tests *may* lag behind official releases while someone or something triggers that pipeline (I don't *think* that's a major issue for this project), and we'd potentially be missing out on updates to older tags if we are not careful about the implementation. I looked through the Tailscale changelog and I have found at least [one](https://tailscale.com/changelog#2024-01-08) example of a security update that was released for an older version. I suspect neither of us wants to put *that* much effort into supporting a singe test case, for a feature that (since it was never available in released version of the built-in DERP server) currently has no users. With that in mind, how about restricting that testcase to only running against `HEAD`? It'd still be running off the official client code, the feature would still get tested, and considering how rarely their git `HEAD` overlaps with an officially released client, the client does not seem any less realistic than it was before. As a plan B, the official `derper` binary is also 100% compatible with websockets, so just removing the test and documenting that for this specific feature users may want to use that is also a possibility.
adam commented 2025-12-29 02:24:55 +01:00
Author
Owner
Copy Link

@kradalby commented on GitHub (Nov 19, 2024):

As a plan B, the official derper binary is also 100% compatible with websockets, so just removing the test and documenting that for this specific feature users may want to use that is also a possibility.

I wont consider that an option, we are testing the embedded stuff, so that needs to be tested.

I had some time to look at it, and fixing it for HEAD-version test instances is trivial, since the existing code already builds the current version locally.

Let us use only HEAD for this one test then, it is a trade off I am willing to accept for this one test. Ignore other versions.

We need to get something in, lets keep it simple, make it so we can pass it from https://github.com/juanfont/headscale/blob/main/integration/hsic/hsic.go#L262 with a WithBuildTag option. We are currently blocking other merges atm.

@kradalby commented on GitHub (Nov 19, 2024): > As a plan B, the official derper binary is also 100% compatible with websockets, so just removing the test and documenting that for this specific feature users may want to use that is also a possibility. I wont consider that an option, we are testing the embedded stuff, so that needs to be tested. > I had some time to look at it, and fixing it for HEAD-version test instances is trivial, since the existing code already builds the current version locally. Let us use _only_ HEAD for this one test then, it is a trade off I am willing to accept for this one test. Ignore other versions. We need to get something in, lets keep it simple, make it so we can pass it from https://github.com/juanfont/headscale/blob/main/integration/hsic/hsic.go#L262 with a `WithBuildTag` option. We are currently blocking other merges atm.
adam commented 2025-12-29 02:24:55 +01:00
Author
Owner
Copy Link

@enoperm commented on GitHub (Nov 19, 2024):

Let us use only HEAD for this one test then, it is a trade off I am willing to accept for this one test. Ignore other versions.

I think I got it to work. The validation around the websocket derp option is perhaps a bit overzealous right now, since it also results in a build tag and I also put a misuse check on those, but better safe than sorry.

@enoperm commented on GitHub (Nov 19, 2024): > Let us use _only_ HEAD for this one test then, it is a trade off I am willing to accept for this one test. Ignore other versions. I think I got it to work. The validation around the websocket derp option is perhaps a bit overzealous right now, since it also results in a build tag and I also put a misuse check on those, but better safe than sorry.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
CLI
DERP
DNS
Nix
OIDC
SSH
bug
database
documentation
duplicate
enhancement
faq
good first issue
grants
help wanted
might-come
needs design doc
needs investigation
no-stale-bot
out of scope
performance
policy 📝
pull-request
Mirrored from GitHub Pull Request
 question
regression
routes
stale
tags
tailscale-feature-gap
well described ❤️
wontfix
No Label bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adam
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/headscale#855
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?