NIFI-4932: Enable S2S work behind a Reverse Proxy #2510

ijokarumawak · 2018-03-05T06:51:03Z

Adding S2S endpoint Reverse Proxy mapping capability.

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.

ijokarumawak · 2018-03-05T07:02:07Z

Dear reviewers, I've tested this improvement with Nginx and AWS ALB (Application Load Balancer). I used docker to run different Nginx configurations to test, and those docker environments are available here in my github project, which may be useful for reviewing, too. https://github.com/ijokarumawak/nifi-reverseproxy

Please build this PR and see updated administration-guide.html for details. Thank you!

Adding S2S endpoint Reverse Proxy mapping capability.

alopresto · 2018-03-05T19:32:47Z

Thanks Koji, this is really important work. I can't review immediately but when I get a couple of my tickets knocked out, I'll take a look. Hopefully we get multiple sets of eyes on this one.

ijokarumawak · 2018-03-06T00:02:22Z

@alopresto Thank you! The RAT check failed with SVG files for docs. I didn't run contrib check after I added docs, my bad. I'll update it.

mcgilman · 2018-03-19T14:52:16Z

Will review...

mcgilman · 2018-03-27T19:01:34Z

...ramework/nifi-site-to-site/src/main/java/org/apache/nifi/remote/PeerDescriptionModifier.java

+        map.put(format("%s.secure", prefix), String.valueOf(peer.isSecure()));
+    }
+
+    public boolean isModificationNeeded(final SiteToSiteTransportProtocol protocol) {


Does it make sense to make this method private and invoke it from within the modify method? It seems like the general usage is to check if modification is needed and then invoke modify. Alternatively, we could invoke modify unconditionally and check if modification is needed within. If modification is not needed, then target could just be returned.

@mcgilman The reason of this method being separated is to avoid unnecessary object creations at the caller. I imagine most of deployments will not use this configuration, hence if caller side (e.g. SiteToSiteResource) always create objects to call modify unconditionally, but doing nothing actually because not having modification definitions, that would be waste or resources.

mcgilman · 2018-03-27T19:06:09Z

...ramework/nifi-site-to-site/src/main/java/org/apache/nifi/remote/PeerDescriptionModifier.java

+
+    private static final String PROPERTY_PREFIX = "nifi.remote.route.";
+
+    public PeerDescriptionModifier(final NiFiProperties properties) {


A couple comments on this method...

Can we make this code a little more readable by not using single letter variable names? It made it difficult to understand what was happening here.

Can we add better error handling throughout this method? For instance, if the properties are misconfigured it's likely this could fail exceptionally with IndexOutOfBounds or EL Parsing Exception. Can we handle those specifics and log the underlying issue and then continue to fail. We should be able to identify when the routes are misconfigured and help drive the user to the exact issue.

@mcgilman Agreed. I've changed variable names for better readability. Also changed validation logic to throw Exceptions with detailed error message.

For EL parsing, I tried several invalid ELs, but couldn't get EL parsing exception when it's compiled. Instead it errors out when it is evaluated.

More unit tests are added as well for invalid configuration scenarios.
I confirmed NiFi does not start with invalid configs.

mcgilman · 2018-03-27T19:07:42Z

...ramework/nifi-site-to-site/src/main/java/org/apache/nifi/remote/PeerDescriptionModifier.java

+                }
+            });
+            return route;
+        }).filter(Route::isValid).collect(Collectors.groupingBy(r -> r.protocol));


If a route is invalid, would it be better to fail startup or ignore the route? I have some concern that a user wouldn't notice the WARN message in the log when filtering an invalid route.

I changed it to throw Exceptions if invalid.

mcgilman · 2018-03-27T19:10:53Z

...ramework/nifi-site-to-site/src/main/java/org/apache/nifi/remote/PeerDescriptionModifier.java

+
+            String targetIsSecure = secure == null ? null : secure.evaluateExpressions(variables, null);
+            if (isBlank(targetIsSecure)) {
+                targetIsSecure = "false";


Is there a reason we are defaulting this to false here? If so, is this something that should be called out in the documentation?

@mcgilman The blank check is not needed since Boolean.valueOf returns false for null/empty string. I will remove these lines and also callout that default is 'false' in the docs.

mcgilman · 2018-03-27T19:16:15Z

nifi-docs/src/main/asciidoc/administration-guide.adoc

+image:s2s-rproxy-servername.svg["Server name to Node mapping"]
+
+1. Client1 initiates Site-to-Site protocol, the request is routed to one of upstream NiFi nodes. The NiFi node computes Site-to-Site port for RAW. By routing 'example1', port 10443 is returned.
+2. Client1 asks peers to 'nifi.example.com:10443', the request is routed to 'nifi0:8081'. The NiFi node computes available peers, by 'example1' routing rule, 'nifi0:8081' is converted to 'nifi0.example.com:10443', so are nifi1 and nifi2. As a result, 'nifi0.eample.com:10443', 'nifi1.example.com:10443' and 'nifi2.example.com:10443' are returned.


Type: 'eample'

Thanks, fixed.

mcgilman · 2018-03-27T19:18:08Z

nifi-docs/src/main/asciidoc/administration-guide.adoc

+5. Client2 asks peers from 'nifi1:8081'. The 'example1' does not match, so the original 'nifi0:8081', 'nifi1:8081' and 'nifi2:8081' are returned as they are.
+6. Client2 decides to use 'nifi2:8081' for further communication.
+
+nifi.properties (all node has the same routing configuration)


Would it make sense to move this example configuration above these 1 - 6 steps? The steps reference 'example1' which was confusing until I realized that 'example1' was used as the route name in the configuration.

Good point. I added more wording to the listed texts so that readers can be guided to the nifi.properties example. I wanted to make the diagram and the steps closer together.

ijokarumawak · 2018-03-29T02:54:26Z

@mcgilman Thanks for reviewing. I've incorporated all feedback. Please take a look it again. Thanks!

mcgilman · 2018-03-29T20:37:34Z

...ramework/nifi-site-to-site/src/main/java/org/apache/nifi/remote/PeerDescriptionModifier.java

+            route.protocol = SiteToSiteTransportProtocol.valueOf(protocolAndRoutingName[0].toUpperCase());
+            route.name = protocolAndRoutingName[1];
+            routeDefinition.getValue().forEach(propertyKey -> {
+                final String routingConfigName = propertyKey.substring(propertyKey.lastIndexOf('.') + 1);


I believe we should still better handle the property parse. I indirectly mentioned this in the last review with my comment to IndexOutOfBoundsException. Sorry if that wasn't clear previously. If a user misconfigures their properties the error isn't helpful:

Caused by: java.lang.ArrayIndexOutOfBoundsException: 1
at org.apache.nifi.remote.PeerDescriptionModifier.lambda$new$4(PeerDescriptionModifier.java:93)
at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
at java.util.HashMap$EntrySpliterator.forEachRemaining(HashMap.java:1683)

There is another substring method above that is likely susceptible to the same issue.

mcgilman · 2018-03-29T20:39:08Z

...ramework/nifi-site-to-site/src/main/java/org/apache/nifi/remote/PeerDescriptionModifier.java

+                final String routingConfigValue = properties.getProperty(propertyKey);
+                switch (routingConfigName) {
+                    case "when":
+                        route.predicate = Query.prepare(routingConfigValue);


I also could not make prepare throw but the JavaDocs state that it can. Could we add a try/catch around this switch to gracefully handle if it does throw?

mcgilman · 2018-03-29T20:55:49Z

@ijokarumawak Just wanted to add that I have verified this capability running standalone and clustered and everything seems to be working nicely. Just a couple more minor error handling cases. Thanks!

- Use regex to check property key processing. - Catch AttributeExpressionLanguageParsingException.

ijokarumawak · 2018-04-02T03:10:26Z

@mcgilman Thanks for clarifying the String manipulation exception, I just didn't have enough imagination to come up with such invalid inputs. I switched to use regex to check and parse property keys. Now it should be more robust and provide more user friendly error messages.

Also, added try/catch for EL parse failure. Thanks!

mcgilman · 2018-04-03T19:45:05Z

Thanks @ijokarumawak! This has been merged to master.

NIFI-4932: Enable S2S work behind a Reverse Proxy

71ed0f1

Adding S2S endpoint Reverse Proxy mapping capability.

ijokarumawak force-pushed the nifi-4932 branch from 9c86dde to 71ed0f1 Compare March 5, 2018 07:48

Added license header to SVG files.

7e8918a

mcgilman reviewed Mar 27, 2018

View reviewed changes

Incorporated review comments.

ea11a2e

mcgilman reviewed Mar 29, 2018

View reviewed changes

Incorporated review comments.

6deeb3c

- Use regex to check property key processing. - Catch AttributeExpressionLanguageParsingException.

asfgit closed this in 1913b1e Apr 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-4932: Enable S2S work behind a Reverse Proxy #2510

NIFI-4932: Enable S2S work behind a Reverse Proxy #2510

ijokarumawak commented Mar 5, 2018

ijokarumawak commented Mar 5, 2018

alopresto commented Mar 5, 2018

ijokarumawak commented Mar 6, 2018

mcgilman commented Mar 19, 2018

mcgilman Mar 27, 2018

ijokarumawak Mar 28, 2018

mcgilman Mar 27, 2018 •

edited

ijokarumawak Mar 29, 2018

mcgilman Mar 27, 2018

ijokarumawak Mar 29, 2018

mcgilman Mar 27, 2018

ijokarumawak Mar 29, 2018

mcgilman Mar 27, 2018

ijokarumawak Mar 29, 2018

mcgilman Mar 27, 2018

ijokarumawak Mar 29, 2018

ijokarumawak commented Mar 29, 2018

mcgilman Mar 29, 2018

mcgilman Mar 29, 2018

mcgilman commented Mar 29, 2018

ijokarumawak commented Apr 2, 2018

mcgilman commented Apr 3, 2018


		private static final String PROPERTY_PREFIX = "nifi.remote.route.";

		public PeerDescriptionModifier(final NiFiProperties properties) {

NIFI-4932: Enable S2S work behind a Reverse Proxy #2510

NIFI-4932: Enable S2S work behind a Reverse Proxy #2510

Conversation

ijokarumawak commented Mar 5, 2018

For all changes:

For code changes:

For documentation related changes:

Note:

ijokarumawak commented Mar 5, 2018

alopresto commented Mar 5, 2018

ijokarumawak commented Mar 6, 2018

mcgilman commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcgilman Mar 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijokarumawak commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcgilman commented Mar 29, 2018

ijokarumawak commented Apr 2, 2018

mcgilman commented Apr 3, 2018

mcgilman Mar 27, 2018 •

edited