grnet / flowspy

GRNET Firewall on Demand platform. Powers
https://fod.grnet.gr
GNU General Public License v3.0
20 stars 8 forks source link

Support port ranges #14

Open safts opened 7 years ago

safts commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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.