Skip to content

NIFI-4175 - Add HTTP proxy support to *SFTP processors #2018

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.

Sign up for GitHub

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

Closed
trixpan wants to merge 1 commit into from
Closed

NIFI-4175 - Add HTTP proxy support to *SFTP processors #2018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ...nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/FetchSFTP.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ protected List<PropertyDescriptor> getSupportedPropertyDescriptors() {
properties.add(SFTPTransfer.HOST_KEY_FILE);
properties.add(SFTPTransfer.STRICT_HOST_KEY_CHECKING);
properties.add(SFTPTransfer.USE_COMPRESSION);
properties.add(SFTPTransfer.PROXY_HOST);
properties.add(SFTPTransfer.PROXY_PORT);
properties.add(SFTPTransfer.PROXY_USERNAME);
properties.add(SFTPTransfer.PROXY_PASSWORD);
return properties;
}

Expand Down
4 changes: 4 additions & 0 deletions ...e/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/GetSFTP.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ protected void init(final ProcessorInitializationContext context) {
properties.add(SFTPTransfer.USE_KEEPALIVE_ON_TIMEOUT);
properties.add(SFTPTransfer.USE_COMPRESSION);
properties.add(SFTPTransfer.USE_NATURAL_ORDERING);
properties.add(SFTPTransfer.PROXY_HOST);
properties.add(SFTPTransfer.PROXY_PORT);
properties.add(SFTPTransfer.PROXY_USERNAME);
properties.add(SFTPTransfer.PROXY_PASSWORD);
this.properties = Collections.unmodifiableList(properties);
}

Expand Down
4 changes: 4 additions & 0 deletions .../nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListSFTP.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ protected List<PropertyDescriptor> getSupportedPropertyDescriptors() {
properties.add(SFTPTransfer.CONNECTION_TIMEOUT);
properties.add(SFTPTransfer.DATA_TIMEOUT);
properties.add(SFTPTransfer.USE_KEEPALIVE_ON_TIMEOUT);
properties.add(SFTPTransfer.PROXY_HOST);
properties.add(SFTPTransfer.PROXY_PORT);
properties.add(SFTPTransfer.PROXY_USERNAME);
properties.add(SFTPTransfer.PROXY_PASSWORD);
return properties;
}

Expand Down
4 changes: 4 additions & 0 deletions ...e/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutSFTP.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ protected void init(final ProcessorInitializationContext context) {
properties.add(SFTPTransfer.STRICT_HOST_KEY_CHECKING);
properties.add(SFTPTransfer.USE_KEEPALIVE_ON_TIMEOUT);
properties.add(SFTPTransfer.USE_COMPRESSION);
properties.add(SFTPTransfer.PROXY_HOST);
properties.add(SFTPTransfer.PROXY_PORT);
properties.add(SFTPTransfer.PROXY_USERNAME);
properties.add(SFTPTransfer.PROXY_PASSWORD);
this.properties = Collections.unmodifiableList(properties);
}

Expand Down
46 changes: 46 additions & 0 deletions ...ndard-processors/src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import com.jcraft.jsch.ChannelSftp.LsEntrySelector;
import com.jcraft.jsch.JSch;
import com.jcraft.jsch.JSchException;
import com.jcraft.jsch.ProxyHTTP;
import com.jcraft.jsch.Session;
import com.jcraft.jsch.SftpException;

Expand Down Expand Up @@ -92,6 +93,39 @@ public class SFTPTransfer implements FileTransfer {
.defaultValue("true")
.required(true)
.build();
public static final PropertyDescriptor PROXY_HOST = new PropertyDescriptor.Builder()
Copy link
Contributor

@pvillard31 pvillard31 Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if, since FileTransfer is only used for FTP and SFTP, it would make sens to move the proxy-related properties to FileTransfer instead of duplicating code. I believe, as far as I can see, that we could use the existing properties in FTPTransfer for SFTPTransfer as well with HTTP Proxy and SOCK proxy. Thoughts?

Copy link
Contributor Author

@trixpan trixpan Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pvillard31 I saw that and considered it but ended up adopting the current approach due to my view that I prefer not supporting SOCKS ... Don't get me wrong, I appreciate the reason why SOCKS exists but most processors are only able to support HTTP so I rather provide... consistency (I know I have been sounding repetitive lately... 😄 )

Since FTP already supports SOCKS I reckon we can leave it there, but for SFTP I rather not add it at all.

Let me know if you agree and I will adjust accordingly.

Copy link
Contributor

@pvillard31 pvillard31 Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't have any opinion on this subject but I agree that if we do not add SOCKS option to SFTP processors, then it's OK to have the corresponding properties in SFTPTransfer.

Copy link
Contributor

@jugi92 jugi92 Oct 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
We would really like to see that the development goes into the direction that SOCKS Proxy with authentication will also be available.
In many bigger companies it is a basic requirement and for us it is not possible to provide those parameters through the -D Options as mentioned in this comment: https://community.hortonworks.com/questions/30339/how-to-configure-proxy-server-details-with-user-an.html
As JSCH offers all the possibilities and a reference implementation it should be possible to integrate it and broaden the functionality of Nifi and thus make it more attractive to companies.
Please feel free to discuss my opinion.
Best regards

.name("PROXY_HOST")
.displayName("Proxy Host")
.description("The fully qualified hostname or IP address of the proxy server")
.expressionLanguageSupported(true)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
public static final PropertyDescriptor PROXY_PORT = new PropertyDescriptor.Builder()
.name("PROXY_PORT")
.displayName("Proxy Port")
.description("The port of the proxy server")
.expressionLanguageSupported(true)
.addValidator(StandardValidators.PORT_VALIDATOR)
.build();
public static final PropertyDescriptor PROXY_USERNAME = new PropertyDescriptor.Builder()
.name("PROXY_USERNAME")
.displayName("Proxy Username")
.description("Proxy Username")
.expressionLanguageSupported(true)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.required(false)
.build();
public static final PropertyDescriptor PROXY_PASSWORD = new PropertyDescriptor.Builder()
.name("PROXY_PASSWORD")
.displayName("Proxy Password")
.description("Proxy Password")
.expressionLanguageSupported(true)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.required(false)
.sensitive(true)
.build();



/**
* Dynamic property which is used to decide if the {@link #ensureDirectoryExists(FlowFile, File)} method should perform a {@link ChannelSftp#ls(String)} before calling
Expand Down Expand Up @@ -401,6 +435,18 @@ private ChannelSftp getChannel(final FlowFile flowFile) throws IOException {
ctx.getProperty(HOSTNAME).evaluateAttributeExpressions(flowFile).getValue(),
ctx.getProperty(PORT).evaluateAttributeExpressions(flowFile).asInteger().intValue());

if (ctx.getProperty(PROXY_HOST).evaluateAttributeExpressions(flowFile).isSet()) {
final ProxyHTTP proxy = new ProxyHTTP(
ctx.getProperty(PROXY_HOST).evaluateAttributeExpressions(flowFile).getValue(),
ctx.getProperty(PROXY_PORT).evaluateAttributeExpressions(flowFile).asInteger()
);
// Check if Username is set and populate the proxy accordingly
if (ctx.getProperty(PROXY_USERNAME).evaluateAttributeExpressions(flowFile).isSet()) {
proxy.setUserPasswd(ctx.getProperty(PROXY_USERNAME).evaluateAttributeExpressions(flowFile).getValue(), ctx.getProperty(PROXY_PASSWORD).evaluateAttributeExpressions(flowFile).getValue());
Copy link
Contributor

@pvillard31 pvillard31 Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

[WARNING] src/main/java/org/apache/nifi/processors/standard/util/SFTPTransfer.java[445] (sizes) LineLength: Line is longer than 200 characters (found 206).

Copy link
Contributor Author

@trixpan trixpan Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... missed that one. Will fix.

}
session.setProxy(proxy);
}

final String hostKeyVal = ctx.getProperty(HOST_KEY_FILE).getValue();
if (hostKeyVal != null) {
jsch.setKnownHosts(hostKeyVal);
Expand Down