Merge pull request #2657 from cloudfoundry/fix_ip_allocation_from_reserved_range_in_dynamic_networks

Fix IP Allocation Bug: Reserved Range Not Detected

Author: beyhan

src/bosh-director/lib/bosh/director/deployment_plan/ip_provider/ip_repo.rb

@@ -106,7 +106,7 @@ def try_to_allocate_dynamic_ip(reservation, subnet)
       ip_address_cidr = find_next_available_ip(addresses_we_cant_allocate, first_range_address, subnet.prefix)
 
       if !(subnet.range == ip_address_cidr || subnet.range.include?(ip_address_cidr))
-       raise NoMoreIPsAvailableAndStopRetrying
+        raise NoMoreIPsAvailableAndStopRetrying
       end
 
       save_ip(ip_address_cidr, reservation, false)
@@ -123,12 +123,12 @@ def find_next_available_ip(addresses_we_cant_allocate, first_range_address, pref
 
       while found == false
         current_prefix = to_ipaddr("#{current_ip.base_addr}/#{prefix}")
+        filtered_ips.reject! { |ip| ip.to_range.last.to_i < current_prefix.to_i }
 
-        if filtered_ips.any? { |ip| current_prefix.include?(ip) }
-          filtered_ips.reject! { |ip| ip.to_i < current_prefix.to_i }
-          actual_ip_prefix = filtered_ips.first.count
-          if actual_ip_prefix > current_prefix.count
-            current_ip = to_ipaddr(current_ip.to_i + actual_ip_prefix)
+        blocking_ip = filtered_ips.first
+        if blocking_ip&.overlaps?(current_prefix)
+          if blocking_ip.count > current_prefix.count
+            current_ip = to_ipaddr(blocking_ip.to_i + blocking_ip.count)
           else
             current_ip = to_ipaddr(current_ip.to_i + current_prefix.count)
           end

src/bosh-director/lib/bosh/director/ip_addr_or_cidr.rb

@@ -84,6 +84,14 @@ def first
         Bosh::Director::IpAddrOrCidr.new(@ipaddr.to_range.first.to_i)
       end
 
+      def overlaps?(other)
+        my_first = @ipaddr.to_range.first.to_i
+        my_last = @ipaddr.to_range.last.to_i
+        other_first = other.to_range.first.to_i
+        other_last = other.to_range.last.to_i
+        my_first <= other_last && other_first <= my_last
+      end
+
       private
 
       def max_addresses

src/bosh-director/spec/unit/bosh/director/deployment_plan/ip_provider/ip_repo_spec.rb

@@ -492,6 +492,201 @@ def fail_saving_ips(ips, fail_error)
             it_behaves_like :retries_on_race_condition
           end
         end
+
+        context 'when handling CIDR blocks and overlapping ranges' do
+          def save_ip_string(ip_string)
+            ip_addr = to_ipaddr(ip_string)
+            Bosh::Director::Models::IpAddress.new(
+              address_str: ip_addr.to_s,
+              network_name: 'my-manual-network',
+              instance: instance_model,
+              task_id: Bosh::Director::Config.current_job.task_id
+            ).save
+          end
+
+          context 'when database has individual IPs that are contained in a reserved CIDR block' do
+            it 'deduplicates and skips the entire CIDR block' do
+              network_spec['subnets'].first['range'] = '10.0.11.32/27'
+              network_spec['subnets'].first['gateway'] = '10.0.11.33'
+              network_spec['subnets'].first['reserved'] = ['10.0.11.32 - 10.0.11.35', '10.0.11.63']
+              network_spec['subnets'].first['static'] = ['10.0.11.36', '10.0.11.37', '10.0.11.38', '10.0.11.39', '10.0.11.40']
+
+              save_ip_string('10.0.11.32/32')
+              save_ip_string('10.0.11.33/32')
+
+              ip_address = ip_repo.allocate_dynamic_ip(reservation, subnet)
+              expect(ip_address).to eq(cidr_ip('10.0.11.41'))
+            end
+          end
+
+          context 'when multiple overlapping CIDR blocks exist' do
+            it 'deduplicates to largest block only' do
+              network_spec['subnets'].first['range'] = '192.168.1.0/24'
+              network_spec['subnets'].first['gateway'] = '192.168.1.1'
+              network_spec['subnets'].first['reserved'] = [
+                '192.168.1.0 - 192.168.1.15',
+                '192.168.1.0 - 192.168.1.3',
+                '192.168.1.4 - 192.168.1.7',
+                '192.168.1.8',
+              ]
+
+              ip_address = ip_repo.allocate_dynamic_ip(reservation, subnet)
+              expect(ip_address).to eq(cidr_ip('192.168.1.16'))
+            end
+          end
+
+          context 'when nested CIDR blocks exist' do
+            it 'deduplicates to outermost block' do
+              network_spec['subnets'].first['range'] = '192.168.1.0/24'
+              network_spec['subnets'].first['gateway'] = '192.168.1.1'
+              network_spec['subnets'].first['reserved'] = [
+                '192.168.1.0/24',
+                '192.168.1.0/26',
+                '192.168.1.0/28',
+              ]
+
+              ip_address = ip_repo.allocate_dynamic_ip(reservation, subnet)
+              expect(ip_address).to be_nil
+            end
+          end
+
+          context 'when adjacent non-overlapping CIDR blocks exist' do
+            it 'preserves all blocks and skips each correctly' do
+              network_spec['subnets'].first['range'] = '10.0.0.0/24'
+              network_spec['subnets'].first['gateway'] = '10.0.0.1'
+              network_spec['subnets'].first['reserved'] = [
+                '10.0.0.0 - 10.0.0.3',
+                '10.0.0.4 - 10.0.0.7',
+                '10.0.0.8 - 10.0.0.11',
+              ]
+
+              ip_address = ip_repo.allocate_dynamic_ip(reservation, subnet)
+              expect(ip_address).to eq(cidr_ip('10.0.0.12'))
+            end
+          end
+
+          context 'when large CIDR block contains scattered individual IPs' do
+            it 'deduplicates scattered IPs within the block' do
+              network_spec['subnets'].first['range'] = '10.1.1.0/24'
+              network_spec['subnets'].first['gateway'] = '10.1.1.1'
+              network_spec['subnets'].first['reserved'] = ['10.1.1.0/24']
+              network_spec['subnets'].first['static'] = []
+
+              save_ip_string('10.1.1.5/32')
+              save_ip_string('10.1.1.50/32')
+              save_ip_string('10.1.1.100/32')
+              save_ip_string('10.1.1.200/32')
+
+              ip_address = ip_repo.allocate_dynamic_ip(reservation, subnet)
+              expect(ip_address).to be_nil
+            end
+          end
+
+          context 'when handling AWS reserved IP ranges' do
+            it 'correctly skips reserved ranges with database IPs' do
+              network_spec['subnets'].first['range'] = '10.0.11.32/27'
+              network_spec['subnets'].first['gateway'] = '10.0.11.33'
+              network_spec['subnets'].first['reserved'] = ['10.0.11.32 - 10.0.11.35', '10.0.11.63']
+              network_spec['subnets'].first['static'] = []
+
+              save_ip_string('10.0.11.32/32')
+              save_ip_string('10.0.11.33/32')
+              save_ip_string('10.0.11.34/32')
+
+              ip_address = ip_repo.allocate_dynamic_ip(reservation, subnet)
+              expect(ip_address).to eq(cidr_ip('10.0.11.36'))
+            end
+          end
+
+          context 'when candidate block lands inside a reserved CIDR that started before it' do
+            it 'does not allocate an address inside the reserved range' do
+              # Reserved: 10.0.0.130/23 covers 10.0.130.0 - 10.0.131.255 (512 IPs).
+              # Walking forward from 10.0.129.0/24: that first candidate overlaps
+              # the /23, so we advance by 512, landing at 10.0.131.0.  But .131.0
+              # is still inside the /23.  The pruning step must NOT discard the /23
+              # just because its base (.130.0) < new candidate base (.131.0).
+              # The first address outside the reserved range is 10.0.132.0.
+              network_spec['subnets'].first['range'] = '10.0.128.0/21'
+              network_spec['subnets'].first['gateway'] = '10.0.128.1'
+              network_spec['subnets'].first['reserved'] = [
+                '10.0.128.0 - 10.0.129.255',
+                '10.0.130.0 - 10.0.131.255',
+                '10.0.132.0 - 10.0.133.0',
+              ]
+              network_spec['subnets'].first['static'] = []
+
+              ip_address = ip_repo.allocate_dynamic_ip(reservation, subnet)
+              expect(ip_address).to eq(cidr_ip('10.0.133.1'))
+            end
+          end
+        end
+
+        context 'when allocating dynamic IPs from an IPv6 subnet' do
+          let(:ipv6_network_spec) do
+            {
+              'name' => 'my-manual-network',
+              'subnets' => [
+                {
+                  'range' => '2001:db8::/120',
+                  'gateway' => '2001:db8::1',
+                  'dns' => [],
+                  'static' => [],
+                  'reserved' => [],
+                  'cloud_properties' => {},
+                  'az' => 'az-1',
+                },
+              ],
+            }
+          end
+
+          let(:ipv6_network) do
+            ManualNetwork.parse(
+              ipv6_network_spec,
+              availability_zones,
+              per_spec_logger,
+            )
+          end
+
+          let(:ipv6_subnet) do
+            ManualNetworkSubnet.parse(
+              ipv6_network.name,
+              ipv6_network_spec['subnets'].first,
+              availability_zones,
+            )
+          end
+
+          let(:ipv6_reservation) { Bosh::Director::DesiredNetworkReservation.new_dynamic(instance_model, ipv6_network) }
+
+          it 'returns the first available IPv6 address' do
+            ip_address = ip_repo.allocate_dynamic_ip(ipv6_reservation, ipv6_subnet)
+            expect(ip_address).to eq(cidr_ip('2001:db8::2'))
+          end
+
+          it 'allocates sequential IPv6 addresses' do
+            first = ip_repo.allocate_dynamic_ip(ipv6_reservation, ipv6_subnet)
+            second = ip_repo.allocate_dynamic_ip(ipv6_reservation, ipv6_subnet)
+            expect(first).to eq(cidr_ip('2001:db8::2'))
+            expect(second).to eq(cidr_ip('2001:db8::3'))
+          end
+
+          context 'when there are reserved IPv6 ranges' do
+            it 'skips reserved addresses' do
+              ipv6_network_spec['subnets'].first['reserved'] = ['2001:db8::2 - 2001:db8::4']
+
+              ip_address = ip_repo.allocate_dynamic_ip(ipv6_reservation, ipv6_subnet)
+              expect(ip_address).to eq(cidr_ip('2001:db8::5'))
+            end
+          end
+
+          context 'when there are reserved IPv6 CIDR blocks' do
+            it 'skips the entire CIDR block' do
+              ipv6_network_spec['subnets'].first['reserved'] = ['2001:db8::/124']
+
+              ip_address = ip_repo.allocate_dynamic_ip(ipv6_reservation, ipv6_subnet)
+              expect(ip_address).to eq(cidr_ip('2001:db8::10'))
+            end
+          end
+        end
       end
 
       describe :allocate_vip_ip do

src/bosh-director/spec/unit/bosh/director/ip_addr_or_cidr_spec.rb

@@ -131,5 +131,102 @@ module Bosh::Director
         end
       end
     end
+
+    describe '#overlaps?' do
+      context 'when two /32 IPs are the same' do
+        it 'returns true' do
+          a = IpAddrOrCidr.new('10.0.0.1')
+          b = IpAddrOrCidr.new('10.0.0.1')
+          expect(a.overlaps?(b)).to be true
+        end
+      end
+
+      context 'when two /32 IPs are different' do
+        it 'returns false' do
+          a = IpAddrOrCidr.new('10.0.0.1')
+          b = IpAddrOrCidr.new('10.0.0.2')
+          expect(a.overlaps?(b)).to be false
+        end
+      end
+
+      context 'when a /32 is inside a CIDR block' do
+        it 'returns true (IP inside block)' do
+          ip = IpAddrOrCidr.new('192.168.1.5')
+          block = IpAddrOrCidr.new('192.168.1.0/25')
+          expect(ip.overlaps?(block)).to be true
+          expect(block.overlaps?(ip)).to be true
+        end
+      end
+
+      context 'when a /32 is outside a CIDR block' do
+        it 'returns false' do
+          ip = IpAddrOrCidr.new('192.168.2.1')
+          block = IpAddrOrCidr.new('192.168.1.0/25')
+          expect(ip.overlaps?(block)).to be false
+          expect(block.overlaps?(ip)).to be false
+        end
+      end
+
+      context 'when two CIDR blocks overlap partially' do
+        it 'returns true' do
+          a = IpAddrOrCidr.new('192.168.1.0/30')
+          b = IpAddrOrCidr.new('192.168.1.2/30')
+          expect(a.overlaps?(b)).to be true
+        end
+      end
+
+      context 'when two CIDR blocks are adjacent but non-overlapping' do
+        it 'returns false' do
+          a = IpAddrOrCidr.new('192.168.1.0/30')
+          b = IpAddrOrCidr.new('192.168.1.4/30')
+          expect(a.overlaps?(b)).to be false
+          expect(b.overlaps?(a)).to be false
+        end
+      end
+
+      context 'when a smaller block is nested inside a larger block' do
+        it 'returns true' do
+          outer = IpAddrOrCidr.new('10.0.0.0/24')
+          inner = IpAddrOrCidr.new('10.0.0.128/25')
+          expect(outer.overlaps?(inner)).to be true
+          expect(inner.overlaps?(outer)).to be true
+        end
+      end
+
+      context 'when CIDR blocks are completely disjoint' do
+        it 'returns false' do
+          a = IpAddrOrCidr.new('10.0.0.0/24')
+          b = IpAddrOrCidr.new('10.0.1.0/24')
+          expect(a.overlaps?(b)).to be false
+        end
+      end
+
+      context 'with the exact scenario that triggers the IPAddr coercion bug' do
+        it 'correctly detects overlap between /32 and /25' do
+          ip32 = IpAddrOrCidr.new('192.168.1.0')
+          block25 = IpAddrOrCidr.new('192.168.1.0/25')
+          expect(ip32.overlaps?(block25)).to be true
+
+          # A /32 NOT in the /25 range
+          ip_outside = IpAddrOrCidr.new('192.168.1.200')
+          expect(ip_outside.overlaps?(block25)).to be false
+        end
+      end
+
+      context 'with IPv6 addresses' do
+        it 'detects overlapping IPv6 ranges' do
+          a = IpAddrOrCidr.new('fd00::/64')
+          b = IpAddrOrCidr.new('fd00::1')
+          expect(a.overlaps?(b)).to be true
+          expect(b.overlaps?(a)).to be true
+        end
+
+        it 'returns false for non-overlapping IPv6 ranges' do
+          a = IpAddrOrCidr.new('fd00::/64')
+          b = IpAddrOrCidr.new('fd01::1')
+          expect(a.overlaps?(b)).to be false
+        end
+      end
+    end
   end
 end

src/spec/integration/global_networking/ip_reservations/allocating_dynamic_ips_spec.rb

@@ -249,5 +249,27 @@ def deploy_with_static_ip(deployment_name, ip, range)
       expect(second_deploy_instances.first.ips).to eq(first_deploy_instances.first.ips)
       expect(second_deploy_instances.first.vm_cid).to eq(first_deploy_instances.first.vm_cid)
     end
+
+    it 'does not allocate IPs within a CIDR reserved range' do
+      cloud_config_hash = SharedSupport::DeploymentManifestHelper.simple_cloud_config
+      cloud_config_hash['networks'].first['subnets'] = [
+        {
+          'range' => '192.168.1.0/24',
+          'gateway' => '192.168.1.1',
+          'dns' => [],
+          'static' => [],
+          'reserved' => ['192.168.1.0/28'],
+          'cloud_properties' => {},
+        }
+      ]
+
+      manifest_hash = SharedSupport::DeploymentManifestHelper.deployment_manifest(name: 'my-deploy', instances: 2)
+
+      upload_cloud_config(cloud_config_hash: cloud_config_hash)
+      deploy_simple_manifest(manifest_hash: manifest_hash)
+
+      deployed_ips = director.instances(deployment_name: 'my-deploy').map(&:ips).flatten
+      expect(deployed_ips).to match_array(['192.168.1.16', '192.168.1.17'])
+    end
   end
 end