Allow direct proxy configuration #435

leeavital · 2017-05-24T17:15:52Z

This allows users to bypass the proxy settings set at the JVM with -Dhttps.proxyHost/-Dhttps.proxyPort. This can be useful if you are using a third-party library which needs to make proxied http calls, but the library does not allow you to set an external proxy.

Previously okhttp would default to using a direct connection if the proxy failed, but that changed in 3.5.0 (https://github.com/square/okhttp/blob/master/CHANGELOG.md)

This change is

ash211

I think going direct instead of using a proxy is specified by passing an absent into the optional proxy configuration higher up in the API, on the ClientConfig.

Is leaving ClientConfig#proxy() empty not going direct?

ash211 · 2017-05-24T17:20:17Z

service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java


    /**
     * The hostname and port of the HTTP/HTTPS Proxy. Recognized formats include those recognized by {@link
     * com.google.common.net.HostAndPort}, for instance {@code foo.com:80}, {@code 192.168.3.100:8080}, etc.
     */
-    public abstract String hostAndPort();
+    public abstract Optional<String> hostAndPort();


this is an API break that would affect consumers

true. I can add a (deprecated) method that returns a non-optional String and throws when the type is direct. That should not break any existing consumers.

leeavital · 2017-05-24T18:22:01Z

@ash211 ommiting the proxy setting will use ProxySelect.getDefault().select(uri), which will respect settings like http.proxyHost. I'm forced to use those flags because I'm using a third-party library which needs to be proxied, but doesn't let me set the proxy at the library level

markelliot · 2017-05-24T18:27:35Z

If you’re using a third-party library, why are http-remoting settings involved? From: Lee Avital <notifications@github.com> Reply-To: palantir/http-remoting <reply@reply.github.com> Date: Wednesday, May 24, 2017 at 11:22 AM To: palantir/http-remoting <http-remoting@noreply.github.com> Cc: Subscribed <subscribed@noreply.github.com> Subject: Re: [palantir/http-remoting] Allow direct proxy configuration (#435) @ash211[github.com]<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ash211&d=DwMFaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=Vp81aOxWZvuVgVK_wp-VF3pIYG92B19LcCw6XKeYC0U&m=VKtXO7jYINXqQ362B_ClWyqZrmPIVSx8XZjv2hxxvIg&s=N01mIc76PQacdSmMD_9dpa4mh8wBmBbFzRMLcds287Q&e=> ommiting the proxy setting will use ProxySelect.getDefault().select(uri), which will respect settings like http.proxyHost. I'm forced to use those flags because I'm using a third-party library which needs to be proxied, but doesn't let me set the proxy at the library level — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub[github.com]<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_palantir_http-2Dremoting_pull_435-23issuecomment-2D303809568&d=DwMFaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=Vp81aOxWZvuVgVK_wp-VF3pIYG92B19LcCw6XKeYC0U&m=VKtXO7jYINXqQ362B_ClWyqZrmPIVSx8XZjv2hxxvIg&s=vA5rlkHD3I-qMI6egpaUEy7Issl5HKM7HYMJ7G--UW8&e=>, or mute the thread[github.com]<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AAp3Q1pu7Ax-5FMoNYu8y9QF-2D7y4pRcgP9ks5r9HVKgaJpZM4Nlahy&d=DwMFaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=Vp81aOxWZvuVgVK_wp-VF3pIYG92B19LcCw6XKeYC0U&m=VKtXO7jYINXqQ362B_ClWyqZrmPIVSx8XZjv2hxxvIg&s=07nXI-QCMWaM2Uxt1dH2MWcWB-dYm2-qF60KUSkYmUo&e=>.

leeavital · 2017-05-24T18:28:53Z

The third party library is forcing me to configure proxy settings globally. I need to tell service created by http-remoting to explicitly ignore those global settings.

uschi2000 · 2017-05-24T18:33:01Z

I'd like to make this explicit by having users configure one of three things: NO_PROXY, SYSTEM_PROXY, PROXY. Default is probably SYSTEM_PROXY. We need to do this in a way that doesn't break existing config.

…

On Wed, May 24, 2017 at 11:29 AM Lee Avital ***@***.***> wrote: The third party library is forcing me to configure proxy settings globally. I need to tell service created by http-remoting to explicitly ignore those global settings. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#435 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGOdwZ_c_FEbnAcZvJzs9rxNmBNeGInUks5r9HbmgaJpZM4Nlahy> .

leeavital · 2017-05-24T18:34:58Z

Ok, could I can change/add to the names of the Type enum so it reads:

enum Type {
    noProxy, systemProxy, proxy
}

and leave the rest of my PR the same? I don't think that breaks any existing config

leeavital · 2017-05-24T19:22:43Z

Actually, I think my current approach does this exactly what you want. the default is the system proxy, you can configure directly use of a special proxy or a direct proxy

uschi2000 · 2017-05-24T20:19:33Z

but the config space is a little weird, for instance i could say proxy=direct and still specify a hostAndIPort.

leeavital · 2017-05-24T20:20:09Z

There are preconditions that prevent you from doing that

uschi2000 · 2017-05-24T20:42:38Z

let me summarize what i understand the possible valid configurations are:

usesSystemProxy:
  uris: 
    - https://foo

usesExplicitProxy:
  uris: 
    - https://foo
  proxy:
    hostAndPort: myProxy:8080

# same as above
usesExplicitProxy2:
  uris: 
    - https://foo
  proxy:
    type: http
    hostAndPort: myProxy:8080

usesNoProxyEvenIfSystemProxyIsSet:
  uris: 
    - https://foo
  proxy:
    type: direct

and these configurations are invalid:

invalidDirect:
  uris: 
    - https://foo
  proxy:
    type: direct
    hostAndPort: myProxy:8080

there are still undefined cases, e.g., type=http, but no hostAndPort given

leeavital · 2017-05-24T21:50:50Z

You're right, that's a mistake on my part. Is the ask that I fix that (it should trigger a failure) or that I change the configuration further?

leeavital · 2017-05-25T17:09:37Z

@uschi2000 the case you mentioned is handled correctly now

@ash211 I had to do some interesting naming because of erasure issues, but I managed to get it so the existing source API is unbroken

uschi2000 · 2017-05-25T17:56:23Z

service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java

+        /**
+         * Use a direct connection. This option will bypass any JVM-level configured proxy settings.
+         */
+        direct,


other enums in this project are UPPER_CASE, please do the same here.

These leak into the configuration, which usually doesn't contain upper case values. Glad to change, but that's why I had made this particular enum lowercase

SslConfiguration uses UPPER_CASE, i think. What are example of lower-case configs?

ash211 · 2017-05-25T17:56:46Z

service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java

    /**
     * The hostname and port of the HTTP/HTTPS Proxy. Recognized formats include those recognized by {@link
     * com.google.common.net.HostAndPort}, for instance {@code foo.com:80}, {@code 192.168.3.100:8080}, etc.
     */
-    public abstract String hostAndPort();
+    @JsonProperty("hostAndPort")


these are snake case host-and-port usually, but might also need to support hostAndPort for backcompat

See the builder specified below. This supports deserializing in snake-case and serializing in camel

do you mean deserializing the camel-cased hostAndPort? I don't think that was ever supported because of the annotation @JsonDeserialize(builder = ProxyConfiguration.Builder.class)

uschi2000 · 2017-05-25T17:58:29Z

service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java

@@ -36,45 +36,105 @@
 @Style(visibility = Style.ImplementationVisibility.PACKAGE, builder = "new")
 public abstract class ProxyConfiguration {


please update the ServiceConfiguration#proxyConfiguration docs: if absent, uses system proxy configuration.

uschi2000 · 2017-05-25T17:58:45Z

service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java


    /**
     * Credentials if the proxy needs authentication.
     */
    public abstract Optional<BasicCredentials> credentials();

+    @Value.Default


document default.

uschi2000 · 2017-05-25T18:01:31Z

service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java

-        Preconditions.checkArgument(host.hasPort(), "Given hostname does not contain a port number: " + host);
+        switch (type()) {
+            case http:
+                HostAndPort host = HostAndPort.fromString(maybeHostAndPort().get());


doesn't this NPE when type=http and no host is specified?

Nice catch yes

leeavital · 2017-05-25T20:16:16Z

enum is now upper cased

uschi2000 · 2017-05-25T20:16:29Z

service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java

+        }
+
+        if (credentials().isPresent()) {
+            Preconditions.checkArgument(type() == Type.HTTP, "credentials only valid for http proxies");


nit: "for HTTP proxies"

uschi2000 · 2017-05-25T20:16:36Z

service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java

+        switch (type()) {
+            case HTTP:
+                Preconditions.checkArgument(maybeHostAndPort().isPresent(), "host-and-port must be "
+                        + "configured for an http proxy");


uschi2000

one last nit, sorry. otherwise looks good to me.

uschi2000 · 2017-05-25T20:20:03Z

@ash211 anything left regarding the serialization format?

ash211 · 2017-05-25T20:21:16Z

@uschi2000 I'm good as long as it's not a backcompat break! Doesn't look like it, but I'm not familiar with Jackson intricacies

Lee Avital added 4 commits May 24, 2017 12:20

Allow proxies to be direct

d1660bd

Fix checkstyle

3d86ae1

Fix checkstyle

22205d2

Tests

4b82941

ash211 reviewed May 24, 2017

View reviewed changes

Lee Avital added 3 commits May 24, 2017 18:24

Address case where proxy is direct but hostandport is configured

0acc399

checkpoint

3c089ce

Use lazy

56cd9a8

uschi2000 reviewed May 25, 2017

View reviewed changes

ash211 reviewed May 25, 2017

View reviewed changes

uschi2000 reviewed May 25, 2017

View reviewed changes

Lee Avital added 2 commits May 25, 2017 15:29

Address review feedback

9bbe491

Uppercase

b15b303

uschi2000 reviewed May 25, 2017

View reviewed changes

uschi2000 approved these changes May 25, 2017

View reviewed changes

Uppercase HTTP

5b7ca0d

uschi2000 approved these changes May 25, 2017

View reviewed changes

uschi2000 merged commit c26fc9f into palantir:develop May 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow direct proxy configuration #435

Allow direct proxy configuration #435

leeavital commented May 24, 2017 •

edited by uschi2000

ash211 left a comment

ash211 May 24, 2017

leeavital May 24, 2017

leeavital commented May 24, 2017

markelliot commented May 24, 2017 via email

leeavital commented May 24, 2017

uschi2000 commented May 24, 2017 via email

leeavital commented May 24, 2017

leeavital commented May 24, 2017

uschi2000 commented May 24, 2017 •

edited

leeavital commented May 24, 2017

uschi2000 commented May 24, 2017

leeavital commented May 24, 2017

leeavital commented May 25, 2017

uschi2000 May 25, 2017

leeavital May 25, 2017

uschi2000 May 25, 2017

ash211 May 25, 2017

leeavital May 25, 2017

leeavital May 25, 2017

uschi2000 May 25, 2017

uschi2000 May 25, 2017

uschi2000 May 25, 2017

leeavital May 25, 2017

leeavital commented May 25, 2017

uschi2000 May 25, 2017

uschi2000 May 25, 2017

uschi2000 left a comment

uschi2000 commented May 25, 2017

ash211 commented May 25, 2017

		@@ -36,45 +36,105 @@
		@Style(visibility = Style.ImplementationVisibility.PACKAGE, builder = "new")
		public abstract class ProxyConfiguration {

Allow direct proxy configuration #435

Allow direct proxy configuration #435

Conversation

leeavital commented May 24, 2017 • edited by uschi2000

ash211 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leeavital commented May 24, 2017

markelliot commented May 24, 2017 via email

leeavital commented May 24, 2017

uschi2000 commented May 24, 2017 via email

leeavital commented May 24, 2017

leeavital commented May 24, 2017

uschi2000 commented May 24, 2017 • edited

leeavital commented May 24, 2017

uschi2000 commented May 24, 2017

leeavital commented May 24, 2017

leeavital commented May 25, 2017

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

leeavital commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uschi2000 left a comment

Choose a reason for hiding this comment

uschi2000 commented May 25, 2017

ash211 commented May 25, 2017

leeavital commented May 24, 2017 •

edited by uschi2000

uschi2000 commented May 24, 2017 •

edited