NIFI-4199: Consistent proxy support across components #2704

ijokarumawak · 2018-05-15T07:23:45Z

This PR adds ProxyConfigService into following processors. However, due to the restriction of underlying libraries, SOCKS and SOCKS+Auth are partially supported.

Category	Processors	HTTP	HTTP+Auth	SOCKS	SOCKS+Auth
FTP	FetchFTP GetFTP ListFTP PutFTP	OK	OK	OK	N/A
SFTP (jsch)	FetchSFTP GetSFTP ListSFTP PutSFTP	OK	OK	OK	OK
HTTP (HTTPClient)	GetHTTP PostHTTP	OK	OK	N/A	N/A
HTTP (OKHttp)	InvokeHTTP	OK	OK	OK	N/A
ES (OkHttp)	PutElasticsearchHttp QueryElasticsearchHttp FetchElasticsearchHttp FetchElasticsearchHttp PutElasticsearchHttpRecord	OK	OK	OK	N/A
AWS	PutS3Object ListS3 FetchS3Object DeleteS3Object PutKinesisFirehose PutKinesisStream PutLambda PutDynamoDB DeleteDynamoDB GetDynamoDB	OK	OK	N/A	N/A
Azure	FetchAzureBlobStorage ListAzureBlobStorage PutAzureBlobStorage DeleteAzureBlobStorage GetAzureQueueStorage PutAzureQueueStorage	OK	N/A	OK	N/A

Azure Storage client uses HTTPS, and since Azure SDK uses Java HttpURLConnection, we can not add authentication header from NiFi. The only way to set credential for Proxy auth is setting System properties. https://stackoverflow.com/questions/34877470/basic-proxy-authentication-for-https-urls-returns-http-1-0-407-proxy-authenticat/34980074
Azure EventHub processors (ConsumeAzureEventHub, GetAzureEventHub, PutAzureEventHub) do not support proxy because it uses proton-j in Apache Qpid Proton. IOHandler.handleBound uses Java NIO SocketChannel, which doesn't support Proxy.

Thank you for submitting a contribution to Apache NiFi.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?

For code changes:

Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

MikeThomsen · 2018-05-15T09:57:42Z

@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.

jugi92 · 2018-05-15T11:41:13Z

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.

ottobackwards · 2018-05-15T12:24:42Z

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

ijokarumawak · 2018-05-15T23:52:07Z

@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.

ijokarumawak · 2018-05-16T00:09:17Z

@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.

ijokarumawak · 2018-05-16T00:13:19Z

@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.

ijokarumawak · 2018-05-16T02:57:59Z

Now this PR includes SFTP processors and SOCKS proxy support for SFTP as well.

ijokarumawak · 2018-05-16T07:30:29Z

Elasticsearch processors are also included in this PR now.
I'm researching on AWS and Azure processors now, but those can be done separately.

jugi92 · 2018-05-16T08:09:50Z

...standard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java

+                    break;
+                case SOCKS:
+                    final ProxySOCKS5 proxySOCKS5 = new ProxySOCKS5(proxyConfig.getProxyServerHost(), proxyConfig.getProxyServerPort());
+                    session.setProxy(proxySOCKS5);


can we add:
if (proxyConfig.hasCredential()) { socksProxy.setUserPasswd(proxyConfig.getProxyUserName(), proxyConfig.getProxyUserPassword()); }

@jugi92 Thanks! I actually didn't know SOCKS protocol supports user authentication.. It seems only SFTP processors (thanks to the underlying jsch) support that. I've added your suggestion and confirmed with Dante SOCKS server with authentication.
https://gist.github.com/ijokarumawak/b3a31378bdc0a6c6b9922a138e9ec9c1

I will update this PR shortly.

ottobackwards · 2018-05-16T10:40:04Z

@ijokarumawak I'm talking about passing around an HttpClientBuilder when not everyone uses that.

ijokarumawak · 2018-05-17T05:23:30Z

@ottobackwards You are talking about these code specifically?

HTTPUtils.setProxy(context, clientBuilder, credentialsProvider);

Then yes, the above util method accepts HttpClientBuilder and useful only for processors those 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?

ijokarumawak · 2018-05-17T05:28:06Z

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

ijokarumawak · 2018-05-17T09:20:49Z

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.

- Added ProxyConfigurationService to manage centralized proxy configurations - Adopt ProxyConfigurationService at FTP and HTTP processors

This closes apache#2018. Signed-off-by: Koji Kawamura <[email protected]>

- Fixed check style issue - Use the same proxy related PropertyDescriptors from FTPTransfer and SFTPTransfer - Dropped FlowFile EL evaluation support to make it align with other processors spec, Now it supports VARIABLE_REGISTRY - Added ProxyConfigurationService to SFTP processors - Added SOCKS proxy support to SFTP processors

…ssors - ElasticsearchHttp processors now support SOCKS proxy, too - Added proxy support to PutElasticsearchHttpRecord - Moved more common property descriptors to AbstractElasticsearchHttpProcessor and just return static unmodifiable property descriptor list at each implementation processors

NIFI-4196 - Fix jUnit errors This closes apache#2016. Signed-off-by: Koji Kawamura <[email protected]>

- Applied ProxyConfigService to S3 processors - Added proxy support to following processors: - PutKinesisFirehose, PutKinesisStream - PutDynamoDB, DeleteDynamoDB, GetDynamoDB - PutKinesisStream - All AWS processors support HTTP proxy now

jvwing

@ijokarumawak Thanks for this PR, this is a much needed approach to Proxy configuration.

I have reviewed your changes with respect to the AWS processors only. The code looks good, I recommend only a few minor tweaks. I tested a flow with some S3 processors using the StandardProxyConfigurationService, the separate AWS processor PROXY_HOST properties, and no proxy. Everything worked fine in my tests.

jvwing · 2018-05-18T05:13:24Z

...s-bundle/nifi-aws-processors/src/test/java/org/apache/nifi/processors/aws/s3/TestListS3.java

@@ -311,5 +312,10 @@ public void testGetPropertyDescriptors() throws Exception {
        assertTrue(pd.contains(ListS3.PREFIX));
        assertTrue(pd.contains(ListS3.USE_VERSIONS));
        assertTrue(pd.contains(ListS3.MIN_AGE));
+        assertTrue(pd.contains(ProxyConfigurationService.PROXY_CONFIGURATION_SERVICE));
+        assertTrue(pd.contains(ListS3.PROXY_HOST));


Minor tweak: The check for PROXY_HOST and PROXY_HOST_PORT duplicates checks above on lines 309-310. I believe this is why we add 5 lines of new assertions, but the count of property descriptors only goes up by 3 from 17 to 20. It doesn't make any difference, really, but the math was bothering me.

@jvwing Good catch, thanks. I removed the duplicated PROXY_HOST and PROXY_HOST_PORT. The missing one was LIST_TYPE. So, 17 + 3 = 20. 3 additions are PROXY_USER, PROXY_PASS and LIST_TYPE.

jvwing · 2018-05-18T05:15:10Z

...s-abstract-processors/src/main/java/org/apache/nifi/processors/aws/AbstractAWSProcessor.java

@@ -92,6 +94,23 @@
            .addValidator(StandardValidators.PORT_VALIDATOR)
            .build();

+    public static final PropertyDescriptor PROXY_USERNAME = new PropertyDescriptor.Builder()
+            .name("Proxy Username")


I recommend a separate name vs displayName for PROXY_USERNAME.

I will update it.

jvwing · 2018-05-18T05:15:29Z

...s-abstract-processors/src/main/java/org/apache/nifi/processors/aws/AbstractAWSProcessor.java

+            .build();
+
+    public static final PropertyDescriptor PROXY_PASSWORD = new PropertyDescriptor.Builder()
+            .name("Proxy Password")


I recommend a separate name vs displayName for PROXY_PASSWORD.

I will update it.

- Each processor has different supporting Proxy specs - Show supported spec to ProxyConfigurationService property doc - Validate not only Proxy type, but also with Authentication

- Fixed TestListS3 property descriptor check - Separate name and displayName

ijokarumawak · 2018-05-18T08:53:14Z

Added 3 more commits.

Added proxy support to Azure processors.
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.
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:

ijokarumawak · 2018-05-18T08:55:20Z

I really stop updating this PR. No more addition from my side. Let's wrap this up. Thanks for reviewing!

ottobackwards · 2018-05-18T12:02:30Z

Should the tests for InvokeHTTP be updated to test with the changes?

MikeThomsen · 2018-05-20T18:24:09Z

@ijokarumawak I'm going to start reviewing this. Once we get this done, I could use a hand with a review on this lookup service I wrote which I'm partly holding back so I can do its proxy support via your changes here.

MikeThomsen

+1 LGTM. 2/3 builds pass so everything looks in order. As mentioned, this ticket may require ongoing refactoring of additional components, so since LGTM now we'll merge so we can keep scope creep down.

MikeThomsen · 2018-05-20T19:40:17Z

@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.

ijokarumawak · 2018-05-21T01:11:05Z

@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!

ottobackwards · 2018-05-21T15:08:10Z

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.

ottobackwards · 2018-05-21T15:16:16Z

Once I prove out my fix and update my pr, I'll guess I'll do a PR against master with that fix?

ottobackwards · 2018-05-21T15:40:36Z

https://issues.apache.org/jira/browse/NIFI-5220

ottobackwards · 2018-05-21T16:48:25Z

#2727

ijokarumawak force-pushed the nifi-4199 branch from 7c890e9 to 65c9841 Compare May 16, 2018 00:15

ijokarumawak mentioned this pull request May 16, 2018

NIFI-4175 - Add HTTP proxy support to *SFTP processors #2018

Closed

10 tasks

ijokarumawak force-pushed the nifi-4199 branch from 900188c to 0ac1a3f Compare May 16, 2018 05:30

jugi92 reviewed May 16, 2018

View reviewed changes

ijokarumawak force-pushed the nifi-4199 branch from 0ac1a3f to 873c3a6 Compare May 17, 2018 05:13

ijokarumawak mentioned this pull request May 17, 2018

NIFI-4196 - Expose AWS proxy authentication settings #2016

Closed

11 tasks

ijokarumawak force-pushed the nifi-4199 branch from 873c3a6 to 367261a Compare May 17, 2018 08:56

ijokarumawak and others added 6 commits May 17, 2018 18:25

NIFI-4199: Added ProxyConfigurationService

465ddd1

- Added ProxyConfigurationService to manage centralized proxy configurations - Adopt ProxyConfigurationService at FTP and HTTP processors

NIFI-4175 - Add HTTP proxy support to *SFTP processors

7df77dd

This closes apache#2018. Signed-off-by: Koji Kawamura <[email protected]>

NIFI-4196 - Expose AWS proxy authentication settings

8da9898

NIFI-4196 - Fix jUnit errors This closes apache#2016. Signed-off-by: Koji Kawamura <[email protected]>

ijokarumawak force-pushed the nifi-4199 branch from 367261a to b771666 Compare May 17, 2018 09:26

NIFI-4199: Add ProxyConfigService to Azure processors

945808d

jvwing reviewed May 18, 2018

View reviewed changes

ijokarumawak added 2 commits May 18, 2018 17:38

NIFI-4199: More explicit validation and docs for Proxy spec

7cdcbf4

- Each processor has different supporting Proxy specs - Show supported spec to ProxyConfigurationService property doc - Validate not only Proxy type, but also with Authentication

NIFI-4199: Incorporated review comments

45a0e5e

- Fixed TestListS3 property descriptor check - Separate name and displayName

MikeThomsen approved these changes May 20, 2018

View reviewed changes

asfgit closed this in 2834fa4 May 20, 2018

NIFI-4199: Consistent proxy support across components #2704

NIFI-4199: Consistent proxy support across components #2704

Uh oh!

Conversation

ijokarumawak commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

MikeThomsen commented May 15, 2018

Uh oh!

jugi92 commented May 15, 2018

Uh oh!

ottobackwards commented May 15, 2018

Uh oh!

ijokarumawak commented May 15, 2018

Uh oh!

ijokarumawak commented May 16, 2018

Uh oh!

ijokarumawak commented May 16, 2018

Uh oh!

ijokarumawak commented May 16, 2018

Uh oh!

ijokarumawak commented May 16, 2018

Uh oh!

jugi92 May 16, 2018

Choose a reason for hiding this comment

Uh oh!

ijokarumawak May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ottobackwards commented May 16, 2018

Uh oh!

ijokarumawak commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijokarumawak commented May 17, 2018

Uh oh!

ijokarumawak commented May 17, 2018

Uh oh!

jvwing left a comment

Choose a reason for hiding this comment

Uh oh!

jvwing May 18, 2018

Choose a reason for hiding this comment

Uh oh!

ijokarumawak May 18, 2018

Choose a reason for hiding this comment

Uh oh!

jvwing May 18, 2018

Choose a reason for hiding this comment

Uh oh!

ijokarumawak May 18, 2018

Choose a reason for hiding this comment

Uh oh!

jvwing May 18, 2018

Choose a reason for hiding this comment

Uh oh!

ijokarumawak May 18, 2018

Choose a reason for hiding this comment

Uh oh!

ijokarumawak commented May 18, 2018

Uh oh!

ijokarumawak commented May 18, 2018

Uh oh!

ottobackwards commented May 18, 2018

Uh oh!

MikeThomsen commented May 20, 2018

Uh oh!

MikeThomsen left a comment

Choose a reason for hiding this comment

Uh oh!

MikeThomsen commented May 20, 2018

Uh oh!

ijokarumawak commented May 21, 2018

Uh oh!

ottobackwards commented May 21, 2018

ijokarumawak commented May 15, 2018 •

edited

Loading

ijokarumawak May 17, 2018 •

edited

Loading

ijokarumawak commented May 17, 2018 •

edited

Loading