add proxy support using hpagent #374
Conversation
|
Hello, is it possible to get some eyes on this review? |
There was a problem hiding this comment.
Hey @clarktlaugh, thanks for taking this on!
I have a couple of small comments to address before we can merge this but the bulk of the improvement looks good. My main concern is whether or not specifying the proxy address in the documented inputs would make this feature easier to discover and use for people.
| @@ -45,6 +45,7 @@ | |||
| "homepage": "https://github.com/hashicorp/vault-action#readme", | |||
| "dependencies": { | |||
| "got": "^11.8.5", | |||
| "hpagent": "^1.0.0", | |||
There was a problem hiding this comment.
I see the latest version is 1.2.0 if we want to start with an up-to-date new dependency
| @@ -37,6 +39,26 @@ async function exportSecrets() { | |||
| } | |||
| } | |||
|
|
|||
| if (process.env['http_proxy'] || process.env['HTTP_PROXY']) { | |||
There was a problem hiding this comment.
We should add this setting in the list of inputs supported by the action so it is explicit and documented.
Unless these environment variables have a special meaning and are required for the agent to work properly?
There was a problem hiding this comment.
These are pretty standard environment variables that, if needed, could be added to the .env file for a self-hosted runner, negating the need for them to be explicitly provided in the action inputs.
When using this action on github.com, these variables would not be set and would not be needed. They are only necessary in a corporate environment where network "compartments" might require a proxy to be configured in order to reach them.
That's why I did not add them as inputs, but rather just made the decision to pick them up from the runner's environment.
There was a problem hiding this comment.
I think it make sense since they are standard, not vault-specific and the use case is somewhat niche. It's probably worth a mention in the README though, otherwise people would need to read the source code to know proxy servers are supported by the action.
| proxy: process.env['https_proxy'] || process.env['HTTPS_PROXY'] | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
We generate the dist/index when releasing new versions its fine to not commit it if you prefer. It often leads to annoying merge conflicts to resolve.
There was a problem hiding this comment.
I wasn't sure about that part... whether it needed to be added to the commit or not. I can definitely remove it.
| maxFreeSockets: 256, | ||
| proxy: process.env['https_proxy'] || process.env['HTTPS_PROXY'] | ||
| }) | ||
| } |
There was a problem hiding this comment.
I understand this might be very difficult to test functionally but can we add a small UT that at least checks the agent is set or unset depending of the config received?
|
Thank you for the feedback -- I'll get working on updating the PR. |
maxcoulombe
added
the
waiting-for-response
|
Thanks for the submission! Are you still interested in carrying this forward? |
Yes, sorry -- it's been a while since I've checked back here. I will update my PR in the next few days. |
|
hi, news about this PR? |
|
Hello, it would be really useful to add these proxy capabilities. |
This PR adds support for using an http proxy server using the
hpagentpackage (https://www.npmjs.com/package/hpagent). It only makes changes to thedefaultOptionsif one of the regular proxy EVs is set (http(s)_proxy,HTTP(s)_PROXY).This change is important in order to allow this action to work within a corporate environment that requires the use of an http proxy to communicate between runners and a Github Enterprise deployment in isolated network compartments.
Resolves #138