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

Feat/add corporate proxy support #124

Merged
merged 2 commits into from
Mar 19, 2021
Merged

Feat/add corporate proxy support #124

merged 2 commits into from
Mar 19, 2021

Conversation

jbdelpech
Copy link
Contributor

  • 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

I propose this PR in response of the following issue : https://issues.jenkins.io/browse/JENKINS-62779

The point is to support corporate proxy. Before calling GitlabApi, we retrieve proxy parameters from Plugin Manager configuration.

My code is based on gitlab4j-api proxy implementation.

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

About test : I didn't add any... but no tests have been broken. I did test the configuration on my Jenkins with this new version :

  • Without proxy
  • With unauthenticated corporate proxy

I do not have any environment to test with authenticated proxy.

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

@jbdelpech
Copy link
Contributor Author

jbdelpech commented Mar 10, 2021

Hello,

Thanks for approving this PR. Just to now, when do you plan to merge and to release it ?

Thanks

}
throw new IllegalStateException(
String.format("No server found with the name: %s", serverName));
}

public static Map<String, Object> getProxyConfig () {
Copy link
Contributor

@eugenelesnov eugenelesnov Mar 11, 2021

Choose a reason for hiding this comment

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

I think we could support https proxy settings..But i'm not sure

Copy link
Contributor Author

@jbdelpech jbdelpech Mar 11, 2021

Choose a reason for hiding this comment

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

It's a good point. In Jenkins plugin manager proxy configuration, serverUrl does not include protocol (http or https), and there is no field to specify the protocol to use.
Based on this configuration, we are not able to determine if it is http or https protocol. We could presume that if it is 80, use HTTP and 443 use HTTPS, but proxy port can still be 8443 or something else.

I think if we want to support both http and https, this plugin must have it's own proxy user settings and proxy support implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, is it good for you ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is. I wish I would have write access here..meh

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 hope so !

@jbdelpech
Copy link
Contributor Author

Hello, do you have any update about who can merge this PR ?

@markjacksonfishing markjacksonfishing merged commit ab9fdfd into jenkinsci:master Mar 19, 2021
@jbdelpech jbdelpech deleted the feat/add-corporate-proxy-support branch March 19, 2021 13:48
@madriga1
Copy link

madriga1 commented Apr 9, 2021

Hello,

Thanks for approving this PR. Just to now, when do you plan to merge and to release it ?

Thanks

We've ran into this issue when issuing a scan. Any idea of when this will be released?

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.

4 participants