stratum / fabric-tna

The SD-Fabric data plane
https://docs.sd-fabric.org/
30 stars 15 forks source link

[SDFAB-970] Require IP proto when matching on transport port in classifier flows #471

Closed ccascone closed 2 years ago

ccascone commented 2 years ago

Currently, inserting a classifier flow with criteria UDP_DST=100 causes both TCP and UDP traffic with destination port 100 to be matched. This is because ONOS defines distinct criteria for TCP and UDP protocols, e.g., UDP_DST and TCP_DST, while FabricInterpreter maps both to the same generic L4 match fields (e.g., l4_dport): https://github.com/stratum/fabric-tna/blob/ac45f2615d3e55e98789c6e4da66a33d60cef939/src/main/java/org/stratumproject/fabric/tna/behaviour/FabricInterpreter.java#L103

To avoid undefined/wrong classification, this PR adds a check to require the IP_PROTO criterion to be specified whenever the classifier includes a match on an L4 port.

Moreover, this change allows using the CLI to remove flows previously inserted via the REST API:

karaf@root > classifier-flow-get 1 BEST_EFFORT
[DefaultTrafficSelector{criteria=[UDP_DST:100]}]

karaf@root > classifier-flow-remove -p UDP -dp 100 1 BEST_EFFORT
...
There is no such Flow Classifier Rule DefaultTrafficSelector{criteria=[IP_PROTO:17, UDP_DST:100]} for slice 1 and TC BEST_EFFORT

karaf@root > classifier-flow-remove -dp 100 1 BEST_EFFORT                                                                                                      ...
There is no such Flow Classifier Rule DefaultTrafficSelector{criteria=[]} for slice 1 and TC BEST_EFFORT

This PR also introduces some clean ups and simplifications:

codecov[bot] commented 2 years ago

Codecov Report

Merging #471 (da1eca8) into main (8891e3f) will increase coverage by 0.19%. The diff coverage is 36.36%.

:exclamation: Current head da1eca8 differs from pull request most recent head 8d53001. Consider uploading reports for the commit 8d53001 to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #471      +/-   ##
============================================
+ Coverage     69.16%   69.35%   +0.19%     
- Complexity      696      697       +1     
============================================
  Files            60       63       +3     
  Lines          4634     4634              
  Branches        514      503      -11     
============================================
+ Hits           3205     3214       +9     
+ Misses         1168     1158      -10     
- Partials        261      262       +1     
Impacted Files Coverage Δ
...ratumproject/fabric/tna/behaviour/FabricUtils.java 76.00% <ø> (-1.59%) :arrow_down:
...ct/fabric/tna/slicing/cli/AbstractFlowCommand.java 0.00% <0.00%> (ø)
...project/fabric/tna/slicing/cli/FlowAddCommand.java 0.00% <0.00%> (ø)
...project/fabric/tna/slicing/cli/FlowGetCommand.java 0.00% <ø> (ø)
...ject/fabric/tna/slicing/cli/FlowRemoveCommand.java 0.00% <0.00%> (ø)
...oject/fabric/tna/slicing/cli/SliceIdCompleter.java 0.00% <0.00%> (ø)
.../fabric/tna/slicing/cli/TrafficClassCompleter.java 0.00% <0.00%> (ø)
...oject/fabric/tna/slicing/api/SlicingException.java 38.09% <66.66%> (-0.37%) :arrow_down:
...atumproject/fabric/tna/slicing/SlicingManager.java 84.01% <95.23%> (+0.62%) :arrow_up:
...atumproject/fabric/tna/stats/StatisticDataKey.java 96.55% <0.00%> (-3.45%) :arrow_down:
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8891e3f...8d53001. Read the comment docs.