fix: remove custom proxy handling #143

parkerbxyz · 2024-06-13T22:46:48Z

Undici has added native support for proxy handling, so it is no longer necessary for us to have our own custom proxy handling.

Reverts #102 and resolves #134.

Depends on nodejs/node#43187.

Undici has added native support for proxy handling, so it is no longer necessary for us to implement our own custom proxy handling.

parkerbxyz · 2024-06-13T23:12:45Z

👋 @dmitrijrub @mikakesan Since you helped us test proxy functionality (#99) when we added support for it in #102, would you be willing to help us test this branch (134-remove-custom-proxy-functionality) to ensure you're still able to use this action with a proxy? 🙏

dmitrijrub · 2024-06-14T07:43:24Z

hi @parkerbxyz, what are the changes? should I just use uses: actions/create-github-app-token@134-remove-custom-proxy-functionality ?

parkerbxyz · 2024-06-14T15:19:26Z

should I just use uses: actions/create-github-app-token@134-remove-custom-proxy-functionality ?

Yes, exactly! 🙂

dmitrijrub · 2024-06-17T14:00:40Z

hmmm, looks like its failing

Run actions/create-github-app-token@134-remove-custom-proxy-functionality
owner and repositories not set, creating token for the current repository ("dt-arc")
Failed to create token for "dt-arc" (attempt 1): Connect Timeout Error
Failed to create token for "dt-arc" (attempt 2): Connect Timeout Error
Failed to create token for "dt-arc" (attempt 3): Connect Timeout Error
Failed to create token for "dt-arc" (attempt 4): Connect Timeout Error
RequestError [HttpError]: Connect Timeout Error
    at /home/runner/_work/_actions/actions/create-github-app-token/134-remove-custom-proxy-functionality/dist/main.cjs:3[7](https://github.com/company-company/dt-arc/actions/runs/9512488741/job/26317660736#step:10:8)097:11
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async hook4 (/home/runner/_work/_actions/actions/create-github-app-token/134-remove-custom-proxy-functionality/dist/main.cjs:39469:1[8](https://github.com/org-company/dt-arc/actions/runs/9512488741/job/26317660736#step:10:9))
    at async getTokenFromRepository (/home/runner/_work/_actions/actions/create-github-app-token/134-remove-custom-proxy-functionality/dist/main.cjs:3[9](https://github.com/telia-company/dt-arc/actions/runs/9512488741/job/26317660736#step:10:10)783:20)
    at async RetryOperation._fn (/home/runner/_work/_actions/actions/create-github-app-token/134-remove-custom-proxy-functionality/dist/main.cjs:39660:24) {
  status: 500,
  request: {
    method: 'GET',
    url: xxxx
    headers: {
      accept: 'application/vnd.github.v3+json',
      'user-agent': 'actions/create-github-app-token',
      authorization: 'bearer [REDACTED]'
    },
    request: {
      fetch: [AsyncFunction: fetch],
      hook: [Function: bound hook4] AsyncFunction
    }
  },
  response: undefined,
  attemptNumber: 4,
  retriesLeft: 0
}
Error: Connect Timeout Error

parkerbxyz · 2024-07-29T17:52:34Z

Noting this in case it is helpful later: nodejs/undici#1650 (comment)

`EnvHttpProxyAgent` automatically reads the proxy configuration from the environment variables.

Proxy support should be automatic

parkerbxyz · 2024-09-04T01:09:50Z

hmmm, looks like its failing

Thank you so much for testing that, @dmitrijrub! I've made some changes in hopes of fixing the failure you encountered. Would you mind testing again and letting us know how it goes, please? 🙏

gr2m · 2024-09-04T03:06:42Z

lib/request.js

@@ -1,41 +1,10 @@
 import core from "@actions/core";
 import { request } from "@octokit/request";
-import { ProxyAgent, fetch as undiciFetch } from "undici";


I wonder if we need to explicitly use EnvHttpProxyAgent?
https://github.com/nodejs/undici/blob/7f635e51f6170f4b59abedc7cb45e6bcda7f056d/docs/docs/api/EnvHttpProxyAgent.md#L4

See this comment: nodejs/undici#1650 (comment)

I just wanted to leave that not for when we have another look at it, I don't have time to dig into it now

I tried that in 5cb4b2f, but several tests started failing as a result. After reading through nodejs/undici#2994, I got the impression it was unnecessary to do this.

Added EnvHttpProxyAgent and made it the default global dispatcher.

Automatically detect/support HTTP_PROXY, HTTPS_PROXY, & NO_PROXY

Super random drive-by comment. I got here from that issue. 😅

According to this commit, they ended up not using this as the default agent:
nodejs/undici@f31d3f9

Reasoning:
nodejs/undici#2994 (comment)

So it appears the PR description is wrong. Looks like they'll consider making it the default in a major release.

For now, I was able to get HTTPS_PROXY env variable to work via:

import { EnvHttpProxyAgent, setGlobalDispatcher } from "undici"; setGlobalDispatcher(new EnvHttpProxyAgent());

Thanks, @bruce-c-liu! I found some additional context here:

https://github.com/nodejs/TSC/blob/41602b95211f30107696a007e5da45b9e933536c/meetings/2024-09-04.md?plain=1#L88-L115

gr2m · 2024-09-07T22:30:42Z

lib/request.js

-  /* c8 ignore next */
-  request: proxyUrl ? { fetch: proxyFetch } : {},
+  request: {
+    dispatcher: envHttpProxyAgent,


I don't think this will do anything. There is. no dispatch option in options.request, see all implemented options in https://github.com/octokit/request.js#request

What we have to do is to implement a fetch wrapper

function fetchWithProxyAgent(url, options) { return fetch(url, { ... options, dispatcher: envHttpProxyAgent }) }

And then use pass that custom fetch implementation as options.request.fetch

Suggested change

dispatcher: envHttpProxyAgent,

fetch: fetchWithProxyAgent,

parkerbxyz · 2024-10-04T19:39:27Z

Tests are currently failing due to our current use of the global dispatcher in our tests and local dispatcher in the fetch method. The problem can be reproduced with the following example:

import { EnvHttpProxyAgent, MockAgent, setGlobalDispatcher } from "undici";

const mockAgent = new MockAgent();
mockAgent.disableNetConnect();
setGlobalDispatcher(mockAgent);
const mockPool = mockAgent.get("https://example.test");

mockPool
  .intercept({
    path: `/`,
    method: "GET",
  })
  .reply(200);

await fetch("https://example.test", {
  // Comment out the following line to see the test succeed
  dispatcher: new EnvHttpProxyAgent(),
});

console.log("OK");

The global dispatcher is not being used when a local dispatcher is set. We need to verify if this is a feature or a bug.

parkerbxyz · 2024-10-31T23:15:45Z

The global dispatcher is not being used when a local dispatcher is set. We need to verify if this is a feature or a bug.

nodejs/undici#3792

parkerbxyz · 2024-11-01T00:39:21Z

Deno appears to have native proxy support: https://docs.deno.com/runtime/reference/env_variables/#special-environment-variables

gr2m · 2024-11-01T16:01:14Z

Deno appears to have native proxy support

I'm down for using Deno instead of Node.js, as long as we still generate the built files with all dependencies and execute that code instead of loading dependencies dynamically at run time.

peter-evans · 2024-11-04T13:06:05Z

I just received a really helpful PR here: peter-evans/create-pull-request#3475

And that lead me to realise that proxy support can be simplified to this: peter-evans/create-pull-request#3483

It passes the proxy support tests I have, so looks good.

Just thought I would mention this in case it helps here.

gr2m · 2024-12-19T23:36:12Z

Thank you @peter-evans! It looks like you use node-fetch-native as a dependency which depends on node-fetch, which is a huge dependency. We try to keep this action slim, so I'd like to avoid that.

We are interested in your test setup though and couldn't quite figure it out. Can you give us some pointers how you test the env-based proxy handling?

peter-evans · 2024-12-20T08:55:05Z

It looks like you use node-fetch-native as a dependency which depends on node-fetch, which is a huge dependency. We try to keep this action slim, so I'd like to avoid that.

I see. Well I'm interested to see what solution you end up with here then.

We are interested in your test setup though and couldn't quite figure it out. Can you give us some pointers how you test the env-based proxy handling?

I created a small docker image to launch a forward proxy based on glider. In the test, I start the forward proxy and then configure the firewall on the actions runner. The firewall settings deny outgoing, except through the proxy. So it makes sure that the action is definitely using the proxy when it's configured.

fix: remove custom proxy handling

ae98c18

Undici has added native support for proxy handling, so it is no longer necessary for us to implement our own custom proxy handling.

parkerbxyz self-assigned this Jun 13, 2024

parkerbxyz linked an issue Jun 13, 2024 that may be closed by this pull request

Remove custom proxy functionality #134

Open

fix: use unidici fetch

98eac26

gr2m force-pushed the 134-remove-custom-proxy-functionality branch from 47d08ea to 98eac26 Compare June 13, 2024 23:11

Update code comment

fa33f86

gr2m self-assigned this Jun 14, 2024

parkerbxyz added 4 commits September 3, 2024 16:15

fix: implement EnvHttpProxyAgent

5cb4b2f

`EnvHttpProxyAgent` automatically reads the proxy configuration from the environment variables.

Merge branch 'main' into pr/143

28dfb1e

Merge branch 'main' into 134-remove-custom-proxy-functionality

19e441f

Update node version for undici proxy support

1f00388

parkerbxyz force-pushed the 134-remove-custom-proxy-functionality branch from bbbe985 to 1f00388 Compare September 4, 2024 00:18

parkerbxyz added 4 commits September 3, 2024 17:21

Don't warn if using a node version greater than 20

6aa1ac7

Remove EnvHttpProxyAgent implementation

073d254

Proxy support should be automatic

Update package-lock.json

adf089d

Update dist files

4e61ab3

gr2m reviewed Sep 4, 2024

View reviewed changes

parkerbxyz added 4 commits September 6, 2024 13:33

Merge branch 'main' into pr/143

ed89981

Re-implement EnvHttpProxyAgent as local agent

02b34af

Use native fetch

ff0675e

Update dist files

7a505dc

gr2m reviewed Sep 7, 2024

View reviewed changes

gr2m mentioned this pull request Oct 7, 2024

Support custom permissions #3

Open

parkerbxyz added 2 commits October 31, 2024 16:18

Merge branch 'main' into 134-remove-custom-proxy-functionality

a0d0cbe

Re-implement global dispatcher

489a969

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove custom proxy handling #143

fix: remove custom proxy handling #143

parkerbxyz commented Jun 13, 2024 •

edited

Loading

parkerbxyz commented Jun 13, 2024 •

edited

Loading

dmitrijrub commented Jun 14, 2024

parkerbxyz commented Jun 14, 2024 •

edited

Loading

dmitrijrub commented Jun 17, 2024 •

edited by gr2m

Loading

parkerbxyz commented Jul 29, 2024

parkerbxyz commented Sep 4, 2024

gr2m Sep 4, 2024

parkerbxyz Sep 4, 2024

bruce-c-liu Sep 6, 2024

parkerbxyz Sep 6, 2024

gr2m Sep 7, 2024 •

edited

Loading

parkerbxyz commented Oct 4, 2024 •

edited

Loading

parkerbxyz commented Oct 31, 2024

parkerbxyz commented Nov 1, 2024

gr2m commented Nov 1, 2024

peter-evans commented Nov 4, 2024

gr2m commented Dec 19, 2024

peter-evans commented Dec 20, 2024

fix: remove custom proxy handling #143

Are you sure you want to change the base?

fix: remove custom proxy handling #143

Conversation

parkerbxyz commented Jun 13, 2024 • edited Loading

parkerbxyz commented Jun 13, 2024 • edited Loading

dmitrijrub commented Jun 14, 2024

parkerbxyz commented Jun 14, 2024 • edited Loading

dmitrijrub commented Jun 17, 2024 • edited by gr2m Loading

parkerbxyz commented Jul 29, 2024

parkerbxyz commented Sep 4, 2024

gr2m Sep 4, 2024

Choose a reason for hiding this comment

parkerbxyz Sep 4, 2024

Choose a reason for hiding this comment

bruce-c-liu Sep 6, 2024

Choose a reason for hiding this comment

parkerbxyz Sep 6, 2024

Choose a reason for hiding this comment

gr2m Sep 7, 2024 • edited Loading

Choose a reason for hiding this comment

parkerbxyz commented Oct 4, 2024 • edited Loading

parkerbxyz commented Oct 31, 2024

parkerbxyz commented Nov 1, 2024

gr2m commented Nov 1, 2024

peter-evans commented Nov 4, 2024

gr2m commented Dec 19, 2024

peter-evans commented Dec 20, 2024

parkerbxyz commented Jun 13, 2024 •

edited

Loading

parkerbxyz commented Jun 13, 2024 •

edited

Loading

parkerbxyz commented Jun 14, 2024 •

edited

Loading

dmitrijrub commented Jun 17, 2024 •

edited by gr2m

Loading

gr2m Sep 7, 2024 •

edited

Loading

parkerbxyz commented Oct 4, 2024 •

edited

Loading