-
Notifications
You must be signed in to change notification settings - Fork 84
[474] Add proxy options to http client to use Ethsigner behind proxy #475
Conversation
|
||
applyTlsOptions(clientOptions, config); | ||
return clientOptions; | ||
} | ||
|
||
private static Optional<ProxyOptions> proxyOptions() { | ||
var proxyHost = System.getProperty("http.proxyHost", "none"); | ||
var proxyPort = Integer.valueOf(System.getProperty("http.proxyPort", "80")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integer.valueOf
can throw NumberFormatException. catch block can be used to assign default value of 80 to proxy port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should be able to cover all the options in ProxyOptions
i.e. https://vertx.io/docs/apidocs/io/vertx/core/net/ProxyOptions.html . Specifying only host/port will work for unauthenticated proxy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @usmansaleem, I added two commits to the PR
ethsigner/core/src/main/java/tech/pegasys/ethsigner/core/WebClientOptionsFactory.java
Outdated
Show resolved
Hide resolved
ethsigner/core/src/main/java/tech/pegasys/ethsigner/core/WebClientOptionsFactory.java
Outdated
Show resolved
Hide resolved
ethsigner/core/src/main/java/tech/pegasys/ethsigner/core/WebClientOptionsFactory.java
Outdated
Show resolved
Hide resolved
ethsigner/commandline/src/main/java/tech/pegasys/ethsigner/EthSignerBaseCommand.java
Show resolved
Hide resolved
ethsigner/commandline/src/main/java/tech/pegasys/ethsigner/EthSignerBaseCommand.java
Outdated
Show resolved
Hide resolved
…rouped with the other downstream config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for you contribution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for taking your time to submit the PR.
PR Description
Add configuration to the vertx http client used by Ethsigner to access blockchain node behind a proxy.
Fixed Issue(s)
fixes #474
Documentation
A change in the documentation should be made to use config variables for proxy :
--downstream-http-proxy-host : Hostname for proxy connect, no proxy if null (default: null)
--downstream-http-proxy-port : Port for proxy connect (default: 80)
--downstream-http-proxy-username : Username for proxy connect, no authentication if null (default: null)
--downstream-http-proxy-password : Password for proxy connect, no authentication if null (default: null)