Issue in flow programming:

	Add a flow, uninstall it, try to remove it. Remove seems to fail from gui behavior.
	Try to install the flow. It will fail and uncaught exception in forwardingrulesmanager is printed.
Fix:
	Handle invalid flow status toggle request in forwardingrulesmanager.
	IForwardingrulesmanager to exposed a per node and per flow name api for toggling the flow status.
	Flow.web to evaluate api return status by status code and not by status description,
	because a Status with StatusCode.SUCCESS does not imply the description is "Success".

Change-Id: I33567b473828721bd6548ea222f4b7bcf83c0306
Signed-off-by: aboch <aboch@cisco.com>

diff --git a/opendaylight/forwardingrulesmanager/src/main/java/org/opendaylight/controller/forwardingrulesmanager/IForwardingRulesManager.java b/opendaylight/forwardingrulesmanager/src/main/java/org/opendaylight/controller/forwardingrulesmanager/IForwardingRulesManager.java
index 3a1250becfbdc1ac6d23aea3228d997fc6ead7ac..60d74295ee4ff0542f87cecd225d46d4d61d8bdd 100644 (file)
--- a/opendaylight/forwardingrulesmanager/src/main/java/org/opendaylight/controller/forwardingrulesmanager/IForwardingRulesManager.java
+++ b/opendaylight/forwardingrulesmanager/src/main/java/org/opendaylight/controller/forwardingrulesmanager/IForwardingRulesManager.java
@@ -255,6 +255,17 @@ public interface IForwardingRulesManager {
      * @return the {@code Status} object indicating the result of this action
      */
     public Status toggleStaticFlowStatus(FlowConfig configObject);
      * @return the {@code Status} object indicating the result of this action
      */
     public Status toggleStaticFlowStatus(FlowConfig configObject);

+    
+    /**
+     * Toggle the installation status of the specified configured flow
+     * If the flow configuration status is active, this call will
+     * change the flow status to inactive and vice-versa
+     *
+     * @param name for the static flow
+     * @param node on which the flow is attached
+     * @return the {@code Status} object indicating the result of this action
+     */
+    public Status toggleStaticFlowStatus(String name, Node node);

 
     public Map<String, PortGroupConfig> getPortGroupConfigs();
 
 
     public Map<String, PortGroupConfig> getPortGroupConfigs();
 

diff --git a/opendaylight/forwardingrulesmanager/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/ForwardingRulesManagerImpl.java b/opendaylight/forwardingrulesmanager/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/ForwardingRulesManagerImpl.java
index 08d34127a5a58107b731ad6c6cc666853d30c478..bdaae5b28ce40245a43e040b1d46e13963c11277 100644 (file)
--- a/opendaylight/forwardingrulesmanager/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/ForwardingRulesManagerImpl.java
+++ b/opendaylight/forwardingrulesmanager/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/ForwardingRulesManagerImpl.java
@@ -1451,8 +1451,20 @@ public class ForwardingRulesManagerImpl implements IForwardingRulesManager,
         return status;
     }
 
         return status;
     }
 

+
+
+       @Override
+       public Status toggleStaticFlowStatus(String name, Node node) {
+               return toggleStaticFlowStatus(getStaticFlow(name, node));
+       }
+       

     @Override
     public Status toggleStaticFlowStatus(FlowConfig config) {
     @Override
     public Status toggleStaticFlowStatus(FlowConfig config) {

+       if (config == null) {
+               String msg = "Invalid request: null flow config";
+               log.warn(msg);
+               return new Status(StatusCode.BADREQUEST, msg);
+       }

         // Validity check for api3 entry point
         if (config.isInternalFlow()) {
                String msg = "Invalid operation: Controller generated flow " +
         // Validity check for api3 entry point
         if (config.isInternalFlow()) {
                String msg = "Invalid operation: Controller generated flow " +

diff --git a/opendaylight/northbound/flowprogrammer/src/main/java/org/opendaylight/controller/flowprogrammer/northbound/FlowProgrammerNorthbound.java b/opendaylight/northbound/flowprogrammer/src/main/java/org/opendaylight/controller/flowprogrammer/northbound/FlowProgrammerNorthbound.java
index e4ca83a260903d9fd808cae8901daaa2239820a7..ed3abde0ab22bc84049f3f8787436d5a775a8fda 100644 (file)
--- a/opendaylight/northbound/flowprogrammer/src/main/java/org/opendaylight/controller/flowprogrammer/northbound/FlowProgrammerNorthbound.java
+++ b/opendaylight/northbound/flowprogrammer/src/main/java/org/opendaylight/controller/flowprogrammer/northbound/FlowProgrammerNorthbound.java
@@ -366,9 +366,7 @@ public class FlowProgrammerNorthbound {
                     + RestMessages.NOFLOW.toString());
         }
 
                     + RestMessages.NOFLOW.toString());
         }
 

-        Status status = frm.toggleStaticFlowStatus(new FlowConfig("", name,
-                node, "", "", "", "", "", "", "", "", "", "", "", "", "", "",
-                "", "", "", null));
+        Status status = frm.toggleStaticFlowStatus(staticFlow);

         if (status.isSuccess()) {
             return Response.ok().build();
         }
         if (status.isSuccess()) {
             return Response.ok().build();
         }

diff --git a/opendaylight/web/flows/src/main/java/org/opendaylight/controller/flows/web/Flows.java b/opendaylight/web/flows/src/main/java/org/opendaylight/controller/flows/web/Flows.java
index 297c99c0b282e02a726f0d3a532b3e70735f116a..77470224fb5a5ba51d01c35d2e55d8e6907131d0 100644 (file)
--- a/opendaylight/web/flows/src/main/java/org/opendaylight/controller/flows/web/Flows.java
+++ b/opendaylight/web/flows/src/main/java/org/opendaylight/controller/flows/web/Flows.java
@@ -43,9 +43,9 @@ import com.google.gson.Gson;
 @RequestMapping("/")
 public class Flows implements IOneWeb {
        private static final UserLevel AUTH_LEVEL = UserLevel.CONTAINERUSER;
 @RequestMapping("/")
 public class Flows implements IOneWeb {
        private static final UserLevel AUTH_LEVEL = UserLevel.CONTAINERUSER;

-    private final String WEB_NAME = "Flows";
-    private final String WEB_ID = "flows";
-    private final short WEB_ORDER = 2;
+    private static final String WEB_NAME = "Flows";
+    private static final String WEB_ID = "flows";
+    private static final short WEB_ORDER = 2;

 
     public Flows() {
         ServiceHelper.registerGlobalService(IOneWeb.class, this, null);
 
     public Flows() {
         ServiceHelper.registerGlobalService(IOneWeb.class, this, null);


@@ -77,14 +77,12 @@ public class Flows implements IOneWeb {
         // fetch frm
         IForwardingRulesManager frm = (IForwardingRulesManager) ServiceHelper
                 .getInstance(IForwardingRulesManager.class, "default", this);
         // fetch frm
         IForwardingRulesManager frm = (IForwardingRulesManager) ServiceHelper
                 .getInstance(IForwardingRulesManager.class, "default", this);

-        if (frm == null)
-            return null;
+        if (frm == null) { return null; }

 
         // fetch sm
         ISwitchManager switchManager = (ISwitchManager) ServiceHelper
                 .getInstance(ISwitchManager.class, "default", this);
 
         // fetch sm
         ISwitchManager switchManager = (ISwitchManager) ServiceHelper
                 .getInstance(ISwitchManager.class, "default", this);

-        if (switchManager == null)
-            return null;
+        if (switchManager == null) { return null; }

         
         // get static flow list
         List<FlowConfig> staticFlowList = frm.getStaticFlows();
         
         // get static flow list
         List<FlowConfig> staticFlowList = frm.getStaticFlows();


@@ -111,8 +109,7 @@ public class Flows implements IOneWeb {
     public Map<String, Object> getNodePorts() {
         ISwitchManager switchManager = (ISwitchManager) ServiceHelper
                 .getInstance(ISwitchManager.class, "default", this);
     public Map<String, Object> getNodePorts() {
         ISwitchManager switchManager = (ISwitchManager) ServiceHelper
                 .getInstance(ISwitchManager.class, "default", this);

-        if (switchManager == null)
-            return null;
+        if (switchManager == null) { return null; }

 
         Map<String, Object> nodes = new HashMap<String, Object>();
         Map<Short, String> port;
 
         Map<String, Object> nodes = new HashMap<String, Object>();
         Map<Short, String> port;


@@ -121,7 +118,7 @@ public class Flows implements IOneWeb {
             port = new HashMap<Short, String>(); // new port
             Set<NodeConnector> nodeConnectorSet = node.getNodeConnectors();
 
             port = new HashMap<Short, String>(); // new port
             Set<NodeConnector> nodeConnectorSet = node.getNodeConnectors();
 

-            if (nodeConnectorSet != null)
+            if (nodeConnectorSet != null) {

                 for (NodeConnector nodeConnector : nodeConnectorSet) {
                     String nodeConnectorName = ((Name) switchManager
                             .getNodeConnectorProp(nodeConnector,
                 for (NodeConnector nodeConnector : nodeConnectorSet) {
                     String nodeConnectorName = ((Name) switchManager
                             .getNodeConnectorProp(nodeConnector,


@@ -130,6 +127,7 @@ public class Flows implements IOneWeb {
                              nodeConnectorName + "("
                              + nodeConnector.getNodeConnectorIDString() + ")");
                 }
                              nodeConnectorName + "("
                              + nodeConnector.getNodeConnectorIDString() + ")");
                 }

+            }

             
             // add ports
             Map<String, Object> entry = new HashMap<String, Object>();
             
             // add ports
             Map<String, Object> entry = new HashMap<String, Object>();


@@ -183,7 +181,7 @@ public class Flows implements IOneWeb {
     @ResponseBody
     public String actionFlow(@RequestParam(required = true) String action,
             @RequestParam(required = false) String body, @RequestParam(required = true) String nodeId) {
     @ResponseBody
     public String actionFlow(@RequestParam(required = true) String action,
             @RequestParam(required = false) String body, @RequestParam(required = true) String nodeId) {

-       if (!authorize(UserLevel.NETWORKADMIN)) {
+       if (!isUserAuthorized(UserLevel.NETWORKADMIN)) {

                return "Operation not authorized";
        }
        
                return "Operation not authorized";
        }
        


@@ -195,19 +193,19 @@ public class Flows implements IOneWeb {
         FlowConfig flow = gson.fromJson(body, FlowConfig.class);
         Node node = Node.fromString(nodeId);
         flow.setNode(node);
         FlowConfig flow = gson.fromJson(body, FlowConfig.class);
         Node node = Node.fromString(nodeId);
         flow.setNode(node);

-        Status result = null;
+        Status result = new Status(StatusCode.BADREQUEST, "Invalid request");

         if (action.equals("add")) {
             result = frm.addStaticFlow(flow, false);
         }
 
         if (action.equals("add")) {
             result = frm.addStaticFlow(flow, false);
         }
 

-        return result.getDescription();
+        return (result.isSuccess())? StatusCode.SUCCESS.toString(): result.getDescription();

     }
     
     @RequestMapping(value = "/flow/{nodeId}/{name}", method = RequestMethod.POST)
     @ResponseBody
     public String removeFlow(@PathVariable("nodeId") String nodeId, @PathVariable("name") String name,
                @RequestParam(required = true) String action) {
     }
     
     @RequestMapping(value = "/flow/{nodeId}/{name}", method = RequestMethod.POST)
     @ResponseBody
     public String removeFlow(@PathVariable("nodeId") String nodeId, @PathVariable("name") String name,
                @RequestParam(required = true) String action) {

-       if (!authorize(UserLevel.NETWORKADMIN)) { return "Operation not authorized"; }
+       if (!isUserAuthorized(UserLevel.NETWORKADMIN)) { return "Operation not authorized"; }

        
        IForwardingRulesManager frm = (IForwardingRulesManager) ServiceHelper
                 .getInstance(IForwardingRulesManager.class, "default", this);
        
        IForwardingRulesManager frm = (IForwardingRulesManager) ServiceHelper
                 .getInstance(IForwardingRulesManager.class, "default", this);


@@ -215,39 +213,32 @@ public class Flows implements IOneWeb {
         
         Status result = null;
         Node node = Node.fromString(nodeId);
         
         Status result = null;
         Node node = Node.fromString(nodeId);

-        if (node == null) {
-            return null;
-        }
+        if (node == null) { return null; }

         if (action.equals("remove")) {
                result = frm.removeStaticFlow(name, node);
         } else if (action.equals("toggle")) {
         if (action.equals("remove")) {
                result = frm.removeStaticFlow(name, node);
         } else if (action.equals("toggle")) {

-               FlowConfig config = frm.getStaticFlow(name, node);
-               result = frm.toggleStaticFlowStatus(config);
+               result = frm.toggleStaticFlowStatus(name, node);

         } else {
                result = new Status(StatusCode.BADREQUEST, "Unknown action");
         }
         
         } else {
                result = new Status(StatusCode.BADREQUEST, "Unknown action");
         }
         

-        return result.getDescription();
+        return (result.isSuccess())? StatusCode.SUCCESS.toString(): result.getDescription();

     }
     
     /**
     }
     
     /**

-     * Is the operation permitted for the given level
+     * Returns whether the current user's level is same or above
+     * the required authorization level. 

      * 
      * 

-     * @param level
+     * @param requiredLevel the authorization level required

      */
      */

-    private boolean authorize(UserLevel level) {
+    private boolean isUserAuthorized(UserLevel requiredLevel) {

        IUserManager userManager = (IUserManager) ServiceHelper
                 .getGlobalInstance(IUserManager.class, this);
        IUserManager userManager = (IUserManager) ServiceHelper
                 .getGlobalInstance(IUserManager.class, this);

-        if (userManager == null) {
-               return false;
-        }
+        if (userManager == null) { return false; }

         
         String username = SecurityContextHolder.getContext().getAuthentication().getName();
         UserLevel userLevel = userManager.getUserLevel(username);
         
         String username = SecurityContextHolder.getContext().getAuthentication().getName();
         UserLevel userLevel = userManager.getUserLevel(username);

-        if (userLevel.toNumber() <= level.toNumber()) {
-               return true;
-        }
-        return false;
+        return (userLevel.ordinal() <= requiredLevel.ordinal());

     }
 
 }
     }
 
 }