Consider allowing Cidr construction from addresses where host part is not zero
Closed this issue · 12 comments
I'm getting an address from a list of interfaces and then need to construct a CIDR from one of those host addresses and a specific network length.
However, this library seems to always want to return host part of address was not zero, and since bitstring is private I can't clip(24) on an Ipv4Inet object to force the address down to 24 bits, for example.
Is there a way to pass an address to the Cidr constructor to ignore the host bits?
Hi.
You should be able to construct an Ipv4Inet object and then use Inet::network to get the corresponding Ipv4Cidr.
To move from an Ipv4Inet to one with a different network length (e.g. to get the Ipv4Cidr for it) you need to construct a new Ipv4Inet with the Inet::address of the old one and the new network length.
(All that works with IPv6 too of course.)
Hope that helps :)
cheers,
Stefan
Would you consider relaxing this check and instead having the type fixup the host part to be zero, or otherwise providing an alternative constructor to allow this more relaxed construction?
Alternatively, could the error message be made more actionable? For example, it already knows that for 10.0.0.1/24 the user probably should be using 10.0.0.0/24. It would be great if the error message actually said that, as the current "host part is not zero" is hard for the lay-person to understand and know how to resolve without asking for assistance.
Yes, agreed. Perhaps a ::new() that performs the zeroing, and an unsafe ::new_unchecked() with documentation stating that the host portion of the CIDR must be zero. This makes a lot of sense to me, personally. I ultimately had to figure out a way to work around this.
IpCidr::new explicitly documents the requirement for the host part to be zero; I'm not gonna change that, especially because I believe in properly validating input. I actually don't want a program to ignore the host part if the input should have been a network/prefix, because it indicates I screwed something up.
And this is really the way it should be, and I think many other implementations agree with me on this. (I just checked python ipaddress.ip_network and postgres cidr).
As stated above the proper way to ignore the host part is to use IpInet::new(...).network() (network() is now availably directly on the inet types too, trait not needed anymore). The same goes for the from_str constructor (instead of new), usually used through str::parse.
I think this should only be used if you actually want to accept an "interface configuration address", and want to extract the network from it, but if you really think your program should just do it's best to guess what the user meant, this is the way to go.
(I'm also not going to add unsafe *_unchecked variants. Those should do exactly the same as the safe version, just without (some) checks, and that only makes sense if the checks are a performance problem, and there are valid scenarios where the expected assertions hold but just can't be proved through the type system. This is not relevant to this issue at all.)
I also don't see a good reason to add a Cidr::new_but_ignore_hostpart constructor variant - because it would only cover new, but not from_str/parse.
I am open to improve the documentation though; I'd appreciate feedback in which places it would be useful to show how to ignore the host part.
For clarity, in my use case, users provide CIDRs like 10.0.0.1/24 as string input to my program, which calls through to AnyIpCidr::from_str.
When that fails, I'd like the error message that is displayed to them to be instructive.
I understand that you don't want to change your interface, I'm just looking for a low effort way to provide maximum clarity; I'd rather not have to parse the CIDR myself and replicate the internal logic of the cidr library.
Would you consider expanding the InvalidHostPart variant to something like:
InvalidHostPart {
suggested: IpCidr
}
and having the Display impl of NetworkParseError display something like:
host part of address was not zero, did you mean {suggested}?
Hm. I admit I don't have a good solution for AnyIpCidr, good point.
Extending the NetworkParseError::InvalidHostPart variant sounds ok (although it would increase the size of the error a lot, the size of the Result might stay about the same); but it would mean bumping the version to 0.3.*.
@stbuehler db749ad looks perfect for my use case! Would you consider rolling that out as a release? In the meantime, I'm pointing my build to that branch of this repo
I guess I still don't fully understand the rationale here. If the host part is always 0, that means it's irrelevant information. In which case, a CIDR could just be specified by a bit count.
Further it's mostly backward compatible if the host part of the CIDR is specified to always be zero as relaxing that check shouldn't break anything except code that explicitly uses the error enum for it, right?
@wez:
@stbuehler db749ad looks perfect for my use case! Would you consider rolling that out as a release? In the meantime, I'm pointing my build to that branch of this repo
I've decided against extending the error. The other errors (e.g. std::net::AddrParseError) don't contain detailed information either, so it'd be rather inconsistent. It seems you (mostly?) tried to use this for better error messages; I think you'd better wrap the parser to show the invalid input explicitly (together with the error message).
I'll expose some parser methods (including some additional ones), so you could choose to either just ignore host-bits or have your parser check the host-bits explicitly with a pretty message by using the Inet-parser (see https://stbuehler.github.io/rustdocs/cidr/cidr/parsers/index.html for a preview).
(As a sidenote: I will probably drop accepting "127" as short for "127.0.0.0/32" and similar in 0.3.0; the parsers module should provide options for this too.)
I guess I still don't fully understand the rationale here. If the host part is always 0, that means it's irrelevant information. In which case, a CIDR could just be specified by a bit count.
Further it's mostly backward compatible if the host part of the CIDR is specified to always be zero as relaxing that check shouldn't break anything except code that explicitly uses the error enum for it, right?
A CIDR is "mathematically" a bitstring; this bitstring is a prefix of all addresses that are part of that CIDR. Which is why it is often called a "prefix". To properly ("canonically") represent it we append 0s ("host part") to make it a full address and print that together with the length of the bitstring. And I decided (similar as other projects did) to only accept canonically formatted representations, because other representations very likely indicate a mistake in the input. If you think this isn't the case in your application, feel free to use any of the mentioned alternatives.
(This also means the CIDR is more than just the bit count.)
@stbuehler Thanks, that gives a lot of choice and flexibility and works for me!