From f6ad3fea8cb94b6421534e8cdee8c9e8141a8ddf Mon Sep 17 00:00:00 2001 From: Rain Date: Thu, 16 Oct 2025 02:21:08 +0000 Subject: [PATCH 1/2] [spr] initial version Created using spr 1.3.6-beta.1 --- Cargo.lock | 2 + nexus/reconfigurator/planning/Cargo.toml | 2 + nexus/reconfigurator/planning/src/example.rs | 262 +++++++++++++++++++ 3 files changed, 266 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 2b9f7875155..3971428ccf7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7029,6 +7029,7 @@ dependencies = [ "cockroach-admin-types", "daft", "debug-ignore", + "dns-server", "dropshot", "expectorate", "gateway-client", @@ -7039,6 +7040,7 @@ dependencies = [ "illumos-utils", "indexmap 2.11.4", "internal-dns-resolver", + "internal-dns-types", "ipnet", "itertools 0.14.0", "maplit", diff --git a/nexus/reconfigurator/planning/Cargo.toml b/nexus/reconfigurator/planning/Cargo.toml index ed54003c58f..2f771ab1596 100644 --- a/nexus/reconfigurator/planning/Cargo.toml +++ b/nexus/reconfigurator/planning/Cargo.toml @@ -55,9 +55,11 @@ uuid.workspace = true omicron-workspace-hack.workspace = true [dev-dependencies] +dns-server.workspace = true dropshot.workspace = true expectorate.workspace = true hex-literal.workspace = true +internal-dns-types.workspace = true maplit.workspace = true omicron-common = { workspace = true, features = ["testing"] } omicron-test-utils.workspace = true diff --git a/nexus/reconfigurator/planning/src/example.rs b/nexus/reconfigurator/planning/src/example.rs index 31e4d1d61e8..16c4725ff76 100644 --- a/nexus/reconfigurator/planning/src/example.rs +++ b/nexus/reconfigurator/planning/src/example.rs @@ -957,11 +957,26 @@ pub fn extract_tuf_repo_description( #[cfg(test)] mod tests { + use std::collections::BTreeMap; + use std::time::Duration; + + use anyhow::anyhow; use chrono::{DateTime, Utc}; + use iddqd::IdOrdMap; + use internal_dns_resolver::QorbResolver; + use internal_dns_resolver::Resolver; + use internal_dns_types::names::ServiceName; use nexus_sled_agent_shared::inventory::{OmicronZoneConfig, ZoneKind}; use nexus_types::deployment::BlueprintZoneConfig; use nexus_types::deployment::BlueprintZoneDisposition; + use nexus_types::deployment::execution::blueprint_internal_dns_config; + use nexus_types::deployment::execution::overridables; + use nexus_types::internal_api::params::DnsConfigParams; + use omicron_common::address::REPO_DEPOT_PORT; + use omicron_common::address::get_sled_address; + use omicron_common::api::external::Generation; use omicron_test_utils::dev::test_setup_log; + use slog_error_chain::InlineErrorChain; use super::*; @@ -1103,6 +1118,253 @@ mod tests { logctx.cleanup_successful(); } + /// Test that services set up by the example system are reachable via DNS. + /// + /// This test catches issues like #9176, where a too-large DNS response can + /// cause packet fragmentation and queries to be lost. + #[tokio::test] + async fn dns_resolution_works() { + static TEST_NAME: &str = "dns_resolution_works"; + let logctx = test_setup_log(TEST_NAME); + + // Build a somewhat exaggerated, fully populated system with twice the + // number of Nexuses as a system in typical use. + let (example, blueprint) = + ExampleSystemBuilder::new(&logctx.log, TEST_NAME) + .nsleds(32) + .nexus_count(6) + .build(); + let sleds_by_id = blueprint + .sleds + .keys() + .map(|sled_id| { + let sled_details = example + .input + .sled_lookup(SledFilter::Commissioned, *sled_id) + .unwrap(); + let sled_agent = + example.collection.sled_agents.get(sled_id).unwrap_or_else( + || panic!("sled {} not found in collection", sled_id), + ); + nexus_types::deployment::execution::Sled::new( + *sled_id, + sled_details.policy, + get_sled_address(sled_details.resources.subnet), + REPO_DEPOT_PORT, + sled_agent.sled_role, + ) + }) + .collect::>(); + + let dns_config = blueprint_internal_dns_config( + &blueprint, + &sleds_by_id, + Generation::new(), + &overridables::DEFAULT, + ) + .expect("built DNS configuration"); + eprintln!("DNS config: {dns_config:#?}"); + let params = DnsConfigParams { + generation: Generation::new().next(), + serial: 0, + time_created: Utc::now(), + zones: vec![dns_config], + }; + + // Initialize a DNS server. + let dns = dns_server::TransientServer::new(&logctx.log) + .await + .expect("created DNS server"); + dns.initialize_with_config(&logctx.log, ¶ms) + .await + .expect("initialized DNS server"); + + // Query with the simple DNS resolver. + { + let resolver = Resolver::new_from_addrs( + logctx.log.clone(), + &[dns.dns_server.local_address()], + ) + .expect("created DNS resolver"); + + // Ensure that all service names can be looked up via the resolver. + let mut service_names_with_errors = BTreeMap::new(); + for service in service_names_to_query(&blueprint) { + eprintln!("*** looking up DNS for {:?}", service); + // Large packets can be fragmented which would cause lookups to + // timeout. Add a short timeout (5s) to catch this. + let addrs = match tokio::time::timeout( + Duration::from_secs(5), + resolver.lookup_all_socket_v6(service), + ) + .await + { + Ok(Ok(addrs)) => addrs, + Ok(Err(e)) => { + service_names_with_errors.insert(service, anyhow!(e)); + continue; + } + Err(e) => { + service_names_with_errors.insert(service, anyhow!(e)); + continue; + } + }; + + assert!( + !addrs.is_empty(), + "service name {:?} should return at least one address", + service + ); + + // TODO: add assertions on the returned addresses. + eprintln!("*** lookup successful: {:?}", addrs); + } + + if !service_names_with_errors.is_empty() { + let mut errors = Vec::new(); + for (service, error) in service_names_with_errors { + errors.push(format!( + "{:?}: {}", + service, + InlineErrorChain::new(error.as_ref()) + )); + } + panic!( + "DNS lookup errors for some service names:\n{}", + errors.join("\n"), + ); + } + } + + // Query with the qorb DNS resolver. + { + let resolver = + QorbResolver::new(vec![dns.dns_server.local_address()]); + + // Ensure that all service names can be looked up via the qorb + // resolver. + let mut service_names_with_errors = BTreeMap::new(); + for service in service_names_to_query(&blueprint) { + eprintln!("*** using qorb to look up DNS for {:?}", service); + let mut srv_resolver = resolver.for_service(service); + let mut monitor_rx = srv_resolver.monitor(); + + // Wait for at least one result to be returned. Like above, we + // don't want to wait forever, so set a reasonable timeout. + let backends = match tokio::time::timeout( + Duration::from_secs(5), + monitor_rx.wait_for(|backends| !backends.is_empty()), + ) + .await + { + Ok(Ok(backends)) => backends, + Ok(Err(e)) => { + service_names_with_errors.insert(service, anyhow!(e)); + continue; + } + Err(e) => { + service_names_with_errors.insert(service, anyhow!(e)); + continue; + } + }; + + // TODO: add assertions on the returned addresses. + eprintln!("*** qorb lookup successful: {:?}", &**backends); + } + } + + logctx.cleanup_successful(); + } + + /// Returns the list of DNS service names expected in an example system. + fn service_names_to_query(blueprint: &Blueprint) -> Vec { + let mut out = vec![ + ServiceName::Clickhouse, + // ClickhouseAdminKeeper and ClickhouseAdminServer are not currently + // part of the example system + ServiceName::ClickhouseAdminSingleServer, + ServiceName::ClickhouseNative, + // ClickhouseClusterNative, ClickhouseKeeper and ClickhouseServer + // are not currently part of the example system + // + // Cockroach is not currently part of the example system + // + // ExternalDns is not currently part of the example system + ServiceName::InternalDns, + ServiceName::Nexus, + ServiceName::NexusLockstep, + // Oximeter is not currently part of the example system + ServiceName::OximeterReader, + // ManagementGatewayService is not currently part of the example + // system + ServiceName::RepoDepot, + // Wicketd is not currently part of the example system + // + // Dendrite and Tfport are not currently part of the example system + ServiceName::CruciblePantry, + // XXX the SledAgent service name doesn't appear to be used? + // + // Crucible is handled below + // + // BoundaryNtp is not currently part of the example system + // + // InternalNtp is too large to fit in a single DNS packet and times + // out, but DNS lookups for it aren't used anywhere. + // + // Maghemite and Mgda are not currently part of the example system + ]; + + // Each Crucible zone should be queryable. + for (_, zone) in + blueprint.all_omicron_zones(BlueprintZoneDisposition::is_in_service) + { + if zone.kind() == ZoneKind::Crucible { + out.push(ServiceName::Crucible(zone.id)); + } + } + + out + } + + #[expect(unused)] + fn match_service_names(service: ServiceName) { + // Add a match statement here to ensure that new service names that get + // added are considered. + // + // When adding a new variant, ensure that it is covered by + // service_names_to_query above, either by adding it to the list of + // services to query, or by explaining why it isn't included. + match service { + ServiceName::Clickhouse => {} + ServiceName::ClickhouseAdminKeeper => {} + ServiceName::ClickhouseAdminServer => {} + ServiceName::ClickhouseAdminSingleServer => {} + ServiceName::ClickhouseNative => {} + ServiceName::ClickhouseClusterNative => {} + ServiceName::ClickhouseKeeper => {} + ServiceName::ClickhouseServer => {} + ServiceName::Cockroach => {} + ServiceName::InternalDns => {} + ServiceName::ExternalDns => {} + ServiceName::Nexus => {} + ServiceName::NexusLockstep => {} + ServiceName::Oximeter => {} + ServiceName::OximeterReader => {} + ServiceName::ManagementGatewayService => {} + ServiceName::RepoDepot => {} + ServiceName::Wicketd => {} + ServiceName::Dendrite => {} + ServiceName::Tfport => {} + ServiceName::CruciblePantry => {} + ServiceName::SledAgent(_) => {} + ServiceName::Crucible(_) => {} + ServiceName::BoundaryNtp => {} + ServiceName::InternalNtp => {} + ServiceName::Maghemite => {} + ServiceName::Mgd => {} + } + } + fn blueprint_zones_of_kind( blueprint: &Blueprint, kind: ZoneKind, From 74043666fcdbc8f5f8e2a32881a6a7504ae599ab Mon Sep 17 00:00:00 2001 From: Rain Date: Thu, 16 Oct 2025 03:18:29 +0000 Subject: [PATCH 2/2] also handle error cases Created using spr 1.3.6-beta.1 --- Cargo.lock | 1 + nexus/reconfigurator/planning/Cargo.toml | 1 + nexus/reconfigurator/planning/src/example.rs | 270 ++++++++++++++----- 3 files changed, 199 insertions(+), 73 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3971428ccf7..6f179f6ce7d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7035,6 +7035,7 @@ dependencies = [ "gateway-client", "gateway-types", "hex-literal", + "hickory-resolver 0.25.2", "id-map", "iddqd", "illumos-utils", diff --git a/nexus/reconfigurator/planning/Cargo.toml b/nexus/reconfigurator/planning/Cargo.toml index 2f771ab1596..b81909d441e 100644 --- a/nexus/reconfigurator/planning/Cargo.toml +++ b/nexus/reconfigurator/planning/Cargo.toml @@ -59,6 +59,7 @@ dns-server.workspace = true dropshot.workspace = true expectorate.workspace = true hex-literal.workspace = true +hickory-resolver.workspace = true internal-dns-types.workspace = true maplit.workspace = true omicron-common = { workspace = true, features = ["testing"] } diff --git a/nexus/reconfigurator/planning/src/example.rs b/nexus/reconfigurator/planning/src/example.rs index 16c4725ff76..5509c461174 100644 --- a/nexus/reconfigurator/planning/src/example.rs +++ b/nexus/reconfigurator/planning/src/example.rs @@ -962,8 +962,11 @@ mod tests { use anyhow::anyhow; use chrono::{DateTime, Utc}; + use hickory_resolver::ResolveErrorKind; + use hickory_resolver::proto::ProtoErrorKind; use iddqd::IdOrdMap; use internal_dns_resolver::QorbResolver; + use internal_dns_resolver::ResolveError; use internal_dns_resolver::Resolver; use internal_dns_types::names::ServiceName; use nexus_sled_agent_shared::inventory::{OmicronZoneConfig, ZoneKind}; @@ -1187,42 +1190,104 @@ mod tests { ) .expect("created DNS resolver"); - // Ensure that all service names can be looked up via the resolver. - let mut service_names_with_errors = BTreeMap::new(); - for service in service_names_to_query(&blueprint) { - eprintln!("*** looking up DNS for {:?}", service); + let mut mismatched = BTreeMap::new(); + for (service, expected_result) in service_names_to_query(&blueprint) + { // Large packets can be fragmented which would cause lookups to - // timeout. Add a short timeout (5s) to catch this. - let addrs = match tokio::time::timeout( + // timeout. Don't query services that we expect to have + // fragmented packets. + if expected_result == Err(QueryError::PacketFragmented) { + continue; + } + + eprintln!( + "** looking up DNS for {:?} (expected: {:?})", + service, expected_result + ); + + // For other results, we want to catch situations where a packet + // is fragmented. Add a short timeout (5s) for this. + let lookup_fut = tokio::time::timeout( Duration::from_secs(5), resolver.lookup_all_socket_v6(service), - ) - .await - { - Ok(Ok(addrs)) => addrs, - Ok(Err(e)) => { - service_names_with_errors.insert(service, anyhow!(e)); - continue; + ); + match (lookup_fut.await, expected_result) { + (Ok(Ok(addrs)), Ok(())) => { + if addrs.is_empty() { + mismatched.insert( + service, + anyhow!("no addresses returned"), + ); + } + + // TODO: add assertions on the returned addresses. + eprintln!("*** lookup successful: {addrs:?}"); } - Err(e) => { - service_names_with_errors.insert(service, anyhow!(e)); + (Ok(Ok(addrs)), Err(e)) => { + mismatched.insert( + service, + anyhow!( + "expected Err({e:?}), but got Ok({addrs:?})" + ), + ); + } + (Ok(Err(e)), Ok(())) => { + mismatched.insert( + service, + anyhow!(e) + .context("expected Ok(()), but got an error"), + ); + } + (Ok(Err(e)), Err(QueryError::NoRecordsFound)) => { + // "No records found" is returned as a hickory ProtoError. + if let ResolveError::Resolve(resolve_error) = &e { + if let ResolveErrorKind::Proto(proto_error) = + resolve_error.kind() + { + if let ProtoErrorKind::NoRecordsFound { + .. + } = proto_error.kind() + { + // This is the expected error case. + eprintln!( + "*** no records found as expected" + ) + } else { + mismatched.insert( + service, + anyhow!( + "expected NoRecordsFound error, \ + but got a different proto error: {e:?}" + ), + ); + } + } else { + mismatched.insert( + service, + anyhow!( + "expected Proto error with NoRecordsFound, \ + but got a different resolve error: {e:?}" + ), + ); + } + } + } + (_, Err(QueryError::PacketFragmented)) => { + unreachable!("we don't query PacketFragmented") + } + (Err(e), _) => { + mismatched.insert( + service, + anyhow!(e).context("got unexpected timeout"), + ); continue; } - }; - - assert!( - !addrs.is_empty(), - "service name {:?} should return at least one address", - service - ); - - // TODO: add assertions on the returned addresses. - eprintln!("*** lookup successful: {:?}", addrs); + } } - if !service_names_with_errors.is_empty() { + if !mismatched.is_empty() { let mut errors = Vec::new(); - for (service, error) in service_names_with_errors { + for (service, error) in mismatched { errors.push(format!( "{:?}: {}", service, @@ -1230,7 +1295,7 @@ mod tests { )); } panic!( - "DNS lookup errors for some service names:\n{}", + "unexpected DNS lookup results for some services:\n{}", errors.join("\n"), ); } @@ -1241,10 +1306,17 @@ mod tests { let resolver = QorbResolver::new(vec![dns.dns_server.local_address()]); - // Ensure that all service names can be looked up via the qorb - // resolver. - let mut service_names_with_errors = BTreeMap::new(); - for service in service_names_to_query(&blueprint) { + // Ensure that service names can be looked up via the qorb resolver. + let mut services_with_errors = BTreeMap::new(); + for (service, expected_result) in service_names_to_query(&blueprint) + { + // We can't really use qorb to query error cases, since those + // are handled internally by the resolver. Just query the + // success cases. + if expected_result.is_err() { + continue; + } + eprintln!("*** using qorb to look up DNS for {:?}", service); let mut srv_resolver = resolver.for_service(service); let mut monitor_rx = srv_resolver.monitor(); @@ -1259,11 +1331,11 @@ mod tests { { Ok(Ok(backends)) => backends, Ok(Err(e)) => { - service_names_with_errors.insert(service, anyhow!(e)); + services_with_errors.insert(service, anyhow!(e)); continue; } Err(e) => { - service_names_with_errors.insert(service, anyhow!(e)); + services_with_errors.insert(service, anyhow!(e)); continue; } }; @@ -1277,63 +1349,115 @@ mod tests { } /// Returns the list of DNS service names expected in an example system. - fn service_names_to_query(blueprint: &Blueprint) -> Vec { - let mut out = vec![ - ServiceName::Clickhouse, - // ClickhouseAdminKeeper and ClickhouseAdminServer are not currently - // part of the example system - ServiceName::ClickhouseAdminSingleServer, - ServiceName::ClickhouseNative, - // ClickhouseClusterNative, ClickhouseKeeper and ClickhouseServer - // are not currently part of the example system - // - // Cockroach is not currently part of the example system - // - // ExternalDns is not currently part of the example system - ServiceName::InternalDns, - ServiceName::Nexus, - ServiceName::NexusLockstep, - // Oximeter is not currently part of the example system - ServiceName::OximeterReader, - // ManagementGatewayService is not currently part of the example - // system - ServiceName::RepoDepot, - // Wicketd is not currently part of the example system - // - // Dendrite and Tfport are not currently part of the example system - ServiceName::CruciblePantry, - // XXX the SledAgent service name doesn't appear to be used? - // - // Crucible is handled below - // - // BoundaryNtp is not currently part of the example system - // - // InternalNtp is too large to fit in a single DNS packet and times - // out, but DNS lookups for it aren't used anywhere. - // - // Maghemite and Mgda are not currently part of the example system - ]; + fn service_names_to_query( + blueprint: &Blueprint, + ) -> BTreeMap> { + let mut out = BTreeMap::new(); + + out.insert(ServiceName::Clickhouse, Ok(())); + + // ClickhouseAdminKeeper and ClickhouseAdminServer are not currently + // part of the example system + out.insert( + ServiceName::ClickhouseAdminKeeper, + Err(QueryError::NoRecordsFound), + ); + out.insert( + ServiceName::ClickhouseAdminServer, + Err(QueryError::NoRecordsFound), + ); + + out.insert(ServiceName::ClickhouseAdminSingleServer, Ok(())); + out.insert(ServiceName::ClickhouseNative, Ok(())); + + // ClickhouseClusterNative, ClickhouseKeeper and ClickhouseServer + // are not currently part of the example system + out.insert( + ServiceName::ClickhouseClusterNative, + Err(QueryError::NoRecordsFound), + ); + out.insert( + ServiceName::ClickhouseKeeper, + Err(QueryError::NoRecordsFound), + ); + out.insert( + ServiceName::ClickhouseServer, + Err(QueryError::NoRecordsFound), + ); + + // Cockroach is not currently part of the example system + out.insert(ServiceName::Cockroach, Err(QueryError::NoRecordsFound)); + + // ExternalDns is not currently part of the example system + out.insert(ServiceName::ExternalDns, Err(QueryError::NoRecordsFound)); + + out.insert(ServiceName::InternalDns, Ok(())); + out.insert(ServiceName::Nexus, Ok(())); + out.insert(ServiceName::NexusLockstep, Ok(())); + + // Oximeter is not currently part of the example system + out.insert(ServiceName::Oximeter, Err(QueryError::NoRecordsFound)); + + out.insert(ServiceName::OximeterReader, Ok(())); + + // ManagementGatewayService is not currently part of the example + // system + out.insert( + ServiceName::ManagementGatewayService, + Err(QueryError::NoRecordsFound), + ); + + out.insert(ServiceName::RepoDepot, Ok(())); + + // Wicketd is not currently part of the example system + out.insert(ServiceName::Wicketd, Err(QueryError::NoRecordsFound)); + + // Dendrite and Tfport are not currently part of the example system + out.insert(ServiceName::Dendrite, Err(QueryError::NoRecordsFound)); + out.insert(ServiceName::Tfport, Err(QueryError::NoRecordsFound)); + + out.insert(ServiceName::CruciblePantry, Ok(())); + + // XXX the SledAgent service name doesn't appear to be used? + // + // Crucible is handled below + // + // BoundaryNtp is not currently part of the example system + out.insert(ServiceName::BoundaryNtp, Err(QueryError::NoRecordsFound)); + + // InternalNtp is too large to fit in a single DNS packet and times + // out, but DNS lookups for it aren't used anywhere. + out.insert(ServiceName::InternalNtp, Err(QueryError::PacketFragmented)); + + // Maghemite and Mgd are not currently part of the example system + out.insert(ServiceName::Maghemite, Err(QueryError::NoRecordsFound)); + out.insert(ServiceName::Mgd, Err(QueryError::NoRecordsFound)); // Each Crucible zone should be queryable. for (_, zone) in blueprint.all_omicron_zones(BlueprintZoneDisposition::is_in_service) { if zone.kind() == ZoneKind::Crucible { - out.push(ServiceName::Crucible(zone.id)); + out.insert(ServiceName::Crucible(zone.id), Ok(())); } } out } + #[derive(Clone, Copy, Debug, Eq, PartialEq)] + enum QueryError { + NoRecordsFound, + PacketFragmented, + } + #[expect(unused)] fn match_service_names(service: ServiceName) { // Add a match statement here to ensure that new service names that get // added are considered. // // When adding a new variant, ensure that it is covered by - // service_names_to_query above, either by adding it to the list of - // services to query, or by explaining why it isn't included. + // service_names_to_query above. match service { ServiceName::Clickhouse => {} ServiceName::ClickhouseAdminKeeper => {}