- Notifications
You must be signed in to change notification settings - Fork35
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 to0423107Comparekylecarbs commentedAug 8, 2023
Option 1 seems correct to me as well. For the naming, should we call this |
code-asher commentedAug 8, 2023
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 |
code-asher commentedAug 8, 2023
Or |
liorb-canva commentedAug 9, 2023
@code-asher option 1 seems fine as well for us |
0423107 to534be09CompareThis was causing issues with the header flag as those contain equalsigns.
36c842b to2625f25Comparecode-asher commentedAug 14, 2023
I 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 to811673fCompareI got this once, not sure how to reproduce it now though.
Forgot escape adds its own wrapping quotes.
src/extension.ts Outdated
| ...config, | ||
| headers:{ | ||
| ...(awaitstorage.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 toa7f21beCompare
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:
--headerI seem to get the value set twice. For example if I usecoder --header X-MY-CUSTOM-HEADER=hello-therethen on the server end I seex-my-custom-header: hello-there, hello-thereso we should look into that.CODER_URLright 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-processor something like that. The CLI executes the process just as the extension does and sets the headers. We set--credentials-processin 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--headerflags 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-flagswheremy-wrapperis 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-commandhas 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.