- Notifications
You must be signed in to change notification settings - Fork24
Add customizable header command#119
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
8b4e951
to0423107
CompareOption 1 seems correct to me as well. For the naming, should we call this |
Ooo yeah good point, technically could you actually use this for Coder credentials by supplying the token header? But we might not want to encourage that type of use. I think Maybe since we have |
Or |
liorb-canva commentedAug 9, 2023
@code-asher option 1 seems fine as well for us |
0423107
to534be09
CompareThis was causing issues with the header flag as those contain equalsigns.
36c842b
to2625f25
CompareI am not sure if we need to check the Coder version before setting the new flag but since it only sets if you provide the value maybe it is OK to just rely on the user only adding it if their version supports the new flag? |
This will be called before requests and added to the SSH config.
2625f25
to811673f
CompareI got this once, not sure how to reproduce it now though.
Forgot escape adds its own wrapping quotes.
src/extension.ts Outdated
...config, | ||
headers: { | ||
...(await storage.getHeaders()), | ||
...creds.headers, |
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.
hey@code-asher this does not compile..
I've tried to run it locally and it failed
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.
Yeah just realized I messed this up, fixing now.
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.
Pushed a fix!
code-asherAug 18, 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.
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.
Turns out I had another issue where the URL in storage was not set yet (for the login request) resulting in no headers. I think everything is working now, will test a bit more then merge this in. Let me know if you find anything else wrong!
First, I replaced the wrong line, needed to replace the second one.Secondly, seems you cannot actually spread like this becauseconfig.headers actually has a bunch of functions on it.
To avoid an error like "127.0.0.1:80 ECONNREFUSED". Before we did notlog the error so this did not matter so much but now we do to catchheader issues.
At this stage the URL may not be set. Or it could be set to theprevious URL. We need to use the URL of the actual request.
2bb02f9
toa7f21be
Compare
Uh oh!
There was an error while loading.Please reload this page.
The command will be executed before requests and before configuring SSH to set custom headers.
Some notes:
--header
I seem to get the value set twice. For example if I usecoder --header X-MY-CUSTOM-HEADER=hello-there
then on the server end I seex-my-custom-header: hello-there, hello-there
so we should look into that.CODER_URL
right now. Are there others needed?More importantly, I am not sure about the config SSH portion. Since configuration might only happen on the first connect, the headers we hardcode into the SSH config could expire.I have two main ideas for fixing the SSH config issue:Modify the CLI to call out to the same process to get the headers.--credentials-process
or something like that. The CLI executes the process just as the extension does and sets the headers. We set--credentials-process
in the SSH config on the plugin side and are off to the races.Add a second VS Code setting for a proxy command wrapper which gets passed the path to the Coder binary and is responsible for adding the appropriate--header
flags and passing through any other flags. This way this credential process is called every time SSH is invoked. Or instead of a separate setting it could be an environment variable/flag and we reuse the same process; it can switch behavior depending on what task it needs to do (output JSON or execute the Coder binary).An example of the second option:ProxyCommand my-wrapper --wrapper-flags -- --coder-flags
wheremy-wrapper
is essentiallyexec $CODER_BINARY --header my-custom-header "$@"
except everything after--
should get passed through to the Coder binary and the rest can be flags to the wrapper itself in case we need them.Thoughts? I am inclined to go the first route since the second requires more complexity on the user side but the second does at least have the advantage (or danger) of allowing more control over how the Coder binary executes.I also considered substitution in the proxy command (something similar tocoder $(/path/to/credentials-process) ssh dev.dev --stdio
) but I am not sure that is portable.Sorted the above,
--header-command
has been added to the CLI although we will need the next release before this can be used in the plugin.Closes#76 and supersedes#81.