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

Support routing data through an HTTP proxy #11891

Merged
merged 3 commits into from
Nov 10, 2021
Merged

Conversation

maytasm
Copy link
Contributor

@maytasm maytasm commented Nov 8, 2021

Support routing data through a HTTP proxy

Description

This adds the ability for the HttpClient to connect through a HTTP proxy. We
augment the channel factory to check if it is supposed to be proxied and, if so,
we connect to the proxy host first, issue a CONNECT command through to the final
recipient host and then give the channel to the normal http client for usage.

Note that this is most useful for any extension (within Druid) that is using Druid's HttpClient and needs to make requests to endpoints outside of the Druid network where proxy is required.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

imply-cheddar and others added 2 commits November 8, 2021 12:06
* Support routing data through an HTTP proxy

This adds the ability for the HttpClient to connect through an HTTP proxy.  We
augment the channel factory to check if it is supposed to be proxied and, if so,
we connect to the proxy host first, issue a CONNECT command through to the final
recipient host and *then* give the channel to the normal http client for usage.
Copy link
Contributor

@techdocsmith techdocsmith left a comment

Choose a reason for hiding this comment

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

Style suggestions for clarity.

Comment on lines 361 to 363
It is possible to adds the ability for the HttpClient of your extension to connect through a HTTP proxy.
Internally, Druid will augment the channel factory to check if it is supposed to be proxied and, if so, connect to the proxy host first,
issue a CONNECT command through to the final recipient host and then give the channel to the normal http client for usage.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
It is possible to adds the ability for the HttpClient of your extension to connect through a HTTP proxy.
Internally, Druid will augment the channel factory to check if it is supposed to be proxied and, if so, connect to the proxy host first,
issue a CONNECT command through to the final recipient host and then give the channel to the normal http client for usage.
You can add the ability for the `HttpClient` of your extension to connect through an HTTP proxy.
Internally, Druid augments the channel factory to check if it should proxy connections. If so the client connects to the proxy host first to issue a subsequent `CONNECT` command through to the final recipient host. After the connection is established, the channel passes back to the original HTTP client for usage.

I'm not sure what "normal" means in the connection here. It seems like the way that Druid handles it by passing the channel back and forth may be an implementation detail. I am wondering if I am enabling proxying why I need to know that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded as suggestion and removed implementation details

Comment on lines 365 to 367
To enable this feature and allow for proxy in your extension's HTTP client,
first, add `HttpClientProxyConfig` as a `@JsonProperty` to the HTTP config class of your extension.
Next, in the extension's module class, add `HttpProxyConfig` to `HttpClientConfig`. For example,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To enable this feature and allow for proxy in your extension's HTTP client,
first, add `HttpClientProxyConfig` as a `@JsonProperty` to the HTTP config class of your extension.
Next, in the extension's module class, add `HttpProxyConfig` to `HttpClientConfig`. For example,
To enable proxy connection for your extension's HTTP client:
1. Add `HttpClientProxyConfig` as a `@JsonProperty` to the HTTP config class of your extension.
2. In the extension's module class, add `HttpProxyConfig` to `HttpClientConfig`. For example, where `HttpClientConfig` is the HTTP config class for your extension:

(Not sure if I interpreted this correctly, but include the information from the note on line 375 in the for example statement 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.

Done

.withReadTimeout(config.getReadTimeout().toStandardDuration())
.withHttpProxyConfig(config.getProxyConfig());
```
(note that the `config` variable in the example above is the HTTP config class of your extension)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(note that the `config` variable in the example above is the HTTP config class of your extension)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@techdocsmith techdocsmith left a comment

Choose a reason for hiding this comment

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

Docs LGTM

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

Successfully merging this pull request may close these issues.

6 participants