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.

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

[JENKINS-62779] handle proxy hosts exclusions #133

Merged
merged 2 commits into from
Apr 20, 2021

Conversation

thomasgl-orange
Copy link
Contributor

@thomasgl-orange thomasgl-orange commented Apr 12, 2021

See JENKINS-62779.

This is a follow-up to #124, recently released in 1.5.5. This PR has introduced support for using an HTTP proxy server (the one configured in the Jenkins Update Center settings) when connecting to a GitLab server. But the proxy exclusions settings (the "No Proxy Host" list) is ignored with this implementation. Quoting @jbdelpech:

Unfortunately, ProxyClientConfig does not support no_proxy parameter. At least, there is few chance to have multiple gitlab servers behind proxy and directly reachable.

So, this was actually an improvement for some users, but also a regression for some others:

  • people stuck behind a corporate proxy for internet access, who want to reach gitlab.com => 1.5.5 fixes their use case
  • people stuck behind a corporate proxy for internet access, who want to reach an internal (on-premise) GitLab server => used to work fine up to 1.5.4, may break with 1.5.5 (if the proxy is restricted to internet only). And deconfiguring the Jenkins Update Center proxy is not an option, since these settings are not specific to the GitLab Branch Source plugin (they're used by many other plugins, by some core features like some tools installers, and by the Update Center itself).

This PR adds support for taking the "no proxy" hosts list into account. We know the target GitLab server host, thus we can test whether it matches the proxy exclusions list or not, and if it does we can avoid passing proxy properties to the gitlab4j API.
A trick for testing the "no proxy" hosts list for a given target host is to use ProxyConfiguration.getNoProxyHostPatterns() (see usage examples).

There are no new unit tests in this PR. I only did minimal manual testing via the "Test Connection" button with different proxy settings, and also checked that existing unit tests are still okay.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

CC for review: @jbdelpech, @markyjackson-taulia and @eugenelesnov (from #124), and @LinuxSuRen (because this fixes a regression of the recently released version 1.5.5)

@efriandika
Copy link

I agree with @thomasgl-orange
I found the problem too at my side.. It breaks all of our current pipeline.. :)
Hopefully this PR would be merged shortly :)

@stuartgrieve
Copy link

This issue has impacted all of our pipelines too!

Copy link
Contributor

@jbdelpech jbdelpech left a comment

Choose a reason for hiding this comment

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

Hello, thanks @thomasgl-orange for proposing a fix, I’m sorry to see that my last PR broke your pipelines 🙇‍♂️ . I did not identified your use case.

I made a small feedback.

@thomasgl-orange
Copy link
Contributor Author

Build failed for infra issues, I will close and reopen the PR to force a rebuild.

@thomasgl-orange
Copy link
Contributor Author

@efriandika, @stuartgrieve:
An incremental build is available here, if you want to try that rather than downgrading to 1.5.4:
https://repo.jenkins-ci.org/incrementals/io/jenkins/plugins/gitlab-branch-source/1.5.6-rc596.c99e1c847ee7/gitlab-branch-source-1.5.6-rc596.c99e1c847ee7.hpi
(you can upload the .hpi file in the "Advanced" tab of the Update Center)

@efriandika
Copy link

Great @thomasgl-orange...
I will try it in next few hours..

Thanks for your works...

Copy link
Contributor

@TobiX TobiX left a comment

Choose a reason for hiding this comment

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

I was affected by this (GitLab on-premise, but proxy needed for internet connection) and this fixed it for me (tested with the incremental)

@cobexer
Copy link

cobexer commented Apr 19, 2021

Can we expect a speedy release of the updated plugin?

Copy link
Member

@LinuxSuRen LinuxSuRen left a comment

Choose a reason for hiding this comment

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

LGTM

hi @thomasgl-orange, thanks for your excellent PR.

@LinuxSuRen LinuxSuRen merged commit ae2e6e7 into jenkinsci:master Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants