- Notifications
You must be signed in to change notification settings - Fork928
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
github-actionsbot commentedMar 12, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
@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. |
Thanks@kylecarbs. I've made the relevant changes. |
recheck |
Hi@kylecarbs, |
@JoshVee certainly! |
bpmct commentedMar 20, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Edit, just saw your comment:coder/tailscale#12 (comment) |
@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! |
There was a problem hiding this 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! 👍🏻
dcf5d50 toaa99c6cCompare| for k, v := range h.headers { | ||
| req.Header.Add(k, v) | ||
| for k, v := range h.header { | ||
| for _, vv := range v { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We probablyshould, but feels like it's not super hard to add in the future anyhow.
codersdk/workspaceagents.go Outdated
| Header() http.Header | ||
| }) | ||
| if ok { | ||
| header = headerTransport.Header().Clone() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed!
aa99c6c tob543213Compare
Uh oh!
There was an error while loading.Please reload this page.
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
--headerCLI flags.This functionality utilizes the changes introduced in the following PRs: