Proxy environment variable support

[steve-norman-rft]

Working through this with @droidpl last week and found that octokit doesn't work with proxies without some customisation.

I think this is probably the simplest solution (and possibly worth speaking to the Octokit team about implementing something similar rather than making any consumers have to re-implement this every time).

Some sample code that I used to test various scenarios:

(async () => {
  const { Octokit } = require("@octokit/rest");
  const ProxyAgent = require('proxy-agent');

  console.log(`https_proxy: ${process.env.https_proxy}`)
  console.log(`no_proxy: ${process.env.no_proxy}`)

  const octokitProxy = new Octokit({
    request: { agent: new ProxyAgent()}
  })

  const proxyMeta = await octokitProxy.meta.get()
  console.log("Proxy Request")
  console.log(proxyMeta.data.hooks)
})()

And some results.

No proxy environment variables set, octokit call times out:

:~/ghtest> node proxy.js 
https_proxy: undefined
no_proxy: undefined
(node:3742) UnhandledPromiseRejectionWarning: HttpError: request to https://api.github.com/meta failed, reason: connect ETIMEDOUT 140.82.112.6:443

Configured with https_proxy and a no_proxy. The url being requested is api.github.com so no_proxy doesn't match in this case so works:

:~/ghtest> https_proxy=http://webproxy:80 no_proxy=github.com node proxy.js 
https_proxy: http://webproxy:80
no_proxy: github.com
Proxy Request
[
  '192.30.252.0/22',
  '185.199.108.0/22',
  '140.82.112.0/20',
  '143.55.64.0/20',
  '2a0a:a440::/29',
  '2606:50c0::/32'
]

no_proxy is prefixed with a . so should not be used for anything ending in .github.com. As expected request isn't routed through the proxy so times out:

:~/ghtest> https_proxy=http://webproxy:80 no_proxy=.github.com node proxy.js 
https_proxy: http://webproxy:80
no_proxy: .github.com
(node:4424) UnhandledPromiseRejectionWarning: HttpError: request to https://api.github.com/meta failed, reason: connect ETIMEDOUT 140.82.113.6:443


`no_proxy` is explicitly bypassing `api.github.com` and as expected the request times out:
```bash
:~/ghtest> https_proxy=http://webproxy:80 no_proxy=api.github.com node proxy.js 
https_proxy: http://webproxy:80
no_proxy: api.github.com
(node:4861) UnhandledPromiseRejectionWarning: HttpError: request to https://api.github.com/meta failed, reason: connect ETIMEDOUT 140.82.114.5:443

This time no_proxy is set to localhost, so https_proxy should be used as it works:

:~/ghtest> https_proxy=http://webproxy:80 no_proxy=localhost node proxy.js 
https_proxy: http://webproxy:80
no_proxy: localhost
Proxy Request
[
  '192.30.252.0/22',
  '185.199.108.0/22',
  '140.82.112.0/20',
  '143.55.64.0/20',
  '2a0a:a440::/29',
  '2606:50c0::/32'
]

All requests should be routed through https_proxy and appears to work:

:~/ghtest> https_proxy=http://webproxy:80 node proxy.js 
https_proxy: http://webproxy:80
no_proxy: undefined
Proxy Request
[
  '192.30.252.0/22',
  '185.199.108.0/22',
  '140.82.112.0/20',
  '143.55.64.0/20',
  '2a0a:a440::/29',
  '2606:50c0::/32'
]

I can keep using my fork, but this might be useful for other consumers (and potentially other enterprise clients who use proxy servers).

[steve-norman-rft]

I've tried to bring in the latest changes on main but it isn't building or packing:

steve@Intrepid enterprise-members-report-action % npm run-script build

> enterprise-members-report-action@1.0.1 build /Users/steve/Dev/nodejs/enterprise-members-report-action
> tsc

src/main.ts:23:20 - error TS2571: Object is of type 'unknown'.

23     core.setFailed(error.message)
                      ~~~~~

And

steve@Intrepid enterprise-members-report-action % npm run-script pack

> enterprise-members-report-action@1.0.1 pack /Users/steve/Dev/nodejs/enterprise-members-report-action
> ncc build --source-map --license licenses.txt

ncc: Version 0.30.0
ncc: Compiling file index.js into CJS
ncc: Using typescript@4.4.2 (local user-provided)
Error: [tsl] ERROR in /Users/steve/Dev/nodejs/enterprise-members-report-action/src/main.ts(23,20)
      TS2571: Object is of type 'unknown'.
    at /Users/steve/Dev/nodejs/enterprise-members-report-action/node_modules/@vercel/ncc/dist/ncc/index.js.cache.js:37:1862764
    at /Users/steve/Dev/nodejs/enterprise-members-report-action/node_modules/@vercel/ncc/dist/ncc/index.js.cache.js:37:336439
    at _done (eval at create (/Users/steve/Dev/nodejs/enterprise-members-report-action/node_modules/@vercel/ncc/dist/ncc/index.js.cache.js:20:117892), <anonymous>:9:1)
    at eval (eval at create (/Users/steve/Dev/nodejs/enterprise-members-report-action/node_modules/@vercel/ncc/dist/ncc/index.js.cache.js:20:117892), <anonymous>:34:22)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! enterprise-members-report-action@1.0.1 pack: `ncc build --source-map --license licenses.txt`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the enterprise-members-report-action@1.0.1 pack script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

Guessing something broke with yesterday's dependabot updates.

[stoe]

I'm not sure if it's worth to check to change to @octokit/action here since it already implements proxy support, rather than implementing it here manually
octokit/action.js#329

[steve-norman-rft]

As is the case with a lot of proxy implementations it appears to be a bit "basic". Will raise an issue there with the details and see what they say as well.

[gr2m]

@stoe just for my understanding, is the proxy handling something that is always needed for Actions in GHES instances? Are the environment variables always the same or can they differ/be set by the customer? I'm happy to use the superior proxy-agent dependency in @octokit/action, I just want to fully understand the context before making the change.

[stoe]

@gr2m, they won't be always needed as this depends on the implementation of the self-hosted runner, but when used they should always follow our documented pattern under https://docs.github.com/en/actions/hosting-your-own-runners/using-a-proxy-server-with-self-hosted-runners

[stoe]

Also, this is independent of GHES and would also apply for self-hosted runners on GHEC (github.com).

[gr2m]

Got it, thank you for clarifying. As it's documented in the GitHub docs I consider it best practice, and will implement it for the Action SDK (@octokit/action). I think you should be able to safely use it instead of @octokit/rest. I'm happy to help with the transition.

I do plan a bigger overhaul of @octokit/action (to make it behave more like @octokit/app) but that won't happen any time soon and when we get there, I'll help you with the upgrade as well.

[steve-norman-rft]

Updated with the fix needed to workaround changes in the latest dependabot changes in case anyone needs that.

[droidpl]

Ok so given the comments in the PR, what do you think if I do a quick update to the code and implement the octokit/action instead of the one we have and then we retest, since that one supports the proxy? @steve-norman-rft

I can spend some time tomorrow working on this.

[steve-norman-rft]

octokit/action doesn't properly support proxies yet. I've been trying to get that working today but having "fun" getting the unit tests to be something useful. Would seem more sensible to get the octokit\action working completely before changing this over to use it.

We can talk about it tomorrow though.

[droidpl]

Ok let's do that as I have a training in the morning and won't manage to get through this before our weekly. I am curious to know what is failing for you on the octokit/actions implementation.

[steve-norman-rft]

The proxy support in octokit/action only supports the http(s)_proxy and not the no_proxy variables so is incomplete and might cause issues.

The proxy-agent module should handle things a lot better from what I can tell, I'm just trying to get unit tests for it to be a bit cleverer to ensure that the proxy is being used correctly (as the proxy is now abstracted away I'm running round in circles trying to spy on the ProxyAgent module).

[droidpl]

Ok let's have the sync tomorrow and see where I can chime in. I am happy to merge this PR if that fixes entirely the proxy support but at the same time I think @gr2m should make sure this is captured as an issue to get it improved in the actions sdk 😺

[steve-norman-rft]

This PR is probably the wrong way to do it, making use of the octokit/action is probably the best way (assuming it works), but would rather that had a "safer" proxy implementation.

My fork of this is working for the time being, seems silly to make changes here now until octokit/action has better proxy support (which is where I'm banging my head at the moment)

[gr2m]

@steve-norman-rft we can chat at octokit/action.js#342, let me know what problems you run into.

[droidpl]

Apologies @gr2m I didn't know that PR existed! 😸 lets keep the discussion there

[droidpl]

X-ref as this comes from a fork that I can't edit. We can use this to replace it with the newest version of @octokit/action when additional support is added. #79 changes the version of the action from one to the other

[droidpl]

With #79 fixed, whenever we get the fixes from octokit/action.js#342 and the PR octokit/action.js#349 we should be good to go updating the dependency here

[droidpl]

Closing this PR as it's not relevant anymore with the changes already in the main branch