You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by kemixkoo <gi...@git.apache.org> on 2018/09/23 05:12:26 UTC
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
GitHub user kemixkoo opened a pull request:
https://github.com/apache/nifi/pull/3020
NIFI-5625: support the variables for the properties of HTTP processors
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](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kemixkoo/nifi NIFI-5625_support_expression
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/3020.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3020
----
commit 46bc622eb79fc29573de496def0a0a7e49248933
Author: Kemix Koo <ke...@...>
Date: 2018-09-23T05:09:22Z
NIFI-5625: support the variables for the properties of HTTP processors
----
---
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3020#discussion_r219712932
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java ---
@@ -714,11 +721,11 @@ private void setSslSocketFactory(OkHttpClient.Builder okHttpClientBuilder, SSLCo
}
private void setAuthenticator(OkHttpClient.Builder okHttpClientBuilder, ProcessContext context) {
- final String authUser = trimToEmpty(context.getProperty(PROP_BASIC_AUTH_USERNAME).getValue());
+ final String authUser = trimToEmpty(context.getProperty(PROP_BASIC_AUTH_USERNAME).evaluateAttributeExpressions().getValue());
// If the username/password properties are set then check if digest auth is being used
if (!authUser.isEmpty() && "true".equalsIgnoreCase(context.getProperty(PROP_DIGEST_AUTH).getValue())) {
- final String authPass = trimToEmpty(context.getProperty(PROP_BASIC_AUTH_PASSWORD).getValue());
+ final String authPass = trimToEmpty(context.getProperty(PROP_BASIC_AUTH_PASSWORD).evaluateAttributeExpressions().getValue());
--- End diff --
See comment about `password` above.
---
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
Posted by kemixkoo <gi...@git.apache.org>.
Github user kemixkoo commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3020#discussion_r219841379
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PostHTTP.java ---
@@ -211,25 +215,29 @@
.required(false)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.defaultValue(VersionInfo.getUserAgent("Apache-HttpClient", "org.apache.http.client", HttpClientBuilder.class))
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
public static final PropertyDescriptor COMPRESSION_LEVEL = new PropertyDescriptor.Builder()
.name("Compression Level")
.description("Determines the GZIP Compression Level to use when sending the file; the value must be in the range of 0-9. A value of 0 indicates that the file will not be GZIP'ed")
.required(true)
.addValidator(StandardValidators.createLongValidator(0, 9, true))
.defaultValue("0")
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
public static final PropertyDescriptor ATTRIBUTES_AS_HEADERS_REGEX = new PropertyDescriptor.Builder()
.name("Attributes to Send as HTTP Headers (Regex)")
.description("Specifies the Regular Expression that determines the names of FlowFile attributes that should be sent as HTTP Headers")
.addValidator(StandardValidators.REGULAR_EXPRESSION_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
--- End diff --
Oh, should be one mistake.
---
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3020#discussion_r219714056
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PostHTTP.java ---
@@ -726,7 +737,7 @@ public long getContentLength() {
contentType = StringUtils.isBlank(contentTypeValue) ? DEFAULT_CONTENT_TYPE : contentTypeValue;
}
- final String attributeHeaderRegex = context.getProperty(ATTRIBUTES_AS_HEADERS_REGEX).getValue();
+ final String attributeHeaderRegex = context.getProperty(ATTRIBUTES_AS_HEADERS_REGEX).evaluateAttributeExpressions().getValue();
--- End diff --
See comment about `attributeHeaderRegex` above.
---
[GitHub] nifi issue #3020: NIFI-5625: support the variables for the properties of HTT...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/3020
Thanks for your submission. I left some comments, specifically on the `password` fields and a field containing a regex for attribute headers.
`GetHTTP` and `PostHTTP` are legacy processors and have been effectively deprecated. `InvokeHTTP` should be used for all remote HTTP operations moving forward. I'll open a separate Jira to mark them as `@Deprecated`.
Please also provide new or updated unit test cases for these processors.
---
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3020#discussion_r219712927
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java ---
@@ -632,7 +639,7 @@ public void setUpClient(final ProcessContext context) throws IOException, Unreco
}
// check the trusted hostname property and override the HostnameVerifier
- String trustedHostname = trimToEmpty(context.getProperty(PROP_TRUSTED_HOSTNAME).getValue());
+ String trustedHostname = trimToEmpty(context.getProperty(PROP_TRUSTED_HOSTNAME).evaluateAttributeExpressions().getValue());
--- End diff --
See comment about `trustedHostname` above.
---
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3020#discussion_r219712916
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java ---
@@ -367,6 +372,7 @@
+ "on the normal truststore hostname verifier. Only valid with SSL (HTTPS) connections.")
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.required(false)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
--- End diff --
I'm not very comfortable with this field supporting EL either. Do you have a specific use case that requires it?
---
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3020#discussion_r219712821
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/GetHTTP.java ---
@@ -177,19 +180,22 @@
.name("Username")
.description("Username required to access the URL")
.required(false)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
public static final PropertyDescriptor PASSWORD = new PropertyDescriptor.Builder()
.name("Password")
.description("Password required to access the URL")
.required(false)
.sensitive(true)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
--- End diff --
Our policy so far has been that passwords do not support expression language, for a couple reasons:
1. How to evaluate if a password `abc${def}` should be interpreted as `abc` + *the value of(`def`)* or the literal string `abc${def}`
1. The variable registry is not designed to store sensitive values securely, so if a password is stored here, it can be accessed by an unauthorized user
---
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3020#discussion_r219712935
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java ---
@@ -947,11 +954,11 @@ private Request configureRequest(final ProcessContext context, final ProcessSess
Request.Builder requestBuilder = new Request.Builder();
requestBuilder = requestBuilder.url(url);
- final String authUser = trimToEmpty(context.getProperty(PROP_BASIC_AUTH_USERNAME).getValue());
+ final String authUser = trimToEmpty(context.getProperty(PROP_BASIC_AUTH_USERNAME).evaluateAttributeExpressions().getValue());
// If the username/password properties are set then check if digest auth is being used
if (!authUser.isEmpty() && "false".equalsIgnoreCase(context.getProperty(PROP_DIGEST_AUTH).getValue())) {
- final String authPass = trimToEmpty(context.getProperty(PROP_BASIC_AUTH_PASSWORD).getValue());
+ final String authPass = trimToEmpty(context.getProperty(PROP_BASIC_AUTH_PASSWORD).evaluateAttributeExpressions().getValue());
--- End diff --
See comment about `password` above.
---
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3020#discussion_r219712894
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java ---
@@ -318,6 +321,7 @@
.description("The password to be used by the client to authenticate against the Remote URL.")
.required(false)
.sensitive(true)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
--- End diff --
See comment about `password` above.
---
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3020#discussion_r219714033
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PostHTTP.java ---
@@ -183,26 +183,30 @@
.description("How long to wait when attempting to connect to the remote server before giving up")
.required(true)
.defaultValue("30 sec")
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.TIME_PERIOD_VALIDATOR)
.build();
public static final PropertyDescriptor DATA_TIMEOUT = new PropertyDescriptor.Builder()
.name("Data Timeout")
.description("How long to wait between receiving segments of data from the remote server before giving up and discarding the partial file")
.required(true)
.defaultValue("30 sec")
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.TIME_PERIOD_VALIDATOR)
.build();
public static final PropertyDescriptor USERNAME = new PropertyDescriptor.Builder()
.name("Username")
.description("Username required to access the URL")
.required(false)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
public static final PropertyDescriptor PASSWORD = new PropertyDescriptor.Builder()
.name("Password")
.description("Password required to access the URL")
.required(false)
.sensitive(true)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
--- End diff --
See comment about `password` above.
---
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3020#discussion_r219714031
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PostHTTP.java ---
@@ -211,25 +215,29 @@
.required(false)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.defaultValue(VersionInfo.getUserAgent("Apache-HttpClient", "org.apache.http.client", HttpClientBuilder.class))
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
public static final PropertyDescriptor COMPRESSION_LEVEL = new PropertyDescriptor.Builder()
.name("Compression Level")
.description("Determines the GZIP Compression Level to use when sending the file; the value must be in the range of 0-9. A value of 0 indicates that the file will not be GZIP'ed")
.required(true)
.addValidator(StandardValidators.createLongValidator(0, 9, true))
.defaultValue("0")
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
public static final PropertyDescriptor ATTRIBUTES_AS_HEADERS_REGEX = new PropertyDescriptor.Builder()
.name("Attributes to Send as HTTP Headers (Regex)")
.description("Specifies the Regular Expression that determines the names of FlowFile attributes that should be sent as HTTP Headers")
.addValidator(StandardValidators.REGULAR_EXPRESSION_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
--- End diff --
Accepting Expression Language in regex fields is difficult. Can you please share what testing you performed for this field?
---
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3020#discussion_r219714049
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PostHTTP.java ---
@@ -474,8 +484,8 @@ public void process(final HttpResponse response, final HttpContext httpContext)
clientBuilder.setRetryHandler(retryHandler);
clientBuilder.disableContentCompression();
- final String username = context.getProperty(USERNAME).getValue();
- final String password = context.getProperty(PASSWORD).getValue();
+ final String username = context.getProperty(USERNAME).evaluateAttributeExpressions().getValue();
+ final String password = context.getProperty(PASSWORD).evaluateAttributeExpressions().getValue();
--- End diff --
See comment about `password` above.
---
[GitHub] nifi issue #3020: NIFI-5625: support the variables for the properties of HTT...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/3020
Mind rebasing this against the latest master? Please and thanks!
---
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3020#discussion_r219712848
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/GetHTTP.java ---
@@ -438,8 +446,8 @@ public void onTrigger(final ProcessContext context, final ProcessSessionFactory
clientBuilder.setSslcontext(sslContextService.createSSLContext(ClientAuth.REQUIRED));
}
- final String username = context.getProperty(USERNAME).getValue();
- final String password = context.getProperty(PASSWORD).getValue();
+ final String username = context.getProperty(USERNAME).evaluateAttributeExpressions().getValue();
+ final String password = context.getProperty(PASSWORD).evaluateAttributeExpressions().getValue();
--- End diff --
See comment above about `password` supporting EL.
---
[GitHub] nifi pull request #3020: NIFI-5625: support the variables for the properties...
Posted by kemixkoo <gi...@git.apache.org>.
Github user kemixkoo commented on a diff in the pull request:
https://github.com/apache/nifi/pull/3020#discussion_r219832541
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/GetHTTP.java ---
@@ -177,19 +180,22 @@
.name("Username")
.description("Username required to access the URL")
.required(false)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
public static final PropertyDescriptor PASSWORD = new PropertyDescriptor.Builder()
.name("Password")
.description("Password required to access the URL")
.required(false)
.sensitive(true)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
--- End diff --
Yes, I totally agree with the policy for password. but, In my case, I have several processors with the same password, when I change the account from dev to prod, I have to do change for all processors.
Also, I saw the "PROP_PROXY_PASSWORD" property support the EL in InvokeHTTP.
---