Thanks Jayapal for reviewing. I have updated the patch. Now isNetworkAWithinNetworkB method uses IP ranges of cidrs for comparison. Also updated the unittests for the same.
Thanks, Saksham -----Original Message----- From: Jayapal Reddy Uradi [mailto:jayapalreddy.ur...@citrix.com] Sent: Monday, December 23, 2013 6:03 PM To: <dev@cloudstack.apache.org> Subject: Re: Faulty method isNetworkAWithinNetworkB ? Hi Saksham, Always the higher suffix cidr will be in lower suffix cidr. 10.1.1.0/24 will have 256 addresses and 10.1.1.0/25 will have 128 addresses[1]. /25 will be completely in /24 but not wise versa. The below are incorrect. > isNetworkAWithinNetworkB("10.1.1.0/24", "10.1.1.0/25") returns true > isNetworkAWithinNetworkB("10.1.1.0/22", "10.1.1.0/23") returns true I think you can change isNetworkAWithinNetworkB method to compare respective ip ranges for cidrs. What about changing method name isNetworkACompletelyWithinNetworkB() ? [1]https://www.dan.me.uk/ipsubnets?ip=10.1.1.0 Thanks, Jayapal On 13-Dec-2013, at 4:49 PM, Saksham Srivastava <saksham.srivast...@citrix.com> wrote: > Hi, > > I encountered a method isNetworkAWithinNetworkB(cidrA, cidrB) in > NetUtils.java which should return true if cidrA is a subset of cidrB. > The method returns flawed output in many scenarios. After unittesting it I > found : > > isNetworkAWithinNetworkB("10.1.1.0/24", "10.1.1.0/25") returns true > isNetworkAWithinNetworkB("10.1.1.0/25", "10.1.1.0/24") returns true > isNetworkAWithinNetworkB("10.1.1.0/23", "10.1.1.0/22") returns true > isNetworkAWithinNetworkB("10.1.1.0/22", "10.1.1.0/23") returns true > > Due to this I am able to create VPC tiers with cidr 10.1.0.0/24 even > when the VPC super cidr has been defined as 10.1.1.0/25 IMO the > simpler/cleaner way to compare cidrs should be to compare the respective IP > ranges. I have an old patch [1] in RB which uses the IP ranges to compare 2 > cidrs. > We could leverage that to replace isNetworkAWithinNetworkB() or in case of > any other suggestions please share. > > Thanks, > Saksham > > [1] https://reviews.apache.org/r/14124/diff/#index_header >
