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

Don't let proxy configuration interfere with remote execution. #6340

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@EdSchouten
Copy link
Contributor

EdSchouten commented Oct 9, 2018

The existing proxy helper code adjusts JVM properties for setting the
HTTP proxy. This is, based on the source code comments, done to make
jgit use the right proxy.

This code is problematic for three reasons:

  • The properties are never cleared between
    ProxyHelper.createProxyIfNeeded() calls, meaning that if a successive
    Git repository were to be part of NO_PROXY, it would still use the
    previously configured proxy.

  • Credentials are never cleared, meaning that credentials belonging to
    one proxy may later be sent to the next one.

  • The proxy settings end up being used by gRPC for remote execution
    later on, which completely breaks remote builds.

As the new git_repository() no longer uses jgit, remove the code that
sets the JVM properties. It's simply too brittle.

Don't let proxy configuration interfere with remote execution.
The existing proxy helper code adjusts JVM properties for setting the
HTTP proxy. This is, based on the source code comments, done to make
jgit use the right proxy.

This code is problematic for two reasons:

- The properties are never cleared between
  ProxyHelper.createProxyIfNeeded() calls, meaning that if a successive
  Git repository were to be part of NO_PROXY, it would still use the
  previously configured proxy.

- Credentials are never cleared, meaning that credentials belonging to
  one proxy may later be sent to the next one.

- The proxy settings end up being used by gRPC for remote execution
  later on, which completely breaks remote builds.

As the new git_repository() no longer uses jgit, remove the code that
sets the JVM properties. It's simply too brittle.

@googlebot googlebot added the cla: yes label Oct 9, 2018

@ulfjack ulfjack requested review from aehlig and buchgr Oct 11, 2018

@ulfjack

This comment has been minimized.

Copy link
Contributor

ulfjack commented Oct 11, 2018

I like it, but I don't know if anything else might be reading the system property.

@bazel-io bazel-io closed this in 043064f Oct 12, 2018

@ulfjack

This comment has been minimized.

Copy link
Contributor

ulfjack commented Oct 12, 2018

This was approved by @buchgr.

@EdSchouten EdSchouten deleted the EdSchouten:bazel-re-proxy branch Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment