fix: Correctly handle proxies with undici

[PeterC89]

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #2259 🦕

This PR does a number of things:

• Removes redundant request options
• Removes old http-proxy-agent based proxying as this isn't supported by native node fetch
• Adds undici's EnvHttpProxyAgent to support proxies using standard environment vars
• Bumps minimum node version to 20 as 18 is EOL

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[meiswjn]

Nice PR!
@chingor13 any ETA on this already? Thanks!

[kauppfbi]

Hey @chingor13, is there any update on this PR? Any plans to introduce and release it soon, or do you plan a different implementation for #2259?

[schulmak]

Any updates on this PR? We would really appreciate a merge if nothing blocks it!

[hmrc87]

@chingor13 heavily needed here

[ferrarimarco]

Hi @PeterC89. Can you please rebase against the latest main, and elaborate on the changes you're proposing? thanks.

[meiswjn]

@ferrarimarco Thanks for the response. I think Peter explained it already, especially in #2259 - what part is unclear? Perhaps I can also jump in.
@PeterC89 can you rebase this? Alternatively, @ferrarimarco you could also rebase it as the maintainer?

[ferrarimarco]

There are a bunch of changes in this PR, but there's no explanation about why they are needed. 😄

[PeterC89]

@ferrarimarco
The current implementation doesn't work due to:

1. A move to node native fetch / undici in the underlying ocktokit libraries, which doesn't use the same agent method and instead uses dispatchers for handling proxies.

2. The original implementation is incorrect anyway for proxy connections using HTTP CONNECT, as it wrongly assumes that to connect to a https endpoint, you have to use a https proxy, which is not the case.

Additionally Node18 was removed as it was EOL, at this time GitHub are also deprecating Node20 on their runners as it goes EOL in April.

Given that is has been nearly 2 years since the original issue was raised we have moved on as a team from trying to use release-please as it seemed like there was no support forthcoming to solve this issue.
I will look to rebase this PR or I can close if preferred.

[ferrarimarco]

Thanks for updating the PR description.

I suggest limiting changes in this PR to what's absolutely necessary to fix the issue, so review is easier.

Also, I'd suggest to add tests where applicable. I see that this PR removes tests, and doesn't add any, so might be useful to add some. Note that we don't look at testing the proxy feature end-to-end, but rather baseline testing to check that we have everything in place as needed.

[PeterC89]

@ferrarimarco The existing tests are wrong anyway, hence why they were removed.
It's difficult to test that a proxy is working without end to end testing but I'll take a look.

[PeterC89]

@ferrarimarco I remember the other reason for bumping to node 18 now, Uncidi needs File from Buffer which isn't available until Node 18.13
Additionally the latest version of undici, which I assume would be desired to ensure there are minimal security vulnerabilities, requires Node 20.
Therefore I will continue with requiring Node 20 as part of this PR.

[meiswjn]

@ferrarimarco Would it be possible to review this before another rebase is necessary? :) I guess Peter provided the changes requested. Thanks!