Fix bug in Static Route configuration path

ISSUE: Multiple equivalent static route configurations with different names not detected
CHANGE: In addStaticRoute() perform proper equality check on existing StaticRoute objects
        (using available StaticRoute compareTo() method) instead of checking on StaticRouteConfig
        objects which can never be positive as same name config is detected at the beginning of method

Change-Id: I51d2e1a641f612f5e79eb11063a868739a275209
Signed-off-by: Alessandro Boch <aboch@cisco.com>

diff --git a/opendaylight/forwarding/staticrouting/src/main/java/org/opendaylight/controller/forwarding/staticrouting/internal/StaticRoutingImplementation.java b/opendaylight/forwarding/staticrouting/src/main/java/org/opendaylight/controller/forwarding/staticrouting/internal/StaticRoutingImplementation.java
index ec6da387691f617cefb810c5624e67ddab613770..eac854c10674f796fae64ec174aaef07aad4aed5 100644 (file)
--- a/opendaylight/forwarding/staticrouting/src/main/java/org/opendaylight/controller/forwarding/staticrouting/internal/StaticRoutingImplementation.java
+++ b/opendaylight/forwarding/staticrouting/src/main/java/org/opendaylight/controller/forwarding/staticrouting/internal/StaticRoutingImplementation.java
@@ -97,6 +97,7 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
         }
     }
 
         }
     }
 

+    @Override

     public ConcurrentMap<String, StaticRouteConfig> getStaticRouteConfigs() {
         return staticRouteConfigs;
     }
     public ConcurrentMap<String, StaticRouteConfig> getStaticRouteConfigs() {
         return staticRouteConfigs;
     }


@@ -258,11 +259,13 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
     }
 
     private void notifyHostUpdate(HostNodeConnector host, boolean added) {
     }
 
     private void notifyHostUpdate(HostNodeConnector host, boolean added) {

-        if (host == null)
+        if (host == null) {

             return;
             return;

+        }

         for (StaticRoute s : staticRoutes.values()) {
         for (StaticRoute s : staticRoutes.values()) {

-            if (s.getType() == StaticRoute.NextHopType.SWITCHPORT)
+            if (s.getType() == StaticRoute.NextHopType.SWITCHPORT) {

                 continue;
                 continue;

+            }

             if (s.getNextHopAddress().equals(host.getNetworkAddress())) {
                 if (added) {
                     s.setHost(host);
             if (s.getNextHopAddress().equals(host.getNetworkAddress())) {
                 if (added) {
                     s.setHost(host);


@@ -285,8 +288,9 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
     }
 
     public boolean isIPv4AddressValid(String cidr) {
     }
 
     public boolean isIPv4AddressValid(String cidr) {

-        if (cidr == null)
+        if (cidr == null) {

             return false;
             return false;

+        }

 
         String values[] = cidr.split("/");
         Pattern ipv4Pattern = Pattern
 
         String values[] = cidr.split("/");
         Pattern ipv4Pattern = Pattern


@@ -322,6 +326,7 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
         return 0;
     }
 
         return 0;
     }
 

+    @Override

     public StaticRoute getBestMatchStaticRoute(InetAddress ipAddress) {
         ByteBuffer bblongestPrefix = null;
         try {
     public StaticRoute getBestMatchStaticRoute(InetAddress ipAddress) {
         ByteBuffer bblongestPrefix = null;
         try {


@@ -349,10 +354,9 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
         return longestPrefixRoute;
     }
 
         return longestPrefixRoute;
     }
 

+    @Override

     public Status addStaticRoute(StaticRouteConfig config) {
     public Status addStaticRoute(StaticRouteConfig config) {

-        Status status;
-
-        status = config.isValid();
+        Status status = config.isValid();

         if (!status.isSuccess()) {
             return status;
         }
         if (!status.isSuccess()) {
             return status;
         }


@@ -361,22 +365,29 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
                                 "A valid Static Route configuration with this name " +
                                                 "already exists. Please use a different name");
         }
                                 "A valid Static Route configuration with this name " +
                                                 "already exists. Please use a different name");
         }

-        for (StaticRouteConfig s : staticRouteConfigs.values()) {
-            if (s.equals(config)) {
+
+        // Update database
+        StaticRoute sRoute = new StaticRoute(config);
+
+        for (Map.Entry<String, StaticRoute> entry : staticRoutes.entrySet()) {
+            if (entry.getValue().compareTo(sRoute) == 0) {

                 return new Status(StatusCode.CONFLICT,
                 return new Status(StatusCode.CONFLICT,

-                                "This conflicts with an existing Static Route " +
-                                        "Configuration. Please check the configuration " +
-                                                "and try again");
+                        "This conflicts with an existing Static Route " +
+                                "Configuration. Please check the configuration " +
+                                        "and try again");

             }
         }
             }
         }

+        staticRoutes.put(config.getName(), sRoute);

 
 

+        // Update config databse

         staticRouteConfigs.put(config.getName(), config);
         staticRouteConfigs.put(config.getName(), config);

-        StaticRoute sRoute = new StaticRoute(config);
-        staticRoutes.put(config.getName(), sRoute);
+
+        // Notify

         checkAndUpdateListeners(sRoute, true);
         return status;
     }
 
         checkAndUpdateListeners(sRoute, true);
         return status;
     }
 

+    @Override

     public Status removeStaticRoute(String name) {
         staticRouteConfigs.remove(name);
         StaticRoute sRoute = staticRoutes.remove(name);
     public Status removeStaticRoute(String name) {
         staticRouteConfigs.remove(name);
         StaticRoute sRoute = staticRoutes.remove(name);


@@ -424,8 +435,9 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
         allocateCaches();
         retrieveCaches();
         this.executor = Executors.newFixedThreadPool(1);
         allocateCaches();
         retrieveCaches();
         this.executor = Executors.newFixedThreadPool(1);

-        if (staticRouteConfigs.isEmpty())
+        if (staticRouteConfigs.isEmpty()) {

             loadConfiguration();
             loadConfiguration();

+        }

 
         /*
          *  Slow probe to identify any gateway that might have silently appeared
 
         /*
          *  Slow probe to identify any gateway that might have silently appeared

diff --git a/opendaylight/forwarding/staticrouting/src/test/java/org/opendaylight/controller/forwarding/staticrouting/StaticRouteTest.java b/opendaylight/forwarding/staticrouting/src/test/java/org/opendaylight/controller/forwarding/staticrouting/StaticRouteTest.java
index 4a497a058c0df7a1bdc5b16c5a0cac656010c8f3..25b2dacde10656cba2475775136265d85817fc18 100644 (file)
--- a/opendaylight/forwarding/staticrouting/src/test/java/org/opendaylight/controller/forwarding/staticrouting/StaticRouteTest.java
+++ b/opendaylight/forwarding/staticrouting/src/test/java/org/opendaylight/controller/forwarding/staticrouting/StaticRouteTest.java
@@ -83,9 +83,9 @@ public class StaticRouteTest {
         Assert.assertFalse(staticRoute1.equals(staticRoute3));
         Assert.assertFalse(staticRoute1.equals(staticRoute4));
 
         Assert.assertFalse(staticRoute1.equals(staticRoute3));
         Assert.assertFalse(staticRoute1.equals(staticRoute4));
 

-        Assert.assertTrue(staticRoute1.compareTo(staticRoute2) == 0 ? true : false);
-        Assert.assertFalse(staticRoute1.compareTo(staticRoute3) == 0 ? true : false);
-        Assert.assertTrue(staticRoute1.compareTo(staticRoute4) == 0 ? true : false);
+        Assert.assertTrue(staticRoute1.compareTo(staticRoute2) == 0);
+        Assert.assertFalse(staticRoute1.compareTo(staticRoute3) == 0);
+        Assert.assertTrue(staticRoute1.compareTo(staticRoute4) == 0);

 
         }
 
 
         }