feat: support HTTP proxy in client SDK #467
Conversation
cwaldren-ld
changed the title
| @@ -33,10 +33,10 @@ HttpResult::HeadersType const& HttpResult::Headers() const { | |||
| HttpResult::HttpResult(HttpResult::StatusCode status, | |||
| std::optional<std::string> body, | |||
| HttpResult::HeadersType headers) | |||
| : status_(status), | |||
There was a problem hiding this comment.
reorder to match declaration
| @@ -47,7 +47,7 @@ std::optional<std::string> const& HttpResult::ErrorMessage() const { | |||
| } | |||
|
|
|||
| HttpResult::HttpResult(std::optional<std::string> error_message) | |||
There was a problem hiding this comment.
reorder to match declaration
| HttpRequest::HttpRequest(std::string const& url, | ||
| HttpMethod method, | ||
| config::shared::built::HttpProperties properties, | ||
| HttpRequest::BodyType body) | ||
| : properties_(std::move(properties)), |
There was a problem hiding this comment.
reorder to match decl
| @@ -108,20 +120,42 @@ HttpRequest::HttpRequest(std::string const& url, | |||
| if (uri_components->has_port()) { | |||
| port_ = uri_components->port(); | |||
| } | |||
|
|
|||
| if (properties_.HttpProxy()) { | |||
There was a problem hiding this comment.
Side note: it would be much better if we could validate these URLs earlier in the config process, but that would be a more major refactoring.
| valid_ = true; | ||
| } | ||
|
|
||
| HttpRequest::HttpRequest(HttpRequest& base_request, | ||
| config::shared::built::HttpProperties properties) | ||
| : properties_(std::move(properties)), |
There was a problem hiding this comment.
reorder to match decl
cwaldren-ld
force-pushed
the
cw/sdk-888-http-proxy
branch
from
c80fb12
to
0b9fc72
Compare
| @@ -68,29 +65,51 @@ static http::verb ConvertMethod(HttpMethod method) { | |||
| launchdarkly::detail::unreachable(); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Unclear to me how much this matters. This is for setting the Host field on the HTTP request.
| std::string tcp_host = | ||
| request->HttpProxyHost().value_or(request->Host()); | ||
|
|
||
| std::string tcp_port = request->HttpProxyPort().value_or( |
There was a problem hiding this comment.
This could instead be:
request->Https() ? "https" : "http"
since asio accepts "service names". A bit more abstract.
| @@ -585,50 +591,93 @@ Builder& Builder::custom_ca_file(std::string path) { | |||
| return *this; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Some duplication with our internal HTTP request code.
| if (!url->has_scheme()) { | ||
| return false; | ||
| } | ||
| return url->scheme_id() == boost::urls::scheme::http || |
There was a problem hiding this comment.
No FTP flag delivery allowed.
| tcp_port = http_proxy_components->has_port() | ||
| ? http_proxy_components->port() | ||
| : http_proxy_components->scheme(); | ||
|
|
There was a problem hiding this comment.
More inconsistencies with our internal http lib stuff. Here we can just pass in ->scheme() as the service, whereas in the previous helper code I was passing 80 or 443, since our internal lib doesn't have a scheme() getter. We could add one though.
cwaldren-ld
force-pushed
the
cw/sdk-888-http-proxy
branch
from
4fc7cd8
to
06dff6d
Compare
This supports configuring an HTTP proxy in the client SDK. Although some of the code is shared in common and would be available to the server, I've limited the scope to keep the change manageable.
Users can also set the proxy using the common
http_proxyenvironment variable. The feature is tested via contract test for events, polling, and streaming.This implementation allows an http proxy to be used for https requests from the SDK. That is, the proxy is a forward proxy. This can be useful if TLS between the SDK and the proxy isn't necessary, but it's not e2e secure.
Future work should implement HTTPS proxy support. In that world, the http proxy mode is only used for HTTP requests (for instance, to Relay) and the https proxy mode is used for https requests (to launchdarkly, or if Relay is in TLS mode.)