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

Proxy support for Dropwizard HTTP client #657

Merged
merged 6 commits into from Apr 10, 2015

Conversation

Projects
None yet
5 participants
@arteam
Member

arteam commented Jul 12, 2014

I often encounter with a situation when in closed corporate environments direct access to the Internet is blocked by security reasons. So all connections to external resources happen through a proxy server (usually with BASIC authentication).

Deploying applications in such environments becomes a huge pain, because usually you don't know in advance that an application will need proxy connection support and sometimes you just don't have possibility to change code to add this facility.

I believe, it would be quite useful to have this facility in the Dropwizard config. It frees an application developer from thoughts about environments where an application will be used and shifts this responsibility to operations guys. And they can configure proxy client support just in the application config without using external tools.

PS I've added the docs to proxy configuration. But I'm not a skilled english speaker, so, please, fix them if I messed up something.

@jplock

This comment has been minimized.

Member

jplock commented Jul 13, 2014

+1

@joschi joschi added the feature label Jul 13, 2014

@joschi joschi added this to the 0.8.0 milestone Jul 13, 2014

@NotEmpty
private String host;
@Range(min = 1, max = 65535)

This comment has been minimized.

@joschi

joschi Jul 13, 2014

Member

You could use @PortRange from dropwizard-validation here.

This comment has been minimized.

@arteam

arteam Jul 13, 2014

Member

I thought port 0 makes no sense in connecting to a remote host and in the same time I wanted to use -1 as a marker of the schema default port. Maybe

@PortRange(min = -1)

is a good variant because it allows to specify -1 as the port value and don't set max = 65535 in the annotation.

@Range(min = 1, max = 65535)
@Nullable
private Integer port;

This comment has been minimized.

@joschi

joschi Jul 13, 2014

Member

Many (HTTP-) proxies are using port 8080/tcp. Maybe it makes sense to use it as a default here.

This comment has been minimized.

@arteam

arteam Jul 13, 2014

Member

I think it's better to use the schema default port as a more expected behaviour in case omitted port. 8080 is often used port, but not a standard. And in the case of HTTPS proxies we force users to set 443 port.

@Nullable
private Integer port;
@OneOf({"http", "https", "HTTP", "HTTPS"})

This comment has been minimized.

@joschi

joschi Jul 13, 2014

Member

You can use the ignoreCase parameter to get rid of the "duplicate" elements in this array.

This comment has been minimized.

@arteam

arteam Jul 13, 2014

Member

Thanks, I missed that facility.

this.auth = auth.orNull();
}
public ProxyConfiguration(@NotNull String host, Optional<Integer> port) {

This comment has been minimized.

@joschi

joschi Jul 13, 2014

Member

I think it would make sense to call the other constructor to eliminate duplicate assignments which have to be maintained.

this(host, port, Optional.of(DEFAULT_SCHEME), Optional.absent());

This comment has been minimized.

@arteam

arteam Jul 13, 2014

Member

I agree.
And as we now use default values for the schema and port we can omit optional types and just use 3 constructors with plain values.

this.port = port;
}
public int getPresentPort() {

This comment has been minimized.

@joschi

joschi Jul 13, 2014

Member

If the default value of port was DEFAULT_PORT and its type was int you could basically get rid of this method.

This comment has been minimized.

@arteam

arteam Jul 13, 2014

Member

Makes sense. We can just set -1 as the default value and include it in PortRange annotation so validation is passed.

private static final int DEFAULT_PORT = -1;
@NotEmpty
private String host;

This comment has been minimized.

@joschi

joschi Jul 13, 2014

Member

Have you thought about using Guava's HostAndPort (which is supported by Jackson) instead of separate host and port fields?

This comment has been minimized.

@arteam

arteam Jul 13, 2014

Member

Yes, I have.
In the fact, I think if we would want to make address configuration in one line, we could have just use URL. And Guava HostAndPort is neither one nor the other, because we need to use schema anyway. And configuration like that

proxy:
    host_port: '192.168.52.11:8080'
    schema: 'http'

seems unnatural for me.

}
@NotNull
public String getPresentScheme() {

This comment has been minimized.

@joschi

joschi Jul 13, 2014

Member

If the default value of scheme was DEFAULT_SCHEME you could basically get rid of this method.

This comment has been minimized.

@arteam

arteam Jul 13, 2014

Member

Agree with this too. Default value is more concise approach.

@joschi

This comment has been minimized.

Member

joschi commented Jul 13, 2014

Thank you very much for this PR! Nicely documented. 😄

@arteam

This comment has been minimized.

Member

arteam commented Jul 13, 2014

Thanks for the review! I've tried to fix up things you mentioned.

@jplock

This comment has been minimized.

Member

jplock commented Aug 8, 2014

Any idea if this will get merged in?

@philandstuff

This comment has been minimized.

Contributor

philandstuff commented Aug 11, 2014

I'm currently using DropWizard with a proxy server, but this PR wouldn't solve my problem because I need a whitelist of non-proxyable hosts on the local network. I have raised #681 as an alternative (not mutually exclusive) way to add proxy functionality. More details there.

@coveralls

This comment has been minimized.

coveralls commented Sep 29, 2014

Coverage Status

Coverage increased (+0.71%) when pulling aeaca58 on arteam:proxy-support into a876b65 on dropwizard:master.

@arteam

This comment has been minimized.

Member

arteam commented Sep 29, 2014

I rebased the branch against the master and added a facility to configure non-proxyable hosts.

Does anyone interested in this type of configuration? If #681 is merged, then it would only a matter of taste of configuration type (YAML file or java properties).

@coveralls

This comment has been minimized.

coveralls commented Sep 29, 2014

Coverage Status

Coverage increased (+0.71%) when pulling aeaca58 on arteam:proxy-support into a876b65 on dropwizard:master.

@arteam

This comment has been minimized.

Member

arteam commented Jan 10, 2015

I see #681 has been merged.

Is there any work needed for this PR? If Java properties configuration is sufficient then this PR could be closed or moved to a backlog from 0.8 release.

@joschi joschi removed this from the 0.8.0 milestone Jan 17, 2015

@joschi

This comment has been minimized.

Member

joschi commented Jan 17, 2015

@arteam I'll move this PR out of the 0.8.0 milestone for now but I won't close it for now.

Maybe someone else wants to chime in whether the current state of affairs is sufficient or if this PR should be updated and merged.

@jplock

This comment has been minimized.

Member

jplock commented Apr 1, 2015

@arteam did you want to update this for 0.9.0 or should we close this out?

arteam added some commits Jul 12, 2014

Code-review improvements of proxy configuration
- Use default values for the port and the scheme instead constants and methods getPresentPort and getPresentScheme
- Use @PortRange with min=-1 instead @range
- Use ignoreCase in @OneOf instead enumeration possible schemas in both cases.
- Reuse constructors
Migrations of proxy configuration to new API in the master
- Migration to Apache HTTP Client 4.3
- FEST -> AssertJ
@arteam

This comment has been minimized.

Member

arteam commented Apr 10, 2015

I've rebased this PR against master, it wasn't hard.

I just don't know if anyone need this feature apart me one year back :)

@jplock

This comment has been minimized.

Member

jplock commented Apr 10, 2015

I've got a few projects that are restricted to operating inside of an AWS VPC and need to communicate via a proxy, so I think this would be a great addition.

jplock added a commit that referenced this pull request Apr 10, 2015

Merge pull request #657 from arteam/proxy-support
Proxy support for Dropwizard HTTP client

@jplock jplock merged commit 76542c0 into dropwizard:master Apr 10, 2015

@jplock jplock added this to the 0.9.0 milestone Apr 10, 2015

@jplock

This comment has been minimized.

Member

jplock commented Apr 10, 2015

Thanks a lot for this contribution!

@arteam arteam deleted the arteam:proxy-support branch Jan 24, 2016

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