Drop ipaddress dependency in favor of built in ipaddr

[ekohl]

The ipaddr module is part of Ruby itself and can be used to check for range inclusion.

I don't have the capacity to test this properly and it doesn't appear to be covered by tests. Please review carefully.

[geemus]

Thanks for having a go at this.

It looks like there is another reference that will need changed though, here:

fog-aws/lib/fog/aws/requests/compute/create_network_interface.rb

Line 71 in 66a4183

cidr_block = IPAddress.parse(subnet['cidrBlock'])

Could you take a look at that?

Given this being used across more than one file, maybe we should move the require up to a higher level (maybe https://github.com/fog/fog-aws/blob/master/lib/fog/aws/compute.rb)? And/or we should add the require to the file mentioned above, otherwise we may have risk of load order issues, as it seems somewhat coincidental that the reference above ever worked.

Does that make sense? Otherwise this seems on the right track, though I may need to do some further testing of my own just in case. Thanks!

[ekohl]

I think I got it. Rewriting each_host to use to_range wasn't pretty, but I think this is the best.

[geemus]

Sorry, I'm not super familiar with all of this. Could you help me understand what the drop and slice are needed for here? I tried to look at the IPAddress code for each_host:

https://github.com/ipaddress-gem/ipaddress/blob/8ff186bd8d71da69fa33b999ff69be9d8885c424/lib/ipaddress/ipv4.rb#L448-L452

I can see the +1 and -1 there, which I imagine omit the first and last things in the range, but I think drop(2) and -2 would actually omit 2 from the front and back instead? Certainly possibly that I'm missing something here, but wanted to try and ensure I understood what we expected.

[ekohl]

It was also a bit vague to me, but it has to do with the other comment you placed. In my examples I will use 192.0.2.0/24 as the subnet.

You're right about +1 to drop the network address (192.0.2.0). Then the additional +1 is for the router (see https://github.com/fog/fog-aws/pull/630/files#r803550336). That makes +2 and is .drop(2).

The -1 is the broadcast address (192.0.2.255). The reason I use -2 in there is just Ruby. -1 is the end, so [0..-1] is the same as not putting anything in.

x = ['a', 'b', 'c']
assert x[0..-1] == x

But perhaps it's better to essentially implement to_range manually here and use the integer representation of addresses.

[ekohl]

I took a stab at this and something like this will work with ipaddr 1.2.3:

def usable_addresses(block)
  if block.ipv4?
    mask_addr = IPAddr.new(block.netmask)
    # Exclude the network address and the first IP,
    # which is typically reserved for the gateway).
    begin_addr = (block & mask_addr).to_i + 2
    # Exclude the broadcast address
    end_addr = (block.to_i | (IPAddr::IN4MASK ^ mask_addr.to_i)) - 1

    IPAddr.new(begin_addr, block.family)..IPAddr.new(end_addr, block.family)
  else
    block.to_range
  end
end

Sadly, this needs 1.2.3 which isn't in Ruby 3.0 at least. It's because netmask as only added in ruby/ipaddr@283d16f.

You could add it as a dependency. You'll still be trading one for the other. The alternative is to hack around it get a private instance variable.

[geemus]

Yeah, thanks for the additional detail. At the end of the day this functionality is in the mocks and probably/hopefully not super impactful, so hopefully we are fine to proceed.

[geemus]

Thanks for taking another go at that, it's good to see the tests passing again (though as you suggested, I'm not sure they cover this particular path very well).

Could you help me out on the questions about the differences between the range and each_host variations to make sure we are on the same page?

Thanks!

[geemus]

I noticed that this no longer includes the or and second check: \\ cidr_block.first == p_ip. I wish I could say that I remembered why that was there in the first place, but I at least wanted to confirm we had good reason to drop it.

[ekohl]

I think it was to exclude the first real IP in the subnet since that's often the router. Like 192.0.2.1 is typically the router for 192.0.2.0/24. I chose to pull it out to the range definition above. While this wasn't really considering anything with IPv6, I wasn't quite sure what to do. With IPv6 you have link local addresses (fe80::/10) and typically a router uses that to announce itself, though sometimes an IP from the subnet is used.

Since I don't use AWS myself, I wasn't sure what to do. My goal was to get rid of the ipaddress gem in Foreman's RPM packaging.

[geemus]

Thanks for discussing and working through this with me.

[geemus]

@ekohl - do you need a release?

[ekohl]

That would be great.

[geemus]

Released as part of v3.13.0