Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 6 additions & 8 deletions common/src/address.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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()
}
Expand All @@ -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()
Expand Down Expand Up @@ -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!(
Expand Down
13 changes: 9 additions & 4 deletions common/src/policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment on lines +27 to +35
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments should go a long way in reducing confusion about which of these to use. Thanks for straightening that out!


/// The amount of redundancy for clickhouse servers
///
Expand Down
10 changes: 3 additions & 7 deletions internal-dns/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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()
}

Expand Down
3 changes: 0 additions & 3 deletions nexus-sled-agent-shared/src/inventory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,6 @@ pub enum OmicronZoneType {
},
InternalNtp {
address: SocketAddrV6,
ntp_servers: Vec<String>,
dns_servers: Vec<IpAddr>,
domain: Option<String>,
Comment on lines -248 to -250
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming this means that Sled Agent will no longer be sending this information as part of inventory, and that this is okay because we're still doing offline upgrades so Nexus will always be sync'd up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. This is most obvious in the change to openapi/sled-agent.json. Dropping these fields affects both PUT /omicron-zones (in a backwards-compatible way: if we were to receive a put from an old Nexus, which we can't, we'd ignore the extra fields) and GET /omicron-zones (in a backwards-incompatible way: we won't return these fields, which an old Nexus would expect to receive).

},
Nexus {
/// The address at which the internal nexus server is reachable.
Expand Down
21 changes: 2 additions & 19 deletions nexus/db-model/src/deployment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -431,22 +431,10 @@ impl BpOmicronZone {
Some(SqlU32::from(*gz_address_index));
}
BlueprintZoneType::InternalNtp(
blueprint_zone_type::InternalNtp {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing fields from ZoneTypes rules! 🎉

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,
Expand Down Expand Up @@ -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 {
Expand Down
23 changes: 4 additions & 19 deletions nexus/db-model/src/inventory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment on lines -1478 to -1484
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting a change to the db model types and database schema and then I thought "this is gonna be complicated" because we might have old inventories that have this information. But I guess maybe: no change is strictly necessary because these are NULL-able fields anyway, so they will just be NULL for all new inventories?

Should we still drop the columns? This technically loses data from old inventories where they were populated but I think we're saying this data is useless anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot drop the columns; they were used (i.e., set to non-NULL values) by both internal NTP and boundary NTP; they are still used by boundary NTP. Good question though - I need to update some comments that refer to them as applying to both kinds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comments on these columns in dbinit.sql in 48ee15b

}
OmicronZoneType::Nexus {
internal_address,
Expand Down Expand Up @@ -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
Expand Down
3 changes: 0 additions & 3 deletions nexus/db-queries/src/db/datastore/rack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
),
}],
Expand Down
18 changes: 5 additions & 13 deletions nexus/reconfigurator/execution/src/dns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 0 additions & 3 deletions nexus/reconfigurator/execution/src/omicron_zones.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()],
},
),
});
Expand Down
2 changes: 2 additions & 0 deletions nexus/reconfigurator/planning/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading