Report wireguard endpoint as a candidate when an endpoint override is in place

[bschwind]

Scenario: you have a machine which at one point ran innernet override-endpoint with an endpoint which maybe worked in the past, but doesn't work now. Because the endpoint is overridden, it is the endpoint peers try, along with any extra endpoint candidates reported by the peer.

What doesn't get reported to other peers is the current wireguard endpoint the innernet server sees. It used to only use that wg endpoint if the peer hadn't overridden it.

The easiest fix I can think of is to just add the endpoint the server sees to the list of NAT candidates we give peers to try.

The server logic could perhaps be simplified further - report all possible candidates we know about on the server side, and maybe allow the client to specify additional endpoints it wants to use as candidates. But for now this PR is probably sufficient?

I haven't tested yet, and it might need more tests added to our CI to cover this case.

[strohel]

Oh nice, that's very useful and very sane behavior.

One little technicality. If @mcginty happens to be around, it'd be cool if you could check if your intent with this makes good sense.

[mcginty]

@strohel I chatted with @bschwind about it, and it makes perfect sense to me! It should have been like this all along in my opinion (after candidates were added, that is).

On that note, I think there are a couple of things worth considering to make this even clearer:

• The server should really only report a list of endpoints to try, there's no need to have both an endpoint and candidates field any more is there?
• override-endpoint as a name makes this all a bit more confusing. We might even consider deprecating it in favor of allowing the user to specify an arbitrary number of additional candidates (add-candidate-endpoints?)

[bschwind]

The server should really only report a list of endpoints to try, there's no need to have both an endpoint and candidates field any more is there?

Yes, I think now there is no point in distinguishing the two. We just need to figure out the API and all that for adding/removing endpoint candidates, which I didn't want to tackle in this PR as it involves quite a bit more changes.

[strohel]

@strohel I chatted with @bschwind about it, and it makes perfect sense to me! It should have been like this all along in my opinion (after candidates were added, that is).

Ah cool!

On that note, I think there are a couple of things worth considering to make this even clearer:

• The server should really only report a list of endpoints to try, there's no need to have both an endpoint and candidates field any more is there?

Makes good sense. I guess this would be a breaking change to the server<->client protocol, so I wonder what our different version interoperability story is:

• I guess new clients should be compatible with older servers? It is harder to control client versions, people will usually install the latest version.
• Would be also nice if older clients are compatible with new servers, can be hard to udpate all of them if some are stuck wit outdated OSes etc.

We may also decide the protocol breaking change is not worth it. We can still have just a list of endpoints on Rust level on both server and client, just (de)serialize it to "first: Endpoint + rest: Vec" on the wire. (as a side-effect, it encodes the combined list is non-empty)

• override-endpoint as a name makes this all a bit more confusing. We might even consider deprecating it in favor of allowing the user to specify an arbitrary number of additional candidates (add-candidate-endpoints?)

Yup!

---

Anyways, as @bschwind says, I think this change is a good improvement as-is, let's merge after testing and release as 1.6.2.

The follow-ups can go in later.

[bschwind]

Haven't merged this yet as I'd like to add some CI tests, will hopefully get to that soon.

[bschwind]

@mcginty @strohel I added more tests for this guy, it could use another look.

[strohel]

@bschwind thanks for the tests! LGTM++

[mcginty]

DOUBLE APPROVE