Skip to content
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.

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

Support port ranges #14

Open
safts opened this issue Jun 13, 2017 · 2 comments
Open

Support port ranges #14

safts opened this issue Jun 13, 2017 · 2 comments

Comments

@safts
Copy link
Contributor

safts commented Jun 13, 2017

The user should be able to specify a port range instead of single ports when adding rules. Any discussion / implementation details will be mentioned here.

@cejkato2
Copy link

Current implementation in https://github.com/cejkato2/flowspy/tree/removeaddport uses models.CharField() for sourceport, destinationport, port.
Users are allowed to enter list of ports or port ranges.
Example of a valid input is: 80,100-120,443

From my opinion, there is no need to store ports separatly in DB because it makes no sense to select previously used values by user - it is much faster to write down the numbers than to look it up from some list.
Additionally, it was very user unfriendly to use a special form to add a port before it could be used e.g. in sourceport.

@safts
Copy link
Contributor Author

safts commented Jun 19, 2017

I understand your issues. However, there are some concerns about the whole CharField approach:

  1. Validation is a problem. You need to validate a specific format, which includes dashes and commas and so on. This can break quite easily, and you have to validate this on multiple levels (django admin, django shell, API serializers & so on). And then there's the database level where you cannot actually do any validation of such sort (that would be ensure that the string entered contains the required dashes and commas and makes sence).

  2. It is not really that "clean" as an approach, since you can no longer do SQL queries to find routes affecting ports. E.g. if a Route has a port field that contains the string 80, 100-120, 443 and you want to query for all the Routes affecting port 114, you can't do that. You will have to iterate all Route objects and find the ones that satisfy your criterion with python. So we are kinda throwing all the SQL performance out the window (in such queries at least).

These are the major issues I see with this approach. Instead, I would propose that we:

  1. Keep the MatchPort model, but upgrade it to support port ranges. Rename MatchPort.port to MatchPort.port_start and add MatchPort.port_end. These should be both IntegerFields to allow
    SQL queries.
  2. Create a data migration to reform all existing MatchPort objects
  3. Introduce the required serializers & validators.

As far as the user experience issues you mentioned, this is a different aspect I believe. Even with this
approach, we could add a custom form where the user adds ports in a string format (just as you specified) and the back end parses that and creates the required MatchPort objects.

As far as the API is concerned, we could do the same thing. That would be:

  1. Create a custom field for each of the port, sourceport, destinationport
  2. Use to_internal_value to parse the string & create the required MatchPort objects
  3. Use to_representation to read the associated objects from the database & create that string

This will allow the user to do POST requests using a string and when they GET from the API the same string will be sent for those fields.

I am not that sure about the API though, I think maybe we could expose the models as they are there.

Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants