From 5b85edb96186583eae08d96e01c4ef4ee31e6222 Mon Sep 17 00:00:00 2001 From: Chiradeep Vittal Date: Thu, 16 Aug 2012 11:28:11 -0700 Subject: [PATCH] bug CS-16034 getRandomIp can return -1 unexpectedly also fixes unit test failures --- .../cloud/network/guru/GuestNetworkGuru.java | 3 +- utils/src/com/cloud/utils/net/NetUtils.java | 36 ++++++++++++------- .../com/cloud/utils/net/NetUtilsTest.java | 4 ++- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/server/src/com/cloud/network/guru/GuestNetworkGuru.java b/server/src/com/cloud/network/guru/GuestNetworkGuru.java index d1dc28671ad..212de2f445f 100755 --- a/server/src/com/cloud/network/guru/GuestNetworkGuru.java +++ b/server/src/com/cloud/network/guru/GuestNetworkGuru.java @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Random; import java.util.Set; +import java.util.SortedSet; import java.util.TreeSet; import javax.ejb.Local; @@ -239,7 +240,7 @@ public abstract class GuestNetworkGuru extends AdapterBase implements NetworkGur public Ip4Address acquireIp4Address(Network network, Ip4Address requestedIp, String reservationId) { List ips = _nicDao.listIpAddressInNetwork(network.getId()); String[] cidr = network.getCidr().split("/"); - Set usedIps = new TreeSet(); + SortedSet usedIps = new TreeSet(); if (requestedIp != null && requestedIp.equals(network.getGateway())) { s_logger.warn("Requested ip address " + requestedIp + " is used as a gateway address in network " + network); diff --git a/utils/src/com/cloud/utils/net/NetUtils.java b/utils/src/com/cloud/utils/net/NetUtils.java index e42902270ed..13413385125 100755 --- a/utils/src/com/cloud/utils/net/NetUtils.java +++ b/utils/src/com/cloud/utils/net/NetUtils.java @@ -32,6 +32,7 @@ import java.util.Formatter; import java.util.List; import java.util.Random; import java.util.Set; +import java.util.SortedSet; import java.util.StringTokenizer; import java.util.TreeSet; import java.util.regex.Matcher; @@ -651,39 +652,48 @@ public class NetUtils { * @param avoid set of ips to avoid * @return ip that is within the cidr range but not in the avoid set. -1 if unable to find one. */ - public static long getRandomIpFromCidr(String startIp, int size, Set avoid) { + public static long getRandomIpFromCidr(String startIp, int size, SortedSet avoid) { return getRandomIpFromCidr(ip2Long(startIp), size, avoid); } /** * Given a cidr, this method returns an ip address within the range but - * is not in the avoid list. + * is not in the avoid list. + * Note: the gateway address has to be specified in the avoid list * - * @param startIp ip that the cidr starts with + * @param cidr ip that the cidr starts with * @param size size of the cidr * @param avoid set of ips to avoid * @return ip that is within the cidr range but not in the avoid set. -1 if unable to find one. */ - public static long getRandomIpFromCidr(long cidr, int size, Set avoid) { + public static long getRandomIpFromCidr(long cidr, int size, SortedSet avoid) { assert (size < 32) : "You do know this is not for ipv6 right? Keep it smaller than 32 but you have " + size; long startNetMask = ip2Long(getCidrNetmask(size)); - long startIp = (cidr & startNetMask) + 2; - int range = 1 << (32 - size); + long startIp = (cidr & startNetMask) + 1; //exclude the first ip since it isnt valid, e.g., 192.168.10.0 + int range = 1 << (32 - size); //e.g., /24 = 2^8 = 256 + range = range -1; //exclude end of the range since that is the broadcast address, e.g., 192.168.10.255 - if (avoid.size() > range) { + if (avoid.size() >= range) { return -1; } - - for (int i = 0; i < range; i++) { - int next = _rand.nextInt(range); - if (!avoid.contains(startIp + next)) { - return startIp + next; + + //Reduce the range by the size of the avoid set + //e.g., cidr = 192.168.10.0, size = /24, avoid = 192.168.10.1, 192.168.10.20, 192.168.10.254 + // range = 2^8 - 1 - 3 = 252 + range = range - avoid.size(); + int next = _rand.nextInt(range); //note: nextInt excludes last value + long ip = startIp + next; + for (Long avoidable : avoid) { + if (ip >= avoidable) { + ip++; + } else { + break; } } - return -1; + return ip; } public static String getIpRangeStartIpFromCidr(String cidr, long size) { diff --git a/utils/test/com/cloud/utils/net/NetUtilsTest.java b/utils/test/com/cloud/utils/net/NetUtilsTest.java index bab14066a42..1eccba30df2 100644 --- a/utils/test/com/cloud/utils/net/NetUtilsTest.java +++ b/utils/test/com/cloud/utils/net/NetUtilsTest.java @@ -17,6 +17,7 @@ package com.cloud.utils.net; import java.util.Set; +import java.util.SortedSet; import java.util.TreeSet; import junit.framework.TestCase; @@ -37,7 +38,7 @@ public class NetUtilsTest extends TestCase { ip = NetUtils.getRandomIpFromCidr(cidr, 8, new TreeSet()); assertEquals("The ip " + NetUtils.long2Ip(ip) + " retrieved must be within the cidr " + cidr + "/8", cidr.substring(0, 4), NetUtils.long2Ip(ip).substring(0, 4)); - Set avoid = new TreeSet(); + SortedSet avoid = new TreeSet(); ip = NetUtils.getRandomIpFromCidr(cidr, 30, avoid); assertTrue("We should be able to retrieve an ip on the first call.", ip != -1); avoid.add(ip); @@ -49,6 +50,7 @@ public class NetUtilsTest extends TestCase { assertTrue("We should be able to retrieve an ip on the third call.", ip != -1); assertTrue("ip returned is not in the avoid list", !avoid.contains(ip)); avoid.add(ip); + ip = NetUtils.getRandomIpFromCidr(cidr, 30, avoid); assertEquals("This should be -1 because we ran out of ip addresses: " + ip, ip, -1); }