Skip to content

Commit 04dae50

Browse files
jgallagherjgallagher
authored
Introduce stronger types for link-local addresses and unnumbered peers (#10082)
This is a big chunk of #9832. Stealing from the doc comments in `sled-agent/types/versions/src/stronger_bgp_unnumbered_types/early_networking.rs`, the primary changes in this PR are: * Introduce `SpecifiedIpNet`, a newtype wrapper around `IpNet` that does not allow unspecified IP addresses. * Introduce `SpecifiedIpAddr`, a newtype wrapper around `IpAddr` that does not allow unspecified IP addresses. * Introduce `UplinkAddress`, a stronger type for specifying possibly-link-local IP nets. This is the new type of `UplinkAddressConfig::address`, which was previously an `Option<IpNet>` where both `None` and `Some(UNSPECIFIED)` were treated as link-local. * Introduce `RouterPeerAddress`, a stronger type for specifying possibly-unnumbered BGP peer addresses. This is the new type of `BgpPeerConfig::addr`, which was previously an `IpAddr` where an unspecified address was treated as unnumbered. The rest of the changes are fallout from those: adding new types for any the that contains `UplinkAddressConfig` or `BgpPeerConfig`, since those changed, and updating all the places that create or consume any of those types. I'm hoping this PR is pretty straightforward to review despite its size, because much of the size is either tests or all the noise of redefining a bunch of big structs with a single field changed. The two main things I'd consider part of #9832 that are NOT addressed in this PR: * Database representation; the columns where we store these values still allow `NULL`, `0.0.0.0`, or `::`. Fixing this will require a db migration, so I want to do that separately. * External API - coming in a subsequent PR. A third thing we could consider is whether to push this stronger typing down to maghemite too. For now, in all these cases we convert to or from the stronger types primarily through obnoxiously-long method names that should stick out like sore thumbs (`RouterPeerAddress::from_optional_ip_treating_unspecified_as_unnumbered()` and friends). This should make it obvious where we're switching from strong types to weaker or vice versa.
1 parent f6d8dbe commit 04dae50

43 files changed

Lines changed: 2759 additions & 509 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.github/buildomat/jobs/deploy.sh‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,7 @@ infra_ip_last = \"$UPLINK_IP\"
294294
/^routes/c\\
295295
routes = \\[{nexthop = \"$GATEWAY_IP\", destination = \"0.0.0.0/0\"}\\]
296296
/^addresses/c\\
297-
addresses = \\[{address = \"$UPLINK_IP/24\"} \\]
297+
addresses = \\[{address = {type = \"static\", ip_net = \"$UPLINK_IP/24\"} }\\]
298298
}
299299
" pkg/config-rss.toml
300300
diff -u pkg/config-rss.toml{~,} || true

‎Cargo.lock‎

Lines changed: 4 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎clients/bootstrap-agent-client/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ reqwest = { workspace = true, features = [ "json", "rustls", "stream" ] }
1515
schemars.workspace = true
1616
serde.workspace = true
1717
serde_json.workspace = true
18+
sled-agent-types.workspace = true
1819
sled-hardware-types.workspace = true
1920
slog.workspace = true
2021
uuid.workspace = true

‎clients/bootstrap-agent-lockstep-client/src/lib.rs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,18 @@ progenitor::generate_api!(
2626
replace = {
2727
AllowedSourceIps = omicron_common::api::external::AllowedSourceIps,
2828
Baseboard = sled_hardware_types::Baseboard,
29+
BgpPeerConfig = sled_agent_types::early_networking::BgpPeerConfig,
2930
ImportExportPolicy = sled_agent_types::early_networking::ImportExportPolicy,
31+
LldpAdminStatus = sled_agent_types::early_networking::LldpAdminStatus,
32+
LldpPortConfig = sled_agent_types::early_networking::LldpPortConfig,
33+
PortConfig = sled_agent_types::early_networking::PortConfig,
34+
PortFec = sled_agent_types::early_networking::PortFec,
35+
PortSpeed = sled_agent_types::early_networking::PortSpeed,
36+
RouteConfig = sled_agent_types::early_networking::RouteConfig,
37+
RouterLifetimeConfig = sled_agent_types::early_networking::RouterLifetimeConfig,
3038
SwitchSlot = sled_agent_types::early_networking::SwitchSlot,
39+
TxEqConfig = sled_agent_types::early_networking::TxEqConfig,
40+
UplinkAddressConfig = sled_agent_types::early_networking::UplinkAddressConfig,
3141
},
3242
);
3343

‎clients/nexus-lockstep-client/src/lib.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,12 @@ progenitor::generate_api!(
6969
ReconfiguratorConfigParam = nexus_types::deployment::ReconfiguratorConfigParam,
7070
ReconfiguratorConfigView = nexus_types::deployment::ReconfiguratorConfigView,
7171
RecoverySiloConfig = sled_agent_types_versions::latest::rack_init::RecoverySiloConfig,
72+
RouterPeerType = sled_agent_types::early_networking::RouterPeerType,
7273
SledAgentUpdateStatus = nexus_types::internal_api::views::SledAgentUpdateStatus,
7374
SwitchSlot = sled_agent_types::early_networking::SwitchSlot,
7475
TrustQuorumConfig = nexus_types::trust_quorum::TrustQuorumConfig,
7576
UpdateStatus = nexus_types::internal_api::views::UpdateStatus,
77+
UplinkAddressConfig = sled_agent_types::early_networking::UplinkAddressConfig,
7678
ZoneStatus = nexus_types::internal_api::views::ZoneStatus,
7779
ZpoolName = omicron_common::zpool_name::ZpoolName,
7880
},

‎nexus/db-model/src/bgp.rs‎

Lines changed: 41 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,14 @@ use omicron_common::api::external::IdentityMetadataCreateParams;
1616
use serde::{Deserialize, Serialize};
1717
use sled_agent_types::early_networking::BgpPeerConfig;
1818
use sled_agent_types::early_networking::ImportExportPolicy;
19+
use sled_agent_types::early_networking::InvalidIpAddrError;
1920
use sled_agent_types::early_networking::MaxPathConfig;
2021
use sled_agent_types::early_networking::RouterLifetimeConfig;
2122
use sled_agent_types::early_networking::RouterLifetimeConfigError;
23+
use sled_agent_types::early_networking::RouterPeerIpAddr;
24+
use sled_agent_types::early_networking::RouterPeerIpAddrError;
25+
use sled_agent_types::early_networking::RouterPeerType;
2226
use slog_error_chain::InlineErrorChain;
23-
use std::net::IpAddr;
24-
use std::net::Ipv6Addr;
2527
use uuid::Uuid;
2628

2729
#[derive(
@@ -172,24 +174,52 @@ pub struct BgpPeerView {
172174
pub enum BgpPeerConfigDataError {
173175
#[error("database contains illegal router lifetime value")]
174176
RouterLifetime(#[source] RouterLifetimeConfigError),
177+
#[error("database contains illegal router peer address")]
178+
Address(#[source] RouterPeerIpAddrError),
175179
}
176180

177181
impl TryFrom<BgpPeerView> for BgpPeerConfig {
178182
type Error = BgpPeerConfigDataError;
179183

180184
fn try_from(value: BgpPeerView) -> Result<Self, Self::Error> {
181-
// For unnumbered peers (addr is None), use UNSPECIFIED
182-
let addr = match value.addr {
183-
None => IpAddr::V6(Ipv6Addr::UNSPECIFIED),
184-
Some(addr) => addr.ip(),
185-
};
186-
187-
// TODO-correctness We should have db constraints to ensure these can't
185+
// TODO-correctness We should have db constraints to ensure this can't
188186
// fail.
189187
let router_lifetime =
190188
RouterLifetimeConfig::new(value.router_lifetime.0)
191189
.map_err(BgpPeerConfigDataError::RouterLifetime)?;
192-
let min_ttl = value.min_ttl.map(|val| val.0);
190+
191+
// Convert weaker database representation IP address back to a
192+
// strongly-typed `RouterPeerType`.
193+
let addr = match value
194+
.addr
195+
.map(|addr| RouterPeerIpAddr::try_from(addr.ip()))
196+
{
197+
Some(Ok(ip)) => RouterPeerType::Numbered { ip },
198+
199+
// TODO-cleanup This allows any of three DB values (NULL, `0.0.0.0`,
200+
// `::`) to be converted to `RouterPeerType::Unnumbered`. Should we
201+
// add db constraints to squish that down to one (probably NULL)?
202+
Some(Err(RouterPeerIpAddrError {
203+
err: InvalidIpAddrError::UnspecifiedAddress,
204+
..
205+
}))
206+
| None => RouterPeerType::Unnumbered { router_lifetime },
207+
208+
// We should never see any other kind of invalid address as a peer -
209+
// those will fail if we try to send them to maghemite anyway. Bail
210+
// out as early as we can.
211+
Some(Err(
212+
err @ RouterPeerIpAddrError {
213+
err:
214+
InvalidIpAddrError::LoopbackAddress
215+
| InvalidIpAddrError::MulticastAddress
216+
| InvalidIpAddrError::Ipv4Broadcast
217+
| InvalidIpAddrError::Ipv6UnicastLinkLocal
218+
| InvalidIpAddrError::Ipv4MappedIpv6,
219+
..
220+
},
221+
)) => return Err(BgpPeerConfigDataError::Address(err)),
222+
};
193223

194224
Ok(Self {
195225
asn: *value.asn,
@@ -203,7 +233,7 @@ impl TryFrom<BgpPeerView> for BgpPeerConfig {
203233
enforce_first_as: value.enforce_first_as,
204234
local_pref: value.local_pref.map(|x| x.into()),
205235
md5_auth_key: value.md5_auth_key,
206-
min_ttl,
236+
min_ttl: value.min_ttl.map(|val| val.0),
207237
multi_exit_discriminator: value
208238
.multi_exit_discriminator
209239
.map(|x| x.into()),
@@ -212,7 +242,6 @@ impl TryFrom<BgpPeerView> for BgpPeerConfig {
212242
allowed_export: ImportExportPolicy::NoFiltering,
213243
allowed_import: ImportExportPolicy::NoFiltering,
214244
vlan_id: value.vlan_id.map(|x| x.0),
215-
router_lifetime,
216245
})
217246
}
218247
}

‎nexus/mgs-updates/src/test_util/host_phase_2_test_state.rs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,7 @@ mod api_impl {
271271
use sled_agent_types_versions::v20;
272272
use sled_agent_types_versions::v25;
273273
use sled_agent_types_versions::v26;
274+
use sled_agent_types_versions::v30;
274275
use sled_diagnostics::SledDiagnosticsQueryOutput;
275276
use std::collections::BTreeMap;
276277
use std::collections::BTreeSet;
@@ -771,6 +772,13 @@ mod api_impl {
771772
unimplemented!()
772773
}
773774

775+
async fn write_network_bootstore_config_v30(
776+
_rqctx: RequestContext<Self::Context>,
777+
_body: TypedBody<v30::early_networking::WriteNetworkConfigRequest>,
778+
) -> Result<HttpResponseUpdatedNoContent, HttpError> {
779+
unimplemented!()
780+
}
781+
774782
async fn write_network_bootstore_config_v26(
775783
_rqctx: RequestContext<Self::Context>,
776784
_body: TypedBody<v26::early_networking::WriteNetworkConfigRequest>,

‎nexus/src/app/background/tasks/sync_switch_configuration.rs‎

Lines changed: 70 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -56,14 +56,17 @@ use sled_agent_types::early_networking::BgpPeerConfig as SledBgpPeerConfig;
5656
use sled_agent_types::early_networking::EarlyNetworkConfigBody;
5757
use sled_agent_types::early_networking::EarlyNetworkConfigEnvelope;
5858
use sled_agent_types::early_networking::ImportExportPolicy;
59+
use sled_agent_types::early_networking::InvalidIpAddrError;
5960
use sled_agent_types::early_networking::LldpAdminStatus;
6061
use sled_agent_types::early_networking::LldpPortConfig;
6162
use sled_agent_types::early_networking::MaxPathConfig;
6263
use sled_agent_types::early_networking::PortConfig;
6364
use sled_agent_types::early_networking::RackNetworkConfig;
6465
use sled_agent_types::early_networking::RouteConfig as SledRouteConfig;
66+
use sled_agent_types::early_networking::RouterPeerType;
6567
use sled_agent_types::early_networking::SwitchSlot;
6668
use sled_agent_types::early_networking::TxEqConfig;
69+
use sled_agent_types::early_networking::UplinkAddress;
6770
use sled_agent_types::early_networking::UplinkAddressConfig;
6871
use sled_agent_types::early_networking::WriteNetworkConfigRequest;
6972
use slog_error_chain::InlineErrorChain;
@@ -468,7 +471,7 @@ impl BackgroundTask for SwitchPortSettingsManager {
468471
//
469472
// calculate and apply switch zone SMF changes
470473
//
471-
let uplinks = uplinks(&changes);
474+
let uplinks = uplinks(&changes, &log);
472475

473476
// yeet the messages
474477
for (switch_slot, config) in &uplinks {
@@ -1097,22 +1100,42 @@ impl BackgroundTask for SwitchPortSettingsManager {
10971100
log,
10981101
"failed to convert database peer configs to \
10991102
API peer configs";
1103+
"switch_slot" => ?switch_slot,
1104+
"port" => &port.port_name.to_string(),
1105+
InlineErrorChain::new(&err),
1106+
);
1107+
continue;
1108+
}
1109+
};
1110+
1111+
let addresses = match info
1112+
.addresses
1113+
.iter()
1114+
.map(|a| {
1115+
let address = UplinkAddress::try_from_ip_net_treating_unspecified_as_addrconf(a.address)?;
1116+
Ok(UplinkAddressConfig {
1117+
address,
1118+
vlan_id: a.vlan_id
1119+
})
1120+
})
1121+
.collect::<Result<_, InvalidIpAddrError>>()
1122+
{
1123+
Ok(addresses) => addresses,
1124+
Err(err) => {
1125+
error!(
1126+
log,
1127+
"failed to convert database uplink addresses \
1128+
to API uplink addresses";
1129+
"switch_slot" => ?switch_slot,
1130+
"port" => &port.port_name.to_string(),
11001131
InlineErrorChain::new(&err),
11011132
);
11021133
continue;
11031134
}
11041135
};
11051136

11061137
let mut port_config = PortConfig {
1107-
addresses: info
1108-
.addresses
1109-
.iter()
1110-
.map(|a|
1111-
UplinkAddressConfig {
1112-
address: if a.address.addr().is_unspecified() {None} else {Some(a.address)},
1113-
vlan_id: a.vlan_id
1114-
}
1115-
).collect(),
1138+
addresses,
11161139
autoneg: info
11171140
.links
11181141
.get(0) //TODO breakout support
@@ -1162,11 +1185,15 @@ impl BackgroundTask for SwitchPortSettingsManager {
11621185
;
11631186

11641187
for peer in port_config.bgp_peers.iter_mut() {
1165-
// For unnumbered peers (addr is UNSPECIFIED), pass None
1166-
let peer_addr_for_lookup = if peer.addr.is_unspecified() {
1167-
None
1168-
} else {
1169-
Some(peer.addr)
1188+
// For unnumbered peers, pass None
1189+
//
1190+
// TODO-cleanup Push `RouterPeerAddress` down to all the
1191+
// datastore methods below instead of an `Option`.
1192+
let peer_addr_for_lookup = match peer.addr {
1193+
RouterPeerType::Unnumbered { .. } => None,
1194+
RouterPeerType::Numbered { ip } => {
1195+
Some(IpAddr::from(ip))
1196+
}
11701197
};
11711198

11721199
peer.communities = match self
@@ -1679,6 +1706,7 @@ async fn switch_loopback_addresses(
16791706

16801707
fn uplinks(
16811708
changes: &[(SwitchSlot, nexus_db_model::SwitchPort, PortSettingsChange)],
1709+
log: &slog::Logger,
16821710
) -> HashMap<SwitchSlot, Vec<HostPortConfig>> {
16831711
let mut uplinks: HashMap<SwitchSlot, Vec<HostPortConfig>> = HashMap::new();
16841712
for (switch_slot, port, change) in changes {
@@ -1720,20 +1748,35 @@ fn uplinks(
17201748
None
17211749
};
17221750

1751+
let addrs = match config
1752+
.addresses
1753+
.iter()
1754+
.map(|a| {
1755+
let address = UplinkAddress::try_from_ip_net_treating_unspecified_as_addrconf(a.address)?;
1756+
Ok(UplinkAddressConfig {
1757+
address,
1758+
vlan_id: a.vlan_id
1759+
})
1760+
})
1761+
.collect::<Result<_, InvalidIpAddrError>>()
1762+
{
1763+
Ok(addresses) => addresses,
1764+
Err(err) => {
1765+
error!(
1766+
log,
1767+
"failed to convert database uplink addresses to \
1768+
API uplink addresses";
1769+
"switch_slot" => ?switch_slot,
1770+
"port" => &port.port_name.to_string(),
1771+
InlineErrorChain::new(&err),
1772+
);
1773+
continue;
1774+
}
1775+
};
1776+
17231777
let config = HostPortConfig {
17241778
port: port.port_name.to_string(),
1725-
addrs: config
1726-
.addresses
1727-
.iter()
1728-
.map(|a| UplinkAddressConfig {
1729-
address: if a.address.addr().is_unspecified() {
1730-
None
1731-
} else {
1732-
Some(a.address)
1733-
},
1734-
vlan_id: a.vlan_id,
1735-
})
1736-
.collect(),
1779+
addrs,
17371780
lldp,
17381781
tx_eq,
17391782
};

0 commit comments

Comments
 (0)