Skip to content

feat: support client verify for derp (add integration tests)#2046

Merged
kradalby merged 13 commits into
juanfont:mainfrom
ArcticLampyrid:verify-client
Nov 22, 2024
Merged

feat: support client verify for derp (add integration tests)#2046
kradalby merged 13 commits into
juanfont:mainfrom
ArcticLampyrid:verify-client

Conversation

@ArcticLampyrid

Copy link
Copy Markdown
Contributor
  • have read the CONTRIBUTING.md file
  • raised a GitHub issue or discussed it on the projects chat beforehand
  • added unit tests
  • added integration tests
  • updated documentation if needed
  • updated CHANGELOG.md

Fixes #1953

By implementing /verify endpoint, derper can verify whether the client is in the node list of Headscale by specifying the --verify-client-url parameter to /verify in Headscale.

This is a continued work on PR #1957, which includes integration testing to meet Headscale's merge requirements.

I am willing to participate in updating the related documentation and change logs, if needed. (Of course, I believe @117503445 is also willing)

@117503445

Copy link
Copy Markdown
Contributor

Thank you very much for your work! I have been a bit busy lately, and it would be great if you could help push this matter forward.

@ArcticLampyrid

Copy link
Copy Markdown
Contributor Author

Rebased onto main.

Can you give a review?

@kradalby

kradalby commented Nov 1, 2024

Copy link
Copy Markdown
Collaborator

Im away traveling, but ill get back to it in a few weeks when im back.

@yqs112358

Copy link
Copy Markdown

Very very useful future 👍 Eliminates the need to create Tailscale clients for every derper server I build.

Comment thread hscontrol/handlers.go Outdated
Comment thread hscontrol/handlers.go Outdated
Comment thread hscontrol/handlers.go Outdated
Comment thread integration/derp_verify_endpoint_test.go Outdated
Comment thread integration/derp_verify_endpoint_test.go Outdated
Comment thread integration/hsic/hsic.go Outdated
Comment thread integration/dsic/dsic.go Outdated
@kradalby

Copy link
Copy Markdown
Collaborator

This looks in general good, some comments, and a couple of failing tests to check out. Thanks!

Comment thread hscontrol/handlers.go Outdated
@kradalby

Copy link
Copy Markdown
Collaborator

Thank you, given passing tests this should be good to go

@ArcticLampyrid

Copy link
Copy Markdown
Contributor Author

I believe this PR is ready for review and merging. The failures in the checks seem unrelated to the changes introduced in this PR. Specifically:

  • TestDERPServerWebsocketScenario failure appears to be caused by tailscale/tailscale@020cacb, which no longer links websockets by default other than GOOS=js.
  • The warnings reported by golangci-lint are from files that were not modified in this PR.

Please let me know if there's anything further needed.

@kradalby

Copy link
Copy Markdown
Collaborator

Since its DERP related, tho I agree it's not related, I would prefer having a pr to fix the failing tests first so we can have this one passing clean.

I might be able to get to it this week, but if you have the opportunity that would be appreciated, or @enoperm who I introduced the websocket stuff

@enoperm

enoperm commented Nov 17, 2024

Copy link
Copy Markdown
Contributor

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?

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

@kradalby

Copy link
Copy Markdown
Collaborator

moved discussion to: #2241

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Support for derp's verify-client-url

5 participants