ONOS-2888 Bug fix to avaoid DHCP Server assigning two hosts the same IP
Change-Id: I75dd4bdca43b9b3a0194d9742d84e088eeddac4e
diff --git a/apps/dhcp/src/main/java/org/onosproject/dhcp/DhcpStore.java b/apps/dhcp/src/main/java/org/onosproject/dhcp/DhcpStore.java
index 1637cf7..c9fade9 100644
--- a/apps/dhcp/src/main/java/org/onosproject/dhcp/DhcpStore.java
+++ b/apps/dhcp/src/main/java/org/onosproject/dhcp/DhcpStore.java
@@ -68,11 +68,18 @@
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.
diff --git a/apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DhcpManager.java b/apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DhcpManager.java
index e3a4f43..345d5ad 100644
--- a/apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DhcpManager.java
+++ b/apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DhcpManager.java
@@ -221,7 +221,7 @@
@Override
public Map<HostId, IpAssignment> listMapping() {
- return dhcpStore.listMapping();
+ return dhcpStore.listAssignedMapping();
}
@Override
@@ -658,7 +658,7 @@
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 @@
(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());
}
}
diff --git a/apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DistributedDhcpStore.java b/apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DistributedDhcpStore.java
index 96013b6..dbdadb3 100644
--- a/apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DistributedDhcpStore.java
+++ b/apps/dhcp/src/main/java/org/onosproject/dhcp/impl/DistributedDhcpStore.java
@@ -40,6 +40,7 @@
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 @@
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 @@
}
}
}
- return assignmentInfo.ipAddress();
-
} else if (requestedIP.toInt() != 0) {
// Client has requested an IP.
if (freeIPPool.contains(requestedIP)) {
@@ -166,17 +165,36 @@
IpAssignment assignmentInfo;
if (allocationMap.containsKey(hostId)) {
assignmentInfo = allocationMap.get(hostId).value();
- if ((assignmentInfo.ipAddress().toInt() == ipAddr.toInt()) &&
- (ipAddr.toInt() >= startIPRange.toInt()) && (ipAddr.toInt() <= endIPRange.toInt())) {
+ IpAssignment.AssignmentStatus status = assignmentInfo.assignmentStatus();
- assignmentInfo = IpAssignment.builder()
- .ipAddress(ipAddr)
- .timestamp(new Date())
- .leasePeriod(leaseTime)
- .assignmentStatus(IpAssignment.AssignmentStatus.Option_Assigned)
- .build();
- allocationMap.put(hostId, assignmentInfo);
- return true;
+ 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())
+ .leasePeriod(leaseTime)
+ .assignmentStatus(IpAssignment.AssignmentStatus.Option_Assigned)
+ .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 @@
.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 @@
}
@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 @@
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 @@
@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 @@
}
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;
+ }
}