Add compatibility with only websocket-capable clients #2132
Conversation
The necessary behaviour is already in place, but the wasm build only issued GETs, and the handler was not invoked.
Still needs some way to assert that clients are connected through websockets, rather than the TCP hijacking version of DERP.
enoperm
requested review from
ohdearaugustin,
juanfont and
kradalby
as code owners
WalkthroughThe pull request introduces several enhancements to the Headscale application, including the addition of a new test case for WebSocket interactions with the DERP server, modifications to HTTP method handling, and the introduction of new logging capabilities. Changes to dependency management are also made, along with various refactorings and updates to existing methods and structures to improve testing coverage and functionality. Changes
Possibly related PRs
Poem
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
integration/dockertestutil/logs.go (1)
49-51: Valid suggestion for potential improvement.The comment suggesting the use of
bufio.Writerto simplify the logging process is a valid point. Usingbufio.Writercould potentially improve the efficiency of the logging mechanism by reducing the number of write operations.Consider creating a follow-up issue to track this potential improvement and discuss the implementation details with the team.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (14)
- .github/workflows/test-integration.yaml (1 hunks)
- go.mod (1 hunks)
- hscontrol/app.go (1 hunks)
- hscontrol/derp/server/derp_server.go (3 hunks)
- hscontrol/types/users.go (1 hunks)
- hscontrol/util/net.go (0 hunks)
- integration/dns_test.go (0 hunks)
- integration/dockertestutil/logs.go (3 hunks)
- integration/embedded_derp_test.go (7 hunks)
- integration/general_test.go (3 hunks)
- integration/hsic/hsic.go (1 hunks)
- integration/tailscale.go (2 hunks)
- integration/tsic/tsic.go (5 hunks)
- integration/utils.go (4 hunks)
Files not reviewed due to no reviewable changes (2)
- hscontrol/util/net.go
- integration/dns_test.go
Files skipped from review due to trivial changes (2)
- hscontrol/types/users.go
- integration/general_test.go
Additional comments not posted (28)
integration/tailscale.go (1)
45-46: LGTM!The addition of the
WriteLogsmethod to theTailscaleClientinterface is a valuable enhancement. It provides a well-defined mechanism for logging output to specified writers for standard output and error, which can be crucial for debugging and monitoring the behaviour of Tailscale clients.The method signature follows Go conventions and does not affect existing methods or the overall structure of the interface. It expands the utility of the interface without introducing any breaking changes.
Great work on enhancing the interface's functionality while maintaining compatibility!
integration/dockertestutil/logs.go (2)
Line range hint
17-37: Excellent refactoring to improve modularity and flexibility!The changes to the
WriteLogfunction signature and implementation are spot on. By acceptingio.Writerparameters forstdoutandstderr, the function is now more modular and flexible, allowing for easier testing and reuse of the logging functionality.The separation of concerns between writing logs and managing the logging environment is a great improvement to the codebase's maintainability.
39-55: Great refactoring to clarify responsibilities and improve control flow!The changes to the
SaveLogfunction are excellent. By focusing on preparing the logging environment and invokingWriteLogto perform the actual logging, the responsibilities of the function are now clearer, and the control flow is improved.This refactoring enhances the maintainability of the codebase by separating concerns and making the code easier to understand.
.github/workflows/test-integration.yaml (1)
44-44: Excellent addition of a new test case for the DERP server WebSocket scenario.The inclusion of the
TestDERPServerWebsocketScenariotest case in the test matrix is a valuable enhancement to the integration test suite. It ensures that the WebSocket functionality of the DERP server is thoroughly tested, improving the overall test coverage and helping to catch potential issues related to WebSocket communication.Running this test case against both Postgres and SQLite databases further strengthens the testing process by verifying the WebSocket behaviour across different database configurations.
Well done on expanding the test coverage!
integration/embedded_derp_test.go (10)
18-21: LGTM!The
ClientsSpecstruct provides a clear and concise way to specify the number of clients using different connection types in test scenarios. The field names are descriptive and the use ofinttype is appropriate.
30-35: Looks good!The
specvariable provides a clear and concise way to specify the client configuration for the test scenario. Usinglen(MustTestVersions)for the number of plain clients ensures that the test covers all the required versions.
37-51: Test scenario looks good!The
DERPServerScenariofunction is called with the appropriatespecand a closure that performs additional assertions. Checking that no client used a WebSocket connection ensures that the expected behaviour is met. The test failure is correctly reported usingt.Fail().
54-77: New test scenario looks good!The
TestDERPServerWebsocketScenariofunction is well-structured and tests the expected behaviour correctly. Specifying aspecwhere all clients are expected to use WebSocket connections and checking this in the additional assertions ensures that the scenario is thoroughly tested. The test failure is correctly reported usingt.Fail().
79-83: Function signature changes look good!The modifications to the
DERPServerScenariofunction signature are well-thought-out and provide a clearer way to specify the client configuration and additional assertions for the test scenario. Accepting amap[string]ClientsSpecfor thespecparameter allows for more granular control over the client setup, and the variadicfurtherAssertionsparameter provides flexibility in extending the test scenario with custom assertions.
101-107: Additional options look good!The additional options passed to the
CreateHeadscaleEnvfunction are well-defined and provide the necessary configuration for the Headscale server in the test scenario. Specifying the port number, enabling TLS, setting the hostname as the server URL, and providing additional configuration environment variables ensures that the server is set up correctly for the test.
136-139: Failure condition looks good!The code correctly checks if the number of successful pings is less than the total expected pings and marks the test as failed using
t.FailNow()if the condition is true. Returning immediately after failing the test ensures that the test execution is stopped and prevents further assertions from being executed.
162-164: Failure condition looks good!The code correctly checks if the number of successful pings is less than the total expected pings and marks the test as failed using
t.Fail()if the condition is true. This ensures that the test failure is recorded accurately.
182-184: Additional assertions look good!The code correctly iterates over the
furtherAssertionsslice and calls each function with a pointer to theEmbeddedDERPServerScenario. This allows for extending the test scenario with custom assertions, providing flexibility in verifying additional conditions or behaviours.
Line range hint
188-245: Modifications toCreateHeadscaleEnvlook good!The changes made to the
CreateHeadscaleEnvmethod are well-structured and correctly handle the creation of Tailscale nodes based on the specifiedClientsSpecfor each user. The method appropriately iterates over theusersmap and creates nodes with the default DERP configuration whenclientCount.Plainis greater than 0, and nodes with the DERP-over-WebSocket configuration whenclientCount.WebsocketDERPis greater than 0.The use of the
CreateTailscaleIsolatedNodesInUsermethod with the appropriate options ensures that the nodes are created with the correct configuration. The error handling and return statements are properly implemented.hscontrol/derp/server/derp_server.go (4)
138-142: LGTM!The changes to the
DERPHandlermethod correctly route requests based on the presence of the "Sec-Websocket-Protocol" header, directing WebSocket requests to the newserveWebsocketmethod and other requests to the existingservePlainmethod. This enhances the server's capability to handle different types of connections more effectively.
145-184: Excellent work on the newserveWebsocketmethod!The method correctly handles the WebSocket upgrade process, including:
- Attempting to upgrade the HTTP connection to a WebSocket connection and handling errors appropriately.
- Validating the subprotocol and closing the connection with a policy violation status if it's not "derp".
- Utilising a buffered read-write connection and binary message transmission for communicating with the Tailscale DERP service.
- Delegating the connection handling to the
tailscaleDERP.Acceptmethod with the necessary parameters.The implementation is well-structured and follows best practices for handling WebSocket connections in the context of the DERP server.
4-4: Import statement looks good.The
"bufio"package is correctly imported and is necessary for the implementation of the newserveWebsocketmethod, where it is used to create a buffered read-write connection.
22-22: Import statement looks good.The
"tailscale.com/net/wsconn"package is correctly imported and is necessary for the implementation of the newserveWebsocketmethod, where it is used to create aNetConninstance for the WebSocket connection.go.mod (1)
7-7: Approve the addition of thejackfan.us.kg/coder/websocketpackage as a direct dependency.The addition of the
github.com/coder/websocketpackage as a direct dependency at versionv1.8.12is a positive change. It indicates that the project now directly relies on this package for its WebSocket functionality, which aligns with the objective of enhancing compatibility with WebSocket-capable clients.The removal of the indirect requirement for
github.com/coder/websocketfurther streamlines the dependency management by clarifying that this package is directly used by the project.integration/utils.go (3)
84-101: LGTM!The
didClientUseWebsocketForDERPfunction is well-implemented and enhances the ability to verify WebSocket usage in client logs. It follows a clear logic flow, handles errors appropriately, and has a descriptive name.
346-362: LGTM!The
countMatchingLinesfunction is well-implemented and provides a reusable utility for counting matching lines from anio.Reader. It follows a clear logic flow, uses a buffered scanner for efficient processing of large input, and has a descriptive name.
138-138: LGTM!The change from a fatal log message to a regular log message when a ping fails improves the robustness of the
pingAllHelperfunction. By allowing the function to continue executing even if a ping fails, the test can potentially identify more issues and is more resilient.integration/hsic/hsic.go (1)
464-468: LGTM!The addition of the
WriteLogsmethod to theHeadscaleInContainerstruct is a useful enhancement for more flexible log handling. The implementation looks concise and correct:
- Appropriate method signature using
io.Writerinterfaces for stdout and stderr.- Delegation to
dockertestutil.WriteLogfor the actual logging operation.- Proper error handling by returning the error from
dockertestutil.WriteLog.Great job!
integration/tsic/tsic.go (4)
70-70: LGTM!The new
withWebsocketDERPboolean field in theTailscaleInContainerstruct is well-named and typed appropriately.
132-136: LGTM!The
WithWebsocketDERPfunction provides a clean way to configure thewithWebsocketDERPfield ofTailscaleInContainer. The implementation looks good.
218-225: LGTM!The new code block in the
Newfunction correctly appends theTS_DEBUG_DERP_WS_CLIENTenvironment variable totailscaleOptions.Envbased on the value ofwithWebsocketDERP. The implementation looks good.
372-378: LGTM!The new
LogsandWriteLogsmethods ofTailscaleInContainerprovide convenient ways to access and write container logs using thedockertestutil.WriteLogfunction. The implementations look good.Also applies to: 1037-1041
hscontrol/app.go (1)
428-428: Verify if allowing GET requests to the upgrade endpoint is necessary and safe.Modifying the
NoiseUpgradeHandlerto allow GET requests in addition to POST is an unusual change for an endpoint performing an upgrade operation. While this may enhance compatibility with certain client code, it's important to verify that this change is actually necessary and doesn't introduce any unintended side effects or security risks.Please confirm that allowing GET requests aligns with the expected behaviour of the relevant Tailscale client code and doesn't open up any potential vulnerabilities. If this change is indeed required and safe, please provide a brief explanation for future reference.
|
I do not remember touching |
|
|
integration/dockertestutil/logs.go
Outdated
| // Wouldn't it be simpler to | ||
| // open and wrap the destination files in a | ||
| // bufio.Writer, and pass those in docker.LogsOptions? | ||
| var stdout bytes.Buffer | ||
| var stderr bytes.Buffer |
There was a problem hiding this comment.
Potentially, not tested or was aware of it, happy for that to be changed, maybe you can test in a followup pr?
There was a problem hiding this comment.
I haven't tested it either, I just thought maybe doing the equivalent of > output.file is simpler than doing an extra lap around the same thing. I don't expect any significant improvements or breakages from it either, other than the code becoming shorter. I don't mind doing an extra PR, but should it include only this spot, or all other file operations that may involve redundant io.Writers?
There was a problem hiding this comment.
If you have time, all would of course be useful. Since this is in the integration tests, its not really that critical, but always nice to have it neater.
There was a problem hiding this comment.
I tried to look for other manually buffered disk writes with git grep, but as far as I can see, this was the only spot where it happened.
I can submit an MR with the changes, but... the code did not become much shorter. The tally comes to -5 lines, three of which were the comment I added. I could still see some value in it, because unlike bytes.Buffer, which (at least if I remember correctly) by default extends however many times it needs to fit things into memory, bufio.Writer has a defined buffer size and automatically flushes when it is about to cross it. So, in theory it could make it so that longer logs can be dumped without issue. I'm still waiting for the integration tests to run on github, but I do not expect problems with it.
(My machine does not seem to like the idea of running the entire test suite at once too much - best I could do was 34 to go before it timed out, and I think I might have raised the timeout before reaching that result).
There was a problem hiding this comment.
Aaaand it actually failed ina few tests - must have forgot about a flag.
There was a problem hiding this comment.
fair, this isnt really critical, if it isnt much simpler or neater, it can be dropped. I'm happy either way as it currently does work, good suggestion tho. You can submit it if it seem to work, and we can take if from there but you dont have to spend that much time on it.
There was a problem hiding this comment.
Removed the comment for now. I may take another look at it, but at this point I would not really mind leaving it as it is either.
|
@enoperm looks good now, could you add a changelog entry? |
Sure - would you prefer that I add a specific version number, or use a placeholder like NEXT? The current state of |
|
Next is good like I did here https://github.com/juanfont/headscale/pull/2020/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR3, the release cycle should not be that long this time ™️ |
|
Added new changelog entry, updated the timestamp on the |
Haha thanks, I must have been a bit quick there, well, technically I was really slow 🫠 |
* handle control protocol through websocket The necessary behaviour is already in place, but the wasm build only issued GETs, and the handler was not invoked. * get DERP-over-websocket working for wasm clients * Prepare for testing builtin websocket-over-DERP Still needs some way to assert that clients are connected through websockets, rather than the TCP hijacking version of DERP. * integration tests: properly differentiate between DERP transports * do not touch unrelated code * linter fixes * integration testing: unexport common implementation of derp server scenario * fixup! integration testing: unexport common implementation of derp server scenario * dockertestutil/logs: remove unhelpful comment * update changelog --------- Co-authored-by: Csaba Sarkadi <[email protected]>
I have not touched
CHANGELOG.md, as I can see Headscale already entered a release candidate phase and I do not expect anything other than bugfixes would be accepted at this point. I'll do so if/when the changes are deemed acceptable and it is clear what version of Headscale they may become a part of.This is not a new feature, nor does it change the configuration, the patches merely improve compatibility with existing Tailscale client code, hence no documentation changes (although I'm considering that maybe it could save someone a bunch of debugging if the docs mentioned that the Tailscale DERP-over-websocket code ignores the port number in the configured DERP map).
I'd refrain from calling these changes a bugfix, though, because not supporting a (currently entirely optional) client capability does not quite sound like a bug.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests