Flow request containing an invalid action should be rejected

When ODL Controller receives a request to install a flow, it is processed
by FlowConfig class in forwardingrulesmanager bundle. There is method
named "validate" which validates the matches and actions specified in
the flow request. If this method encounters an invalid action, it simply
ignores it and moves to the next action. The result of this behavior is that
if flow request contains only one action and it happens to be invalid,
the FlowMod request will be sent to the switch without any action. An
OVS conforming switch will install this flow and set the action to drop.
However, ideally Controller should reject this request (or any request
containing an invalid action).

Few invalid unit test cases were also escaping due to this error, which
have been corrected.

Change-Id: I4df3226d6ec9c663476cae1991c194bc36cb2f99
Signed-off-by: maquresh <maquresh@cisco.com>

diff --git a/opendaylight/forwardingrulesmanager/api/src/main/java/org/opendaylight/controller/forwardingrulesmanager/FlowConfig.java b/opendaylight/forwardingrulesmanager/api/src/main/java/org/opendaylight/controller/forwardingrulesmanager/FlowConfig.java
index c89cfc1b9e8914148bc58fc6f3a375baca026e53..e2b6642501edf8acefb9f47743c5166455c8e8db 100644 (file)
--- a/opendaylight/forwardingrulesmanager/api/src/main/java/org/opendaylight/controller/forwardingrulesmanager/FlowConfig.java
+++ b/opendaylight/forwardingrulesmanager/api/src/main/java/org/opendaylight/controller/forwardingrulesmanager/FlowConfig.java
@@ -867,6 +867,7 @@ public class FlowConfig extends ConfigurationObject implements Serializable {
                     }
                     continue;
                 }
                     }
                     continue;
                 }

+

                 sstr = Pattern.compile(ActionType.SET_NEXT_HOP.toString() + "=(.*)").matcher(actiongrp);
                 if (sstr.matches()) {
                     if (!NetUtils.isIPAddressValid(sstr.group(1))) {
                 sstr = Pattern.compile(ActionType.SET_NEXT_HOP.toString() + "=(.*)").matcher(actiongrp);
                 if (sstr.matches()) {
                     if (!NetUtils.isIPAddressValid(sstr.group(1))) {


@@ -875,7 +876,57 @@ public class FlowConfig extends ConfigurationObject implements Serializable {
                     }
                     continue;
                 }
                     }
                     continue;
                 }

-            }
+
+                sstr = Pattern.compile(ActionType.OUTPUT + "=(.*)").matcher(actiongrp);
+                if (sstr.matches()) {
+                    continue;
+                }
+
+                sstr = Pattern.compile(ActionType.DROP.toString()).matcher(actiongrp);
+                if (sstr.matches()) {
+                    continue;
+                }
+
+                sstr = Pattern.compile(ActionType.LOOPBACK.toString()).matcher(actiongrp);
+                if (sstr.matches()) {
+                    continue;
+                }
+
+                sstr = Pattern.compile(ActionType.FLOOD.toString()).matcher(actiongrp);
+                if (sstr.matches()) {
+                    continue;
+                }
+
+                sstr = Pattern.compile(ActionType.FLOOD_ALL.toString()).matcher(actiongrp);
+                if (sstr.matches()) {
+                    continue;
+                }
+
+                sstr = Pattern.compile(ActionType.SW_PATH.toString()).matcher(actiongrp);
+                if (sstr.matches()) {
+                    continue;
+                }
+
+                sstr = Pattern.compile(ActionType.HW_PATH.toString()).matcher(actiongrp);
+                if (sstr.matches()) {
+                    continue;
+                }
+
+                sstr = Pattern.compile(ActionType.CONTROLLER.toString()).matcher(actiongrp);
+                if (sstr.matches()) {
+                    continue;
+                }
+
+                sstr = Pattern.compile(ActionType.POP_VLAN.toString()).matcher(actiongrp);
+                if (sstr.matches()) {
+                    continue;
+                }
+
+                // If execution reached here, it means there is an Action
+                // which is not recognized by the controller. Return error
+
+                return new Status(StatusCode.BADREQUEST, String.format("%s is an UnSupported Action", actiongrp));
+           }

         } catch (NumberFormatException e) {
             return new Status(StatusCode.BADREQUEST, String.format("Invalid number format %s", e.getMessage()));
         }
         } catch (NumberFormatException e) {
             return new Status(StatusCode.BADREQUEST, String.format("Invalid number format %s", e.getMessage()));
         }

diff --git a/opendaylight/forwardingrulesmanager/api/src/test/java/org/opendaylight/controller/forwardingrulesmanager/frmTest.java b/opendaylight/forwardingrulesmanager/api/src/test/java/org/opendaylight/controller/forwardingrulesmanager/frmTest.java
index 685ccdb7c44ea78931de795bb21547c1bd116b36..3c367b9af45d267816bca893c257b35d8c42c8f8 100644 (file)
--- a/opendaylight/forwardingrulesmanager/api/src/test/java/org/opendaylight/controller/forwardingrulesmanager/frmTest.java
+++ b/opendaylight/forwardingrulesmanager/api/src/test/java/org/opendaylight/controller/forwardingrulesmanager/frmTest.java
@@ -760,9 +760,9 @@ public class frmTest {
         actions.add(ActionType.SET_NW_SRC.toString() + "=1.1.1.1");
         actions.add(ActionType.SET_NW_DST.toString() + "=2.2.2.2");
         actions.add(ActionType.CONTROLLER.toString());
         actions.add(ActionType.SET_NW_SRC.toString() + "=1.1.1.1");
         actions.add(ActionType.SET_NW_DST.toString() + "=2.2.2.2");
         actions.add(ActionType.CONTROLLER.toString());

-        actions.add(ActionType.SET_NW_TOS.toString() + "1");
-        actions.add(ActionType.SET_TP_SRC.toString() + "60");
-        actions.add(ActionType.SET_TP_DST.toString() + "8080");
+        actions.add(ActionType.SET_NW_TOS.toString() + "=1");
+        actions.add(ActionType.SET_TP_SRC.toString() + "=60");
+        actions.add(ActionType.SET_TP_DST.toString() + "=8080");

         actions.add(ActionType.SET_NEXT_HOP.toString() + "=1.1.1.1");
 
         return actions;
         actions.add(ActionType.SET_NEXT_HOP.toString() + "=1.1.1.1");
 
         return actions;