Add NetFromRange and NetFromInterval helpers

[tacerus]

When retrieving set elements it can be desired to format the result in the same way nft would, which is merging intervals to CIDR representations. To make this easier, introduce helper functions which allow for conversion of IP address ranges to CIDR networks.

#346

[tacerus]

When using this for the case described in #346, it also needs to be considered that nft will not print a CIDR mask if it is 32 / 128 - I'm not sure this should be considered in the functions introduced here (which I think would require having the functions return something other than net.IPNets) or in later logic.

Edit: I am thinking to add another function like FormatIpNet which takes an IPNet as input, and returns a string (either with or without the mask, depending on how nft would treat it).

[nickgarlis]

What happens if there is no previous IP ?

[tacerus]

From my understanding it will just return the address itself, for example calling Prev() on 0.0.0.0, will yield 0.0.0.0.

[nickgarlis]

This is the IntervalOpen scenario. Consider a CIDR like 128.0.0.0/1. In that instance, the user would have to call NetIntervalRange by setting last into an empty IP or nil.

I'd suggest an empty IP instead of a nil value for consistency with NetInterval but I have no strong opinions about this.

[tacerus]

I see - should I just pass both input values to NetFromRange as is if last is empty?

[nickgarlis]

I am not sure. NetFromRange is a bit different since it never expects a null or otherwise invalid IP, so returning an error there is reasonable.

NetFromInterval, however, is a different situation. An empty last value doesn’t inherently mean the interval is invalid, so treating it as an error wouldn’t be correct. That’s why I’m leaning toward allowing an empty IP to represent the open end of the interval.

[nickgarlis]

So what I would do here is, if last is null or empty, set previous to max IP. Otherwise, calculate the previous IP like you're doing. Perhaps it'd be good to have a test case.

[tacerus]

I implemented this now, however I unfortunately don't understand why it should be the maximum address.

Edit: if the added logic is correct, what would be an IPv6 example to add to the tests?

[nickgarlis]

If you have an interval with no explicit last element and the IntervalOpen flag is set, you’d end up passing something like (first: 128.0.0.0, last: 0.0.0.0). In this case, you want to interpret it as the full range rather than a half-open one, i.e. (first: 128.0.0.0, last: 255.255.255.255), which can be represented exactly as the CIDR 128.0.0.0/1.

For IPv6, the upper half range would be 8000::/1 (8000::, ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff).

[tacerus]

Thanks for the explanation!

[nickgarlis]

Shoudn't 0.0.0.0/0 be (0.0.0.0 - 255.255.255.255) ?

[tacerus]

Good point, and the test passes with 0.0.0.0 - 255.255.255.255 => 0.0.0.0/0 as well. But I guess it is right that 0.0.0.1 - 255.255.255.254 yields 0.0.0.0/0 too?

[nickgarlis]

My understanding is that 0.0.0.1 - 255.255.255.254 can only fit in 0.0.0.0/0 but this shouldn't mean that 0.0.0.0/0 is the exact CIDR for this range. Perhaps your implementation tries to find the best CIDR for the range instead ?

I checked to see how nft chooses to represent this range and it seems to print it as is and not as a CIDR.

You can test by adding these set elements

c.AddSet(set, []nftables.SetElement{
	{Key: net.ParseIP("0.0.0.1").To4()},
	{Key: net.ParseIP("255.255.255.255").To4(), IntervalEnd: true},
})

which produces

set my_set {
	type ipv4_addr
	flags interval
	elements = { 0.0.0.1-255.255.255.254 }
}

[tacerus]

Good catch, I did not consider this. Should we process ranges which do not match an exact CIDR here at all? I think it's not an error if it does not match, but I understand the "nearest" match is not quite expected either - what do you think of returning nil for these?

(Then some later additional logic can decide to print first and last as is, depending on the use case.)

[nickgarlis]

Should we process ranges which do not match an exact CIDR here at all?

I think that those ranges should not return a CIDR.

An option could be to return an ok variable. Something like this.

cidr, ok := nftables.NetFromInterval(first, last)

An error would also be fine I guess but then it'd have to be typed so that it can be handled.

[tacerus]

Added this now and expanded the tests.

I still have it return the nearest network though, do you prefer to have it not return any network if ok is false anyways?

Another point, for the end address comparison I copied the logic from the EndIP() function of another module (https://github.com/3th1nk/cidr/blob/v0.3.0/cidr.go#L154), is that an issue license wise? Unfortunately I'm not sure how to rewrite it.

[nickgarlis]

I still have it return the nearest network though, do you prefer to have it not return any network if ok is false anyways?

Yes, I think i'd make more sense for it to be the exact network or nothing.

When it comes to the linked library, it seems to be MIT so it should be fine but I'd ask @stapelberg.

[tacerus]

OK, I adjusted it to return nil if no exact match is found.