Skip to content

feat: allow DERP headers to be set #6572

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Mar 21, 2023

Conversation

JoshVee
Copy link
Contributor

@JoshVee JoshVee commented Mar 12, 2023

This PR adds the ability to set custom HTTP headers to be passed to the Tailscale connection. This allows the websocket-based DERP connection to function behind reverse proxies or tunnels (for example, authentication headers).

The DERP header functionality is controlled via the existing --header CLI flags.

This functionality utilizes the changes introduced in the following PRs:

@github-actions
Copy link

github-actions bot commented Mar 12, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@JoshVee
Copy link
Contributor Author

JoshVee commented Mar 12, 2023

I have read the CLA Document and I hereby sign the CLA

@JoshVee
Copy link
Contributor Author

JoshVee commented Mar 12, 2023

recheck

@kylecarbs
Copy link
Member

@JoshVee can we make it always use the global HTTP headers instead? I'd rather not add a new top-level flag specifically for DERP.

@JoshVee
Copy link
Contributor Author

JoshVee commented Mar 13, 2023

Thanks @kylecarbs. I've made the relevant changes.

@JoshVee
Copy link
Contributor Author

JoshVee commented Mar 15, 2023

recheck

@JoshVee
Copy link
Contributor Author

JoshVee commented Mar 16, 2023

Hi @kylecarbs,
Any chance we can get this included in the next release? This is a key feature for our use case.
Thanks.

@kylecarbs
Copy link
Member

@JoshVee certainly!

@bpmct
Copy link
Member

bpmct commented Mar 20, 2023

This is a key feature for our use case.

Can you explain why you need this, is it for a specific LoadBalancer or proxy?

Edit, just saw your comment: coder/tailscale#12 (comment)

cdrcommunity added a commit to coder/cla that referenced this pull request Mar 21, 2023
@kylecarbs
Copy link
Member

@JoshVee doing a release today that will include this. I'm going to make a few minor changes on your branch if that's alright!

Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like scaletest needs some changes, but other than that, LGTM! 👍🏻

@kylecarbs kylecarbs force-pushed the joshvee-websocket-headers branch from dcf5d50 to aa99c6c Compare March 21, 2023 17:41
for k, v := range h.headers {
req.Header.Add(k, v)
for k, v := range h.header {
for _, vv := range v {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, noticed we don't seem to handle the multi-value when first parsing the headers. Should we?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should, but feels like it's not super hard to add in the future anyhow.

Header() http.Header
})
if ok {
header = headerTransport.Header().Clone()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Accidentally deleted this comment): We seem to clone in both the function and here, not an issue per-se just pointing it out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I'll change that!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@kylecarbs kylecarbs force-pushed the joshvee-websocket-headers branch from aa99c6c to b543213 Compare March 21, 2023 17:45
@kylecarbs kylecarbs enabled auto-merge (squash) March 21, 2023 17:49
@kylecarbs kylecarbs merged commit 97f77c4 into coder:main Mar 21, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants