Remove ipnetwork from public API

[MarkusPettersson98]

This PR tries to address a painpoint of maintaining pfctl: Upgrading ipnetwork is considered a breaking change because ipnetwork is part of the public API.

The upcoming release will bump the ipnetwork crate anyway, so lets take the opportunity to change pfctl such that the dependency on ipnetwork just becomes an implementation detail that any library consumer doesn't have to worry about.

To solve this issue, I used the newtype pattern to expose an IpNetwork type that pfctl owns. To make the migration straightforward, I gave the new type the same name as the equivalent type in the ipnetwork crate, and I even relaxed the constructor a bit while adding more constructors for use const contexts.

TODO

• Document exactly how to migrate old pfctl code
• Consider if IpNetwork has to be exposed at all, of if we can get away by only exposing the Ip data type
• Revisit IpNetwork constructors - which one do we want / need?

---

This change is 

[dlon]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

---

src/rule/ip.rs line 48 at r1 (raw file):

impl IpNetwork {
    pub fn new(ip: impl Into<IpAddr>, prefix: u8) -> IpNetwork {
        Self::new_checked(ip.into(), prefix).unwrap()

I wonder if we really need the unchecked version. I think the constructor requiring an explicit unwrap is a bit nicer.

[pronebird]

IpNetwork seems like a very rudimental type that should change as often as std::net::IpAddr. What exactly breaks the compatibility that you need to re-invent the IpNetwork?

[MarkusPettersson98]

IpNetwork seems like a very rudimental type that should change as often as std::net::IpAddr. What exactly breaks the compatibility that you need to re-invent the IpNetwork?

By having ipnetwork::IpNetwork be part of the public API, bumping the ipnetwork crate in pfctl-rs becomes a breaking change for library consumers that additionally use & import the ipnetwork crate themselves, as they need to keep their ipnetwork dependency in sync with the ipnetwork version that pfctl uses. As you say, the actual IpNetwork type is pretty much done and will remain stable over time, so there is no legitimate case for having version upgrades of ipnetwork be a breaking change.

Alternative solutions to this problem are welcome 😊

[pronebird]

IpNetwork seems like a very rudimental type that should change as often as std::net::IpAddr. What exactly breaks the compatibility that you need to re-invent the IpNetwork?

By having ipnetwork::IpNetwork be part of the public API, bumping the ipnetwork crate in pfctl-rs becomes a breaking change for library consumers that additionally use & import the ipnetwork crate themselves, as they need to keep their ipnetwork dependency in sync with the ipnetwork version that pfctl uses. As you say, the actual IpNetwork type is pretty much done and will remain stable over time, so there is no legitimate case for having version upgrades of ipnetwork be a breaking change.

Alternative solutions to this problem are welcome 😊

The only solution that I can think of is to set the dependency to ipnetwork = "0" since very much any 0.x version should satisfy pfctl-rs needs. But that leaves me wonder what Cargo would do in a workspace setting and whether it would match the crates with client code.

[MarkusPettersson98]

For those who are interested the problem is better articulated in this RFC: https://rust-lang.github.io/rfcs/1977-public-private-dependencies.html

[pronebird]

So what's the plan, are you going with update to ipnetwork 0.21.1 or rolling out your own IpNetwork?

[MarkusPettersson98]

So what's the plan, are you going with update to ipnetwork 0.21.1 or rolling out your own IpNetwork?

There is currently no plan. I would like to revisit this sooner rather than later, but other stuff takes priority atm. Sorry:/