feat: add http and https proxy support

[jbdelpech]

I propose this PR to add HTTP and HTTPS proxy support.

The point is to support corporate proxy based on HTTP_PROXY/HTTPS_PROXY environment variables.

My code is based on Got proxy support recommandation.

I chose to use hpagent lib because it looked like it is the most maintained.

I tested with 2 configurations :

• No proxy
• HTTPS Proxy (no auth)

According to hpagent library, you can add username:password in proxy url to pass through authenticated http/https proxy.

Feel free to propose any improvments, I'm kind of new here :)

[jbdelpech]

Hello there,

Would you mind to review this PR please ?

@nfriend, not sure if it is your responsability ?

[nfriend]

@jbdelpech Thanks for the ping! Sure, happy to review. I probably won't be able to get to this today, but I'll try to take a look Monday.

[nfriend]

Sorry, I actually won't be able to get to this today, but it's on my radar for Tuesday or Wednesday!

[nfriend]

@jbdelpech This is looking great! Thanks for putting this together! 💯

Unfortunately, I'm not able to test this without a lot of setup. Have you tested this in an environment with a proxy? I also want to make sure this works without any proxy configuration.

Left a few small comments, but overall this looks nice. Back to you 🏓

[jbdelpech]

@nfriend Thanks for your feedback. I just pushed a new update. Tell me if you want me to squash / fixup commits.

I tested configuration without proxy from my home network and with HTTPS proxy (no auth) from my enterprise network and the both works great.

Please keep me in touch for the next steps 😄

[nfriend]

@jbdelpech Awesome, thanks for the changes! This looks good to me 👍

@travi or @pvdlg, would you mind doing another round of review here and merging if this looks good to you? 🙏

[travi]

thanks for the ping @nfriend.

i'll first apologize for coming in late in the game here with a new set of questions. also, it has been a while since i've had to deal with proxy configurations, so please don't hesitate to call me crazy if i don't seem to have enough context.

i have two thoughts to share that i'd like feedback from the two of you on:

• is the additional dependency worth it for this? it feels like there isn't an obvious choice of which one to use based on the research that you mentioned. it would also mean that all consumers of this plugin would have the additional dependency installed, regardless of the need to support a proxy. since we haven't added support up to this point, i have to assume that most users of this plugin do not need proxy support, so the new dependency would not be useful to them
• the github plugin does already have at least some level of proxy support, mentioned in options and explained further here. that implementation is a bit simpler and does not require an additional dependency. that plugin is heavily used and we havent gotten feedback about it lacking necessary functionality. are there requirements that are targeted here that would not be satisfied if that implementation were used here? i think it would be beneficial if the two implementation were similar.

[zbikmarc]

I am very interested in this PR as my runners are running in separated part of network and require going through (non transparent) proxy to access GitLab API. Unfortunately I was unable to force semantic-release to talk to gitlab.com without changing code as it ignores HTTP(S)_PROXY env variables. So let me jump in and respond from my perspective:

• is the additional dependency worth it for this? it feels like there isn't an obvious choice of which one to use based on the research that you mentioned. it would also mean that all consumers of this plugin would have the additional dependency installed, regardless of the need to support a proxy. since we haven't added support up to this point, i have to assume that most users of this plugin do not need proxy support, so the new dependency would not be useful to them

Based on my corporate experience, if some company required it, they just forked repo and never pushed changes upstream. Other option would be to use transparent proxy but it is not always best solution and due to different reasons it can not be implemented everywhere.

• the github plugin does already have at least some level of proxy support, mentioned in options and explained further here. that implementation is a bit simpler and does not require an additional dependency. that plugin is heavily used and we havent gotten feedback about it lacking necessary functionality. are there requirements that are targeted here that would not be satisfied if that implementation were used here? i think it would be beneficial if the two implementation were similar.

Technically it has dependency: https://github.com/semantic-release/github/blob/bea917fe2f3253a5fd29d4bec2963ba0c9eeb87a/package.json#L27-L28
However instead of using hpagent, like in this PR, github implementation is using two packages: http-proxy-agent and https-proxy-agent to incorporate http proxy support.

Hope it helps and will speed up approval process.

[jbdelpech]

Hello @travi, thanks for your feedback. It’s a good point.

As said by @zbikmarc (thanks for your feedback too !) : semantic-release/github plugin imports HttpProxyAgent and HttpsProxyAgent.
Hpagent extends HttpProxyAgent and HttpsProxyAgent to provide a wrapper to add a proxy authentication header based on username:password in proxy url.

The only difference is that semantic-release/github plugin does not support authenticated proxy. It is not a big deal to support authenticated proxy with these agents : it just needs to add auth parameter. See httpProxyAgent code

The only remaining question is : do you want semantic-release/gitlab to support proxy by adding proxy dependencies, assuming an additional dependency cost ?

To be consistent with github plugins, I’m OK to implement proxy support by the same way than semantic-release/github plugin does.

[jbdelpech]

Hello there, 🆙 @travi I'm interested by your point of view.

[travi]

appologies for the delay here. things have been a bit crazy lately, so i will admit that the responses did slip by me initially. i appreciate your patience and the ping to bring me back here.

To be consistent with github plugins, I’m OK to implement proxy support by the same way than semantic-release/github plugin does.

i think there would be value in keeping the implementations in sync between the github and gitlab plugins for this. ideally we could find a way to extract an implementation that is actually used by both, but that is going much farther than i think is necessary to start with. if it solves your problem to match the existing implementation from the github plugin, i think it would be best to start there. that could at least get you unblocked.

if there is enough value in expanding support for things like an authenticated proxy after the initial implementation, let's get an issue opened that could be prioritized independently. just be sure that it captures details like the package you found and the fact that it builds on top of the current dependencies. i would also want to capture that we would want to enhance both plugins so we can decide if we want to make adjustments on how we keep them in sync.

[kitos9112]

After having read this thread I understand the concerns around the dependencies and that probably, the final fix needs to land in both GitLab and Github plugins in order to be equally implemented for these two plugins. Nonetheless, what would it be the interim technical debt after getting this merged and hence unblock certain implementations in corporate environments that sit behind corporate proxies?

[travi]

Nonetheless, what would it be the interim technical debt after getting this merged and hence unblock certain implementations in corporate environments that sit behind corporate proxies?

as volunteer maintainers, we have to be careful of taking on technical debt of any sort because it becomes our responsibility to continue the maintenance burden long term. we are not willing to accept implementations that we are not comfortable maintaining in order to unblock someone that is looking for a quick fix.

if it solves your problem to match the existing implementation from the github plugin, i think it would be best to start there. that could at least get you unblocked.

this is the solution that was suggested to move this forward without the need to update both plugins initially. since this PR has not been updated to accomplish this, we would be willing to review a different PR if this is pressing enough for you to contribute such a change. please still keep in mind that we are still a small group of volunteer maintainers and that it may take some time to work through getting your contribution accepted.

[kitos9112]

@travi - Yes, it's fully understandable that maintaining this repository as a volunteer, the decision of having to introduce technical debt should be carefully considered. Moreover, when adding support for proxy authentication is not normally a first-class need in average enterprise environments.

I'd love to cherry-pick the contribution made to the Github plugin already and place it here as a new PR, but I really lack NodeJS skills to satisfy that :(

[alexjfisher]

This blows up if gitlabUrl hasn't been set.

[alexjfisher]

userGitlabUrl might not be defined. So I think

Suggested change

	proxy: getProxyConfiguration(userGitlabUrl, HTTP_PROXY, HTTPS_PROXY),
	proxy: getProxyConfiguration(defaultedGitlabUrl, HTTP_PROXY, HTTPS_PROXY),

should work.

[b3nk3]

Hi there - what is the latest on this?

I can see that the PR has been accepted, but reading through it is unclear whether this will be merged in future or not.

Could you advise? @nfriend

[nfriend]

I can see that the PR has been accepted

@b3nk3 Based on this comment above, I don't think this PR has been accepted. It sounds like the recommended approach is to try to match the GitHub plugin's implementation in a new PR:

if it solves your problem to match the existing implementation from the github plugin, i think it would be best to start there. that could at least get you unblocked.

Hopefully that helps clarify!

[b3nk3]

Hey @nfriend I assumed this has been accepted as you have it as approved.

I did take a look at the github implementation, but it turns out they are using 2 packages in their get-client.js file

excerpt from their package.json

"http-proxy-agent": "^5.0.0",
"https-proxy-agent": "^5.0.0",

Hpagent extends HttpProxyAgent and HttpsProxyAgent to provide a wrapper to add a proxy authentication header based on username:password in proxy url.

Given that these packages have not been updated in the past two years, and hpagent is still actively developed and - as opposed to the comment above, to the best of my knowledge - has no dependencies itself , I'd argue that it may be best to revisit the validity of this PR.

[travi]

Given that these packages have not been updated in the past two years

what is the downside of these being stable packages? is there something pressing about them that is not being updated, but needs to be? lack of changes in a package is not a negative thing by itself

[b3nk3]

what is the downside of these being stable packages? is there something pressing about them that is not being updated, but needs to be? lack of changes in a package is not a negative thing by itself

it's not so much as them being stable or maintained, but they have dependencies, as opposed to hpagent, that only has dev dependencies. You have pointed this out yourself.

is the additional dependency worth it for this?

Actually, staying at that point, adding 1 dependency over 2, is already a win. Especially, when the 1 added dependency does not rely on anything else. Wouldn't you agree?

[travi]

Actually, staying at that point, adding 1 dependency over 2, is already a win.

a diverging implementation that does not get us closer to an implementation that could be extracted and shared between the two plugins is not a win for longer term maintenance or enabling updates of the proxy implementation to benefit both.

as has been clarified several times in this thread, the best way to move this request forward is to help us match the existing implementation to enable us to extract a shared implementation. once that is complete, we would be more interested in the trade-offs around how the implementation could be improved, but we need to get past those steps before any of those benefits have potential payoff.

[b3nk3]

This makes sense of course, tho not entirely, as if we have a better solution already (I'd argue we do), why not abstract it away, and implement that in the GitHub plugin?

I found it bizarre that we should be implementing a mediocre solution just for the sake of it, rather then improving, then rolling out that improved solution where it is needed?

[naffers]

Looking at the GitHub module the HTTP client in use there is @octokit/rest which I think is using node-fetch under the hood.
Whereas the currently implemented HTTP client for the GitLab module is got.

Looking at the docs on proxy support for got they appear to recommend using hpagent which this PR and #310 have implemented.

Whether got works nicely with http-proxy-agent and https-proxy-agent which is what is being suggested with making it match the GitHub implementation I can't easily discern.
I suspect not if their note:

The proxy-agent family doesn't follow newest Node.js features and lacks support.

Is a reference to that family of proxy modules that both the above belong to.

So if we're to follow through with the suggested change to align the 2 modules, we're likely going to need to swap out the usage of got for node-fetch to allow us to smoothly integrate with those 2 *proxy-agent modules without the requirement for another additional dependency, and/or some glue code to make them work with got.

Is this definitely an appropriate resolution for this, considering we know the proxy support added via this PR definitely works with the currently implemented HTTP client library?

Switching out HTTP client library to one that still isn't the same as the GitHub module sounds like additional effort/risk for little gain considering it'll still differ between the 2 modules as @octokit/rest is a GitHub API specific library.

Would appreciate if you can reconsider such ask and get either this or #310 approved, merged and released.

edit: Raised a PR #316 that hopefully will help drive this discussion forward

[fgreinacher]

Closing this in favor of #310, thanks for your work so far @jbdelpech!