core,okhttp,netty,alts,testing: Plumb proxy resolved addr to transports #4137

zpencer · 2018-02-24T00:07:30Z

This PR has two commits, and the second commit has a lot of low
impact code churn due to an internal API cleanup.

First commit:
ProxyDetector should resolve proxy address and plumb results

ProxyDetector is now responsible for resolving the proxy's
`InetSocketAddress`, and `ProxyParameters` asserts that the address is
resolved. The results are plumbed through using a special subclass of
`SocketAddress`.

If a proxy should be used but the proxy can not be resolved, the
`DnsNameResolver` will re-attempt the resolution later.

Remove the unit test testing for unresolved proxy addresses, since
it's no longer applicable.

Second commit:
Plumb ProxiedInetSocketAddress down to transports

Do not extract the `ProxyParameters` field and pass it into
`ClientTransportFactory.newClientTransport`.

fixes #4029

ejona86 · 2018-03-01T17:51:06Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

+          final ProxyParameters proxy;
+          try {
+            proxy = proxyDetector.proxyFor(destination);
+          } catch (Exception e) {


Just catch IOException?

ejona86 · 2018-03-01T17:53:14Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

          if (proxy != null) {
-            EquivalentAddressGroup server = new EquivalentAddressGroup(destination);
+            EquivalentAddressGroup server =
+                new EquivalentAddressGroup(new ProxiedInetSocketAddress(destination, proxy));


ProxiedInetSocketAddress is weird, and it can't be internal long-term. Any NR should be able to support proxies.

The discussion about it should probably include @zhangkun83. (IIRC, the problem here is how should flattening by pick-first impact the attributes of the EquivalentAddressGroup.)

ProxiedInetSocketAddress was my suggestion. I don't see a problem making it public if we need to. Can you elaborate your concerns?

@zhangkun83 and I talked at length about this offline. It seems we're going to be mucking with how NameResolvers and LBs communicate again. That will need some other requirements to be gathered. But in the short term we want some sort of PairSocketAddress (I'm saying that name because it is obviously bad; we need a different name) that would contain (SocketAddress, Attr). It basically shoe-horns something similar to the deleted ResolvedServerInfo without a full redesign of the APIs.

Does this imply EquivalentAddressGroup::getAddresses() would return a list of PairSocketAddress? Sounds like we will just commit to making this class public now, and reusing it later for NameResolvers+LBs.

It would continue to return a list of SocketAddress. No, we're not ready to make this public. There needs to be a longer-term design and that needs some investigation into requirements.

ack, changed it to PairSocketAddress

ejona86 · 2018-03-01T17:53:42Z

core/src/main/java/io/grpc/internal/DnsNameResolver.java

@@ -205,6 +202,21 @@ public void run() {
          }
        }
      }
+
+    private void rescheduleWithCause(Listener savedListener, Exception cause) {


Note that this will cause some git conflicts with #4105. (It looks like it'll be unnecessary after that other PR.)

Rebased and removed unnecessary rescheduling

ejona86 · 2018-03-01T18:01:41Z

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

@@ -147,8 +147,6 @@ void handleUncaughtThrowable(Throwable t) {
  // Only null after channel is terminated. Must be assigned from the channelExecutor.
  private NameResolver nameResolver;

-  private final ProxyDetector proxyDetector;


Note that we should plumb the proxyDetector eventually through here to the Name Resolver. We shouldn't pass the proxyDetector is the constructor like we do now. We could pass it in as Attributes params.

(CC @zhangkun83, I wonder if we should revisit that params argument to the NameResolver and do something more similar to Params/Helper from LB. That can be later though.)

Will application provide ProxyDetector? Do we want NameResolvers to decide whether to use proxy or not?

We want proxies to make the final determination, but we should provide the detector. We shouldn't assume such information is global. There's #3957 for example.

ProxyDetector is now responsible for resolving the proxy's `InetSocketAddress`, and `ProxyParameters` asserts that the address is resolved. The results are plumbed through using a special subclass of `SocketAddress`. If a proxy should be used but the proxy can not be resolved, we the `DnsNameResolver` will re-attempt the resolution later. Remove the unit test testing for unresolved proxy addresses, since it's no longer applicable.

Do not extract the `ProxyParameters` field and pass it into `ClientTransportFactory.newClientTransport`.

This reverts commit 37a5ac4.

This reverts commit 08a904d.

This reverts commit 1c3cfc9.

- PairSocketAddress - use correct DnsNameResolver error handling - let ProxyDetector be passed in as getNameResolverParams

ejona86 · 2018-03-08T23:54:24Z

core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java

@@ -47,12 +49,17 @@ public DnsNameResolver newNameResolver(URI targetUri, Attributes params) {
      Preconditions.checkArgument(targetPath.startsWith("/"),
          "the path component (%s) of the target (%s) must start with '/'", targetPath, targetUri);
      String name = targetPath.substring(1);
+      // We want to eventually let the ProxyDetector be passed in from ManagedChannel


I don't see the point of this change until ManagedChannelImpl is actually passing the value. It's small and it's more complex to do it half-way. It also makes an awkward test with testProxyDetectorSpecified. Do this change all-at-once (either now or later).

ejona86 · 2018-03-08T23:57:39Z

core/src/main/java/io/grpc/internal/PairSocketAddress.java

+
+  @VisibleForTesting
+  PairSocketAddress(SocketAddress address, Attributes attributes) {
+    this.address = address;


checkNotNull for both arguments?

jjpet123 · 2018-03-22T21:30:56Z

Hi! @ejona86 @zpencer

Quick question, it is my understanding that this will fix the "UnresolvedAddressException" we hit in 1.90 when passing proxy hostnames to the system property "https.proxyHost" (we are NOT using GRPC_PROXY_EXP), is that right? Please confirm, thanks!

zpencer · 2018-03-23T16:11:43Z

Yes, this PR fixes the unresolved address issue and allows you to use -Dhttps.proxyHost and optionally with a password via java.net.Authenticator.

zpencer assigned ejona86 Feb 24, 2018

zpencer requested a review from ejona86 February 24, 2018 00:07

ericgribkoff mentioned this pull request Feb 28, 2018

NetworkOnMainThread exception while resolving IP address. #4154

Closed

ejona86 reviewed Mar 1, 2018

View reviewed changes

ericgribkoff mentioned this pull request Mar 2, 2018

core: prefer getHostString() to getHostName() in ProxyDetectorImpl #4165

Merged

zpencer added 8 commits March 8, 2018 14:50

Plumb ProxiedInetSocketAddress down to transports

a83486a

Do not extract the `ProxyParameters` field and pass it into `ClientTransportFactory.newClientTransport`.

fix signature for cronet

65be2e8

fix signature for cronet tests

77c26ac

Revert "fix signature for cronet tests"

9e18063

This reverts commit 37a5ac4.

Revert "fix signature for cronet"

9e22265

This reverts commit 08a904d.

Revert "Plumb ProxiedInetSocketAddress down to transports"

5005b1d

This reverts commit 1c3cfc9.

PR comments

2ac1eea

- PairSocketAddress - use correct DnsNameResolver error handling - let ProxyDetector be passed in as getNameResolverParams

zpencer force-pushed the plumb-proxy-resolved-addr branch from 37a5ac4 to 2ac1eea Compare March 8, 2018 23:42

add test for ProxyDetector via name resolver attrs

50c699a

ejona86 approved these changes Mar 9, 2018

View reviewed changes

add null checks to PairProxyAddress, punt proxy in NameResovler params

ac2de61

zpencer merged commit 402c174 into grpc:master Mar 12, 2018

zpencer deleted the plumb-proxy-resolved-addr branch March 12, 2018 18:15

ejona86 mentioned this pull request Apr 4, 2018

Determine final NameResolver to LoadBalancer communication #4302

Open

ejona86 mentioned this pull request Aug 9, 2018

Support forward proxies with CONNECT #2193

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core,okhttp,netty,alts,testing: Plumb proxy resolved addr to transports #4137

core,okhttp,netty,alts,testing: Plumb proxy resolved addr to transports #4137

zpencer commented Feb 24, 2018 •

edited

Loading

ejona86 Mar 1, 2018

zpencer Mar 8, 2018

ejona86 Mar 1, 2018

zhangkun83 Mar 1, 2018

ejona86 Mar 2, 2018

zpencer Mar 8, 2018

ejona86 Mar 8, 2018

zpencer Mar 8, 2018

ejona86 Mar 1, 2018

zpencer Mar 8, 2018

ejona86 Mar 1, 2018

zhangkun83 Mar 1, 2018

ejona86 Mar 1, 2018

ejona86 Mar 8, 2018

ejona86 Mar 8, 2018

jjpet123 commented Mar 22, 2018 •

edited

Loading

zpencer commented Mar 23, 2018 •

edited

Loading

core,okhttp,netty,alts,testing: Plumb proxy resolved addr to transports #4137

core,okhttp,netty,alts,testing: Plumb proxy resolved addr to transports #4137

Conversation

zpencer commented Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjpet123 commented Mar 22, 2018 • edited Loading

zpencer commented Mar 23, 2018 • edited Loading

zpencer commented Feb 24, 2018 •

edited

Loading

jjpet123 commented Mar 22, 2018 •

edited

Loading

zpencer commented Mar 23, 2018 •

edited

Loading