From 4615bdaca5860b4969b8001e9f5afc1bb1e3cee8 Mon Sep 17 00:00:00 2001 From: Krunal Jain Date: Wed, 25 Jul 2018 17:24:28 -0700 Subject: [PATCH] # This is a combination of 8 commits. # This is the 1st commit message: IP Reservation functionality Addressing PR comments Fixing build Fixing nits Fixing commenting issues Fixing review comments Test Fixing CIDR Validation # This is the commit message #1: Addressing PR comments # This is the commit message #2: Adding test cases for carry over # This is the commit message #3: Fixing Error messages # This is the commit message #4: Fixing test comment # This is the commit message #5: Fixing test case names # This is the commit message #6: Adding global variables for IPmask and IPRange increment step # This is the commit message #7: Fixing validate cidr function --- pkg/cloud_provider/file/fake.go | 6 +- pkg/cloud_provider/file/file.go | 9 +- pkg/csi_driver/controller.go | 88 +++++--- pkg/csi_driver/controller_test.go | 13 +- pkg/util/ip_reservation.go | 183 ++++++++++++---- pkg/util/ip_reservation_test.go | 335 ++++++++++++++++++++++++------ 6 files changed, 496 insertions(+), 138 deletions(-) diff --git a/pkg/cloud_provider/file/fake.go b/pkg/cloud_provider/file/fake.go index 21c84aa0b..8d4852fd6 100644 --- a/pkg/cloud_provider/file/fake.go +++ b/pkg/cloud_provider/file/fake.go @@ -73,9 +73,9 @@ func (manager *fakeServiceManager) GetInstance(ctx context.Context, obj *Service } } -func (manager *fakeServiceManager) ListInstances(ctx context.Context, parent string) ([]*ServiceInstance, error) { +func (manager *fakeServiceManager) ListInstances(ctx context.Context, obj *ServiceInstance) ([]*ServiceInstance, error) { instances := []*ServiceInstance{ - &ServiceInstance{ + { Project: "test-project", Location: "test-location", Name: "test", @@ -84,7 +84,7 @@ func (manager *fakeServiceManager) ListInstances(ctx context.Context, parent str ReservedIpRange: "192.168.92.32/29", }, }, - &ServiceInstance{ + { Project: "test-project", Location: "test-location", Name: "test", diff --git a/pkg/cloud_provider/file/file.go b/pkg/cloud_provider/file/file.go index 6744a5141..91f7be497 100644 --- a/pkg/cloud_provider/file/file.go +++ b/pkg/cloud_provider/file/file.go @@ -56,7 +56,7 @@ type Service interface { CreateInstance(ctx context.Context, obj *ServiceInstance) (*ServiceInstance, error) DeleteInstance(ctx context.Context, obj *ServiceInstance) error GetInstance(ctx context.Context, obj *ServiceInstance) (*ServiceInstance, error) - ListInstances(ctx context.Context, parent string) ([]*ServiceInstance, error) + ListInstances(ctx context.Context, obj *ServiceInstance) ([]*ServiceInstance, error) } type gcfsServiceManager struct { @@ -232,11 +232,14 @@ func (manager *gcfsServiceManager) DeleteInstance(ctx context.Context, obj *Serv return nil } -func (manager *gcfsServiceManager) ListInstances(ctx context.Context, parent string) ([]*ServiceInstance, error) { - instances, err := manager.instancesService.List(parent).Context(ctx).Do() +// ListInstances returns a list of active instances in a project at a specific location +func (manager *gcfsServiceManager) ListInstances(ctx context.Context, obj *ServiceInstance) ([]*ServiceInstance, error) { + // Calling cloud provider service to get list of active instances. - indicates we are looking for instances in all the locations for a project + instances, err := manager.instancesService.List(locationURI(obj.Project, "-")).Context(ctx).Do() if err != nil { return nil, err } + var activeInstances []*ServiceInstance for _, activeInstance := range instances.Instances { serviceInstance, err := cloudInstanceToServiceInstance(activeInstance) diff --git a/pkg/csi_driver/controller.go b/pkg/csi_driver/controller.go index 409b13136..978871446 100644 --- a/pkg/csi_driver/controller.go +++ b/pkg/csi_driver/controller.go @@ -19,7 +19,6 @@ package driver import ( "fmt" "strings" - "sync" csi "github.com/container-storage-interface/spec/lib/go/csi/v0" "github.com/golang/glog" @@ -52,7 +51,7 @@ const ( paramTier = "tier" paramLocation = "location" paramNetwork = "network" - paramReservedIPV4CIDR = "cidr" + paramReservedIPV4CIDR = "reserved-ipv4-cidr" ) // controllerServer handles volume provisioning @@ -61,14 +60,14 @@ type controllerServer struct { } type controllerServerConfig struct { - driver *GCFSDriver - fileService file.Service - metaService metadata.Service - reservedIPRanges map[string]bool - mutex sync.Mutex + driver *GCFSDriver + fileService file.Service + metaService metadata.Service + ipAllocator *util.IPAllocator } func newControllerServer(config *controllerServerConfig) csi.ControllerServer { + config.ipAllocator = util.NewIPAllocator(make(map[string]bool)) return &controllerServer{config: config} } @@ -85,16 +84,7 @@ func (s *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolu if err := s.config.driver.validateVolumeCapabilities(req.GetVolumeCapabilities()); err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) } - s.config.mutex.Lock() - instances, err := s.config.fileService.ListInstances(ctx, "-") - if err != nil { - return nil, status.Error(codes.Aborted, err.Error()) - } - s.config.reservedIPRanges = make(map[string]bool) - for _, instance := range instances { - s.config.reservedIPRanges[instance.Network.ReservedIpRange] = true - } capBytes := getRequestCapacity(req.GetCapacityRange()) glog.V(5).Infof("Using capacity bytes %q for volume %q", capBytes, name) @@ -114,16 +104,64 @@ func (s *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolu return nil, status.Error(codes.AlreadyExists, err.Error()) } } else { + // If we are creating a new instance, we need pick an unused /29 range from reserved-ipv4-cidr + // If the param was not provided, we default reservedIPRange to "" and cloud provider takes care of the allocation + if reservedIPV4CIDR, ok := req.GetParameters()[paramReservedIPV4CIDR]; ok { + err := s.config.ipAllocator.ValidateCIDR(reservedIPV4CIDR) + if err != nil { + return nil, err + } + + reservedIPRange, err := s.reserveIPRange(ctx, newFiler, reservedIPV4CIDR) + + // Possible cases are 1) CreateInstanceAborted, 2)CreateInstance running in background + // The ListInstances response will contain the reservedIPRange if the operation was started + // In case of abort, the /29 IP is released and available for reservation + defer s.config.ipAllocator.ReleaseIPRange(reservedIPRange) + if err != nil { + return nil, err + } + + // Adding the reserved IP range to the instance object + newFiler.Network.ReservedIpRange = reservedIPRange + } + // Create the instance filer, err = s.config.fileService.CreateInstance(ctx, newFiler) if err != nil { return nil, status.Error(codes.Internal, err.Error()) } } - s.config.mutex.Unlock() return &csi.CreateVolumeResponse{Volume: fileInstanceToCSIVolume(filer, modeInstance)}, nil } +// reserveIPRange returns the available IP in the cidr +func (s *controllerServer) reserveIPRange(ctx context.Context, filer *file.ServiceInstance, cidr string) (string, error) { + cloudInstancesReservedIPRanges, err := s.getCloudInstancesReservedIPRanges(ctx, filer) + if err != nil { + return "", err + } + unreservedIPBlock, err := s.config.ipAllocator.GetUnreservedIPRange(cidr, cloudInstancesReservedIPRanges) + if err != nil { + return "", err + } + return unreservedIPBlock, nil +} + +// getCloudInstancesReservedIPRanges gets the list of reservedIPRanges from cloud instances +func (s *controllerServer) getCloudInstancesReservedIPRanges(ctx context.Context, filer *file.ServiceInstance) (map[string]bool, error) { + instances, err := s.config.fileService.ListInstances(ctx, filer) + if err != nil { + return nil, status.Error(codes.Aborted, err.Error()) + } + // Initialize an empty reserved list. It will be populated with all the reservedIPRanges obtained from the cloud instances + cloudInstancesReservedIPRanges := make(map[string]bool) + for _, instance := range instances { + cloudInstancesReservedIPRanges[instance.Network.ReservedIpRange] = true + } + return cloudInstancesReservedIPRanges, nil +} + // DeleteVolume deletes a GCFS instance func (s *controllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest) (*csi.DeleteVolumeResponse, error) { glog.V(4).Infof("DeleteVolume called with request %v", *req) @@ -225,7 +263,6 @@ func (s *controllerServer) generateNewFileInstance(name string, capBytes int64, tier := defaultTier network := defaultNetwork location := s.config.metaService.GetZone() - reservedIPV4CIDR := "" // Validate parameters (case-insensitive). for k, v := range params { switch strings.ToLower(k) { @@ -236,15 +273,11 @@ func (s *controllerServer) generateNewFileInstance(name string, capBytes int64, location = v case paramNetwork: network = v - case paramReservedIPV4CIDR: - reservedIPV4CIDR, err := util.GetUnreservedIPBlock(s.config.reservedIPRanges, v) - if err != nil { - return nil, err - } - if reservedIPV4CIDR == "" { - return nil, fmt.Errorf("Invalid unreserved IP block received for cidr %s", v) - } + // Ignore the cidr flag as it is not passed to the cloud provider + // It will be used to get unreserved IP in the reserveIPV4Range function + case paramReservedIPV4CIDR: + continue case "csiprovisionersecretname", "csiprovisionersecretnamespace": default: return nil, fmt.Errorf("invalid parameter %q", k) @@ -256,8 +289,7 @@ func (s *controllerServer) generateNewFileInstance(name string, capBytes int64, Location: location, Tier: tier, Network: file.Network{ - Name: network, - ReservedIpRange: reservedIPV4CIDR, + Name: network, }, Volume: file.Volume{ Name: newInstanceVolume, diff --git a/pkg/csi_driver/controller_test.go b/pkg/csi_driver/controller_test.go index cf6ca4af3..7c2692c5a 100644 --- a/pkg/csi_driver/controller_test.go +++ b/pkg/csi_driver/controller_test.go @@ -28,12 +28,13 @@ import ( ) const ( - testProject = "test-project" - testLocation = "test-location" - testIp = "test-ip" - testCSIVolume = "test-csi" - testVolumeId = "modeInstance/test-location/test-csi/vol1" - testBytes = 1 * util.Tb + testProject = "test-project" + testLocation = "test-location" + testIp = "test-ip" + testCSIVolume = "test-csi" + testVolumeId = "modeInstance/test-location/test-csi/vol1" + testReservedIPV4CIDR = "192.168.92.0/26" + testBytes = 1 * util.Tb ) func initTestController(t *testing.T) csi.ControllerServer { diff --git a/pkg/util/ip_reservation.go b/pkg/util/ip_reservation.go index a2e7b5d1e..5aaf55372 100644 --- a/pkg/util/ip_reservation.go +++ b/pkg/util/ip_reservation.go @@ -17,71 +17,176 @@ limitations under the License. package util import ( - "bytes" "fmt" + "math" "net" + "sync" ) -// GetUnreservedIPBlock returns an unreserved /29 IP block. It accepts the list of currently reserved -// IPs and the requested CIDR as arguments and returns the /29 IP available in that CIDR -// Potential error cases: 1) No /29 Block in the CIDR is unreserved -// Parsing the CIDR resulted in an error -func GetUnreservedIPBlock(reservedIPRanges map[string]bool, cidr string) (string, error) { +const ( + ipRangeSize = 29 + byteMax = 255 + ipV4Bits = 32 +) + +var ( + // step size for IP range increment + incrementStep29IPRange = (byte)(math.Exp2(ipV4Bits - ipRangeSize)) + // mask for IP range + ipRangeMask = net.CIDRMask(ipRangeSize, ipV4Bits) +) + +// IPAllocator struct consists of shared resources that are used to keep track of the /29 IPRanges currently reserved by service instances +type IPAllocator struct { + // pendingIPRanges set maintains the set of IP ranges that have been reserved by the service instances but pending reservation in the cloud instances + // The key is a IP range currently reserved by a service instance e.g(192.168.92.0/29). Value is a bool to implement map as a set + pendingIPRanges map[string]bool + + // pendingIPRangesMutex is used to synchronize access to the pendingIPRanges set to prevent data races + pendingIPRangesMutex sync.Mutex +} + +// NewIPAllocator is the constructor to initialize the IPAllocator object +// Argument pendingIPRanges map[string]bool is a set of IP ranges currently reserved by service instances but pending reservation in the cloud instances +func NewIPAllocator(pendingIPRanges map[string]bool) *IPAllocator { + // Make a copy of the pending IP ranges and set it in the IPAllocator so that the caller cannot mutate this map outside the library + pendingIPRangesCopy := make(map[string]bool) + for pendingIPRange := range pendingIPRanges { + pendingIPRangesCopy[pendingIPRange] = true + } + return &IPAllocator{ + pendingIPRanges: pendingIPRangesCopy, + } +} + +// holdIPRange adds a particular IP range in the pendingIPRanges set +// Argument ipRange string is an IPV4 range which needs put in pendingIPRanges +func (ipAllocator *IPAllocator) holdIPRange(ipRange string) { + ipAllocator.pendingIPRanges[ipRange] = true +} + +// ReleaseIPRange releases the pending IPRange +// Argument ipRange string is an IPV4 range which needs to be released +func (ipAllocator *IPAllocator) ReleaseIPRange(ipRange string) { + ipAllocator.pendingIPRangesMutex.Lock() + defer ipAllocator.pendingIPRangesMutex.Unlock() + delete(ipAllocator.pendingIPRanges, ipRange) +} + +// ValidateCIDR function validates whether a particular cidr is a valid IP range +// Argument cidr string is is a CIDR range that needs to be validated +func (ipAllocator *IPAllocator) ValidateCIDR(cidr string) error { ip, ipnet, err := net.ParseCIDR(cidr) if err != nil { - return "", fmt.Errorf("Unable to parse CIDR %s", cidr) + return err + } + // The cidr network address needs to be maximum 29 bits + cidrSize, _ := ipnet.Mask.Size() + if cidrSize > ipRangeSize { + return fmt.Errorf("the maximum size of network address in the cidr can be /%d", ipRangeSize) + } + + // The IP specified in the reserved-ipv4-cidr must be aligned to cover /29 IP Range + if ip.String() != ip.Mask(ipRangeMask).String() { + return fmt.Errorf("the IP specified in the reserved-ipv4-cidr must be aligned to cover /29 IP Range") } - buffer := bytes.NewBufferString("") - for cidrIPBlock := cloneIP(ip.Mask(ipnet.Mask)); ipnet.Contains(cidrIPBlock) && err == nil; err = incrementIP(cidrIPBlock, 8) { + return nil +} + +// GetUnreservedIPRange returns an unreserved /29 IP block. +// cidr: Provided cidr address in which we need to look for an unreserved /29 IP range +// cloudInstancesReservedIPRanges: All the used IP ranges in the cloud instances +// All the used IP ranges in the service instances not updated in cloud instances is extracted from the pendingIPRanges list in the IPAllocator +// Finally a final reservedIPRange list is created by merging these two lists +// Potential error cases: +// 1) No /29 IP range in the CIDR is unreserved +// 2) Parsing the CIDR resulted in an error +func (ipAllocator *IPAllocator) GetUnreservedIPRange(cidr string, cloudInstancesReservedIPRanges map[string]bool) (string, error) { + ip, ipnet, err := net.ParseCIDR(cidr) + if err != nil { + return "", err + } + var reservedIPRanges = make(map[string]bool) + + // The final reserved list is obtained by combining the cloudInstancesReservedIPRanges list and the pendingIPRanges list in the ipAllocator + for cloudInstancesReservedIPRange := range cloudInstancesReservedIPRanges { + reservedIPRanges[cloudInstancesReservedIPRange] = true + } + + // Lock is placed here so that the pendingIPRanges list captures all the IPs pending reservation in the cloud instances + ipAllocator.pendingIPRangesMutex.Lock() + defer ipAllocator.pendingIPRangesMutex.Unlock() + for reservedIPRange := range ipAllocator.pendingIPRanges { + reservedIPRanges[reservedIPRange] = true + } + + for cidrIP := cloneIP(ip.Mask(ipnet.Mask)); ipnet.Contains(cidrIP) && err == nil; cidrIP, err = incrementIP(cidrIP, incrementStep29IPRange) { overLap := false - buffer.WriteString(cidrIPBlock.String()) - buffer.WriteString("/29") for reservedIPRange := range reservedIPRanges { - err = validateCIDROverlap(buffer.String(), reservedIPRange) + _, reservedIPNet, err := net.ParseCIDR(reservedIPRange) if err != nil { - overLap = true + return "", err + } + // Creating IPnet object using IP and mask + cidrIPNet := &net.IPNet{ + IP: cidrIP, + Mask: ipRangeMask, + } + + // Find if the current IP range in the CIDR overlaps with any of the reserved IP ranges. If not, this can be returned + overLap, err = isOverlap(cidrIPNet, reservedIPNet) + + // Error while processing ipnet + if err != nil { + return "", err + } + if overLap { break } } if !overLap { - return buffer.String(), nil + ipRange := fmt.Sprint(cidrIP.String(), "/", ipRangeSize) + ipAllocator.holdIPRange(ipRange) + return ipRange, nil } - buffer.Reset() } - return "", fmt.Errorf("All of the /29 IP ranges in the cidr %s are reserved", cidr) + + // No unreserved IP range available in the entire CIDR range since we did not return + return "", fmt.Errorf("all of the /29 IP ranges in the cidr %s are reserved", cidr) } -// validateCIDROverlap checks if two cidrs have any overlapping IPs -func validateCIDROverlap(cidr1 string, cidr2 string) error { - _, ipnet1, err := net.ParseCIDR(cidr1) - if err != nil { - return err +// isOverlap checks if two ipnets have any overlapping IPs +func isOverlap(ipnet1 *net.IPNet, ipnet2 *net.IPNet) (bool, error) { + if ipnet1 == nil || ipnet2 == nil { + return true, fmt.Errorf("invalid ipnet object provided for cidr overlap check") } + return ipnet1.Contains(ipnet2.IP) || ipnet2.Contains(ipnet1.IP), nil +} - _, ipnet2, err := net.ParseCIDR(cidr2) - if err != nil { - return err - } +// Increment the given IP value by the provided step. The step is a byte with maximum value maximum byte value +func incrementIP(ip net.IP, step byte) (net.IP, error) { + incrementedIP := cloneIP(ip) + incrementedIP = incrementedIP.To4() - if ipnet1.Contains(ipnet2.IP) || ipnet2.Contains(ipnet1.IP) { - return fmt.Errorf("The cidr ranges %s and %s overlap", cidr1, cidr2) + // Step can be added directly to the Least Significant Byte and we can return the result + if incrementedIP[3] < byteMax-step { + incrementedIP[3] += step + return incrementedIP, nil } - return nil -} + // Step addition in the Least Significant Byte resulted in overflow + // Propogating the carry addition to the higher order bytes and calculating value of the current byte + incrementedIP[3] = incrementedIP[3] - byteMax + step - 1 -// Increment the given IP value by the provided step -func incrementIP(ip net.IP, step byte) error { - if uint8(255)-uint8(ip[len(ip)-1]) < uint8(step) { - return fmt.Errorf("IP overflow occured when incrementing ip %s with step %d", ip.String(), step) - } - for j := len(ip) - 1; j >= 0; j-- { - ip[j] += step - if ip[j] > 0 { - break + for ipByte := 2; ipByte >= 0; ipByte-- { + // Rollover occurs when value changes from maximum byte value to 0 as maximum propagated carry is 1 + if incrementedIP[ipByte] != byteMax { + incrementedIP[ipByte]++ + return incrementedIP, nil } + incrementedIP[ipByte] = 0 } - return nil + return nil, fmt.Errorf("ip range overflowed while incrementing IP %s by step %d", ip.String(), step) } // Clone the provided IP and return the copy diff --git a/pkg/util/ip_reservation_test.go b/pkg/util/ip_reservation_test.go index 3c9419df0..be6ca869f 100644 --- a/pkg/util/ip_reservation_test.go +++ b/pkg/util/ip_reservation_test.go @@ -17,97 +17,314 @@ limitations under the License. package util import ( + "fmt" + "math" "net" "testing" ) -func TestAllIPBlocksAvailable(t *testing.T) { - ips := [8]string{"192.168.92.32/29", "192.168.92.40/29", "192.168.92.48/29", "192.168.92.56/29"} - - cidr := "192.168.92.32/27" - - ip, err := GetUnreservedIPBlock(make(map[string]bool), cidr) - if err != nil { - t.Errorf(err.Error()) - } - if ip != ips[0] { - t.Errorf("Expected IP %s to be released from the free IP pool but got IP %s", ips[0], ip) +func initTestIPAllocator() *IPAllocator { + pendingIPRanges := make(map[string]bool) + return &IPAllocator{ + pendingIPRanges: pendingIPRanges, } } -func TestSomeIPBlocksAvailable(t *testing.T) { - ips := [8]string{"192.168.92.32/29", "192.168.92.40/29", "192.168.92.48/29", "192.168.92.56/29"} - - cidr := "192.168.92.32/27" - reservedIPBlocks := make(map[string]bool) - for i := 0; i < 2; i++ { - reservedIPBlocks[ips[i]] = true +func TestValidateCIDR(t *testing.T) { + cases := []struct { + name string + cidr string + errorExpected bool + }{ + { + name: "Valid /29 CIDR format", + cidr: "192.168.92.192/29", + errorExpected: false, + }, + { + name: "Invalid CIDR format", + cidr: "192.168.92.192", + errorExpected: true, + }, + { + name: "Invalid CIDR format with network address greater than 29 bits", + cidr: "192.168.92.249/30", + errorExpected: true, + }, + { + name: "Valid CIDR format with IP network address less than 29 bits", + cidr: "192.168.92.248/28", + errorExpected: false, + }, } - ip, err := GetUnreservedIPBlock(reservedIPBlocks, cidr) - if err != nil { - t.Errorf(err.Error()) - } - if ip != ips[2] { - t.Errorf("Expected IP %s to be released from the free IP pool but got IP %s", ips[2], ip) + ipAllocator := initTestIPAllocator() + for _, test := range cases { + err := ipAllocator.ValidateCIDR(test.cidr) + if test.errorExpected && err == nil { + t.Errorf("error while validating cidr %s, expected error while validating, got response as valid", test.cidr) + } else if !test.errorExpected && err != nil { + t.Errorf("error while validating cidr %s, expected valid response, got error %s", test.cidr, err.Error()) + } } } -func TestNoIPBlocksAvailable(t *testing.T) { - ips := [8]string{"192.168.92.33/29", "192.168.92.42/29", "192.168.92.49/29", "192.168.92.58/29"} - - cidr := "192.168.92.32/27" - reservedIPBlocks := make(map[string]bool) - for _, ip := range ips { - reservedIPBlocks[ip] = true +func TestGetUnReservedIPRange(t *testing.T) { + // Using IPs which are not the beginning IPs of /29 CIDRs to evaluate the edge case + ips := [8]string{"192.168.92.3/29", "192.168.92.10/29", "192.168.92.20/29", "192.168.92.28/29"} + cases := []struct { + name string + cidr string + pendingIPRanges map[string]bool + cloudProviderReservedIPRanges map[string]bool + expected string + errorExpected bool + }{ + { + name: "0 Pending, 0 Used", + cidr: "192.168.92.0/27", + pendingIPRanges: make(map[string]bool), + cloudProviderReservedIPRanges: make(map[string]bool), + expected: "192.168.92.0/29", + errorExpected: false, + }, + { + name: "0 Pending, 1 Used", + cidr: "192.168.92.0/27", + pendingIPRanges: make(map[string]bool), + cloudProviderReservedIPRanges: map[string]bool{ + ips[0]: true, + }, + expected: "192.168.92.8/29", + errorExpected: false, + }, + { + name: "1 Pending 0 Used", + cidr: "192.168.92.0/27", + pendingIPRanges: make(map[string]bool), + cloudProviderReservedIPRanges: map[string]bool{ + ips[0]: true, + }, + expected: "192.168.92.8/29", + errorExpected: false, + }, + { + name: "1 Pending 1 Used", + cidr: "192.168.92.0/27", + pendingIPRanges: map[string]bool{ + ips[0]: true, + }, + cloudProviderReservedIPRanges: map[string]bool{ + ips[1]: true, + }, + expected: "192.168.92.16/29", + errorExpected: false, + }, + { + name: "2 Pending 1 Used", + cidr: "192.168.92.0/27", + pendingIPRanges: map[string]bool{ + ips[0]: true, + ips[2]: true, + }, + cloudProviderReservedIPRanges: map[string]bool{ + ips[1]: true, + }, + expected: "192.168.92.24/29", + errorExpected: false, + }, + { + name: "Pending and used IPs out of CIDR range", + cidr: "192.168.92.0/27", + pendingIPRanges: map[string]bool{ + "192.168.33.33/29": true, + "192.168.44.44/29": true, + }, + cloudProviderReservedIPRanges: map[string]bool{ + "192.255.255.0/29": true, + "192.168.255.255/29": true, + }, + expected: "192.168.92.0/29", + errorExpected: false, + }, + { + name: "Unreserved IP Range obtained with carry over to significant bytes", + cidr: "192.168.0.0/16", + // Using a function for this case as we reserve 32 IP ranges + pendingIPRanges: getIPRanges("192.168.0.0/16", 32, t), + // Reserving IP ranges from 192.168.0.0/29 to 192.168.1.248 + cloudProviderReservedIPRanges: getIPRanges("192.168.1.0/16", 32, t), + expected: "192.168.2.0/29", + errorExpected: false, + }, + { + name: "2 Pending 2 Used. Unreserved IPRange unavailable", + cidr: "192.168.92.0/27", + pendingIPRanges: map[string]bool{ + ips[0]: true, + ips[2]: true, + }, + cloudProviderReservedIPRanges: map[string]bool{ + ips[1]: true, + ips[3]: true, + }, + errorExpected: true, + }, } - ip, err := GetUnreservedIPBlock(reservedIPBlocks, cidr) - if err == nil { - t.Errorf("Expected error as all IPs in cidr %s had been reserved but got IP %s as unreserved", cidr, ip) + for _, test := range cases { + ipAllocator := initTestIPAllocator() + ipAllocator.pendingIPRanges = test.pendingIPRanges + ipRange, err := ipAllocator.GetUnreservedIPRange(test.cidr, test.cloudProviderReservedIPRanges) + if err != nil && !test.errorExpected { + t.Errorf("test %q failed: got error %s, expected %s", test.name, err.Error(), test.expected) + } else if err == nil && test.errorExpected { + t.Errorf("test %q failed: got reserved IP range %s, expected error", test.name, ipRange) + } else if ipRange != test.expected { + t.Errorf("test %q failed: got reserved IP range %s, expected %s", test.name, ipRange, test.expected) + } } } -func TestValidateCIDR(t *testing.T) { - cidr1 := "192.168.92.32/27" - cidr2 := "192.168.92.48/26" - - err := validateCIDROverlap(cidr1, cidr2) - if err == nil { - t.Errorf("Expected error as cidr %s overlaps with cidr %s", cidr1, cidr2) +func getIPRanges(cidr string, ipRangesCount int, t *testing.T) map[string]bool { + ip, ipnet, err := net.ParseCIDR(cidr) + ipRangeMask := net.CIDRMask(ipRangeSize, ipV4Bits) + i := 0 + incrementStep := (byte)(math.Exp2(ipV4Bits - ipRangeSize)) + ipRanges := make(map[string]bool) + // Break out of the loop if + // 1) We have the required number of IP ranges in the set + // 2) IP range overflow occurs and IP increment is not possible + // 3) The incremented IP range is not contained in the cidr + for cidrIP := ip.Mask(ipRangeMask); ipnet.Contains(cidrIP) && err == nil && i < ipRangesCount; cidrIP, err = incrementIP(cidrIP, incrementStep) { + i++ + ipRangeString := fmt.Sprint(cidrIP.String(), "/", ipRangeSize) + ipRanges[ipRangeString] = true } - - cidr2 = "192.168.22.67/26" - err = validateCIDROverlap(cidr1, cidr2) if err != nil { - t.Errorf("Got overlapping error for non overlapping cidrs %s and %s", cidr1, cidr2) + t.Errorf(err.Error()) + } else if i != ipRangesCount { + t.Errorf("The required number of IP ranges %d are not available in the CIDR %s", ipRangesCount, cidr) } + return ipRanges +} +func TestValidateCIDROverlap(t *testing.T) { + cases := []struct { + name string + cidr1 string + cidr2 string + expected bool + errorExpected bool + }{ + { + name: "Overlapping CIDRs", + cidr1: "192.168.92.0/29", + cidr2: "192.168.92.48/26", + expected: true, + errorExpected: false, + }, + { + name: "Non overlapping CIDRs", + cidr1: "192.168.92.0/29", + cidr2: "192.168.22.67/26", + expected: false, + errorExpected: false, + }, + { + name: "Non overlapping CIDRs with same cidr size", + cidr1: "192.168.92.247/29", + cidr2: "192.168.92.248/29", + expected: false, + errorExpected: false, + }, + { + name: "Overlapping CIDRs with same cidr size", + cidr1: "192.168.92.249/29", + cidr2: "192.168.92.255/29", + expected: true, + errorExpected: false, + }, + { + name: "Invalid CIDR provided", + cidr1: "192.168.92.0", + cidr2: "192.168.22.67/26", + errorExpected: true, + }, + } + + for _, test := range cases { + _, ipnet1, _ := net.ParseCIDR(test.cidr1) + _, ipnet2, _ := net.ParseCIDR(test.cidr2) + overlap, err := isOverlap(ipnet1, ipnet2) + if err != nil && !test.errorExpected { + t.Errorf("test %q failed: got error %s, expected cidr overlap between %s and %s to be %t", test.name, err.Error(), test.cidr1, test.cidr2, test.expected) + } else if err == nil && test.errorExpected { + t.Errorf("test %q failed: got cidr overlap value %t, expected error", test.name, overlap) + } else if !test.errorExpected && overlap != test.expected { + t.Errorf("test %q failed: got overlap for cidr %s and %s as %t, expected %t", test.name, test.cidr1, test.cidr2, test.expected, test.expected) + } + } } func TestIncrementIP(t *testing.T) { - currentIP := "192.168.92.32" - nextIP := "192.168.92.40" - ip := net.ParseIP(currentIP) - incrementIP(ip, 8) - if ip.String() != nextIP { - t.Errorf("Error while incrementing IP expected %s but got %s", nextIP, ip.String()) + cases := []struct { + name string + currentIP string + step byte + expected string + errorExpected bool + }{ + { + name: "Valid IP increment without carry forward to significant bytes", + currentIP: "192.168.92.32", + step: 143, + expected: "192.168.92.175", + errorExpected: false, + }, + { + name: "Valid increment with carry forward to significant bytes with maximum step size", + currentIP: "192.255.255.253", + step: 255, + expected: "193.0.0.252", + errorExpected: false, + }, + { + name: "Valid increment with carry forward to significant bytes", + currentIP: "0.255.255.255", + step: 8, + expected: "1.0.0.7", + errorExpected: false, + }, + { + name: "Invalid increment", + currentIP: "255.255.255.253", + step: 3, + errorExpected: true, + }, } - currentIP = "255.255.255.254" - ip = net.ParseIP(currentIP) - step := 8 - err := incrementIP(ip, byte(step)) - if err == nil { - t.Errorf("IP Overflow not caught for IP %s increment by %d", ip.String(), 8) + for _, test := range cases { + currentIP := net.ParseIP(test.currentIP) + incrementedIP, err := incrementIP(currentIP, test.step) + + if err != nil && !test.errorExpected { + t.Errorf("test %q failed: got error %s, expected %s", test.name, err.Error(), test.expected) + } else if err == nil && test.errorExpected { + t.Errorf("test %q failed: got reserved IP range %s, expected error", test.name, incrementedIP.String()) + } else if !test.errorExpected && incrementedIP.String() != test.expected { + t.Errorf("test %q failed: got incremented IP %s, expected %s", test.name, incrementedIP.String(), test.expected) + } } } - func TestCloneIP(t *testing.T) { originalIP := net.ParseIP("192.168.92.32") cloneIP := cloneIP(originalIP) if cloneIP.String() != originalIP.String() { - t.Errorf("Error while cloning IP %s", originalIP.String()) + t.Errorf("error while cloning IP %s", originalIP.String()) + } + if &originalIP == &cloneIP { + t.Errorf("clone function returned the original object") } }