Skip to content
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

Allow direct proxy configuration #435

Merged
merged 10 commits into from May 25, 2017
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -36,45 +36,105 @@
@Style(visibility = Style.ImplementationVisibility.PACKAGE, builder = "new")
public abstract class ProxyConfiguration {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


enum Type {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Use an http-proxy specified by {@link ProxyConfiguration#hostAndPort()} and (optionally)
* {@link ProxyConfiguration#credentials()}.
*/
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.
*/
public abstract String hostAndPort();
@JsonProperty("hostAndPort")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

public abstract Optional<String> maybeHostAndPort();

/**
* @deprecated Use maybeHostAndPort().
*/
@Deprecated
@SuppressWarnings("checkstyle:designforextension")
@JsonIgnore
@Lazy
public String hostAndPort() {
Preconditions.checkState(maybeHostAndPort().isPresent(), "hostAndPort was not configured");
return maybeHostAndPort().get();
}

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

@Value.Default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document default.

@SuppressWarnings("checkstyle:designforextension")
public Type type() {
return Type.http;
}

@Value.Check
protected final void check() {
HostAndPort host = HostAndPort.fromString(hostAndPort());
Preconditions.checkArgument(host.hasPort(), "Given hostname does not contain a port number: " + host);
switch (type()) {
case http:
HostAndPort host = HostAndPort.fromString(maybeHostAndPort().get());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch yes

Preconditions.checkArgument(host.hasPort(),
"Given hostname does not contain a port number: " + host);
break;
case direct:
Preconditions.checkArgument(!maybeHostAndPort().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()) {
Preconditions.checkArgument(type() == Type.http, "credentials only valid for http proxies");
}
}

@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()));
switch (type()) {
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:
return Proxy.NO_PROXY;
default:
throw new IllegalStateException("unrecognized proxy type; this is a library error");
}
}

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() {
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 {

@JsonProperty("host-and-port")
Builder hostAndPortKebabCase(String hostAndPort) {
return hostAndPort(hostAndPort);
return maybeHostAndPort(hostAndPort);
}
}
}
Expand Up @@ -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()
.maybeHostAndPort("squid:3128")
.type(ProxyConfiguration.Type.direct)
.build();
}

@Test
public void testToProxy() {
ProxyConfiguration proxyConfiguration = ProxyConfiguration.of("squid:3128");
Expand All @@ -61,13 +76,14 @@ public void testToProxy() {
assertEquals(expected, proxyConfiguration.toProxy());
}


@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\"}}";
String deserializedKebabCase =
"{\"host-and-port\":\"host:80\",\"credentials\":{\"username\":\"username\",\"password\":\"password\"}}";
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);
Expand Down
Expand Up @@ -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}}";
+ "\"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},"
Expand Down
Expand Up @@ -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,\"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,"
Expand Down
@@ -0,0 +1 @@
type: direct