dns: fix search lookup (istio#1099)

* dns: fix search lookup

Fixes istio#1074 (issue was incorrect
basically).

Before our search domain was always ztunnel's (istio-system). This is incorrect; we
should know the search domain of the request.

We don't know the per-pod custom settings
(istio#555), but we can use the
correct defaults.

* Fix unwrap

(cherry picked from commit 2e41538)

src/dns/server.rs

@@ -25,7 +25,7 @@ use hickory_proto::error::ProtoErrorKind;
 use hickory_proto::op::ResponseCode;
 use hickory_proto::rr::rdata::{A, AAAA, CNAME};
 use hickory_proto::rr::{Name, RData, Record, RecordType};
-use hickory_resolver::config::ResolverConfig;
+use hickory_resolver::config::{NameServerConfig, ResolverConfig, ResolverOpts};
 use hickory_resolver::system_conf::read_system_conf;
 use hickory_server::authority::LookupError;
 use hickory_server::server::Request;
@@ -324,6 +324,10 @@ impl Store {
 
         // Iterate over all possible aliases for the requested hostname from the perspective of
         // the client (e.g. <svc>, <svc>.<ns>, <svc>.<ns>.svc, <svc>.<ns>.svc.cluster.local).
+        trace!(
+            "checking aliases {:#?}",
+            self.get_aliases(client, requested_name)
+        );
         for alias in self.get_aliases(client, requested_name) {
             // For each alias, try matching against all possible wildcards.
             //
@@ -333,6 +337,10 @@ impl Store {
             //   'www.example.com' (the alias, itself)
             //   '*.example.com'
             //   '*.com'
+            trace!(
+                "checking against wildcards {:#?}",
+                get_wildcards(&alias.name)
+            );
             for mut search_name in get_wildcards(&alias.name) {
                 // Convert the name to a string for lookup, removing the trailing '.'.
                 search_name.set_fqdn(false);
@@ -353,7 +361,8 @@ impl Store {
                     // its better than *ALL* ServiceEntry doing this
                     .filter(|service| {
                         // Domain will be like `.svc.cluster.local.` (trailing .), so ignore the last character.
-                        let domain = self.svc_domain.to_utf8();
+                        let domain = ".".to_string() + &self.svc_domain.to_utf8();
+
                         let domain = domain
                             .strip_suffix('.')
                             .expect("the svc domain must have a trailing '.'");
@@ -531,6 +540,7 @@ impl Resolver for Store {
 
         // Find the service for the requested host.
         let requested_name = Name::from(request.query().name().clone());
+        trace!("incoming request {requested_name:?}");
         let Some(service_match) = self.find_server(&client, &requested_name) else {
             trace!("unknown host, forwarding");
             // Unknown host. Forward to the upstream resolver.
@@ -600,6 +610,7 @@ impl Resolver for Store {
 }
 
 /// An alias for the requested hostname.
+#[derive(Debug)]
 struct Alias {
     /// The name to be used in the search.
     name: Name,
@@ -616,6 +627,7 @@ impl Display for Alias {
 }
 
 /// Created for an alias generated by stripping a search domain from the requested host.
+#[derive(Debug)]
 struct Stripped {
     /// The requested hostname with the `search_domain` removed.
     name: Name,
@@ -625,6 +637,7 @@ struct Stripped {
 }
 
 /// Returned when a server was successfully found for the requested hostname.
+#[derive(Debug)]
 struct ServerMatch {
     /// The hostname that was used to find the service. This is identical to the
     /// service hostname, except that it is an FQDN [Name].
@@ -708,13 +721,17 @@ pub trait Forwarder: Send + Sync {
 }
 
 /// Creates the appropriate DNS forwarder for the proxy mode.
-pub fn forwarder_for_mode(proxy_mode: ProxyMode) -> Result<Arc<dyn Forwarder>, Error> {
+pub fn forwarder_for_mode(
+    proxy_mode: ProxyMode,
+    cluster_domain: String,
+) -> Result<Arc<dyn Forwarder>, Error> {
     Ok(match proxy_mode {
         ProxyMode::Shared => {
             // TODO(https://github.com/istio/ztunnel/issues/555): Use pod settings if available.
-            Arc::new(SystemForwarder::new()?)
+            // Today, we only support the basic namespace awareness
+            Arc::new(SystemForwarder::new(true, cluster_domain)?)
         }
-        ProxyMode::Dedicated => Arc::new(SystemForwarder::new()?),
+        ProxyMode::Dedicated => Arc::new(SystemForwarder::new(false, cluster_domain)?),
     })
 }
 
@@ -723,20 +740,43 @@ pub fn forwarder_for_mode(proxy_mode: ProxyMode) -> Result<Arc<dyn Forwarder>, E
 /// that would have been used by the client. For shared proxy mode, this will be the resolver
 /// configuration for the ztunnel DaemonSet (i.e. node-level resolver settings).
 struct SystemForwarder {
-    search_domains: Vec<Name>,
+    search_domains: SearchDomains,
     resolver: Arc<dyn Resolver>,
 }
 
+enum SearchDomains {
+    Static(Vec<Name>),
+    Dynamic(String),
+}
+
 impl SystemForwarder {
-    fn new() -> Result<Self, Error> {
-        // Get the resolver config from /etc/resolv.conf.
+    fn new(per_pod: bool, cluster_domain: String) -> Result<Self, Error> {
+        // Get the resolver config from `ztunnel's` /etc/resolv.conf.
         let (cfg, opts) = read_system_conf().map_err(|e| Error::Generic(Box::new(e)))?;
 
         // Extract the parts.
         let domain = cfg.domain().cloned();
         let search_domains = cfg.search().to_vec();
         let name_servers = cfg.name_servers().to_vec();
 
+        Self::from_parts(
+            per_pod,
+            cluster_domain,
+            opts,
+            domain,
+            search_domains,
+            name_servers,
+        )
+    }
+
+    fn from_parts(
+        per_pod: bool,
+        cluster_domain: String,
+        opts: ResolverOpts,
+        domain: Option<Name>,
+        search_domains: Vec<Name>,
+        name_servers: Vec<NameServerConfig>,
+    ) -> Result<Self, Error> {
         // Remove the search list before passing to the resolver. The local resolver that
         // sends the original request will already have search domains applied. We want
         // this resolver to simply use the request host rather than re-adding search domains.
@@ -746,6 +786,13 @@ impl SystemForwarder {
         let resolver = Arc::new(
             dns::forwarder::Forwarder::new(cfg, opts).map_err(|e| Error::Generic(Box::new(e)))?,
         );
+        let search_domains = if per_pod {
+            // Standard Kubernetes search is 'istio-system.svc.cluster.local svc.cluster.local cluster.local'
+            // But we need a *per-pod* one, or we will search for the wrong thing
+            SearchDomains::Dynamic(cluster_domain)
+        } else {
+            SearchDomains::Static(search_domains)
+        };
 
         Ok(Self {
             search_domains,
@@ -756,8 +803,19 @@ impl SystemForwarder {
 
 #[async_trait::async_trait]
 impl Forwarder for SystemForwarder {
-    fn search_domains(&self, _: &Workload) -> Vec<Name> {
-        self.search_domains.clone()
+    fn search_domains(&self, wl: &Workload) -> Vec<Name> {
+        // TODO: https://github.com/istio/ztunnel/issues/555 really get this from pods
+        // Today we hardcode the default search assumptions
+        match &self.search_domains {
+            SearchDomains::Static(s) => s.clone(),
+            SearchDomains::Dynamic(cluster_domain) => vec![
+                Name::from_utf8(format!("{}.svc.{cluster_domain}", wl.namespace))
+                    .expect("inputs must be valid DNS labels"),
+                Name::from_utf8(format!("svc.{cluster_domain}"))
+                    .expect("inputs must be valid DNS labels"),
+                Name::from_utf8(cluster_domain).expect("inputs must be valid DNS labels"),
+            ],
+        }
     }
 
     async fn forward(
@@ -780,7 +838,6 @@ mod tests {
     use prometheus_client::registry::Registry;
 
     use super::*;
-    use crate::metrics;
     use crate::strng;
     use crate::test_helpers::dns::{
         a, aaaa, cname, ip, ipv4, ipv6, n, new_message, new_tcp_client, new_udp_client, run_dns,
@@ -793,6 +850,7 @@ mod tests {
     use crate::xds::istio::workload::PortList as XdsPortList;
     use crate::xds::istio::workload::Service as XdsService;
     use crate::xds::istio::workload::Workload as XdsWorkload;
+    use crate::{metrics, test_helpers};
 
     const NS1: &str = "ns1";
     const NS2: &str = "ns2";
@@ -930,8 +988,6 @@ mod tests {
         assert_eq!(expected, actual);
     }
 
-    // TODO(nmittler): Test headless services (https://github.com/istio/ztunnel/issues/554).
-    // TODO(nmittler): Test truncation once fixed (https://github.com/bluejekyll/trust-dns/issues/1973).
     #[tokio::test]
     async fn lookup() {
         initialize_telemetry();
@@ -1418,6 +1474,39 @@ mod tests {
         assert_eq!(75, resp.answers().len(), "expected UDP to be truncated");
     }
 
+    #[test]
+    fn search_domains() {
+        let opts = ResolverOpts::default();
+        let search = vec![
+            as_name("istio-system.svc.cluster.local"),
+            as_name("svc.cluster.local"),
+            as_name("cluster.local"),
+        ];
+        let f = SystemForwarder::from_parts(
+            true,
+            "cluster.local".to_string(),
+            opts,
+            None,
+            search,
+            vec![],
+        )
+        .unwrap();
+        let make_workload = |ns: &str| Workload {
+            name: "something".into(),
+            namespace: ns.into(),
+            ..test_helpers::test_default_workload()
+        };
+
+        assert_eq!(
+            f.search_domains(&make_workload("default-ns")),
+            vec![
+                as_name("default-ns.svc.cluster.local"),
+                as_name("svc.cluster.local"),
+                as_name("cluster.local"),
+            ]
+        );
+    }
+
     /// Sort the IP records so that we can directly compare them to the expected. The resulting
     /// list will contain CNAME first, followed by A, and then by AAAA. Within each record type,
     /// records will be sorted in ascending order by their string representation.

src/proxyfactory.rs

@@ -104,7 +104,10 @@ impl ProxyFactory {
                     self.config.dns_proxy_addr,
                     self.config.network.clone(),
                     self.state.clone(),
-                    dns::forwarder_for_mode(self.config.proxy_mode)?,
+                    dns::forwarder_for_mode(
+                        self.config.proxy_mode,
+                        self.config.cluster_domain.clone(),
+                    )?,
                     self.dns_metrics.clone().unwrap(),
                     drain,
                     socket_factory.as_ref(),