Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
exceptionfactory
left a comment
exceptionfactory
left a comment
Contributor
There was a problem hiding this comment.
Thanks for proposing this new property @Vinz2168. The basic concept and implementation look good. I noted a few recommendations.
| .description("The range of ports, in the format <start>-<end> (e.g. 50000-50099), that the FTP server uses for data connections in passive mode. " | ||
| + "Restricting the range simplifies firewall and container port mapping configurations. " | ||
| + "If not set, any available port is used for each passive data connection.") | ||
| .required(false) |
Contributor
There was a problem hiding this comment.
Instead of making this property optional, what do you think about having a default of 1025-65534?
|
|
||
| public static final PropertyDescriptor PASSIVE_PORT_RANGE = new PropertyDescriptor.Builder() | ||
| .name("Passive Port Range") | ||
| .description("The range of ports, in the format <start>-<end> (e.g. 50000-50099), that the FTP server uses for data connections in passive mode. " |
Contributor
There was a problem hiding this comment.
A multiline string should be used instead of concatenation for descriptions.
| } else { | ||
| explanation = "Port range must be in the format <start>-<end>, e.g. 50000-50099"; | ||
| } | ||
| return new ValidationResult.Builder().subject(subject).input(input).explanation(explanation).valid(explanation == null).build(); |
Contributor
There was a problem hiding this comment.
For readability, it would be helpful to have each method call on one line.
- Set default Passive Port Range to 1025-65534 and made the property required - Used multiline string for property description - Placed builder method calls on separate lines Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
Thanks for the review @exceptionfactory! I pushed a new commit addressing the recommendations:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NIFI-14719
Adds an optional
Passive Port Rangeproperty toListenFTP(format<start>-<end>, e.g.50000-50099) that restricts the ports the embedded FTP server uses for passive mode data connections. This makes it possible to open a narrow, predictable set of ports on firewalls and to publish them from containers such as Docker. When the property is not set, the current behavior (any available port) is unchanged.Changes:
ListenFTP: newPassive Port Rangeproperty supporting Environment Expression Language, with a validator that checks the<start>-<end>format, the 1-65535 bounds and that start <= endNifiFtpServer: the listener always builds a singleDataConnectionConfiguration, applying the passive ports and, when an SSL Context Service is configured, the implicit TLS settings for data connections, so the two options combine correctlyadditionalDetails.md: documented the new propertyTestListenFTP: validation tests and a test that starts the server and checks that thePASVreply advertises a port within the configured rangeTracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000VerifiedstatusPull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checknifi-standard-processorsverified with-P contrib-check(checkstyle, PMD, RAT) andTestListenFTPpassing.Licensing
LICENSEandNOTICEfilesNo new dependencies.
Documentation
🤖 Generated with Claude Code