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

NIFI-5266: Sanitize ES parameters in PutElasticsearchHttp processors #2760

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mattyb149
Copy link
Contributor

mattyb149 commented Jun 4, 2018

I ran the PutESHttp integration tests as well (after adding new ones to test this behavior). Also fixed a couple of minor issues while I was in here, such as adding unit tests around Expression Language for the ES URL, fixing the result/reason error handling to return the right string, etc.

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.

@alopresto

This comment has been minimized.

Copy link
Contributor

alopresto commented Jun 4, 2018

Reviewing...

@ottobackwards
Copy link
Contributor

ottobackwards left a comment

Quick review with some feedback

@@ -227,7 +226,10 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
List<FlowFile> flowFilesToTransfer = new LinkedList<>(flowFiles);

final StringBuilder sb = new StringBuilder();
final String baseUrl = trimToEmpty(context.getProperty(ES_URL).evaluateAttributeExpressions().getValue());
final String baseUrl = context.getProperty(ES_URL).evaluateAttributeExpressions().getValue().trim();
if (StringUtils.isEmpty(baseUrl)) {

This comment has been minimized.

@ottobackwards

ottobackwards Jun 5, 2018

Contributor

if baseUrl is empty should the exception message be that "Elasticsearch URL evaluates to empty" or something? Your message is always going to be "... not valid: " since baseUrl will be empty.

This comment has been minimized.

@mattyb149

mattyb149 Jun 5, 2018

Author Contributor

D'oh, good catch, I was doing it differently before and left in the useless error message :P Will change in both places.

String jsonString = json.toString();

// Ensure the JSON body is well-formed
try {

This comment has been minimized.

@ottobackwards

ottobackwards Jun 5, 2018

Contributor

mapper should be static

This comment has been minimized.

@ottobackwards

ottobackwards Jun 5, 2018

Contributor

I wonder if a generally available validate json wouldn't be better, and something the next person could use.

This comment has been minimized.

@mattyb149

mattyb149 Jun 5, 2018

Author Contributor

will change to static. A generally available one would be good, just need to find the right place (like a utility JAR) and make sure it doesn't interfere with other bundles using it (like if they need their own Jackson or don't want all the Jackson dependencies when they resolve the utility JAR)

This comment has been minimized.

@zenfenan

zenfenan Jun 18, 2018

Contributor

@ottobackwards @mattyb149 As part of NIFI-5261 and NIFI-5271 introduced a JSON utils package. I'm taking this as an opportunity to share it with you guys and whoever takes a look at this comment. That utility package can be enhanced and introduced here and in your future contributions

@@ -261,7 +259,10 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
OkHttpClient okHttpClient = getClient();
final ComponentLog logger = getLogger();

final String baseUrl = trimToEmpty(context.getProperty(ES_URL).evaluateAttributeExpressions().getValue());
final String baseUrl = context.getProperty(ES_URL).evaluateAttributeExpressions().getValue().trim();
if (StringUtils.isEmpty(baseUrl)) {

This comment has been minimized.

@ottobackwards

ottobackwards Jun 5, 2018

Contributor

Same as previous wrt baseUrl always being empty in the message

@mattyb149 mattyb149 force-pushed the mattyb149:NIFI-5266 branch from 5e5cc7e to 9cdcd3f Jun 5, 2018

@ottobackwards

This comment has been minimized.

Copy link
Contributor

ottobackwards commented Jun 5, 2018

+1 (non-binding), contrib-check build looks good, tests updated.

runner.enqueue("");
runner.run(1, true, true);
runner.assertTransferCount(PutElasticsearchHttpRecord.REL_FAILURE, 0);
runner.assertTransferCount(PutElasticsearchHttpRecord.REL_RETRY, 0);

This comment has been minimized.

@ottobackwards

ottobackwards Jun 5, 2018

Contributor

Should this succeed or fail?

This comment has been minimized.

@mattyb149

mattyb149 Jun 5, 2018

Author Contributor

The error from Elasticsearch (as logged in testIllegalIndexName()) is:
must not contain the following characters [ , ", *, \, <, |, ,, >, /, ?]
According to that, } is a legitimate character in an index name, it is included here to show that a brace doesn't mess with the generated JSON body issued to the REST endpoint

This comment has been minimized.

@ottobackwards

ottobackwards Jun 5, 2018

Contributor

ah, ok sorry. I mis-understood. my +1 stands

@asfgit asfgit closed this in aa33989 Jun 19, 2018

@alopresto

This comment has been minimized.

Copy link
Contributor

alopresto commented Jun 19, 2018

Thanks @mattyb149 . I ran through this using the template you provided. Ran contrib-check and all tests pass. I squashed the commits. +1, merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.