Remove no-longer-needed internal NTP configuration options (#6657)

This grew a bit bigger than I expected. The main goal is to fix #6261:
now that R10 is out the door, we no longer need to tell each internal
NTP zone the specific names of all boundary NTP zones. That grew into a
few other cleanup operations:

* I removed the `ntp_servers`, `dns_servers`, and `domain` fields from
`OmicronZoneType::InternalNtp`. The only thing it needs now is internal
DNS, which it can get the same way every other zone does. (I left
`address` in place for consistency with other zones, but I was tempted
to remove it too: some discussion in #6651.)
* In sled-agent, we no longer try to resolve internal DNS via internal
DNS to populate `/etc/resolve.conf` for zones that expect to access
internal DNS; instead, we populate it with the fixed list of reserved
`INTERNAL_DNS_REDUNDANCY` IP addresses. This was immediately important
in getting tests passing, but is also probably the right thing to do; I
added a comment summarizing the points @davepacheco made in a call
earlier today.
* Renamed `MAX_INTERNAL_DNS_REDUNDANCY` to
`RESERVED_INTERNAL_DNS_REDUNDANCY`, and changed almost all of its users
to use `INTERNAL_DNS_REDUNDANCY` instead. This split was confusing, and
we had helper methods to construct DNS clients that disagreed on which
constant to use. `RESERVED_INTERNAL_DNS_REDUNDANCY` should be more clear
that this is reserved for future work/growth, and
`INTERNAL_DNS_REDUNDANCY` should be the value used in practice.
* Fixed a bug in the reconfigurator planner where the external
networking and DNS subnet allocators were considering all resources used
by the parent blueprint as in use, even if the planner had decided to
expunge the zones owning those resources. The fix for this is kinda
gross; I'm open to better ideas but also I'd like to take a crack at a
larger refactoring once some of the other big planner PRs have landed.

I had tested an R10 -> this branch upgrade on london, but only about
halfway through the above work. I want to retest both an R10 -> branch
upgrade and a fresh RSS install to ensure there are no lingering
expectations on the old internal NTP config path, but since I'd made it
through a successful upgrade, I think this can go ahead and be opened
for review.

common/src/address.rs

@@ -8,7 +8,7 @@
 //! and Nexus, who need to agree upon addressing schemes.
 
 use crate::api::external::{self, Error};
-use crate::policy::{INTERNAL_DNS_REDUNDANCY, MAX_INTERNAL_DNS_REDUNDANCY};
+use crate::policy::INTERNAL_DNS_REDUNDANCY;
 use ipnetwork::Ipv6Network;
 use once_cell::sync::Lazy;
 use oxnet::{Ipv4Net, Ipv6Net};
@@ -306,10 +306,10 @@ impl ReservedRackSubnet {
 
     /// Returns the DNS addresses from this reserved rack subnet.
     ///
-    /// These addresses will come from the first [`MAX_INTERNAL_DNS_REDUNDANCY`]
+    /// These addresses will come from the first [`INTERNAL_DNS_REDUNDANCY`]
     /// `/64s` of the [`RACK_PREFIX`] subnet.
     pub fn get_dns_subnets(&self) -> Vec<DnsSubnet> {
-        (0..MAX_INTERNAL_DNS_REDUNDANCY)
+        (0..INTERNAL_DNS_REDUNDANCY)
             .map(|idx| self.get_dns_subnet(u8::try_from(idx + 1).unwrap()))
             .collect()
     }
@@ -319,10 +319,8 @@ impl ReservedRackSubnet {
 /// subnet
 pub fn get_internal_dns_server_addresses(addr: Ipv6Addr) -> Vec<IpAddr> {
     let az_subnet = Ipv6Subnet::<AZ_PREFIX>::new(addr);
-    let reserved_rack_subnet = ReservedRackSubnet::new(az_subnet);
-    let dns_subnets =
-        &reserved_rack_subnet.get_dns_subnets()[0..INTERNAL_DNS_REDUNDANCY];
-    dns_subnets
+    ReservedRackSubnet::new(az_subnet)
+        .get_dns_subnets()
         .iter()
         .map(|dns_subnet| IpAddr::from(dns_subnet.dns_address()))
         .collect()
@@ -702,7 +700,7 @@ mod test {
 
         // Observe the first DNS subnet within this reserved rack subnet.
         let dns_subnets = rack_subnet.get_dns_subnets();
-        assert_eq!(MAX_INTERNAL_DNS_REDUNDANCY, dns_subnets.len());
+        assert_eq!(INTERNAL_DNS_REDUNDANCY, dns_subnets.len());
 
         // The DNS address and GZ address should be only differing by one.
         assert_eq!(

common/src/policy.rs

@@ -21,13 +21,18 @@ pub const COCKROACHDB_REDUNDANCY: usize = 5;
 
 /// The amount of redundancy for internal DNS servers.
 ///
-/// Must be less than or equal to MAX_INTERNAL_DNS_REDUNDANCY.
+/// Must be less than or equal to RESERVED_INTERNAL_DNS_REDUNDANCY.
 pub const INTERNAL_DNS_REDUNDANCY: usize = 3;
 
-/// The maximum amount of redundancy for internal DNS servers.
+/// The potential number of internal DNS servers we hold reserved for future
+/// growth.
 ///
-/// This determines the number of addresses which are reserved for internal DNS servers.
-pub const MAX_INTERNAL_DNS_REDUNDANCY: usize = 5;
+/// Any consumers interacting with "the number of internal DNS servers" (e.g.,
+/// to construct a DNS client) should operate in terms of
+/// [`INTERNAL_DNS_REDUNDANCY`]. This constant should only be used to reserve
+/// space where we could increase `INTERNAL_DNS_REDUNDANCY` up to at most this
+/// value.
+pub const RESERVED_INTERNAL_DNS_REDUNDANCY: usize = 5;
 
 /// The amount of redundancy for clickhouse servers
 ///

internal-dns/src/resolver.rs

@@ -8,7 +8,7 @@ use hickory_resolver::config::{
 use hickory_resolver::lookup::SrvLookup;
 use hickory_resolver::TokioAsyncResolver;
 use omicron_common::address::{
-    Ipv6Subnet, ReservedRackSubnet, AZ_PREFIX, DNS_PORT,
+    get_internal_dns_server_addresses, Ipv6Subnet, AZ_PREFIX, DNS_PORT,
 };
 use slog::{debug, error, info, trace};
 use std::net::{IpAddr, Ipv6Addr, SocketAddr, SocketAddrV6};
@@ -128,13 +128,9 @@ impl Resolver {
     pub fn servers_from_subnet(
         subnet: Ipv6Subnet<AZ_PREFIX>,
     ) -> Vec<SocketAddr> {
-        ReservedRackSubnet::new(subnet)
-            .get_dns_subnets()
+        get_internal_dns_server_addresses(subnet.net().addr())
             .into_iter()
-            .map(|dns_subnet| {
-                let ip_addr = IpAddr::V6(dns_subnet.dns_address());
-                SocketAddr::new(ip_addr, DNS_PORT)
-            })
+            .map(|ip_addr| SocketAddr::new(ip_addr, DNS_PORT))
             .collect()
     }
 

nexus-sled-agent-shared/src/inventory.rs

@@ -245,9 +245,6 @@ pub enum OmicronZoneType {
     },
     InternalNtp {
         address: SocketAddrV6,
-        ntp_servers: Vec<String>,
-        dns_servers: Vec<IpAddr>,
-        domain: Option<String>,
     },
     Nexus {
         /// The address at which the internal nexus server is reachable.

nexus/db-model/src/deployment.rs

@@ -431,22 +431,10 @@ impl BpOmicronZone {
                     Some(SqlU32::from(*gz_address_index));
             }
             BlueprintZoneType::InternalNtp(
-                blueprint_zone_type::InternalNtp {
-                    address,
-                    ntp_servers,
-                    dns_servers,
-                    domain,
-                },
+                blueprint_zone_type::InternalNtp { address },
             ) => {
                 // Set the common fields
                 bp_omicron_zone.set_primary_service_ip_and_port(address);
-
-                // Set the zone specific fields
-                bp_omicron_zone.ntp_ntp_servers = Some(ntp_servers.clone());
-                bp_omicron_zone.ntp_dns_servers = Some(
-                    dns_servers.iter().cloned().map(IpNetwork::from).collect(),
-                );
-                bp_omicron_zone.ntp_domain.clone_from(domain);
             }
             BlueprintZoneType::Nexus(blueprint_zone_type::Nexus {
                 internal_address,
@@ -649,12 +637,7 @@ impl BpOmicronZone {
                 },
             ),
             ZoneType::InternalNtp => BlueprintZoneType::InternalNtp(
-                blueprint_zone_type::InternalNtp {
-                    address: primary_address,
-                    ntp_servers: ntp_servers?,
-                    dns_servers: ntp_dns_servers?,
-                    domain: self.ntp_domain,
-                },
+                blueprint_zone_type::InternalNtp { address: primary_address },
             ),
             ZoneType::Nexus => {
                 BlueprintZoneType::Nexus(blueprint_zone_type::Nexus {

nexus/db-model/src/inventory.rs

@@ -1467,21 +1467,9 @@ impl InvOmicronZone {
                 inv_omicron_zone.dns_gz_address_index =
                     Some(SqlU32::from(*gz_address_index));
             }
-            OmicronZoneType::InternalNtp {
-                address,
-                ntp_servers,
-                dns_servers,
-                domain,
-            } => {
+            OmicronZoneType::InternalNtp { address } => {
                 // Set the common fields
                 inv_omicron_zone.set_primary_service_ip_and_port(address);
-
-                // Set the zone specific fields
-                inv_omicron_zone.ntp_ntp_servers = Some(ntp_servers.clone());
-                inv_omicron_zone.ntp_dns_servers = Some(
-                    dns_servers.iter().cloned().map(IpNetwork::from).collect(),
-                );
-                inv_omicron_zone.ntp_domain.clone_from(domain);
             }
             OmicronZoneType::Nexus {
                 internal_address,
@@ -1642,12 +1630,9 @@ impl InvOmicronZone {
                     || anyhow!("expected dns_gz_address_index, found none"),
                 )?,
             },
-            ZoneType::InternalNtp => OmicronZoneType::InternalNtp {
-                address: primary_address,
-                ntp_servers: ntp_servers?,
-                dns_servers: ntp_dns_servers?,
-                domain: self.ntp_domain,
-            },
+            ZoneType::InternalNtp => {
+                OmicronZoneType::InternalNtp { address: primary_address }
+            }
             ZoneType::Nexus => OmicronZoneType::Nexus {
                 internal_address: primary_address,
                 external_ip: self

nexus/db-queries/src/db/datastore/rack.rs

@@ -1530,9 +1530,6 @@ mod test {
                     zone_type: BlueprintZoneType::InternalNtp(
                         blueprint_zone_type::InternalNtp {
                             address: "[::1]:80".parse().unwrap(),
-                            ntp_servers: vec![],
-                            dns_servers: vec![],
-                            domain: None,
                         },
                     ),
                 }],

nexus/reconfigurator/execution/src/dns.rs

@@ -727,19 +727,11 @@ mod test {
                     gz_address_index,
                 },
             ),
-            OmicronZoneType::InternalNtp {
-                address,
-                dns_servers,
-                domain,
-                ntp_servers,
-            } => BlueprintZoneType::InternalNtp(
-                blueprint_zone_type::InternalNtp {
-                    address,
-                    ntp_servers,
-                    dns_servers,
-                    domain,
-                },
-            ),
+            OmicronZoneType::InternalNtp { address } => {
+                BlueprintZoneType::InternalNtp(
+                    blueprint_zone_type::InternalNtp { address },
+                )
+            }
             OmicronZoneType::Nexus {
                 external_dns_servers,
                 external_ip,

nexus/reconfigurator/execution/src/omicron_zones.rs

@@ -516,9 +516,6 @@ mod test {
                 zone_type: BlueprintZoneType::InternalNtp(
                     blueprint_zone_type::InternalNtp {
                         address: "[::1]:0".parse().unwrap(),
-                        dns_servers: vec!["::1".parse().unwrap()],
-                        domain: None,
-                        ntp_servers: vec!["some-ntp-server-addr".into()],
                     },
                 ),
             });

nexus/reconfigurator/planning/Cargo.toml

@@ -21,10 +21,12 @@ nexus-sled-agent-shared.workspace = true
 nexus-types.workspace = true
 omicron-common.workspace = true
 omicron-uuid-kinds.workspace = true
+once_cell.workspace = true
 oxnet.workspace = true
 rand.workspace = true
 sled-agent-client.workspace = true
 slog.workspace = true
+slog-error-chain.workspace = true
 static_assertions.workspace = true
 strum.workspace = true
 thiserror.workspace = true