feat: remove derp regions in favor of direct urls #1831
Conversation
There was a problem hiding this comment.
I'm a bit surprised at the interpretation of a derp URL here. If I understand things correctly this currently uses the URL to contact the DERP server, so it only ever uses the hostname part of the URL. I guess I was expecting something more like a defined URL format:
https://hostname_or_ip?stun_only=bool&stun_port=u16
In that case the DerpNode would store a hostname_or_ip field and have a ::from_url() constructor.
Currently it seems like the URL, which could carry all sorts of metadata, is only used as a hostname and all the other fields of it's type are ignored.
I also wonder if a lot of the code that currently takes a region_id, or in this PR derp_url, should take an Arc<DerpNode> instead. Though that's probably a question of only secondary importance.
dignifiedquire
force-pushed
the
feat-no-derp-region
branch
from
ac8ae5b
to
a809706
Compare
dignifiedquire
force-pushed
the
feat-no-derp-region
branch
from
a321743
to
9766e2a
Compare
|
Authentication tokens not sent around in tickets but used to authenticate with your DERP server can be part of the URL later as well, it's nice that it is extensible (see TURN REST API and https://github.com/coturn/coturn README). |
|
I'm rather stuck at what the problem with the pingers is, the relevant log is this: A few things:
@Arqu any change the runner environment has changed? the permissions it has to ping on a raw socket? |
|
oh, and the nighly rust job succeeds. It is being run on the same runner. |
dignifiedquire
changed the title
|
/netsim branch arqu/derp_drop_region |
| region_id = 1 | ||
| host_name = "foo.bar" | ||
| [[derp_nodes]] | ||
| url = "https://foo.bar" |
There was a problem hiding this comment.
Should we make a follow-up issue to be able to set the stun_port in the URL?
There was a problem hiding this comment.
the url sets the derp port already, stun port is a seperate port
There was a problem hiding this comment.
yes, that is what I mean. Currently if the derp server is configured with a non-default STUN port you'd have to manually write the config. We could include the stun_port as a query-parameter on the URL and thus you'd only need to URL to use the derp server.
| fn send_addr_to_vec(addr: &SendAddr) -> Vec<u8> { | ||
| match addr { | ||
| SendAddr::Derp(url) => { | ||
| let mut out = vec![1u8]; |
There was a problem hiding this comment.
maybe this is micro-optimising, but can't this get the vector size exactly right by creating the byteified URL up-front?
| out | ||
| } | ||
| SendAddr::Udp(ip) => { | ||
| let mut out = vec![0u8]; |
There was a problem hiding this comment.
this can also know the vec size up-front
| /// Nearest DERP region ID; 0 means none/unknown. | ||
| my_derp: AtomicU16, | ||
| /// Nearest DERP node ID; 0 means none/unknown. | ||
| my_derp: std::sync::RwLock<Option<Url>>, |
There was a problem hiding this comment.
URL is actually a pretty complex thing. Could we make this an Arc?
also, do we really need a full URL or just a hostname?
| // We will fall back to sending ICMP pings. These should succeed when we have a | ||
| // working pinger. | ||
| icmpv4: have_pinger, | ||
| icmpv4, |
There was a problem hiding this comment.
there is, a couple of lines above
| @@ -992,16 +983,20 @@ mod tests { | |||
|
|
|||
| #[tokio::test(flavor = "current_thread", start_paused = true)] | |||
| async fn test_add_report_history_set_preferred_derp() -> Result<()> { | |||
| fn derp_url(i: u16) -> Url { | |||
| format!("http://{i}.com").parse().unwrap() | |||
| } | |||
There was a problem hiding this comment.
I guess this could have been done as const DERP_URL_1 = format!(...) but it doesn't matter much.
| let delay = DEFAULT_INITIAL_RETRANSMIT * attempt as u32; | ||
|
|
||
| if if_state.have_v4 && derp_node.ipv4.is_enabled() { | ||
| if if_state.have_v4 { |
There was a problem hiding this comment.
I wonder how relevant this is? I guess you could configure the Node to be only IPv4 or IPv6 in case you have some network trouble with one of them. But maybe we just won't need this.
I don't think it would be a very hard thing to add tough. We could add an ipv4_enabled and ipv6_enabled field to DerpNode, and allow getting that info from a query parameter in the URL. Is this worth creating an issue for to keep track of and do as a follow-up?
rklaehn
force-pushed
the
feat-no-derp-region
branch
from
0dd0746
to
bd72597
Compare
|
/netsim branch feat-no-derp-url |
|
|
dignifiedquire
force-pushed
the
feat-no-derp-region
branch
6 times, most recently
from
eb8b8a9
to
d874af0
Compare
|
/netsim branch feat-no-derp-url |
derp servers are only addressed by url now
We used to only do this if the derp server was configured by IP address. Now it is only configured by URL which is always going to be a DNS name. We need to handle this as well.
Co-authored-by: Kasey <kasey@n0.computer>
Co-authored-by: Kasey <kasey@n0.computer>
Co-authored-by: Kasey <kasey@n0.computer>
This reverts commit 4b0ad76.
dignifiedquire
force-pushed
the
feat-no-derp-region
branch
from
51280b0
to
9cdfabf
Compare
Breaking Change: This change removes the notion of derp regions, and simplifies it to derpers, which are identified by their URL. Currently the URL is considered fully, including all path and query params. This might change later depending on usage. - [x] Needs n0-computer/chuck#49 --------- Co-authored-by: Floris Bruynooghe <flub@n0.computer> Co-authored-by: Kasey <kasey@n0.computer> Co-authored-by: Ruediger Klaehn <rklaehn@protonmail.com>
Breaking Change: This change removes the notion of derp regions, and simplifies it to derpers, which are identified by their URL.
Currently the URL is considered fully, including all path and query params. This might change later depending on usage.