You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/05/10 13:43:56 UTC
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #5990: NIFI-9951 Add proxy support to Azure ADLS and Blob v12 processors
exceptionfactory commented on code in PR #5990:
URL: https://github.com/apache/nifi/pull/5990#discussion_r869237170
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java:
##########
@@ -311,4 +316,38 @@ public static void setProxy(final OperationContext operationContext, final Proce
final ProxyConfiguration proxyConfig = ProxyConfiguration.getConfiguration(processContext);
operationContext.setProxy(proxyConfig.createProxy());
}
+
+ public static void configureProxy(final NettyAsyncHttpClientBuilder nettyClientBuilder, final PropertyContext propertyContext) {
+ final ProxyConfiguration proxyConfiguration = ProxyConfiguration.getConfiguration(propertyContext);
+
+ if (proxyConfiguration != ProxyConfiguration.DIRECT_CONFIGURATION) {
+
+ final ProxyOptions proxyOptions = new ProxyOptions(
+ getProxyType(proxyConfiguration),
+ new InetSocketAddress(proxyConfiguration.getProxyServerHost(), proxyConfiguration.getProxyServerPort()));
+
+ final String proxyUserName = proxyConfiguration.getProxyUserName();
+ final String proxyUserPassword = proxyConfiguration.getProxyUserPassword();
+ if (proxyUserName != null && proxyUserPassword != null) {
+ proxyOptions.setCredentials(proxyUserName, proxyUserPassword);
+ }
+
+ nettyClientBuilder.proxy(proxyOptions);
+ }
+ }
+
+ private static ProxyOptions.Type getProxyType(ProxyConfiguration proxyConfiguration) {
+ if (proxyConfiguration.getProxyType() == Proxy.Type.HTTP) {
+ return ProxyOptions.Type.HTTP;
+ } else if (proxyConfiguration.getProxyType() == Proxy.Type.SOCKS) {
+ final SocksVersion socksVersion = proxyConfiguration.getSocksVersion();
+ if (socksVersion != SocksVersion.NOT_SPECIFIED) {
+ return ProxyOptions.Type.valueOf(socksVersion.name());
+ } else {
+ throw new IllegalArgumentException("Version should be set when SOCKS proxy type is used!");
Review Comment:
Use of exclamations in error messages should be avoided. Recommend adjusting the wording for greater clarity:
```suggestion
throw new IllegalArgumentException("SOCKS Version is required for SOCKS Proxy Type");
```
##########
nifi-nar-bundles/nifi-standard-services/nifi-proxy-configuration-api/src/main/java/org/apache/nifi/proxy/SocksVersion.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.proxy;
+
+public enum SocksVersion {
+ NOT_SPECIFIED,
Review Comment:
Recommend removing `NOT_SPECIFIED` as a value on this enum and defaulting to `SOCKS5` since it should be more commonly used.
##########
nifi-nar-bundles/nifi-standard-services/nifi-proxy-configuration-bundle/nifi-proxy-configuration/src/main/java/org/apache/nifi/proxy/StandardProxyConfigurationService.java:
##########
@@ -46,31 +46,41 @@ public class StandardProxyConfigurationService extends AbstractControllerService
.required(true)
.build();
- static final PropertyDescriptor PROXY_SERVER_HOST = new PropertyDescriptor.Builder()
+ public static final PropertyDescriptor SOCKS_VERSION = new PropertyDescriptor.Builder()
+ .name("socks-version")
+ .displayName("Socks version")
+ .description("Socks version.")
Review Comment:
Recommend expanding the description:
```suggestion
.description("SOCKS Protocol Version")
```
##########
nifi-nar-bundles/nifi-standard-services/nifi-proxy-configuration-bundle/nifi-proxy-configuration/src/main/java/org/apache/nifi/proxy/StandardProxyConfigurationService.java:
##########
@@ -46,31 +46,41 @@ public class StandardProxyConfigurationService extends AbstractControllerService
.required(true)
.build();
- static final PropertyDescriptor PROXY_SERVER_HOST = new PropertyDescriptor.Builder()
+ public static final PropertyDescriptor SOCKS_VERSION = new PropertyDescriptor.Builder()
+ .name("socks-version")
+ .displayName("Socks version")
+ .description("Socks version.")
+ .allowableValues(SocksVersion.values())
+ .defaultValue(SocksVersion.NOT_SPECIFIED.name())
Review Comment:
Recommend changing the default value to `SOCKS5`.
##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java:
##########
@@ -311,4 +316,38 @@ public static void setProxy(final OperationContext operationContext, final Proce
final ProxyConfiguration proxyConfig = ProxyConfiguration.getConfiguration(processContext);
operationContext.setProxy(proxyConfig.createProxy());
}
+
+ public static void configureProxy(final NettyAsyncHttpClientBuilder nettyClientBuilder, final PropertyContext propertyContext) {
+ final ProxyConfiguration proxyConfiguration = ProxyConfiguration.getConfiguration(propertyContext);
+
+ if (proxyConfiguration != ProxyConfiguration.DIRECT_CONFIGURATION) {
+
+ final ProxyOptions proxyOptions = new ProxyOptions(
+ getProxyType(proxyConfiguration),
+ new InetSocketAddress(proxyConfiguration.getProxyServerHost(), proxyConfiguration.getProxyServerPort()));
+
+ final String proxyUserName = proxyConfiguration.getProxyUserName();
+ final String proxyUserPassword = proxyConfiguration.getProxyUserPassword();
+ if (proxyUserName != null && proxyUserPassword != null) {
+ proxyOptions.setCredentials(proxyUserName, proxyUserPassword);
+ }
+
+ nettyClientBuilder.proxy(proxyOptions);
+ }
+ }
+
+ private static ProxyOptions.Type getProxyType(ProxyConfiguration proxyConfiguration) {
+ if (proxyConfiguration.getProxyType() == Proxy.Type.HTTP) {
+ return ProxyOptions.Type.HTTP;
+ } else if (proxyConfiguration.getProxyType() == Proxy.Type.SOCKS) {
+ final SocksVersion socksVersion = proxyConfiguration.getSocksVersion();
+ if (socksVersion != SocksVersion.NOT_SPECIFIED) {
+ return ProxyOptions.Type.valueOf(socksVersion.name());
+ } else {
+ throw new IllegalArgumentException("Version should be set when SOCKS proxy type is used!");
+ }
+ } else {
+ throw new IllegalArgumentException("Unsupported proxy type: " + proxyConfiguration.getProxyType() + " please use HTTP or SOCKS type.");
Review Comment:
Recommend simplifying the wording, as this is an unlikely exception.
```suggestion
throw new IllegalArgumentException("Unsupported proxy type: " + proxyConfiguration.getProxyType());
```
##########
nifi-nar-bundles/nifi-standard-services/nifi-proxy-configuration-bundle/nifi-proxy-configuration/src/main/java/org/apache/nifi/proxy/StandardProxyConfigurationService.java:
##########
@@ -46,31 +46,41 @@ public class StandardProxyConfigurationService extends AbstractControllerService
.required(true)
.build();
- static final PropertyDescriptor PROXY_SERVER_HOST = new PropertyDescriptor.Builder()
+ public static final PropertyDescriptor SOCKS_VERSION = new PropertyDescriptor.Builder()
+ .name("socks-version")
+ .displayName("Socks version")
Review Comment:
`SOCKS` should be uppercase:
```suggestion
.displayName("SOCKS Version")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org