internal-dns: return additional records for srv record targets (#3785)

It's not required but recommended for a DNS server to return the address
record(s) in the "Additional" section when a responding to a lookup for
a `SRV` records. This saves the client from doing additional lookups.

Unfortunately, we still do need to do the explicit lookup on targets due
to deficiencies in `trust-dns`.

Fixes #3434

Before:
```
$ dig @fd00:1122:3344:1::1 SRV _cockroach._tcp.control-plane.oxide.internal

; <<>> DiG 9.18.14 <<>> @fd00:1122:3344:1::1 SRV _cockroach._tcp.control-plane.oxide.internal
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 13647
;; flags: qr rd; QUERY: 1, ANSWER: 5, AUTHORITY: 0, ADDITIONAL: 0
;; WARNING: recursion requested but not available

;; QUESTION SECTION:
;_cockroach._tcp.control-plane.oxide.internal. IN SRV

;; ANSWER SECTION:
_cockroach._tcp.control-plane.oxide.internal. 0 IN SRV 0 0 32221 65dd2374-f712-4496-b0ff-11a599fa0c2c.host.control-plane.oxide.internal.
_cockroach._tcp.control-plane.oxide.internal. 0 IN SRV 0 0 32221 b512e7a3-b115-4150-b220-3bf095118cb5.host.control-plane.oxide.internal.
_cockroach._tcp.control-plane.oxide.internal. 0 IN SRV 0 0 32221 b9b20bbf-d0c1-474d-9605-56b2b23a4b5f.host.control-plane.oxide.internal.
_cockroach._tcp.control-plane.oxide.internal. 0 IN SRV 0 0 32221 bedb84f6-3d51-4b21-931a-0b02b326e75a.host.control-plane.oxide.internal.
_cockroach._tcp.control-plane.oxide.internal. 0 IN SRV 0 0 32221 c7d408e5-b1c9-42ea-862f-ce2639a56978.host.control-plane.oxide.internal.

;; Query time: 17 msec
;; SERVER: fd00:1122:3344:1::1#53(fd00:1122:3344:1::1) (UDP)
;; WHEN: Fri Jul 28 00:28:33 UTC 2023
;; MSG SIZE  rcvd: 512
```

After:
```
$ dig @fd00:1122:3344:1::1 SRV _cockroach._tcp.control-plane.oxide.internal

; <<>> DiG 9.18.14 <<>> @fd00:1122:3344:1::1 SRV _cockroach._tcp.control-plane.oxide.internal
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 44496
;; flags: qr rd; QUERY: 1, ANSWER: 5, AUTHORITY: 0, ADDITIONAL: 5
;; WARNING: recursion requested but not available

;; QUESTION SECTION:
;_cockroach._tcp.control-plane.oxide.internal. IN SRV

;; ANSWER SECTION:
_cockroach._tcp.control-plane.oxide.internal. 0 IN SRV 0 0 32221 148c38a0-294e-436a-8ba0-e3a0eb166162.host.control-plane.oxide.internal.
_cockroach._tcp.control-plane.oxide.internal. 0 IN SRV 0 0 32221 6afab2d4-ac54-4bfb-96a8-fbbeabb75247.host.control-plane.oxide.internal.
_cockroach._tcp.control-plane.oxide.internal. 0 IN SRV 0 0 32221 7d07596f-d69e-4335-b0eb-d27fbe8d092f.host.control-plane.oxide.internal.
_cockroach._tcp.control-plane.oxide.internal. 0 IN SRV 0 0 32221 9f339f78-4b69-46a9-9f16-8239152a413a.host.control-plane.oxide.internal.
_cockroach._tcp.control-plane.oxide.internal. 0 IN SRV 0 0 32221 a752818c-c324-4c73-a5bb-a4366ea05ec0.host.control-plane.oxide.internal.

;; ADDITIONAL SECTION:
148c38a0-294e-436a-8ba0-e3a0eb166162.host.control-plane.oxide.internal. 0 IN AAAA fd00:1122:3344:101::5
6afab2d4-ac54-4bfb-96a8-fbbeabb75247.host.control-plane.oxide.internal. 0 IN AAAA fd00:1122:3344:101::3
7d07596f-d69e-4335-b0eb-d27fbe8d092f.host.control-plane.oxide.internal. 0 IN AAAA fd00:1122:3344:101::6
9f339f78-4b69-46a9-9f16-8239152a413a.host.control-plane.oxide.internal. 0 IN AAAA fd00:1122:3344:101::4
a752818c-c324-4c73-a5bb-a4366ea05ec0.host.control-plane.oxide.internal. 0 IN AAAA fd00:1122:3344:101::7

;; Query time: 4 msec
;; SERVER: fd00:1122:3344:1::1#53(fd00:1122:3344:1::1) (UDP)
;; WHEN: Thu Jul 27 16:39:20 PDT 2023
;; MSG SIZE  rcvd: 652
```

Author: luqmana

dns-server/src/dns_server.rs

@@ -213,6 +213,49 @@ impl From<QueryError> for RequestError {
     }
 }
 
+fn dns_record_to_record(
+    name: &Name,
+    record: DnsRecord,
+) -> Result<Record, RequestError> {
+    match record {
+        DnsRecord::A(addr) => {
+            let mut a = Record::new();
+            a.set_name(name.clone())
+                .set_rr_type(RecordType::A)
+                .set_data(Some(RData::A(addr)));
+            Ok(a)
+        }
+
+        DnsRecord::AAAA(addr) => {
+            let mut aaaa = Record::new();
+            aaaa.set_name(name.clone())
+                .set_rr_type(RecordType::AAAA)
+                .set_data(Some(RData::AAAA(addr)));
+            Ok(aaaa)
+        }
+
+        DnsRecord::SRV(crate::dns_types::SRV {
+            prio,
+            weight,
+            port,
+            target,
+        }) => {
+            let tgt = Name::from_str(&target).map_err(|error| {
+                RequestError::ServFail(anyhow!(
+                    "serialization failed due to bad SRV target {:?}: {:#}",
+                    &target,
+                    error
+                ))
+            })?;
+            let mut srv = Record::new();
+            srv.set_name(name.clone())
+                .set_rr_type(RecordType::SRV)
+                .set_data(Some(RData::SRV(SRV::new(prio, weight, port, tgt))));
+            Ok(srv)
+        }
+    }
+}
+
 /// Handle a well-formed, decoded DNS query
 async fn handle_dns_message(
     request: &Request,
@@ -227,55 +270,70 @@ async fn handle_dns_message(
     let name = query.original().name().clone();
     let records = store.query(mr)?;
     let rb = MessageResponseBuilder::from_message_request(mr);
+    let mut additional_records = vec![];
     let response_records = records
         .into_iter()
-        .map(|record| match record {
-            DnsRecord::A(addr) => {
-                let mut a = Record::new();
-                a.set_name(name.clone())
-                    .set_rr_type(RecordType::A)
-                    .set_data(Some(RData::A(addr)));
-                Ok(a)
-            }
-
-            DnsRecord::AAAA(addr) => {
-                let mut aaaa = Record::new();
-                aaaa.set_name(name.clone())
-                    .set_rr_type(RecordType::AAAA)
-                    .set_data(Some(RData::AAAA(addr)));
-                Ok(aaaa)
-            }
-
-            DnsRecord::SRV(crate::dns_types::SRV {
-                prio,
-                weight,
-                port,
-                target,
-            }) => {
-                let tgt = Name::from_str(&target).map_err(|error| {
-                    RequestError::ServFail(anyhow!(
-                        "serialization failed due to bad SRV target {:?}: {:#}",
-                        &target,
-                        error
-                    ))
-                })?;
-                let mut srv = Record::new();
-                srv.set_name(name.clone())
-                    .set_rr_type(RecordType::SRV)
-                    .set_data(Some(RData::SRV(SRV::new(
-                        prio, weight, port, tgt,
-                    ))));
-                Ok(srv)
+        .map(|record| {
+            let record = dns_record_to_record(&name, record)?;
+
+            // DNS allows for the server to return additional records
+            // that weren't explicitly asked for by the client but that
+            // the server expects the client will want. The records
+            // corresponding to a lookup on a SRV target is one such case.
+            // We opportunistically attempt to resolve the target here
+            // and if successful return those additional records in the
+            // response.
+            // NOTE: we only do this one-layer deep.
+            if let Some(RData::SRV(srv)) = record.data() {
+                let target_records =
+                    store.query_name(srv.target()).map(|records| {
+                        records
+                            .into_iter()
+                            .map(|record| {
+                                dns_record_to_record(srv.target(), record)
+                            })
+                            .collect::<Result<Vec<_>, _>>()
+                    });
+                match target_records {
+                    Ok(Ok(target_records)) => {
+                        additional_records.extend(target_records);
+                    }
+                    // Don't bail out if we failed to lookup or
+                    // handle the response as the original request
+                    // did succeed and we only care to do this on
+                    // a best-effort basis.
+                    Err(error) => {
+                        slog::warn!(
+                            &log,
+                            "SRV target lookup failed";
+                            "original_mr" => #?mr,
+                            "target" => ?srv.target(),
+                            "error" => ?error,
+                        );
+                    }
+                    Ok(Err(error)) => {
+                        slog::warn!(
+                            &log,
+                            "SRV target unexpected response";
+                            "original_mr" => #?mr,
+                            "target" => ?srv.target(),
+                            "error" => ?error,
+                        );
+                    }
+                }
             }
+            Ok(record)
         })
         .collect::<Result<Vec<_>, RequestError>>()?;
     debug!(
         &log,
         "dns response";
         "query" => ?query,
-        "records" => ?&response_records
+        "records" => ?&response_records,
+        "additional_records" => ?&additional_records,
     );
-    respond_records(request, rb, header, &response_records).await
+    respond_records(request, rb, header, &response_records, &additional_records)
+        .await
 }
 
 /// Respond to a DNS query with the given set of DNS records
@@ -284,13 +342,14 @@ async fn respond_records(
     rb: MessageResponseBuilder<'_>,
     header: Header,
     response_records: &[Record],
+    additional_records: &[Record],
 ) -> Result<(), RequestError> {
     let mresp = rb.build(
         header,
         response_records.iter().collect::<Vec<&Record>>(),
         vec![],
         vec![],
-        vec![],
+        additional_records,
     );
 
     encode_and_send(&request, mresp, "records").await.map_err(|error| {

dns-server/src/storage.rs

@@ -590,10 +590,20 @@ impl Store {
     ) -> Result<Vec<DnsRecord>, QueryError> {
         let name = mr.query().name();
         let orig_name = mr.query().original().name();
-        self.query_name(name, orig_name)
+        self.query_raw(name, orig_name)
     }
 
-    fn query_name(
+    /// Returns a non-empty list of DNS records associated with the given name.
+    ///
+    /// If the returned set would have been empty, returns `QueryError::NoName`.
+    pub(crate) fn query_name(
+        &self,
+        name: &Name,
+    ) -> Result<Vec<DnsRecord>, QueryError> {
+        self.query_raw(&LowerName::new(name), name)
+    }
+
+    fn query_raw(
         &self,
         name: &LowerName,
         orig_name: &Name,
@@ -843,7 +853,7 @@ mod test {
     fn expect(store: &Store, name: &str, expect: Expect<'_>) {
         let dns_name_orig = Name::from_str(name).expect("bad DNS name");
         let dns_name_lower = LowerName::from(dns_name_orig.clone());
-        let result = store.query_name(&dns_name_lower, &dns_name_orig);
+        let result = store.query_raw(&dns_name_lower, &dns_name_orig);
         println!(
             "expecting {:?} for query of {:?}: {:?}",
             expect, name, result

dns-server/tests/basic_test.rs

@@ -10,7 +10,11 @@ use dns_service_client::{
 use dropshot::{test_util::LogContext, HandlerTaskMode};
 use omicron_test_utils::dev::test_setup_log;
 use slog::o;
-use std::{collections::HashMap, net::Ipv4Addr, net::Ipv6Addr};
+use std::{
+    collections::HashMap,
+    net::Ipv6Addr,
+    net::{IpAddr, Ipv4Addr},
+};
 use trust_dns_resolver::error::ResolveErrorKind;
 use trust_dns_resolver::TokioAsyncResolver;
 use trust_dns_resolver::{
@@ -92,8 +96,13 @@ pub async fn srv_crud() -> Result<(), anyhow::Error> {
 
     // add a srv record
     let name = "hromi".to_string();
-    let srv =
-        Srv { prio: 47, weight: 74, port: 99, target: "outpost47".into() };
+    let target = "outpost47";
+    let srv = Srv {
+        prio: 47,
+        weight: 74,
+        port: 99,
+        target: format!("{target}.{TEST_ZONE}"),
+    };
     let rec = DnsRecord::Srv(srv.clone());
     let input_records = HashMap::from([(name.clone(), vec![rec])]);
     dns_records_create(client, TEST_ZONE, input_records.clone()).await?;
@@ -102,13 +111,25 @@ pub async fn srv_crud() -> Result<(), anyhow::Error> {
     let records = dns_records_list(client, TEST_ZONE).await?;
     assert_eq!(records, input_records);
 
+    // add some aaaa records corresponding to the srv target
+    let addr1 = Ipv6Addr::new(0xfd, 0, 0, 0, 0, 0, 0, 0x1);
+    let addr2 = Ipv6Addr::new(0xfd, 0, 0, 0, 0, 0, 0, 0x2);
+    let input_records = HashMap::from([(
+        target.to_string(),
+        vec![DnsRecord::Aaaa(addr1), DnsRecord::Aaaa(addr2)],
+    )]);
+    dns_records_create(client, TEST_ZONE, input_records.clone()).await?;
+
     // resolve the srv
     let response = resolver.srv_lookup(name + "." + TEST_ZONE + ".").await?;
-    let srvr = response.iter().next().expect("no addresses returned!");
+    let srvr = response.iter().next().expect("no srv records returned!");
     assert_eq!(srvr.priority(), srv.prio);
     assert_eq!(srvr.weight(), srv.weight);
     assert_eq!(srvr.port(), srv.port);
     assert_eq!(srvr.target().to_string(), srv.target + ".");
+    let mut aaaa_records = response.ip_iter().collect::<Vec<_>>();
+    aaaa_records.sort();
+    assert_eq!(aaaa_records, [IpAddr::from(addr1), IpAddr::from(addr2)]);
 
     test_ctx.cleanup().await;
     Ok(())

internal-dns/src/config.rs

@@ -68,7 +68,7 @@ use std::net::Ipv6Addr;
 use uuid::Uuid;
 
 /// Zones that can be referenced within the internal DNS system.
-#[derive(Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)]
 pub enum ZoneVariant {
     /// This non-global zone runs an instance of Dendrite.
     ///
@@ -164,6 +164,12 @@ pub struct Zone {
     variant: ZoneVariant,
 }
 
+impl Zone {
+    pub(crate) fn dns_name(&self) -> String {
+        Host::Zone { id: self.id, variant: self.variant }.dns_name()
+    }
+}
+
 impl DnsConfigBuilder {
     pub fn new() -> Self {
         DnsConfigBuilder {
@@ -342,9 +348,7 @@ impl DnsConfigBuilder {
 
         // Assemble the set of AAAA records for zones.
         let zone_records = self.zones.into_iter().map(|(zone, zone_ip)| {
-            let name =
-                Host::Zone { id: zone.id, variant: zone.variant }.dns_name();
-            (name, vec![DnsRecord::Aaaa(zone_ip)])
+            (zone.dns_name(), vec![DnsRecord::Aaaa(zone_ip)])
         });
 
         // Assemble the set of SRV records, which implicitly point back at
@@ -359,15 +363,7 @@ impl DnsConfigBuilder {
                             prio: 0,
                             weight: 0,
                             port,
-                            target: format!(
-                                "{}.{}",
-                                Host::Zone {
-                                    id: zone.id,
-                                    variant: zone.variant
-                                }
-                                .dns_name(),
-                                DNS_ZONE
-                            ),
+                            target: format!("{}.{}", zone.dns_name(), DNS_ZONE),
                         })
                     })
                     .collect();