Skip to content
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

Add method to subtract IPv4 subnets #129

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion lib/ipaddress/ipv4.rb
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ def <=>(oth)
to_u32 <=> oth.to_u32
end
alias eql? ==

#
# Returns the number of IP addresses included
# in the network. It also counts the network
Expand Down Expand Up @@ -805,6 +805,42 @@ def subnet(subprefix)
end
end

#
# Subtract other from the current range
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we explain this method in more detail? I have decent knowledge of IPv4 addresses, but I don't really understand the use-case or what this does.

Also, is this limited to ranges? If so, any validation around that? Should that be reflected in naming?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add some extended explanation.
This use-case is pretty common if you want to plan network. E.g. you dedicated /18 range for k8s clusters control planes. There are 3 clusters, each of whose occupied /20 range. This method allows to know what ranges are free.

Another use-case is here: https://docs.aws.amazon.com/vpc/latest/userguide/aws-ip-ranges.html#aws-ip-egress-control. It tells "allow all by EC2".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is this limited to ranges? If so, any validation around that? Should that be reflected in naming?

No, it isn't. Individual addresses are essentially ranges of 1 element with /32 netmask.
And current implementation works with individual addresses, I will add test case for that.

# Returns list of ranges that excludes :other
skorobogatydmitry marked this conversation as resolved.
Show resolved Hide resolved
#
# Example:
#
# a = IPAddress '10.0.0.0/16'
skorobogatydmitry marked this conversation as resolved.
Show resolved Hide resolved
# b = IPAddress '10.0.128.0/18'
# c = IPAddress '192.168.0.0/16'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we come up with better names than a, b, c?

Copy link
Author

@skorobogatydmitry skorobogatydmitry May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. But I don't see other test cases follow this..

#
# a.subtract(b)
# =>
# [#<IPAddress::IPv4:0x0000000111e61a88 @address="10.0.0.0", @allocator=0, @octets=[10, 0, 0, 0], @prefix=18, @u32=167772160>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines are long, is there another way you can illustrate the output, without using the string returned by inspect?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a common practice for tests code. Also, it's very demonstrative. I don't see issues having these lines.

# #<IPAddress::IPv4:0x0000000111e61470 @address="10.0.64.0", @allocator=0, @octets=[10, 0, 64, 0], @prefix=18, @u32=167788544>,
# #<IPAddress::IPv4:0x0000000111e60840 @address="10.0.192.0", @allocator=0, @octets=[10, 0, 192, 0], @prefix=18, @u32=167821312>]
# b.subtract(a)
# => []
# a.subtract(c)
# => [#<IPAddress::IPv4:0x0000000107f08978 @address="10.0.0.0", @allocator=0, @octets=[10, 0, 0, 0], @prefix=16, @u32=167772160>]
#
def subtract(other)
raise ArgumentError unless other.is_a? self.class
result = []
if self.include? other
split_bits = 2**(self.prefix - other.prefix)
other_alike_subnets = self.split split_bits
raise "Prefix of subnet and split doesn't match" unless other_alike_subnets.first.prefix == other.prefix

other_alike_subnets.delete other
result = other_alike_subnets
elsif !other.include? self
result << self.clone
end
result
end

#
# Returns the difference between two IP addresses
# in unsigned int 32 bits format
Expand Down
10 changes: 10 additions & 0 deletions test/ipaddress/ipv4_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,16 @@ def test_method_subnet
arr = ["172.16.10.0/24"]
assert_equal arr, @network.subnet(24).map {|s| s.to_string}
end

def test_method_subtract
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this test fairly hard to read and understand. Variable naming and maybe more than 1 test would make it better.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will use the same convention as for the sample.

a = IPAddress '10.0.0.0/16'
b = IPAddress '10.0.128.0/18'
c = IPAddress '192.168.0.0/16'

assert !a.subtract(b).include?(b)
assert b.subtract(a).empty?
assert_equal a, a.subtract(c).first
end

def test_method_supernet
assert_raises(ArgumentError) {@ip.supernet(24)}
Expand Down