[internal-dns] Add utilities in the client library #1177
Conversation
smklein
changed the title
smklein
changed the title
|
|
||
| /// Sets a records on all DNS servers. | ||
| /// | ||
| /// Returns an error if setting the record fails on any server. |
There was a problem hiding this comment.
I think it's pretty likely we'll want to change this behavior to cope with one or more of the DNS servers failing.
However, I really want our behavior to be "fail loud" until we have a good way of coping. If one of the DNS servers starts failing, ideally Nexus could:
- Notice when it is actually setting the records, pick out which server failed
- If it continues to not respond (or throw errors), mark the service as "deprecated", re-establish service redundancy elsewhere
Until then though, if an internal DNS server fails, I want high-visibility on the error. Hopefully this tradeoff makes sense?
There was a problem hiding this comment.
Also of note: This is exclusively on the "setting DNS records" side of things. The "getting DNS records" side should be able to cope with a server going offline already.
There was a problem hiding this comment.
I generally agree about failing loud, but it does seem slightly at odds with the availability-first ethos of DNS. This is probably good for now though, without more information about how the client-side will work.
| /// This method is most efficient when records are sorted by SRV key. | ||
| pub async fn insert_dns_records( | ||
| &self, | ||
| records: &Vec<impl Service>, |
There was a problem hiding this comment.
I'm not sure exactly how to express this, but I can't help but feeling this should be a map. In the case where records is sorted by SRV, then that's effectively what we're doing inside by coalescing all AAAA records and addresses. A map might be frustrating for consumers, though, since it seems like we'll want to impl Service for some types (which returns exactly one AAAA and address) and pass one or more of them.
This isn't a correctness issue, I think, so deferring it is fine. I think the while let loop on L100 could use a quick comment that we are in fact coalescing, mostly for efficiency in terms of actually inserting the KV records. (I think...)
There was a problem hiding this comment.
I can do a &HashMap<SRV, Vec<(AAAA, SocketAddr)>> - that seems to accomplish what you're describing.
Although I do think this is slightly more burden on consumers, I also think it's more clear on both sides. I've updated to use this format (one SRV, many AAAA/addrs).
| /// A wrapper around a DNS resolver, providing a way to conveniently | ||
| /// look up IP addresses of services based on their SRV keys. | ||
| pub struct Resolver { | ||
| inner: Box<TokioAsyncResolver>, |
There was a problem hiding this comment.
What's the motivation for boxing this?
There was a problem hiding this comment.
I'm using it in subsequent patches in an enum (you can provide an IP from a config, or use a resolver), and Clippy complained that it was too large (500+ bytes). Since it's internal to the struct, it shouldn't matter for users of this API.
| .dns_records_delete(&vec![DnsRecordKey { | ||
| name: records[0].aaaa.to_string(), | ||
| }]) |
There was a problem hiding this comment.
Just curious about the API here, but both records[0] and records[1] could be deleted simultaneously, right?
There was a problem hiding this comment.
Yup; might as well show that. Call updated.
Adds utilities in the internal DNS client library to:
This PR has been pulled out of #1216 , where it is used in both Nexus and RSS