Add method to subtract IPv4 subnets #129
Conversation
|
Interesting! Should a.substract(c) just return a, or raise an error? |
|
Also, "substract" or "subtract"? |
I think that it should follow set subtraction logic: if sets a and c doesn't intersect, a - c = a. |
skorobogatydmitry
force-pushed
the
sd/ipv4-substract
branch
from
8394c5f to
0ed9b8b
Compare
|
@bluemonk , can you look at this PR by chance ? |
There was a problem hiding this comment.
Thanks for taking time submitting a PR.
I'm not comfortable merging this in the current state, for a few reasons:
- I think the broader use-case (beyond GCP) need to be explained a bit better.
- The code (method naming, comments, tests) are lacking some polish.
- How about IPv6? I'm thinking that it would be nice for this library to strive towards parity between IPv6 and IPv4 for most methods (where it makes sense).
- Range vs. IPaddress; if this only operate on ranges, should that be validated? Should that be reflected in the method name? What would happen when this is used on a non-range?
Bluemonk is the owner though, he may disagree with me on the above. These are just my thoughts.
lib/ipaddress/ipv4.rb
Outdated
| # | ||
| # a = IPAddress '10.0.0.0/16' | ||
| # b = IPAddress '10.0.128.0/18' | ||
| # c = IPAddress '192.168.0.0/16' |
There was a problem hiding this comment.
Can we come up with better names than a, b, c?
There was a problem hiding this comment.
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>, |
There was a problem hiding this comment.
These lines are long, is there another way you can illustrate the output, without using the string returned by inspect?
There was a problem hiding this comment.
That's a common practice for tests code. Also, it's very demonstrative. I don't see issues having these lines.
| @@ -805,6 +805,42 @@ def subnet(subprefix) | |||
| end | |||
| end | |||
|
|
|||
| # | |||
| # Subtract other from the current range | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
test/ipaddress/ipv4_test.rb
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
I find this test fairly hard to read and understand. Variable naming and maybe more than 1 test would make it better.
There was a problem hiding this comment.
I will use the same convention as for the sample.
What do you mean by GCP ? All examples are cloud-agnostic. I can put ranges like
I tried to follow whatever is in repo. I see lots of spaces, redundant
I am not familiar with how IPv6 addresses work. I can put a stub with NotImplementedException, if you think it's needed. |
skorobogatydmitry
changed the title
Subnets substraction is useful to e.g. calculate GCP internal API address pool using this guide.