Expose passivePortRange via cmd args for the FTP command #12166
Conversation
|
codecov is unhappy because changes under |
|
please review |
There was a problem hiding this comment.
Thanks for the PR.
Why not write a test for the makeService code?
What happen if you don't pass the --passive-port-range ?
I left a few other inline comments.
To merge this, we also need a dedicatd GitHub issue, but that is a minor thing.
The major things blocking this PR:
- missing test for the new code from
makeService - discussion about using
1234:2345vs1234-2345as a port range definition
Do you plan to use the Twisted FTP server for production ?
|
|
||
| def test_passivePortRange(self) -> None: | ||
| """ | ||
| The C{--passive-port-range} option is parsed as expected when provided. |
There was a problem hiding this comment.
What does expected mean?
I guess that we alwasy write test to make sure the code works as exptected
See
https://jml.io/test-docstrings/
maybe
The C{--passive-port-range} option is passed as a colon separated text that is parsed to integers as C{range}.
There was a problem hiding this comment.
Please feel free to go ahead making any adjustment as such as per your preference.
| def opt_passive_port_range(self, port_range_str): | ||
| """ | ||
| Two numbers, colon separated, denoting the range of passive ports (both | ||
| ends inclusive). E.g. "--passive-port-range=30000:31000". |
There was a problem hiding this comment.
Thanks for the PR.
Just asking. Why not use an hyphen as the delimiter ? as in 1234-2345 ?
Which other tool uses a colon for a port range definition?
As far as I know, a hyphen is used by Azure, AWS, Google cloud, ha-proxy.
vsftpd has no range, but insted you define 2 separate options min and max.
proftpd uses a space.
There was a problem hiding this comment.
Which other tool uses a colon for a port range definition?
For one example iptables. Again, as stated in the other comment I have got little preference over either option and please state whether it is the case that you want it changed to a hyphen to pass the review, or that you're merely asking for me to provide further explanations.
| @@ -58,6 +69,8 @@ def makeService(config): | |||
| f.userAnonymous = config["userAnonymous"] | |||
| f.portal = p | |||
| f.protocol = ftp.FTP | |||
| if config.get("passivePortRange"): | |||
There was a problem hiding this comment.
I don't have much experien with the twisted application framework.
What happen if the --passive-port-range is not passed?
Does it returns None? emtpy range ? False ?
I think is best to have a test and an explicit check for the value.
| if config.get("passivePortRange"): | |
| if config.get("passivePortRange") is not None: |
I see that for a missing port, there is a KeyError exception handling and a fallback to a default value.
Just asking. Wny not have something similar here?
There was a problem hiding this comment.
I don't have much experien with the twisted application framework.
twisted.python.usage.Options subclasses typing.Dict. I hope this answers the following lines of questions as well.
Wny not have something similar here?
It's not a not. I would assume that you assumed that it does not fall back to the default behavior upon missing to provide the argument. It does.
|
Hi @adiroiban , thanks for your time drafting the comments.
It's stated in the other comment that there is no existing test case covers such regions, in this and in other similar source code files. Said region of code was initially introduced in 2003 and there has been no test coverage as of today. I acknowledge that it may be your motivation as a reviewer to ask straightly for full test coverage. This means to be a minor patch of exposing a new parameter and it stays in par with the quality of the existing code. It is okay that you would like to create a separate task for test coverage in general for relevant sections of code. It is however a different thing if you would assume that's within the scope of this very patch.
The default behavior applies which is the current behavior without applying this patch.
The 1st belongs to a separate task. I would understand that you or your team may have other opinions. Sorry I do not comply with that. By the way it would have been more reasonable should you create a separate ticket for helping enhance the coverage and ask me to do that. As for the 2nd, I have got no strong preference over a dash or a colon and could have picked either one. Please state if you want it a certain way to be accepted into the code base.
No. Besides, I'm happy to keep responding to the inline comments below yet please understand that not every single question mark gets answered. |
|
please review |
|
Thanks for the feedback. I am leaving this to be reviewed by someone else to break the tied. Twisted is a community based project. The code quality and review rules are define by the current active contributors. If you think that PR is good shape, I am not going to reject the PR. The PR can be reviewed by another contributor and they can decide if they are happy to merge this PR as it is. My understanding is that the current Twisted code quality policy requires to have automated for any change. As part of the coding standard, it is mentioned that development should be test driven https://docs.twisted.org/en/stable/development/coding-standard.html#testing My understanding of TDD, is that any functionality that is touched by a PR, needs to have a test. I know that the code from 2003 does not have tests, but a test can be added in this PR so that the passive port range functionality can be tested in the context of a service. This PR does not have to add tests for other parts of the I was asking about how the code behaves when no port range is passed as I was expecting to see a test for it. Also, what happens if you pass Regarding For a production usage, most of the time the FTP server will be behind a load balancer or port forwarder... so for PASV responses (as many FTP clients don't auto use EPSV), besides the port range, you will also need the public IP . This is why, for production, I assume that you will also need to configure the public IP ... and maybe do it in a single option like Now, can also have So this is not a blocker. |
This makes much sense! My apologies for the confusion as I should have briefly described my use case from the very beginning. Here I came to this change because I used that for starting an adhoc service exposing some media files like large videos from my machine (usually I do with an HTTP server but today the very client supports FTP not HTTP). Thus the need of port range customization arises for firewall configuration (therefore the iptables syntax as well). By default twisted uses port 0 for the OS to decide (as it uses (I think this also answers the other question regarding default value if not passed.) |
Scope and purpose
Introducing a new parameter
--passive-port-rangefor theftpcommand.Sorry there is no existing issue number included because there doesn't seem to be a relevant one.
Contributor Checklist:
please review.Our bot will trigger the review process, by applying the pending review label
and requesting a review from the Twisted dev team.