[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ottobackwards commented on the issue: https://github.com/apache/nifi/pull/2704 https://github.com/apache/nifi/pull/2727 ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ottobackwards commented on the issue: https://github.com/apache/nifi/pull/2704 https://issues.apache.org/jira/browse/NIFI-5220 ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ottobackwards commented on the issue: https://github.com/apache/nifi/pull/2704 Once I prove out my fix and update my pr, I'll guess I'll do a PR against master with that fix? ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ottobackwards commented on the issue: https://github.com/apache/nifi/pull/2704 I found a bug in this in the aws implementation, I am not sure how you would see it in the other processors, I found it when bringing this code into my Gateway Api PR. The issue is that customValidate validates that both host and port need to be set, but not that both user and password need to be set. Since I test for this ( from the InvokeHttp testProxy ), I fail. ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2704 @MikeThomsen Thanks for merging this. Although my original intent was keeping commits made by @trixpan separated (not squashed) to retain his credits, it looks good to me because the original PR 4196 and 4175 are closed as I expected. @trixpan Thanks again for originating this improvements! ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2704 @trixpan @ijokarumawak There are two other tickets referenced, 4196 and 4175(?) in the commit list for this PR. Before I keep squashing, I want to confirm that you want me to keep going and put 3 "This closes #ABCD" statements in there to close this, 4196 and 4175. ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2704 @ijokarumawak I'm going to start reviewing this. Once we get this done, I could use a hand with a review on [this](https://github.com/apache/nifi/pull/2723) lookup service I wrote which I'm partly holding back so I can do its proxy support via your changes here. ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ottobackwards commented on the issue: https://github.com/apache/nifi/pull/2704 Should the tests for InvokeHTTP be updated to test with the changes? ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2704 I really stop updating this PR. No more addition from my side. Let's wrap this up. Thanks for reviewing! ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2704 Added 3 more commits. 1. Added proxy support to Azure processors. 2. Adding more explicit Proxy spec check and doc. Due to the restrictions of underlying libraries, Proxy support spec varies. Based on the investigation summarized in this PR's description, I've used 4 labels to represent spec HTTP, HTTP_AUTH, SOCKS and SOCKS_AUTH. 3. Incorporated review comments. Example screenshots: InvokeHTTP does not support SOCKS_AUTH, so if ProxyConfigurationService is configured with SOCKS and username/password, then it becomes invalid, but SOCKS without auth can be used:  PostHTTP does not support SOCKS at all:  Not only validation, property description shows what proxy is supported:  SFTP processors are the only ones supporting all Proxy specs:  ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2704 I've summarized current capabilities on this PR's description. Please check the table. We can keep expanding the list of processors, but I'd stop here and finish reviewing these processors as the 1st phase. ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2704 Now this PR also includes AWS related processors. I've tested following processors can utilize HTTP forward proxies and support authentication: - PutS3Object - ListS3 - FetchS3Object - DeleteS3Object - PutKinesisFirehose - PutKinesisStream - PutLambda - PutDynamoDB - DeleteDynamoDB - GetDynamoDB ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/2704
@ottobackwards You are talking about these code specifically?
```
HTTPUtils.setProxy(context, clientBuilder, credentialsProvider);
```
Then yes, the above util method accepts HttpClientBuilder and useful for
processors those only use HttpClient library. It's currently used from only
GetHTTP and PostHTTP. It's just a convenient method for those two for now.
Other processors who don't use HttpClient, uses ProxyConfiguration directly
to get proxy settings. Following snippet is copied from AbstractAWSProcessor:
```
// Get Proxy configuration from ProxyConfigurationService if it's used, or
from processor's own proxy configurations, either way, the configurations are
put into the `proxyConfig` instance. And subsequent code do not have to care
how where these settings are set.
final ProxyConfiguration proxyConfig =
ProxyConfiguration.getConfiguration(context, () -> {
if (context.getProperty(PROXY_HOST).isSet()) {
final ProxyConfiguration componentProxyConfig = new
ProxyConfiguration();
String proxyHost =
context.getProperty(PROXY_HOST).evaluateAttributeExpressions().getValue();
Integer proxyPort =
context.getProperty(PROXY_HOST_PORT).evaluateAttributeExpressions().asInteger();
String proxyUsername =
context.getProperty(PROXY_USERNAME).evaluateAttributeExpressions().getValue();
String proxyPassword =
context.getProperty(PROXY_PASSWORD).evaluateAttributeExpressions().getValue();
componentProxyConfig.setProxyType(Proxy.Type.HTTP);
componentProxyConfig.setProxyServerHost(proxyHost);
componentProxyConfig.setProxyServerPort(proxyPort);
componentProxyConfig.setProxyUserName(proxyUsername);
componentProxyConfig.setProxyUserPassword(proxyPassword);
return componentProxyConfig;
}
return ProxyConfiguration.DIRECT_CONFIGURATION;
});
// Apply Proxy settings to underlying SDK/API.
if (Proxy.Type.HTTP.equals(proxyConfig.getProxyType())) {
config.setProxyHost(proxyConfig.getProxyServerHost());
config.setProxyPort(proxyConfig.getProxyServerPort());
if (proxyConfig.hasCredential()) {
config.setProxyUsername(proxyConfig.getProxyUserName());
config.setProxyPassword(proxyConfig.getProxyUserPassword());
}
}
```
Does that answer to your question?
---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ottobackwards commented on the issue: https://github.com/apache/nifi/pull/2704 @ijokarumawak I'm talking about passing around an HttpClientBuilder when not everyone uses that. ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2704 Elasticsearch processors are also included in this PR now. I'm researching on AWS and Azure processors now, but those can be done separately. ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2704 Now this PR includes SFTP processors and SOCKS proxy support for SFTP as well. ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2704 @MikeThomsen We can combine ProxyConfigurationService into ES or Solr, the CS just let users manage proxy settings in a centralized place. I will take a look on #2094 to see how I can help review that one. Thanks. ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/2704
@jugi92 FTPTransfer supports SOCKS proxy. Specifically at these lines:
```
if (proxyType == Proxy.Type.HTTP) {
-client = new FTPHTTPClient(proxyHost, proxyPort,
ctx.getProperty(HTTP_PROXY_USERNAME).getValue(),
ctx.getProperty(HTTP_PROXY_PASSWORD).getValue());
+client = new FTPHTTPClient(proxyHost, proxyPort,
proxyConfig.getProxyUserName(), proxyConfig.getProxyUserPassword());
} else {
client = new FTPClient();
if (proxyType == Proxy.Type.SOCKS) {
client.setSocketFactory(new SocksProxySocketFactory(new
Proxy(proxyType, new InetSocketAddress(proxyHost, proxyPort;
}
}
```
https://github.com/apache/nifi/pull/2704/files#diff-6e7e715d42f332cbe404edd9afbcaafaL533
For processors those don't support SOCKS proxy, following validation code
should be added into their customValidate method, to confirm that
ProxyConfigurationService is configured with the supported proxy type(s):
```
ProxyConfiguration.validateProxyType(validationContext, results,
Proxy.Type.HTTP);
```
ProxyConfigurationService just holds the centralized proxy settings, each
processor is responsible to use the settings with its own relying SDK/API way.
I checked #2018 but the PR doesn't look active. I will take a closer look
on SFTP processor and #2018 to see if I can include SFTP ones into this PR, too.
---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2704 @ottobackwards I assume you were talking about this, #2016. That one adds user/password for proxy authentication at abstract AWS processor. This PR adds ProxyConfigurationService, which can be added on top of #2016 for AWS processors proxy configurations to be managed by the centralized Controller Service. Please look at the FTP and HTTP processors in this PR, AWS ones can adopt the CS same way. ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user ottobackwards commented on the issue: https://github.com/apache/nifi/pull/2704 How will this work with the AWS components? They have proxy as well ( although there is a PR for full support ), but a different builder I think ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user jugi92 commented on the issue: https://github.com/apache/nifi/pull/2704 It would be very nice if the initial proxy service also includes a SOCKS Proxy example. Other processors that implement the Proxy Service can then reuse the existing implementation even better. For example we would probably implement that change for the SFTP processors then. ---
[GitHub] nifi issue #2704: NIFI-4199: Consistent proxy support across components
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2704 @ijokarumawak haven't had a chance to take a look at this, but have you tried it against Solr and Elastic yet? I think the latter's APIs do their own proxy management so that might need a little finessing here. ---
