From d1660bd844c547b5d2befb48373631d08ff55884 Mon Sep 17 00:00:00 2001 From: Lee Avital Date: Wed, 24 May 2017 12:20:35 -0400 Subject: [PATCH 01/10] Allow proxies to be direct --- .../config/service/ProxyConfiguration.java | 21 +++++++++++++------ .../service/ProxyConfigurationTests.java | 4 ++-- .../service/ServiceConfigurationTest.java | 2 +- .../ServiceDiscoveryConfigurationTests.java | 2 +- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java index de707eb6f..89184da9c 100644 --- a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java +++ b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java @@ -35,30 +35,39 @@ @JsonSerialize(as = ImmutableProxyConfiguration.class) @Style(visibility = Style.ImplementationVisibility.PACKAGE, builder = "new") public abstract class ProxyConfiguration { - /** * 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 hostAndPort(); /** * Credentials if the proxy needs authentication. */ public abstract Optional credentials(); + public abstract Optional direct(); + @Value.Check protected final void check() { - HostAndPort host = HostAndPort.fromString(hostAndPort()); - Preconditions.checkArgument(host.hasPort(), "Given hostname does not contain a port number: " + host); + Preconditions.checkArgument(direct().isPresent() || hostAndPort().isPresent(), "proxy configuration must either be direct or configured with host-and-port"); + + if (hostAndPort().isPresent()) { + HostAndPort host = HostAndPort.fromString(hostAndPort().get()); + Preconditions.checkArgument(host.hasPort(), "Given hostname does not contain a port number: " + host); + } } @Lazy @SuppressWarnings("checkstyle:designforextension") @JsonIgnore public Proxy toProxy() { - HostAndPort hostAndPort = HostAndPort.fromString(hostAndPort()); - return new Proxy(Proxy.Type.HTTP, new InetSocketAddress(hostAndPort.getHostText(), hostAndPort.getPort())); + if (hostAndPort().isPresent()) { + HostAndPort hostAndPort = HostAndPort.fromString(hostAndPort().get()); + return new Proxy(Proxy.Type.HTTP, new InetSocketAddress(hostAndPort.getHostText(), hostAndPort.getPort())); + } else { + return Proxy.NO_PROXY; + } } public static ProxyConfiguration of(String hostAndPort) { diff --git a/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java b/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java index 3f21632c0..bf710661e 100644 --- a/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java +++ b/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java @@ -65,9 +65,9 @@ public void testToProxy() { public void serDe() throws Exception { ProxyConfiguration serialized = ProxyConfiguration.of("host:80", BasicCredentials.of("username", "password")); String deserializedCamelCase = - "{\"hostAndPort\":\"host:80\",\"credentials\":{\"username\":\"username\",\"password\":\"password\"}}"; + "{\"hostAndPort\":\"host:80\",\"credentials\":{\"username\":\"username\",\"password\":\"password\"},\"direct\":null}"; String deserializedKebabCase = - "{\"host-and-port\":\"host:80\",\"credentials\":{\"username\":\"username\",\"password\":\"password\"}}"; + "{\"host-and-port\":\"host:80\",\"credentials\":{\"username\":\"username\",\"password\":\"password\"}, \"direct\": null}"; assertThat(ObjectMappers.newClientObjectMapper().writeValueAsString(serialized)) .isEqualTo(deserializedCamelCase); diff --git a/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceConfigurationTest.java b/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceConfigurationTest.java index e10d2ecc7..cfbd255bc 100644 --- a/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceConfigurationTest.java +++ b/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceConfigurationTest.java @@ -42,7 +42,7 @@ public void serDe() throws Exception { + "\"keyStorePassword\":null,\"keyStoreType\":\"JKS\",\"keyStoreKeyAlias\":null}," + "\"connectTimeout\":\"1 day\",\"readTimeout\":\"1 day\",\"writeTimeout\":\"1 day\"," + "\"enableGcmCipherSuites\":null," - + "\"uris\":[\"uri1\"],\"proxyConfiguration\":{\"hostAndPort\":\"host:80\",\"credentials\":null}}"; + + "\"uris\":[\"uri1\"],\"proxyConfiguration\":{\"hostAndPort\":\"host:80\",\"credentials\":null,\"direct\":null}}"; String deserializedKebabCase = "{\"api-token\":\"bearerToken\",\"security\":" + "{\"trust-store-path\":\"truststore.jks\",\"trust-store-type\":\"JKS\",\"key-store-path\":null," + "\"key-store-password\":null,\"key-store-type\":\"JKS\",\"key-store-key-alias\":null}," diff --git a/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceDiscoveryConfigurationTests.java b/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceDiscoveryConfigurationTests.java index ee6670292..96780132b 100644 --- a/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceDiscoveryConfigurationTests.java +++ b/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceDiscoveryConfigurationTests.java @@ -186,7 +186,7 @@ public void serDe() throws Exception { + "{\"service\":{\"apiToken\":null,\"security\":null,\"connectTimeout\":null,\"readTimeout\":null," + "\"writeTimeout\":null,\"enableGcmCipherSuites\":null,\"uris\":[\"uri\"]," + "\"proxyConfiguration\":null}},\"proxyConfiguration\":" - + "{\"hostAndPort\":\"host:80\",\"credentials\":null},\"connectTimeout\":\"1 day\"," + + "{\"hostAndPort\":\"host:80\",\"credentials\":null,\"direct\":null},\"connectTimeout\":\"1 day\"," + "\"readTimeout\":\"1 day\",\"enableGcmCipherSuites\":null}"; String deserializedKebabCase = "{\"api-token\":\"bearerToken\",\"security\":" + "{\"trust-store-path\":\"truststore.jks\",\"trust-store-type\":\"JKS\",\"key-store-path\":null," From 3d86ae16c9b01661cbece183ec8777d94f7d8270 Mon Sep 17 00:00:00 2001 From: Lee Avital Date: Wed, 24 May 2017 12:23:11 -0400 Subject: [PATCH 02/10] Fix checkstyle --- .../palantir/remoting2/config/service/ProxyConfiguration.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java index 89184da9c..1a997c48c 100644 --- a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java +++ b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java @@ -54,7 +54,8 @@ protected final void check() { if (hostAndPort().isPresent()) { HostAndPort host = HostAndPort.fromString(hostAndPort().get()); - Preconditions.checkArgument(host.hasPort(), "Given hostname does not contain a port number: " + host); + Preconditions.checkArgument(host.hasPort(), + "Given hostname does not contain a port number: " + host); } } From 22205d29ca26ed2643c01bfb0e45629a173b0d84 Mon Sep 17 00:00:00 2001 From: Lee Avital Date: Wed, 24 May 2017 12:35:08 -0400 Subject: [PATCH 03/10] Fix checkstyle --- .../config/service/ProxyConfiguration.java | 31 ++++++++++++++----- .../service/ProxyConfigurationTests.java | 8 ++--- .../service/ServiceConfigurationTest.java | 3 +- .../ServiceDiscoveryConfigurationTests.java | 2 +- 4 files changed, 30 insertions(+), 14 deletions(-) diff --git a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java index 1a997c48c..b0b03d48b 100644 --- a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java +++ b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java @@ -35,6 +35,10 @@ @JsonSerialize(as = ImmutableProxyConfiguration.class) @Style(visibility = Style.ImplementationVisibility.PACKAGE, builder = "new") public abstract class ProxyConfiguration { + enum Type { + direct, http; + } + /** * 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. @@ -46,28 +50,39 @@ public abstract class ProxyConfiguration { */ public abstract Optional credentials(); - public abstract Optional direct(); + @Value.Default + @SuppressWarnings("checkstyle:designforextension") + public Type type() { + return Type.http; + } @Value.Check protected final void check() { - Preconditions.checkArgument(direct().isPresent() || hostAndPort().isPresent(), "proxy configuration must either be direct or configured with host-and-port"); - if (hostAndPort().isPresent()) { + Preconditions.checkArgument(type() == Type.http, "host-and-port only valid for http proxies"); HostAndPort host = HostAndPort.fromString(hostAndPort().get()); Preconditions.checkArgument(host.hasPort(), "Given hostname does not contain a port number: " + host); } + + if (credentials().isPresent()) { + Preconditions.checkArgument(type() == Type.http, "credentials only valid for http proxies"); + } } @Lazy @SuppressWarnings("checkstyle:designforextension") @JsonIgnore public Proxy toProxy() { - if (hostAndPort().isPresent()) { - HostAndPort hostAndPort = HostAndPort.fromString(hostAndPort().get()); - return new Proxy(Proxy.Type.HTTP, new InetSocketAddress(hostAndPort.getHostText(), hostAndPort.getPort())); - } else { - return Proxy.NO_PROXY; + switch (type()) { + case http: + HostAndPort hostAndPort = HostAndPort.fromString(hostAndPort().get()); + InetSocketAddress addr = new InetSocketAddress(hostAndPort.getHostText(), hostAndPort.getPort()); + return new Proxy(Proxy.Type.HTTP, addr); + case direct: + return Proxy.NO_PROXY; + default: + throw new IllegalStateException("unrecognized proxy type; this is a library error"); } } diff --git a/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java b/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java index bf710661e..6d7dc4b5c 100644 --- a/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java +++ b/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java @@ -64,10 +64,10 @@ public void testToProxy() { @Test public void serDe() throws Exception { ProxyConfiguration serialized = ProxyConfiguration.of("host:80", BasicCredentials.of("username", "password")); - String deserializedCamelCase = - "{\"hostAndPort\":\"host:80\",\"credentials\":{\"username\":\"username\",\"password\":\"password\"},\"direct\":null}"; - String deserializedKebabCase = - "{\"host-and-port\":\"host:80\",\"credentials\":{\"username\":\"username\",\"password\":\"password\"}, \"direct\": null}"; + String deserializedCamelCase = "{\"hostAndPort\":\"host:80\",\"credentials\":{\"username\":\"username\"," + + "\"password\":\"password\"},\"type\":\"http\"}"; + String deserializedKebabCase = "{\"host-and-port\":\"host:80\",\"credentials\":{\"username\":\"username\"," + + "\"password\":\"password\"},\"type\":\"http\"}"; assertThat(ObjectMappers.newClientObjectMapper().writeValueAsString(serialized)) .isEqualTo(deserializedCamelCase); diff --git a/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceConfigurationTest.java b/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceConfigurationTest.java index cfbd255bc..ebbee2453 100644 --- a/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceConfigurationTest.java +++ b/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceConfigurationTest.java @@ -42,7 +42,8 @@ public void serDe() throws Exception { + "\"keyStorePassword\":null,\"keyStoreType\":\"JKS\",\"keyStoreKeyAlias\":null}," + "\"connectTimeout\":\"1 day\",\"readTimeout\":\"1 day\",\"writeTimeout\":\"1 day\"," + "\"enableGcmCipherSuites\":null," - + "\"uris\":[\"uri1\"],\"proxyConfiguration\":{\"hostAndPort\":\"host:80\",\"credentials\":null,\"direct\":null}}"; + + "\"uris\":[\"uri1\"],\"proxyConfiguration\":{\"hostAndPort\":\"host:80\",\"credentials\":null," + + "\"type\":\"http\"}}"; String deserializedKebabCase = "{\"api-token\":\"bearerToken\",\"security\":" + "{\"trust-store-path\":\"truststore.jks\",\"trust-store-type\":\"JKS\",\"key-store-path\":null," + "\"key-store-password\":null,\"key-store-type\":\"JKS\",\"key-store-key-alias\":null}," diff --git a/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceDiscoveryConfigurationTests.java b/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceDiscoveryConfigurationTests.java index 96780132b..2086a4d97 100644 --- a/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceDiscoveryConfigurationTests.java +++ b/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceDiscoveryConfigurationTests.java @@ -186,7 +186,7 @@ public void serDe() throws Exception { + "{\"service\":{\"apiToken\":null,\"security\":null,\"connectTimeout\":null,\"readTimeout\":null," + "\"writeTimeout\":null,\"enableGcmCipherSuites\":null,\"uris\":[\"uri\"]," + "\"proxyConfiguration\":null}},\"proxyConfiguration\":" - + "{\"hostAndPort\":\"host:80\",\"credentials\":null,\"direct\":null},\"connectTimeout\":\"1 day\"," + + "{\"hostAndPort\":\"host:80\",\"credentials\":null,\"type\":\"http\"},\"connectTimeout\":\"1 day\"," + "\"readTimeout\":\"1 day\",\"enableGcmCipherSuites\":null}"; String deserializedKebabCase = "{\"api-token\":\"bearerToken\",\"security\":" + "{\"trust-store-path\":\"truststore.jks\",\"trust-store-type\":\"JKS\",\"key-store-path\":null," From 4b8294194d3cf6ea097b795809081707c8a72fad Mon Sep 17 00:00:00 2001 From: Lee Avital Date: Wed, 24 May 2017 13:07:28 -0400 Subject: [PATCH 04/10] Tests --- .../config/service/ProxyConfiguration.java | 4 ++++ .../config/service/ProxyConfigurationTests.java | 16 ++++++++++++++++ .../resources/configs/proxy-config-direct.yml | 1 + 3 files changed, 21 insertions(+) create mode 100644 service-config/src/test/resources/configs/proxy-config-direct.yml diff --git a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java index b0b03d48b..2d48b1a14 100644 --- a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java +++ b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java @@ -94,6 +94,10 @@ public static ProxyConfiguration of(String hostAndPort, BasicCredentials credent return new ProxyConfiguration.Builder().hostAndPort(hostAndPort).credentials(credentials).build(); } + public static ProxyConfiguration direct() { + return new ProxyConfiguration.Builder().type(Type.direct).build(); + } + // TODO(jnewman): #317 - remove kebab-case methods when Jackson 2.7 is picked up static final class Builder extends ImmutableProxyConfiguration.Builder { diff --git a/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java b/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java index 6d7dc4b5c..06b4a8f3b 100644 --- a/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java +++ b/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java @@ -53,6 +53,21 @@ public void testDeserializationWithCredentials() throws IOException { assertEquals(expectedProxyConfiguration, actualProxyConfiguration); } + @Test + public void testDeserializationDirect() throws Exception { + URL resource = Resources.getResource("configs/proxy-config-direct.yml"); + ProxyConfiguration config = mapper.readValue(resource, ProxyConfiguration.class); + assertEquals(config, ProxyConfiguration.direct()); + } + + @Test(expected = IllegalArgumentException.class) + public void testDirectProxyWithHostAndPort() { + new ProxyConfiguration.Builder() + .hostAndPort("squid:3128") + .type(ProxyConfiguration.Type.direct) + .build(); + } + @Test public void testToProxy() { ProxyConfiguration proxyConfiguration = ProxyConfiguration.of("squid:3128"); @@ -61,6 +76,7 @@ public void testToProxy() { assertEquals(expected, proxyConfiguration.toProxy()); } + @Test public void serDe() throws Exception { ProxyConfiguration serialized = ProxyConfiguration.of("host:80", BasicCredentials.of("username", "password")); diff --git a/service-config/src/test/resources/configs/proxy-config-direct.yml b/service-config/src/test/resources/configs/proxy-config-direct.yml new file mode 100644 index 000000000..642c5b50c --- /dev/null +++ b/service-config/src/test/resources/configs/proxy-config-direct.yml @@ -0,0 +1 @@ +type: direct \ No newline at end of file From 0acc39953359ee7b1fc9e12bc136ac65f6663877 Mon Sep 17 00:00:00 2001 From: Lee Avital Date: Wed, 24 May 2017 18:24:24 -0400 Subject: [PATCH 05/10] Address case where proxy is direct but hostandport is configured --- .../config/service/ProxyConfiguration.java | 30 +++++++++++++++---- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java index 2d48b1a14..fdbaab627 100644 --- a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java +++ b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java @@ -35,8 +35,19 @@ @JsonSerialize(as = ImmutableProxyConfiguration.class) @Style(visibility = Style.ImplementationVisibility.PACKAGE, builder = "new") public abstract class ProxyConfiguration { + enum Type { - direct, http; + + /** + * Use a direct connection. This option will bypass any JVM-level configured proxy settings. + */ + direct, + + /** + * Use an http-proxy specified by {@link ProxyConfiguration#hostAndPort()} and (optionally) + * {@link ProxyConfiguration#credentials()}. + */ + http; } /** @@ -58,11 +69,18 @@ public Type type() { @Value.Check protected final void check() { - if (hostAndPort().isPresent()) { - Preconditions.checkArgument(type() == Type.http, "host-and-port only valid for http proxies"); - HostAndPort host = HostAndPort.fromString(hostAndPort().get()); - Preconditions.checkArgument(host.hasPort(), - "Given hostname does not contain a port number: " + host); + switch (type()) { + case http: + HostAndPort host = HostAndPort.fromString(hostAndPort().get()); + Preconditions.checkArgument(host.hasPort(), + "Given hostname does not contain a port number: " + host); + break; + case direct: + Preconditions.checkArgument(!hostAndPort().isPresent() && !credentials().isPresent(), + "Neither credential nor host-and-port may be configured for direct proxies"); + break; + default: + throw new IllegalStateException("Unrecognized case; this is a library bug"); } if (credentials().isPresent()) { From 3c089ce0d14f90ef9f2b8523a992f8b76abac9ae Mon Sep 17 00:00:00 2001 From: Lee Avital Date: Thu, 25 May 2017 12:00:07 -0400 Subject: [PATCH 06/10] checkpoint --- .../config/service/ProxyConfiguration.java | 27 ++++++++++++++----- .../service/ProxyConfigurationTests.java | 2 +- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java index fdbaab627..1964c6dae 100644 --- a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java +++ b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java @@ -54,7 +54,20 @@ enum Type { * 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 Optional hostAndPort(); + @JsonProperty("hostAndPort") + public abstract Optional maybeHostAndPort(); + + /** + * @deprecated Use maybeHostAndPort(). + */ + @Deprecated + @Value.Derived + @SuppressWarnings("checkstyle:designforextension") + @JsonIgnore + public String hostAndPort() { + Preconditions.checkState(maybeHostAndPort().isPresent(), "hostAndPort was not configured"); + return maybeHostAndPort().get(); + } /** * Credentials if the proxy needs authentication. @@ -71,12 +84,12 @@ public Type type() { protected final void check() { switch (type()) { case http: - HostAndPort host = HostAndPort.fromString(hostAndPort().get()); + HostAndPort host = HostAndPort.fromString(maybeHostAndPort().get()); Preconditions.checkArgument(host.hasPort(), "Given hostname does not contain a port number: " + host); break; case direct: - Preconditions.checkArgument(!hostAndPort().isPresent() && !credentials().isPresent(), + Preconditions.checkArgument(!maybeHostAndPort().isPresent() && !credentials().isPresent(), "Neither credential nor host-and-port may be configured for direct proxies"); break; default: @@ -94,7 +107,7 @@ protected final void check() { public Proxy toProxy() { switch (type()) { case http: - HostAndPort hostAndPort = HostAndPort.fromString(hostAndPort().get()); + HostAndPort hostAndPort = HostAndPort.fromString(maybeHostAndPort().get()); InetSocketAddress addr = new InetSocketAddress(hostAndPort.getHostText(), hostAndPort.getPort()); return new Proxy(Proxy.Type.HTTP, addr); case direct: @@ -105,11 +118,11 @@ public Proxy toProxy() { } public static ProxyConfiguration of(String hostAndPort) { - return new ProxyConfiguration.Builder().hostAndPort(hostAndPort).build(); + return new ProxyConfiguration.Builder().maybeHostAndPort(hostAndPort).build(); } public static ProxyConfiguration of(String hostAndPort, BasicCredentials credentials) { - return new ProxyConfiguration.Builder().hostAndPort(hostAndPort).credentials(credentials).build(); + return new ProxyConfiguration.Builder().maybeHostAndPort(hostAndPort).credentials(credentials).build(); } public static ProxyConfiguration direct() { @@ -121,7 +134,7 @@ static final class Builder extends ImmutableProxyConfiguration.Builder { @JsonProperty("host-and-port") Builder hostAndPortKebabCase(String hostAndPort) { - return hostAndPort(hostAndPort); + return maybeHostAndPort(hostAndPort); } } } diff --git a/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java b/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java index 06b4a8f3b..eab2e14e4 100644 --- a/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java +++ b/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java @@ -63,7 +63,7 @@ public void testDeserializationDirect() throws Exception { @Test(expected = IllegalArgumentException.class) public void testDirectProxyWithHostAndPort() { new ProxyConfiguration.Builder() - .hostAndPort("squid:3128") + .maybeHostAndPort("squid:3128") .type(ProxyConfiguration.Type.direct) .build(); } From 56cd9a8df62d1174ed5e5af7b991b6e73252b8a7 Mon Sep 17 00:00:00 2001 From: Lee Avital Date: Thu, 25 May 2017 13:07:55 -0400 Subject: [PATCH 07/10] Use lazy --- .../palantir/remoting2/config/service/ProxyConfiguration.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java index 1964c6dae..264e35086 100644 --- a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java +++ b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java @@ -61,9 +61,9 @@ enum Type { * @deprecated Use maybeHostAndPort(). */ @Deprecated - @Value.Derived @SuppressWarnings("checkstyle:designforextension") @JsonIgnore + @Lazy public String hostAndPort() { Preconditions.checkState(maybeHostAndPort().isPresent(), "hostAndPort was not configured"); return maybeHostAndPort().get(); From 9bbe4911aa868740263765fe96a3d67265010ce1 Mon Sep 17 00:00:00 2001 From: Lee Avital Date: Thu, 25 May 2017 15:29:32 -0400 Subject: [PATCH 08/10] Address review feedback --- .../remoting2/config/service/ProxyConfiguration.java | 5 +++++ .../remoting2/config/service/ServiceConfiguration.java | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java index 264e35086..d392a8368 100644 --- a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java +++ b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java @@ -74,6 +74,9 @@ public String hostAndPort() { */ public abstract Optional credentials(); + /** + * The type of Proxy. Defaults to {@link Type#http}. + */ @Value.Default @SuppressWarnings("checkstyle:designforextension") public Type type() { @@ -84,6 +87,8 @@ public Type type() { protected final void check() { switch (type()) { case http: + Preconditions.checkArgument(maybeHostAndPort().isPresent(), "host-and-port must be " + + "configured for an http proxy"); HostAndPort host = HostAndPort.fromString(maybeHostAndPort().get()); Preconditions.checkArgument(host.hasPort(), "Given hostname does not contain a port number: " + host); diff --git a/service-config/src/main/java/com/palantir/remoting2/config/service/ServiceConfiguration.java b/service-config/src/main/java/com/palantir/remoting2/config/service/ServiceConfiguration.java index 50de757e5..48290a11d 100644 --- a/service-config/src/main/java/com/palantir/remoting2/config/service/ServiceConfiguration.java +++ b/service-config/src/main/java/com/palantir/remoting2/config/service/ServiceConfiguration.java @@ -68,7 +68,7 @@ public abstract class ServiceConfiguration { public abstract List uris(); /** - * Proxy configuration for connecting to the service. + * Proxy configuration for connecting to the service. If absent, uses system proxy configuration. */ public abstract Optional proxyConfiguration(); From b15b303bb03caee52f17f29c5d5776a00b8a6a3e Mon Sep 17 00:00:00 2001 From: Lee Avital Date: Thu, 25 May 2017 16:09:40 -0400 Subject: [PATCH 09/10] Uppercase --- .../config/service/ProxyConfiguration.java | 22 +++++++++---------- .../service/ProxyConfigurationTests.java | 6 ++--- .../service/ServiceConfigurationTest.java | 2 +- .../ServiceDiscoveryConfigurationTests.java | 2 +- .../resources/configs/proxy-config-direct.yml | 2 +- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java index d392a8368..e0dacfc18 100644 --- a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java +++ b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java @@ -41,13 +41,13 @@ enum Type { /** * Use a direct connection. This option will bypass any JVM-level configured proxy settings. */ - direct, + DIRECT, /** * Use an http-proxy specified by {@link ProxyConfiguration#hostAndPort()} and (optionally) * {@link ProxyConfiguration#credentials()}. */ - http; + HTTP; } /** @@ -75,34 +75,34 @@ public String hostAndPort() { public abstract Optional credentials(); /** - * The type of Proxy. Defaults to {@link Type#http}. + * The type of Proxy. Defaults to {@link Type#HTTP}. */ @Value.Default @SuppressWarnings("checkstyle:designforextension") public Type type() { - return Type.http; + return Type.HTTP; } @Value.Check protected final void check() { switch (type()) { - case http: + case HTTP: Preconditions.checkArgument(maybeHostAndPort().isPresent(), "host-and-port must be " + "configured for an http proxy"); HostAndPort host = HostAndPort.fromString(maybeHostAndPort().get()); Preconditions.checkArgument(host.hasPort(), "Given hostname does not contain a port number: " + host); break; - case direct: + case DIRECT: Preconditions.checkArgument(!maybeHostAndPort().isPresent() && !credentials().isPresent(), - "Neither credential nor host-and-port may be configured for direct proxies"); + "Neither credential nor host-and-port may be configured for DIRECT proxies"); break; default: throw new IllegalStateException("Unrecognized case; this is a library bug"); } if (credentials().isPresent()) { - Preconditions.checkArgument(type() == Type.http, "credentials only valid for http proxies"); + Preconditions.checkArgument(type() == Type.HTTP, "credentials only valid for http proxies"); } } @@ -111,11 +111,11 @@ protected final void check() { @JsonIgnore public Proxy toProxy() { switch (type()) { - case http: + case HTTP: HostAndPort hostAndPort = HostAndPort.fromString(maybeHostAndPort().get()); InetSocketAddress addr = new InetSocketAddress(hostAndPort.getHostText(), hostAndPort.getPort()); return new Proxy(Proxy.Type.HTTP, addr); - case direct: + case DIRECT: return Proxy.NO_PROXY; default: throw new IllegalStateException("unrecognized proxy type; this is a library error"); @@ -131,7 +131,7 @@ public static ProxyConfiguration of(String hostAndPort, BasicCredentials credent } public static ProxyConfiguration direct() { - return new ProxyConfiguration.Builder().type(Type.direct).build(); + return new ProxyConfiguration.Builder().type(Type.DIRECT).build(); } // TODO(jnewman): #317 - remove kebab-case methods when Jackson 2.7 is picked up diff --git a/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java b/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java index eab2e14e4..f61552b14 100644 --- a/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java +++ b/service-config/src/test/java/com/palantir/remoting2/config/service/ProxyConfigurationTests.java @@ -64,7 +64,7 @@ public void testDeserializationDirect() throws Exception { public void testDirectProxyWithHostAndPort() { new ProxyConfiguration.Builder() .maybeHostAndPort("squid:3128") - .type(ProxyConfiguration.Type.direct) + .type(ProxyConfiguration.Type.DIRECT) .build(); } @@ -81,9 +81,9 @@ public void testToProxy() { public void serDe() throws Exception { ProxyConfiguration serialized = ProxyConfiguration.of("host:80", BasicCredentials.of("username", "password")); String deserializedCamelCase = "{\"hostAndPort\":\"host:80\",\"credentials\":{\"username\":\"username\"," - + "\"password\":\"password\"},\"type\":\"http\"}"; + + "\"password\":\"password\"},\"type\":\"HTTP\"}"; String deserializedKebabCase = "{\"host-and-port\":\"host:80\",\"credentials\":{\"username\":\"username\"," - + "\"password\":\"password\"},\"type\":\"http\"}"; + + "\"password\":\"password\"},\"type\":\"HTTP\"}"; assertThat(ObjectMappers.newClientObjectMapper().writeValueAsString(serialized)) .isEqualTo(deserializedCamelCase); diff --git a/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceConfigurationTest.java b/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceConfigurationTest.java index ebbee2453..d866255c7 100644 --- a/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceConfigurationTest.java +++ b/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceConfigurationTest.java @@ -43,7 +43,7 @@ public void serDe() throws Exception { + "\"connectTimeout\":\"1 day\",\"readTimeout\":\"1 day\",\"writeTimeout\":\"1 day\"," + "\"enableGcmCipherSuites\":null," + "\"uris\":[\"uri1\"],\"proxyConfiguration\":{\"hostAndPort\":\"host:80\",\"credentials\":null," - + "\"type\":\"http\"}}"; + + "\"type\":\"HTTP\"}}"; String deserializedKebabCase = "{\"api-token\":\"bearerToken\",\"security\":" + "{\"trust-store-path\":\"truststore.jks\",\"trust-store-type\":\"JKS\",\"key-store-path\":null," + "\"key-store-password\":null,\"key-store-type\":\"JKS\",\"key-store-key-alias\":null}," diff --git a/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceDiscoveryConfigurationTests.java b/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceDiscoveryConfigurationTests.java index 2086a4d97..b6568c0e8 100644 --- a/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceDiscoveryConfigurationTests.java +++ b/service-config/src/test/java/com/palantir/remoting2/config/service/ServiceDiscoveryConfigurationTests.java @@ -186,7 +186,7 @@ public void serDe() throws Exception { + "{\"service\":{\"apiToken\":null,\"security\":null,\"connectTimeout\":null,\"readTimeout\":null," + "\"writeTimeout\":null,\"enableGcmCipherSuites\":null,\"uris\":[\"uri\"]," + "\"proxyConfiguration\":null}},\"proxyConfiguration\":" - + "{\"hostAndPort\":\"host:80\",\"credentials\":null,\"type\":\"http\"},\"connectTimeout\":\"1 day\"," + + "{\"hostAndPort\":\"host:80\",\"credentials\":null,\"type\":\"HTTP\"},\"connectTimeout\":\"1 day\"," + "\"readTimeout\":\"1 day\",\"enableGcmCipherSuites\":null}"; String deserializedKebabCase = "{\"api-token\":\"bearerToken\",\"security\":" + "{\"trust-store-path\":\"truststore.jks\",\"trust-store-type\":\"JKS\",\"key-store-path\":null," diff --git a/service-config/src/test/resources/configs/proxy-config-direct.yml b/service-config/src/test/resources/configs/proxy-config-direct.yml index 642c5b50c..13acfe891 100644 --- a/service-config/src/test/resources/configs/proxy-config-direct.yml +++ b/service-config/src/test/resources/configs/proxy-config-direct.yml @@ -1 +1 @@ -type: direct \ No newline at end of file +type: DIRECT \ No newline at end of file From 5b7ca0d8f145c05bb873986443f7b850c4cb066a Mon Sep 17 00:00:00 2001 From: Lee Avital Date: Thu, 25 May 2017 16:18:00 -0400 Subject: [PATCH 10/10] Uppercase HTTP --- .../palantir/remoting2/config/service/ProxyConfiguration.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java index e0dacfc18..8f026651c 100644 --- a/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java +++ b/service-config/src/main/java/com/palantir/remoting2/config/service/ProxyConfiguration.java @@ -88,7 +88,7 @@ protected final void check() { switch (type()) { case HTTP: Preconditions.checkArgument(maybeHostAndPort().isPresent(), "host-and-port must be " - + "configured for an http proxy"); + + "configured for an HTTP proxy"); HostAndPort host = HostAndPort.fromString(maybeHostAndPort().get()); Preconditions.checkArgument(host.hasPort(), "Given hostname does not contain a port number: " + host);