You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by mgaido91 <gi...@git.apache.org> on 2018/01/23 12:47:27 UTC
[GitHub] nifi pull request #2426: NIFI-4790: support HTTPS Proxy in InvokeHTTP
GitHub user mgaido91 opened a pull request:
https://github.com/apache/nifi/pull/2426
NIFI-4790: support HTTPS Proxy in InvokeHTTP
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:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] 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.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] 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?
- [x] 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/mgaido91/nifi NIFI-4790
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2426.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 #2426
----
commit 51c867ad9685063acd970d594c875fc8abe11836
Author: Marco Gaido <ma...@...>
Date: 2018-01-19T10:02:56Z
NIFI-4790: support HTTPS Proxy in InvokeHTTP
----
---
[GitHub] nifi issue #2426: NIFI-4790: support HTTPS Proxy in InvokeHTTP
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/2426
+1, merging to master, thanks @mgaido91 @ijokarumawak
---
[GitHub] nifi pull request #2426: NIFI-4790: support HTTPS Proxy in InvokeHTTP
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2426#discussion_r163476469
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java ---
@@ -213,11 +219,21 @@
public static final PropertyDescriptor PROP_SSL_CONTEXT_SERVICE = new PropertyDescriptor.Builder()
.name("SSL Context Service")
- .description("The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections.")
+ .description("The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections."
+ + " It is also used to connect to HTTPS Proxy.")
.required(false)
.identifiesControllerService(SSLContextService.class)
.build();
+ public static final PropertyDescriptor PROP_PROXY_TYPE = new PropertyDescriptor.Builder()
+ .name("Proxy Type")
+ .displayName("Proxy Type")
+ .description("The type of the proxy we are connecting to.")
+ .required(false)
--- End diff --
We usually set ``.required(true)`` when you have a list of allowable values with a default value.
---
[GitHub] nifi pull request #2426: NIFI-4790: support HTTPS Proxy in InvokeHTTP
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2426#discussion_r163515579
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java ---
@@ -213,11 +219,21 @@
public static final PropertyDescriptor PROP_SSL_CONTEXT_SERVICE = new PropertyDescriptor.Builder()
.name("SSL Context Service")
- .description("The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections.")
+ .description("The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections."
+ + " It is also used to connect to HTTPS Proxy.")
.required(false)
.identifiesControllerService(SSLContextService.class)
.build();
+ public static final PropertyDescriptor PROP_PROXY_TYPE = new PropertyDescriptor.Builder()
--- End diff --
Probably we should support Expression Language with "Proxy Type" because Proxy host and port do so.
---
[GitHub] nifi pull request #2426: NIFI-4790: support HTTPS Proxy in InvokeHTTP
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2426#discussion_r163476600
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java ---
@@ -513,6 +530,17 @@ public void onPropertyModified(final PropertyDescriptor descriptor, final String
results.add(new ValidationResult.Builder().subject("Proxy").valid(false).explanation("If Proxy username is set, proxy host must be set").build());
}
+ final boolean proxyTypeSet = validationContext.getProperty(PROP_PROXY_TYPE).isSet();
+
+ if(proxyHostSet && !validationContext.getProperty(PROP_PROXY_TYPE).isSet()) {
--- End diff --
I believe this check could be removed based on the previous comment.
---
[GitHub] nifi pull request #2426: NIFI-4790: support HTTPS Proxy in InvokeHTTP
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2426
---
[GitHub] nifi pull request #2426: NIFI-4790: support HTTPS Proxy in InvokeHTTP
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2426#discussion_r163554821
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java ---
@@ -213,11 +219,22 @@
public static final PropertyDescriptor PROP_SSL_CONTEXT_SERVICE = new PropertyDescriptor.Builder()
.name("SSL Context Service")
- .description("The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections.")
+ .description("The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections."
+ + " It is also used to connect to HTTPS Proxy.")
.required(false)
.identifiesControllerService(SSLContextService.class)
.build();
+ public static final PropertyDescriptor PROP_PROXY_TYPE = new PropertyDescriptor.Builder()
+ .name("Proxy Type")
+ .displayName("Proxy Type")
+ .description("The type of the proxy we are connecting to.")
+ .required(true)
+ .allowableValues(HTTP, HTTPS)
+ .defaultValue(HTTP.getValue())
+ .expressionLanguageSupported(true)
--- End diff --
thanks, I'll make it like this
---
[GitHub] nifi issue #2426: NIFI-4790: support HTTPS Proxy in InvokeHTTP
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/nifi/pull/2426
@ijokarumawak thanks, added the EL support.
I am not sure why Appveyor is failing. The failure seems not related to the PR.
---
[GitHub] nifi pull request #2426: NIFI-4790: support HTTPS Proxy in InvokeHTTP
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2426#discussion_r163554236
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java ---
@@ -213,11 +219,22 @@
public static final PropertyDescriptor PROP_SSL_CONTEXT_SERVICE = new PropertyDescriptor.Builder()
.name("SSL Context Service")
- .description("The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections.")
+ .description("The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections."
+ + " It is also used to connect to HTTPS Proxy.")
.required(false)
.identifiesControllerService(SSLContextService.class)
.build();
+ public static final PropertyDescriptor PROP_PROXY_TYPE = new PropertyDescriptor.Builder()
+ .name("Proxy Type")
+ .displayName("Proxy Type")
+ .description("The type of the proxy we are connecting to.")
+ .required(true)
+ .allowableValues(HTTP, HTTPS)
+ .defaultValue(HTTP.getValue())
+ .expressionLanguageSupported(true)
--- End diff --
Unfortunately, I believe it's not possible to use EL on properties with allowable values (see https://issues.apache.org/jira/browse/NIFI-1832). If we want to use EL on this property, I think it needs to be a classic text property with a custom validator a bit like in InvokeHTTP with the request method property (https://github.com/apache/nifi/blob/master/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java#L152).
---
[GitHub] nifi pull request #2426: NIFI-4790: support HTTPS Proxy in InvokeHTTP
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2426#discussion_r163572837
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java ---
@@ -213,11 +216,21 @@
public static final PropertyDescriptor PROP_SSL_CONTEXT_SERVICE = new PropertyDescriptor.Builder()
.name("SSL Context Service")
- .description("The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections.")
+ .description("The SSL Context Service used to provide client certificate information for TLS/SSL (https) connections."
+ + " It is also used to connect to HTTPS Proxy.")
.required(false)
.identifiesControllerService(SSLContextService.class)
.build();
+ public static final PropertyDescriptor PROP_PROXY_TYPE = new PropertyDescriptor.Builder()
+ .name("Proxy Type")
+ .displayName("Proxy Type")
+ .description("The type of the proxy we are connecting to.")
--- End diff --
Probably good to add in the description:
``Must be either " + HTTP + " or " + HTTPS``
---
[GitHub] nifi pull request #2426: NIFI-4790: support HTTPS Proxy in InvokeHTTP
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2426#discussion_r163476703
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java ---
@@ -513,6 +530,17 @@ public void onPropertyModified(final PropertyDescriptor descriptor, final String
results.add(new ValidationResult.Builder().subject("Proxy").valid(false).explanation("If Proxy username is set, proxy host must be set").build());
}
+ final boolean proxyTypeSet = validationContext.getProperty(PROP_PROXY_TYPE).isSet();
--- End diff --
Will always be true based on the previous comment.
---