Additional options supported by the native API: "Add (Incremental)" "Remove", "Update" and "Replace Allowed IPs" vs adding new allowed ips

[issackelly]

I've been maintaining a private fork for a couple small tweaks but I am about to add new functionality and I wanted to check in to see if you have any desires for contributions or additional behavior.

At the moment there is only set_config -- which limits the things you can do compared to the native API.

Setting the config currently sets REPLACE_PEERS on every change. I expect to add at least Add/Remove incrementally but the other interfaces could probably be addressed at the same time.

[TroyNeubauer]

Yes this sounds like an excellent addition!

[issackelly]

Cool. My first pass does this:

1. Add these three fields to SetPeer

    /// These three flags are used to determine what to do with the peer
    pub remove_peer: bool,
    pub update_peer: bool,
    pub replace_allowed_ips: bool,

I changed the function signature here:

      /// Sets the wireguard configuration of this adapter
 -   pub fn set_config(&self, config: &SetInterface) -> Result<()> {
 +   pub fn set_config(&self, config: &SetInterface, replace_all_peers: bool) -> Result<()> {

and then I'll update the flags below.

I don't really like this yet. I think a better API might not change the current behavior of set_config but instead have a second method like transact_set_interface, with the interface I changed here that takes both set_config (old interface that calls transact_set_interface (replace=true) andupdate_config` (replace=false). I think we should probably do this to make the dramatic behavior differences pretty obvious and to keep your current API the same.

Additionally, If we wanted it to be very rust-y and totally re-intepret the actions that actually happen into more prose: then an enum might be better.

pub enum SetPeer {
  Upsert(SetPeer),
  Remove(SetPeer)
  UpdateIfAlreadyExists(SetPeer)
}

I think that might be better than bools on SetPeer, especially because you can currently set both Update and Remove but the C programmer in me knows that it's all lies under the surface.

I could go either way on the second change.

What would you like to see here?

[TroyNeubauer]

Overall, my goals here have been:

1. Wrap the interfaces in a way that feel authentic to the c functions (a c wireguard-nt programmer should feel at home)
2. Leverage rust type system to make things less error prone and more ergonomic

I'm not against making some breaking changes here to model the underlying c api better.

This library picking replace_all_peers = true was a bit of an oversight, so I actually perfer making some breaking changes to clean this up and reduce API fragmentation!

So Im down for the following:

pub fn set_config(&self, config: &SetInterface, replace_all_peers: bool) -> Result<()>

pub enum SetPeer {
  Upsert(SetPeerInner),
  Remove(SetPeerInner),
  UpdateIfAlreadyExists(SetPeerInner)
}

We'll just have to be careful to callout the breakage in CHANGELOG.md, along with a blessed diff they can use to get the same behavior if they dont care about this new configurability.

Not sure what to call the inner SetPeer struct, but we can bikeshed this later.