Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: remove custom proxy handling #143

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

fix: remove custom proxy handling #143

wants to merge 15 commits into from
+739 −961

Conversation

parkerbxyz
Copy link
Contributor

@parkerbxyz parkerbxyz commented Jun 13, 2024 •
edited
Loading

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.

@parkerbxyz
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 parkerbxyz self-assigned this Jun 13, 2024
@parkerbxyz parkerbxyz linked an issue Jun 13, 2024 that may be closed by this pull request
Remove custom proxy functionality #134
Open
@gr2m gr2m force-pushed the 134-remove-custom-proxy-functionality branch from 47d08ea to 98eac26 Compare June 13, 2024 23:11
@parkerbxyz
Copy link
Contributor Author

parkerbxyz commented Jun 13, 2024 •
edited
Loading

👋 @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? 🙏

@gr2m gr2m self-assigned this Jun 14, 2024
@dmitrijrub
Copy link

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

@parkerbxyz
Copy link
Contributor Author

parkerbxyz commented Jun 14, 2024 •
edited
Loading

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

Yes, exactly! 🙂

@dmitrijrub
Copy link

dmitrijrub commented Jun 17, 2024 •
edited by gr2m
Loading

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
Copy link
Contributor Author

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

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

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
gr2m reviewed Sep 4, 2024
View reviewed changes
lib/request.js
@@ -1,41 +1,10 @@
import core from "@actions/core";
import { request } from "@octokit/request";
import { ProxyAgent, fetch as undiciFetch } from "undici";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
gr2m reviewed Sep 7, 2024
View reviewed changes
lib/request.js
/* c8 ignore next */
request: proxyUrl ? { fetch: proxyFetch } : {},
request: {
dispatcher: envHttpProxyAgent,
Copy link
Contributor

@gr2m gr2m Sep 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gr2m gr2m gr2m left review comments

@bruce-c-liu bruce-c-liu bruce-c-liu left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@gr2m gr2m

@parkerbxyz parkerbxyz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove custom proxy functionality
4 participants
@parkerbxyz @dmitrijrub @gr2m @bruce-c-liu