Extended Proxy Support

[steve-norman-rft]

I see #329 added proxy environment support but it is a very simple implementation that only handles https_proxy.

The person that maintains the proxy agent has an expanded version, https://github.com/TooTallNate/node-proxy-agent, which should do a better job of handling https_proxy and also no_proxy which isn't currently being handled here.

Basic testing seems to show the following works as expected with the core octokit library, adding in proxy depending on both the https_proxy and no_proxy variables.

  const ProxyAgent = require('proxy-agent');

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

[gr2m]

your example looks good to me, shall we add it to the docs with some explanation, or is it something you'd like to see changed in code instead?

[steve-norman-rft]

I think there are probably two options (maybe three).

1. Change action.js to replace the code added for proxy support with this module
2. Add documentation to octokit/rest for how to use proxy
3. Add this into octokit/rest so that people don't have to provide their own implementation.

1 and 2 should be pretty simple. Not sure about 3, that might be a bit more awkward / ethical but a call you could probably make.

[gr2m]

you are talking about octokit/rest but this octokit/action, it's indpendent. They both use similar plugins. I would not add the proxy implementation to octokit/rest. But if it makes sense for octokit/action, we could add it, I just need to fully understand the implications.

It sounds like for now we should document it so people are aware of the module, because it seems to address some problems? We know that today's docs are ... not good. I would probably add a section to https://github.com/octokit/octokit.js/ (which is basically the successor of octokit/rest) under https://github.com/octokit/octokit.js/#octokit-api-client

[steve-norman-rft]

Yes, I'm aware this is the octokit/action. The action I'm using was written by the GitHub Engineering team and it currently uses the basic octokit and didn't work with proxy servers. So I forked and fixed it to use the proxy-agent module. I raised a PR and someone suggested that octokit/action might make more sense to be used as it is an action, and "supports" proxy servers. The main issue with the support currently in octokit/action is that it isn't complete, hence raising the issue here as it would make sense to add full support to octokit/action before changing the GitHub Engineering action to use it.

Documentation place makes sense.

The main problem I have is that if you are writing something that makes use of octokit, any of the variants, then adding the proxy is pretty easy now with the proxy-agent appearing to work pretty well. The problem is where you are trying to use something which consumes octokit as if they don't allow customisation of the octokit parameters then you are stuck and have to fork and fix which isn't always nice to do. I understand that you are also reluctant to add proxy-agent to the base octokit, but it would certainly help the scenarios where you are trying to reuse existing code (and actions are probably a good example of where it is a pain having to customise and makes reuse a pain).

[gr2m]

I understand that you are also reluctant to add proxy-agent to the base octokit, but it would certainly help the scenarios where you are trying to reuse existing code (and actions are probably a good example of where it is a pain having to customise and makes reuse a pain).

The problem is that Octokit is not only used with Node, but also in other JS runtime environments, such as Browsers, Deno, Cloudflare Workers, etc.

The problem is where you are trying to use something which consumes octokit as if they don't allow customisation of the octokit parameters then you are stuck and have to fork and fix which isn't always nice to do

You should be able to pass the request.agent option also per request.

[steve-norman-rft]

The problem is that Octokit is not only used with Node, but also in other JS runtime environments, such as Browsers, Deno, Cloudflare Workers, etc.

OK, that makes more sense, would be helpful if nodejs actually followed the proxy "standards" by default, it would make life a lot easier.

You should be able to pass the request.agent option also per request.

Yes I can, but unless the action / module exposes a way of passing that agent to the octokit it uses then the only option is to fork and customise.

I guess the fix for now is to make octokit/action a bit more robust in the handling of proxy server environment variables (by using proxy-agent instead of https-proxy-agent).

Then add something to the documentation, I think proxy questions have come up before and proxy-agent seems a simple implementation for most people.

[gr2m]

The action I'm using was written by the GitHub Engineering team

Which action is it?

You should be able to pass the request.agent option also per request.

Yes I can, but unless the action / module exposes a way of passing that agent to the octokit it uses then the only option is to fork and customise.

What I mean is you can set when calling octokit.request or octokit.graphql, using the request parameter, e.g.

octokit.request("GET /repos/{owner}/{repo}", {
  owner,
  repo,
  request: { agent }
})
// or if you have the REST API endpoint plugin
octokit.rest.repos.get({
  owner,
  repo,
  request: { agent }
})

[steve-norman-rft]

Which action is it?

https://github.com/ActionsDesk/enterprise-members-report-action

What I mean is you can set when calling octokit.request or octokit.graphql, using the request parameter, e.g.

octokit.request("GET /repos/{owner}/{repo}", {
  owner,
  repo,
  request: { agent }
})
// or if you have the REST API endpoint plugin
octokit.rest.repos.get({
  owner,
  repo,
  request: { agent }
})

In this instance I'm just trying to use an action defined by someone else that makes calls to the GitHub API using octokit. So I don't get an octokit object I can use, it just does everything I need it to and gives me content back. I've raised a PR for that action to support proxy servers, but it might also make sense to change that action to use octokit/action rather than rest directly (although that will need testing).

This is what I mean about reuse, it is hard when trying to consume something that already exists that doesn't allow configuration of everything (which isn't an easy thing to implement).

[gr2m]

I don't get an octokit object I can use

Ok I got it now, thanks for the explanation!

I've raised a PR for that action to support proxy servers, but it might also make sense to change that action to use octokit/action rather than rest directly

I commented on the PR

[gr2m]

Okay I definitely want us to implement what is documented at https://docs.github.com/en/actions/hosting-your-own-runners/using-a-proxy-server-with-self-hosted-runners, and my understanding is that the current proxy agent we use does not take all 3 env variables into account, but proxy-agent does, so let's use that :) We should also document it in the README and reference GitHub's official docs.

Can you start a pull request?

[steve-norman-rft]

I need to stop looking at this today before my head implodes, but here is what I'm trying to change on the tests for the proxy.

Changes made to the action source:

diff --git a/src/index.ts b/src/index.ts
index 7ab0fef..7405cf8 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -6,7 +6,7 @@ export { RestEndpointMethodTypes } from "@octokit/plugin-rest-endpoint-methods";
 
 import { VERSION } from "./version";
 import { OctokitOptions } from "@octokit/core/dist-types/types";
-import { HttpsProxyAgent } from "https-proxy-agent";
+import ProxyAgent from "proxy-agent";
 
 const DEFAULTS = {
   authStrategy: createActionAuth,
@@ -22,7 +22,7 @@ export const Octokit = Core.plugin(
     ...DEFAULTS,
     ...options,
     request: {
-      agent: getHttpsProxyAgent(),
+      agent: new ProxyAgent(),
       ...options.request,
     },
   };

Which in theory should "default" to using proxy-agent but can be overridden I believe (well that it what I think the existing code does).

Obviously the tests need to be changed, specifically

expect(ProxyAgent).toHaveBeenCalledWith("https://127.0.0.1");

and

expect((call[1] as RequestOptions).agent).toBeInstanceOf(HttpsProxyAgent);

Because proxy-agent does everything behind the scenes the tests could be easily updated to just check that the ProxyAgent was called:

expect(ProxyAgent).toHaveBeenCalled();
expect((call[1] as RequestOptions).agent).toBeInstanceOf(ProxyAgent);

But what I think would be better, if possible, is to show that ProxyAgent has actually picked the correct proxy (or no proxy). I was hoping to use Jest.spyOn but it isn't an object and nothing I have done today has worked. I also looked at trying to spy on the module that is used to work out the proxy url, require('proxy-from-env').getProxyForUrl but ran into similar issues. Not enough experience with Jest / Typescript I think (or I'm overthinking it).

(Note that I was going to work on the README change for proxies but it seemed that fully implementing the proxy support first before updating the README seemed the more sensible approach).

[gr2m]

But what I think would be better, if possible, is to show that ProxyAgent has actually picked the correct proxy (or no proxy)

I don't think we need to do that. We will start testing the internals of ProxyAgent. I don't mind if we do, but it's not a deal breaker if it becomes to complicated.

Do you want to try to give this another go or shall I have a look as soon as I find time for it?

[steve-norman-rft]

Do you want to try to give this another go or shall I have a look as soon as I find time for it?

#349 should be the bare minimum already. I just need to test that it actually works in the action module.

[github-actions]

🎉 This issue has been resolved in version 3.15.7 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀