DavidSchinazi / draft-cms-masque-connect-ip

Other
2 stars 1 forks source link

ATOMIC_* is too open-ended #6

Open martinthomson opened 3 years ago

martinthomson commented 3 years ago

When reviewing for adoption, I didn't read that far down. This design is a little frightening in how open-ended it is.

Given that the primary purpose of this is to provide a clean switch for routes without making the interface unusable, it might be easier to define just two messages:

  1. ROUTES - a list of routes; which clears the set of valid routes and replaces them
  2. ADD_ROUTE - (maybe) add a route to the current set of routes

None of the other messages seem to be needed.

DavidSchinazi commented 3 years ago

I think there's value in bundling the IP address assignment with the routes: it allows you to create an atomic config that an endpoint can act on all at once, for example when it creates its utun interface.

achernya commented 3 years ago

Additionally, bundling the IP address assignment with the routes also lets us create an easy signal for "this is ready to go, if you send packets you should expect them to be delivered/relayed". I think if we were to go the ROUTES/ADD_ROUTE option, we'd probably want to add an additional signal for "Ready".

martinthomson commented 3 years ago

Wouldn't it be the case that a new address is unusable until it has routes? In that case, providing an address would be expected to be functionally inoperable until routes were provided. (I would expect the two to be sent in close proximity, of course.)

Related to that use case, was there any thought given to constraining routes to a particular source address? If the proxy says that it has routes, generally that will be from a given allocated address. But in a multi-address configuration, you might get a second address and use routes that are configured for a first address and end up in trouble. Some sort of address identifier might solve that problem, but that's complexity...

achernya commented 3 years ago

There's also the case where an address assignment may not replace previous routes, or routes do not replace previous ones. So having the ATOMIC_* bundles make it easy when you have to reset everything and also lets you expose incremental updates without having to support the full cross product for every message set.

martinthomson commented 3 years ago

So have ROUTES and UPDATEROUTES (with the same data model). You are creating something with ATOMIC* that is vastly more complex than any of your use cases justifies.

DavidSchinazi commented 3 years ago

ATOMIC_* might be more general-purpose than we need, but it certainly isn't complex. It seems pretty straightforward to reason about and implement to me?