Skip to content

Conversation

@schulzh
Copy link

@schulzh schulzh commented Aug 24, 2017

The Jenkins ProxyConfiguration class expects only the hostname (e.g. git.example.com vs https://git.example.com/api/) as a method parameter to createProxy, but the existing code gives it the whole URL.
As a result of this, the validation of no-proxy hosts list is not correct. This is not noticeable when using github.com (altough the https:// should not be there either), but it fails when using enterprise githubs.

@schulzh
Copy link
Author

schulzh commented Aug 28, 2017

@raul-arabaolaza Could you take a look?

@raul-arabaolaza
Copy link
Contributor

raul-arabaolaza commented Aug 28, 2017

@schulzh Gladly, code LGTM (sorry for missing this bug) but it would be great if you can edit your commit message to append [JENKINS-43370] to it so in the future if someone list the commits it makes sense. After that a quick manual test with a proxy and I will merge and release a new version if nothing wrong appears

@schulzh schulzh changed the title Fix proxy resolution for enterprise Githubs [JENKINS-43370] Fix proxy resolution for enterprise Githubs Aug 30, 2017
@schulzh
Copy link
Author

schulzh commented Aug 30, 2017

@raul-arabaolaza done.

@raul-arabaolaza
Copy link
Contributor

@schulzh Great, allow me to do some tests (need to set up a proxy) to check and if all goes fine I will merge and release another version. Probably this weekend

@raul-arabaolaza
Copy link
Contributor

@schulzh Can you give me a way to test this? I have been doing the following and the PR is not working

  • I set up a non existing proxy in the proxy configuration and tried to run the step, it obviously failed
  • I set up a non existing proxy in the proxy configuration and added api.github.com to the non proxy hosts section, it failed again when AFAIU it should have worked

Maybe I need to set up a real proxy?

@schulzh
Copy link
Author

schulzh commented Sep 12, 2017

How did you configure the proxy server in Jenkins?
It should be configured by setting the system properties http(s).proxyHost/proxyPort/nonProxyHosts, e.g. via the start command, like so: java -jar jenkins.war -Dhttp.proxyHost=localhost -Dhttp.proxyPort=8080 -Dhttps.proxyHost=localhost -Dhttps.proxyPort=8080 -Dhttp.nonProxyHosts="localhost|127.0.0.1|api.github.com"
You can check if the system properties got recognized by using the script console:
System.getProperty("http.proxyHost")

@raul-arabaolaza
Copy link
Contributor

@schulzh Yikes, my failure, I was configuring them for the plugin manager and no the entire jenkins instance it seems, will retry the test

@raul-arabaolaza
Copy link
Contributor

Well, I am having the same results... I guess I need to set up a real proxy

@stowns
Copy link

stowns commented Apr 10, 2018

this is def an issue. having to sit at 1.0.2 until this is fixed

@jvallon1
Copy link

jvallon1 commented Jul 9, 2019

@raul-arabaolaza I am trying to follow up with this issue. This PR does fix our problem, and I would like to add a unit-test to verify the behavior. What is missing is a test that attempts to use "github.company.com", with proxy configured with an exception for "company.com". In that case, the proxy code should indicate a direct connection. Further, for an attempt to use "github.com" with an exception "company.com", the proxy code should indicate the use of the proxy is required.

However, when I run "mvn test" (on master), I get occasional failures in the console, but the build of the HPI does not fail. Further, I am unable to run the tests in Eclipse (not a big deal, if native mvn worked). I don't know whether this is an issue with my setup, or with the code. In order to introduce updated unit-tests, I would like to see a successful build.

What forum should I use to follow-up or get help with this?

@marek-sezemsky
Copy link

I can confirm that this patch fixed the "The supplied credentials are invalid to login" for my use case.

@jglick
Copy link
Member

jglick commented Mar 4, 2022

@raul-arabaolaza label bug

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.

6 participants