-
Notifications
You must be signed in to change notification settings - Fork 661
Fix IP Allocation Bug: Reserved Range Not Detected #2657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
db595e4
33221f8
6f689a9
6c17c78
277734b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,49 +96,57 @@ def try_to_allocate_dynamic_ip(reservation, subnet) | |
| addresses_we_cant_allocate.delete_if { |ipaddr| ipaddr.ipv6? } | ||
| end | ||
|
|
||
| addresses_we_cant_allocate.reject! do |ip| | ||
| addresses_we_cant_allocate.any? do |other_ip| | ||
| includes = other_ip.include?(ip) | ||
| includes && other_ip.prefix < ip.prefix | ||
| # Sort by address first, then by prefix (smaller prefix = larger block = earlier) | ||
| sorted_ips = addresses_we_cant_allocate.sort_by { |ip| [ip.to_i, ip.prefix] } | ||
|
|
||
| # Remove IPs contained within larger CIDR blocks | ||
| sorted_ips = sorted_ips.reject.with_index do |ip, index| | ||
| sorted_ips[0...index].any? do |other_ip| | ||
| other_ip.prefix < ip.prefix && other_ip.include?(ip) | ||
| rescue StandardError | ||
| false | ||
|
Comment on lines
+99
to
+107
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this whole change needed? For performance reasons? Logic wise it seems to get to the same result as the previous code or am I wrong?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea of the sorting is to potentially avoid comparisons like this: 10.0.11.32/32.include?(10.0.11.32/30)That is the main change along with the two way comparison further down: blocking_ip = filtered_ips.find do |ip|
current_prefix.include?(ip) || ip.include?(current_prefix)
endI am not confident enough in the "fix" and we haven't been able to isolate the issue well enough. I will move the PR to draft for now. |
||
| end | ||
| end | ||
|
|
||
| ip_address_cidr = find_next_available_ip(addresses_we_cant_allocate, first_range_address, subnet.prefix) | ||
| ip_address_cidr = find_next_available_ip(sorted_ips, first_range_address, subnet.prefix) | ||
|
|
||
| if !(subnet.range == ip_address_cidr || subnet.range.include?(ip_address_cidr)) | ||
| raise NoMoreIPsAvailableAndStopRetrying | ||
| unless subnet.range == ip_address_cidr || subnet.range.include?(ip_address_cidr) | ||
| raise NoMoreIPsAvailableAndStopRetrying | ||
| end | ||
|
|
||
| save_ip(ip_address_cidr, reservation, false) | ||
|
|
||
| ip_address_cidr | ||
| end | ||
|
|
||
| def find_next_available_ip(addresses_we_cant_allocate, first_range_address, prefix) | ||
| def find_next_available_ip(sorted_blocking_ips, first_range_address, prefix) | ||
| # Remove IPs that are below subnet range | ||
| filtered_ips = addresses_we_cant_allocate.sort_by { |ip| ip.to_i }.reject { |ip| ip.to_i < first_range_address.to_i } | ||
| filtered_ips = sorted_blocking_ips.reject { |ip| ip.to_i < first_range_address.to_i } | ||
|
|
||
| current_ip = to_ipaddr(first_range_address.to_i + 1) | ||
| found = false | ||
|
|
||
| while found == false | ||
| loop do | ||
| current_prefix = to_ipaddr("#{current_ip.base_addr}/#{prefix}") | ||
|
|
||
| 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) | ||
| else | ||
| current_ip = to_ipaddr(current_ip.to_i + current_prefix.count) | ||
| end | ||
| # Check both directions for overlap: candidate includes blocking IP, or blocking IP includes candidate | ||
| blocking_ip = filtered_ips.find do |ip| | ||
| (current_prefix.include?(ip) rescue false) || | ||
| (ip.include?(current_prefix) rescue false) | ||
| end | ||
|
|
||
| return current_prefix if blocking_ip.nil? | ||
|
|
||
| if blocking_ip.count > current_prefix.count | ||
| # Blocking range is larger, skip past its entire range | ||
| current_ip = to_ipaddr(blocking_ip.to_i + blocking_ip.count) | ||
| else | ||
| found_cidr = current_prefix | ||
| found = true | ||
| # Blocking IP is smaller or same size, try next aligned position | ||
| current_ip = to_ipaddr(current_prefix.to_i + current_prefix.count) | ||
| end | ||
| end | ||
|
|
||
| found_cidr | ||
| # Clean up blocking IPs that we've passed | ||
| filtered_ips.reject! { |ip| ip.to_i + ip.count < current_ip.to_i } | ||
| end | ||
| end | ||
|
|
||
| def try_to_allocate_vip_ip(reservation, subnet) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,13 +69,18 @@ def self.parse(network_name, subnet_spec, availability_zones, managed = false) | |
| end | ||
| end | ||
|
|
||
| restricted_ips.reject! do |ip| | ||
| restricted_ips.any? do |other_ip| | ||
| includes = other_ip.include?(ip) | ||
| includes && other_ip.prefix < ip.prefix | ||
| sorted_restricted_ips = restricted_ips.to_a.sort_by { |ip| [ip.to_i, ip.prefix] } | ||
|
|
||
| deduplicated_ips = sorted_restricted_ips.reject.with_index do |ip, index| | ||
| sorted_restricted_ips[0...index].any? do |other_ip| | ||
| other_ip.prefix < ip.prefix && other_ip.include?(ip) | ||
| rescue StandardError | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is very broad. perhaps |
||
| false | ||
|
Comment on lines
+72
to
+78
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. Why is this needed? |
||
| end | ||
| end | ||
|
|
||
| restricted_ips.replace(deduplicated_ips) | ||
|
|
||
| each_ip(static_property, false) do |ip| | ||
| if ip_in_array?(ip, restricted_ips) | ||
| raise NetworkStaticIpOutOfRange, "Static IP '#{ip}' is in network '#{network_name}' reserved range" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comment #2657 (comment)