ONOS-2888 Bug fix to avaoid DHCP Server assigning two hosts the same IP

Change-Id: I75dd4bdca43b9b3a0194d9742d84e088eeddac4e

ONOS-2888 Bug fix to avaoid DHCP Server assigning two hosts the same IP
Change-Id: I75dd4bdca43b9b3a0194d9742d84e088eeddac4e
samanwita pal · Gerrit Code Review
Commit da8a950e83294941597a9f04b17a2ed9c4b61a4c da8a950e 1 parent ed340dd5
Showing 4 changed files with 70 additions and 17 deletions
apps/dhcp/src/main/java/org/onosproject/dhcp/DhcpStore.java
apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DhcpManager.java
apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DistributedDhcpStore.java
apps/dhcp/src/test/java/org/onosproject/dhcp/impl/DhcpManagerTest.java
--- a/apps/dhcp/src/main/java/org/onosproject/dhcp/DhcpStore.java
View file @da8a950
+++ b/apps/dhcp/src/main/java/org/onosproject/dhcp/DhcpStore.java
View file @da8a950
@@ -68,11 +68,18 @@ public interface DhcpStore {
     void releaseIP(HostId hostId);
     /**
+     * Returns a collection of all the MacAddress to IPAddress mapping assigned to the hosts.
+     *
+     * @return the collection of the mappings
+     */
+    Map<HostId, IpAssignment> listAssignedMapping();
+
+    /**
      * Returns a collection of all the MacAddress to IPAddress mapping.
      *
      * @return the collection of the mappings
      */
-    Map<HostId, IpAssignment> listMapping();
+    Map<HostId, IpAssignment> listAllMapping();
     /**
      * Assigns the requested IP to the MAC ID (if available) for an indefinite period of time.
--- a/apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DhcpManager.java
View file @da8a950
+++ b/apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DhcpManager.java
View file @da8a950
@@ -221,7 +221,7 @@ public class DhcpManager implements DhcpService {
     @Override
     public Map<HostId, IpAssignment> listMapping() {
-        return dhcpStore.listMapping();
+        return dhcpStore.listAssignedMapping();
     }
     @Override
@@ -658,7 +658,7 @@ public class DhcpManager implements DhcpService {
             IpAssignment ipAssignment;
             Date dateNow = new Date();
-            Map<HostId, IpAssignment> ipAssignmentMap = dhcpStore.listMapping();
+            Map<HostId, IpAssignment> ipAssignmentMap = dhcpStore.listAllMapping();
             for (Map.Entry<HostId, IpAssignment> entry: ipAssignmentMap.entrySet()) {
                 ipAssignment = entry.getValue();
@@ -667,6 +667,7 @@ public class DhcpManager implements DhcpService {
                         (ipAssignment.leasePeriod() > 0) && (timeLapsed > (ipAssignment.leasePeriodMs()))) {
                     dhcpStore.releaseIP(entry.getKey());
+                    // TODO remove only the IP from the host entry when the API is in place.
                     hostProviderService.hostVanished(entry.getKey());
                 }
             }
--- a/apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DistributedDhcpStore.java
View file @da8a950
+++ b/apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DistributedDhcpStore.java
View file @da8a950
@@ -40,6 +40,7 @@ import org.slf4j.LoggerFactory;
 import java.util.Date;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Objects;
 /**
  * Manages the pool of available IP Addresses in the network and
@@ -107,7 +108,7 @@ public class DistributedDhcpStore implements DhcpStore {
             if (status == IpAssignment.AssignmentStatus.Option_Assigned ||
                     status == IpAssignment.AssignmentStatus.Option_Requested) {
                 // Client has a currently Active Binding.
-                if ((ipAddr.toInt() > startIPRange.toInt()) && (ipAddr.toInt() < endIPRange.toInt())) {
+                if (ipWithinRange(ipAddr)) {
                     return ipAddr;
                 }
@@ -126,8 +127,6 @@ public class DistributedDhcpStore implements DhcpStore {
                     }
                 }
             }
-            return assignmentInfo.ipAddress();
-
         } else if (requestedIP.toInt() != 0) {
             // Client has requested an IP.
             if (freeIPPool.contains(requestedIP)) {
@@ -166,9 +165,13 @@ public class DistributedDhcpStore implements DhcpStore {
         IpAssignment assignmentInfo;
         if (allocationMap.containsKey(hostId)) {
             assignmentInfo = allocationMap.get(hostId).value();
-            if ((assignmentInfo.ipAddress().toInt() == ipAddr.toInt()) &&
+            IpAssignment.AssignmentStatus status = assignmentInfo.assignmentStatus();
-                    (ipAddr.toInt() >= startIPRange.toInt()) && (ipAddr.toInt() <= endIPRange.toInt())) {
+
+            if (Objects.equals(assignmentInfo.ipAddress(), ipAddr) && ipWithinRange(ipAddr)) {
+                if (status == IpAssignment.AssignmentStatus.Option_Assigned ||
+                        status == IpAssignment.AssignmentStatus.Option_Requested) {
+                    // Client has a currently active binding with the server.
                     assignmentInfo = IpAssignment.builder()
                             .ipAddress(ipAddr)
                             .timestamp(new Date())
@@ -177,6 +180,21 @@ public class DistributedDhcpStore implements DhcpStore {
                             .build();
                     allocationMap.put(hostId, assignmentInfo);
                     return true;
+                } else if (status == IpAssignment.AssignmentStatus.Option_Expired) {
+                    // Client has an expired binding with the server.
+                    if (freeIPPool.contains(ipAddr)) {
+                        assignmentInfo = IpAssignment.builder()
+                                .ipAddress(ipAddr)
+                                .timestamp(new Date())
+                                .leasePeriod(leaseTime)
+                                .assignmentStatus(IpAssignment.AssignmentStatus.Option_Assigned)
+                                .build();
+                        if (freeIPPool.remove(ipAddr)) {
+                            allocationMap.put(hostId, assignmentInfo);
+                            return true;
+                        }
+                    }
+                }
             }
         } else if (freeIPPool.contains(ipAddr)) {
             assignmentInfo = IpAssignment.builder()
@@ -201,7 +219,7 @@ public class DistributedDhcpStore implements DhcpStore {
                                                     .build();
             Ip4Address freeIP = newAssignment.ipAddress();
             allocationMap.put(hostId, newAssignment);
-            if ((freeIP.toInt() > startIPRange.toInt()) && (freeIP.toInt() < endIPRange.toInt())) {
+            if (ipWithinRange(freeIP)) {
                 freeIPPool.add(freeIP);
             }
         }
@@ -213,17 +231,26 @@ public class DistributedDhcpStore implements DhcpStore {
     }
     @Override
-    public Map<HostId, IpAssignment> listMapping() {
+    public Map<HostId, IpAssignment> listAssignedMapping() {
-        Map<HostId, IpAssignment> allMapping = new HashMap<>();
+        Map<HostId, IpAssignment> validMapping = new HashMap<>();
+        IpAssignment assignment;
         for (Map.Entry<HostId, Versioned<IpAssignment>> entry: allocationMap.entrySet()) {
-            IpAssignment assignment = entry.getValue().value();
+            assignment = entry.getValue().value();
             if (assignment.assignmentStatus() == IpAssignment.AssignmentStatus.Option_Assigned) {
-                allMapping.put(entry.getKey(), assignment);
+                validMapping.put(entry.getKey(), assignment);
             }
         }
-        return allMapping;
+        return validMapping;
+    }
+    @Override
+    public Map<HostId, IpAssignment> listAllMapping() {
+        Map<HostId, IpAssignment> validMapping = new HashMap<>();
+        for (Map.Entry<HostId, Versioned<IpAssignment>> entry: allocationMap.entrySet()) {
+            validMapping.put(entry.getKey(), entry.getValue().value());
+        }
+        return validMapping;
     }
     @Override
@@ -240,7 +267,7 @@ public class DistributedDhcpStore implements DhcpStore {
             Ip4Address freeIP = assignment.ipAddress();
             if (assignment.leasePeriod() < 0) {
                 allocationMap.remove(host);
-                if ((freeIP.toInt() > startIPRange.toInt()) && (freeIP.toInt() < endIPRange.toInt())) {
+                if (ipWithinRange(freeIP)) {
                     freeIPPool.add(freeIP);
                 }
                 return true;
@@ -257,9 +284,10 @@ public class DistributedDhcpStore implements DhcpStore {
     @Override
     public void populateIPPoolfromRange(Ip4Address startIP, Ip4Address endIP) {
         // Clear all entries from previous range.
+        allocationMap.clear();
+        freeIPPool.clear();
         startIPRange = startIP;
         endIPRange = endIP;
-        freeIPPool.clear();
         int lastIP = endIP.toInt();
         Ip4Address nextIP;
@@ -282,4 +310,17 @@ public class DistributedDhcpStore implements DhcpStore {
         }
         return null;
     }
+
+    /**
+     * Returns true if the given ip is within the range of available IPs.
+     *
+     * @param ip given ip address
+     * @return true if within range, false otherwise
+     */
+    private boolean ipWithinRange(Ip4Address ip) {
+        if ((ip.toInt() >= startIPRange.toInt()) && (ip.toInt() <= endIPRange.toInt())) {
+            return true;
+        }
+        return false;
+    }
 }
--- a/apps/dhcp/src/test/java/org/onosproject/dhcp/impl/DhcpManagerTest.java
View file @da8a950
+++ b/apps/dhcp/src/test/java/org/onosproject/dhcp/impl/DhcpManagerTest.java
View file @da8a950
@@ -237,7 +237,11 @@ public class DhcpManagerTest {
         public void releaseIP(HostId hostId) {
         }
-        public Map<HostId, IpAssignment> listMapping() {
+        public Map<HostId, IpAssignment> listAssignedMapping() {
+            return listAllMapping();
+        }
+
+        public Map<HostId, IpAssignment> listAllMapping() {
             Map<HostId, IpAssignment> map = new HashMap<>();
             IpAssignment assignment = IpAssignment.builder()
                                         .ipAddress(Ip4Address.valueOf(EXPECTED_IP))