From f315451857ae669303896d2340fb2137b4a8cfd1 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Wed, 21 May 2025 13:09:03 +0100 Subject: [PATCH 01/27] Initial plumbing. --- Cargo.lock | 26 +-- Cargo.toml | 8 +- clients/sled-agent-client/src/lib.rs | 3 +- common/src/api/external/mod.rs | 189 +++++++++++++++++- illumos-utils/src/opte/firewall_rules.rs | 31 ++- illumos-utils/src/opte/illumos.rs | 2 +- nexus/db-fixed-data/src/vpc_firewall_rule.rs | 52 ++++- nexus/db-model/src/schema_versions.rs | 1 + nexus/db-model/src/vpc_firewall_rule.rs | 42 +++- nexus/db-queries/src/db/datastore/vpc.rs | 10 + nexus/db-schema/src/enums.rs | 1 - nexus/db-schema/src/schema.rs | 2 +- nexus/tests/integration_tests/vpc_firewall.rs | 4 +- openapi/nexus.json | 87 +++++++- openapi/sled-agent.json | 87 +++++++- package-manifest.toml | 12 +- schema/crdb/dbinit.sql | 10 +- schema/crdb/vpc-firewall-icmp/up01.sql | 6 + schema/crdb/vpc-firewall-icmp/up02.sql | 1 + tools/maghemite_ddm_openapi_version | 2 +- tools/maghemite_mg_openapi_version | 4 +- tools/maghemite_mgd_checksums | 4 +- 22 files changed, 504 insertions(+), 80 deletions(-) create mode 100644 schema/crdb/vpc-firewall-icmp/up01.sql create mode 100644 schema/crdb/vpc-firewall-icmp/up02.sql diff --git a/Cargo.lock b/Cargo.lock index 5b317cac72e..4046c08155b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2273,7 +2273,7 @@ dependencies = [ [[package]] name = "ddm-admin-client" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/maghemite?rev=872aae7d76493f2a4f59711b24dde55523536b40#872aae7d76493f2a4f59711b24dde55523536b40" +source = "git+https://github.com/oxidecomputer/maghemite?rev=ec985de970b616ee9977ac7474ab68b91b6e7614#ec985de970b616ee9977ac7474ab68b91b6e7614" dependencies = [ "oxnet", "percent-encoding", @@ -4606,7 +4606,7 @@ dependencies = [ [[package]] name = "illumos-sys-hdrs" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=9e79588b11a69ff3d40f9aa29af97e31a0045c4b#9e79588b11a69ff3d40f9aa29af97e31a0045c4b" +source = "git+https://github.com/oxidecomputer/opte?rev=a2e580038ebbb4aa78afa8b6078c420a2043a2c7#a2e580038ebbb4aa78afa8b6078c420a2043a2c7" dependencies = [ "bitflags 2.9.0", ] @@ -5179,7 +5179,7 @@ dependencies = [ [[package]] name = "kstat-macro" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=9e79588b11a69ff3d40f9aa29af97e31a0045c4b#9e79588b11a69ff3d40f9aa29af97e31a0045c4b" +source = "git+https://github.com/oxidecomputer/opte?rev=a2e580038ebbb4aa78afa8b6078c420a2043a2c7#a2e580038ebbb4aa78afa8b6078c420a2043a2c7" dependencies = [ "quote", "syn 2.0.101", @@ -5713,7 +5713,7 @@ dependencies = [ [[package]] name = "mg-admin-client" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/maghemite?rev=872aae7d76493f2a4f59711b24dde55523536b40#872aae7d76493f2a4f59711b24dde55523536b40" +source = "git+https://github.com/oxidecomputer/maghemite?rev=ec985de970b616ee9977ac7474ab68b91b6e7614#ec985de970b616ee9977ac7474ab68b91b6e7614" dependencies = [ "anyhow", "chrono", @@ -8209,10 +8209,9 @@ dependencies = [ [[package]] name = "opte" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=9e79588b11a69ff3d40f9aa29af97e31a0045c4b#9e79588b11a69ff3d40f9aa29af97e31a0045c4b" +source = "git+https://github.com/oxidecomputer/opte?rev=a2e580038ebbb4aa78afa8b6078c420a2043a2c7#a2e580038ebbb4aa78afa8b6078c420a2043a2c7" dependencies = [ "bitflags 2.9.0", - "cfg-if", "dyn-clone", "illumos-sys-hdrs", "ingot", @@ -8220,15 +8219,15 @@ dependencies = [ "opte-api", "postcard", "serde", - "smoltcp 0.11.0", "tabwriter", "version_check", + "zerocopy 0.8.25", ] [[package]] name = "opte-api" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=9e79588b11a69ff3d40f9aa29af97e31a0045c4b#9e79588b11a69ff3d40f9aa29af97e31a0045c4b" +source = "git+https://github.com/oxidecomputer/opte?rev=a2e580038ebbb4aa78afa8b6078c420a2043a2c7#a2e580038ebbb4aa78afa8b6078c420a2043a2c7" dependencies = [ "illumos-sys-hdrs", "ingot", @@ -8241,7 +8240,7 @@ dependencies = [ [[package]] name = "opte-ioctl" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=9e79588b11a69ff3d40f9aa29af97e31a0045c4b#9e79588b11a69ff3d40f9aa29af97e31a0045c4b" +source = "git+https://github.com/oxidecomputer/opte?rev=a2e580038ebbb4aa78afa8b6078c420a2043a2c7#a2e580038ebbb4aa78afa8b6078c420a2043a2c7" dependencies = [ "libc", "libnet", @@ -8310,14 +8309,12 @@ dependencies = [ [[package]] name = "oxide-vpc" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=9e79588b11a69ff3d40f9aa29af97e31a0045c4b#9e79588b11a69ff3d40f9aa29af97e31a0045c4b" +source = "git+https://github.com/oxidecomputer/opte?rev=a2e580038ebbb4aa78afa8b6078c420a2043a2c7#a2e580038ebbb4aa78afa8b6078c420a2043a2c7" dependencies = [ "cfg-if", "illumos-sys-hdrs", "opte", - "poptrie", "serde", - "smoltcp 0.11.0", "tabwriter", "uuid", "zerocopy 0.8.25", @@ -9351,11 +9348,6 @@ dependencies = [ "universal-hash", ] -[[package]] -name = "poptrie" -version = "0.1.0" -source = "git+https://github.com/oxidecomputer/poptrie?branch=multipath#ca52bef3f87ff1a67d81b3c6e601dcb5fdbcc165" - [[package]] name = "portable-atomic" version = "1.11.0" diff --git a/Cargo.toml b/Cargo.toml index a56ee32e827..2d599c6290e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -504,8 +504,8 @@ lldp_protocol = { git = "https://github.com/oxidecomputer/lldp", package = "prot macaddr = { version = "1.0.1", features = ["serde_std"] } maplit = "1.0.2" newtype_derive = "0.1.6" -mg-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "872aae7d76493f2a4f59711b24dde55523536b40" } -ddm-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "872aae7d76493f2a4f59711b24dde55523536b40" } +mg-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "ec985de970b616ee9977ac7474ab68b91b6e7614" } +ddm-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "ec985de970b616ee9977ac7474ab68b91b6e7614" } multimap = "0.10.1" nexus-auth = { path = "nexus/auth" } nexus-background-task-interface = { path = "nexus/background-task-interface" } @@ -559,7 +559,7 @@ omicron-test-utils = { path = "test-utils" } omicron-workspace-hack = "0.1.0" omicron-zone-package = "0.12.2" oxide-client = { path = "clients/oxide-client" } -oxide-vpc = { git = "https://github.com/oxidecomputer/opte", rev = "9e79588b11a69ff3d40f9aa29af97e31a0045c4b", features = [ "api", "std" ] } +oxide-vpc = { git = "https://github.com/oxidecomputer/opte", rev = "a2e580038ebbb4aa78afa8b6078c420a2043a2c7", features = [ "api", "std" ] } oxlog = { path = "dev-tools/oxlog" } oxnet = "0.1.2" once_cell = "1.21.3" @@ -569,7 +569,7 @@ openapiv3 = "2.0.0" # must match samael's crate! openssl = "0.10" openssl-sys = "0.9" -opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "9e79588b11a69ff3d40f9aa29af97e31a0045c4b" } +opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "a2e580038ebbb4aa78afa8b6078c420a2043a2c7" } oso = "0.27" owo-colors = "4.2.0" oximeter = { path = "oximeter/oximeter" } diff --git a/clients/sled-agent-client/src/lib.rs b/clients/sled-agent-client/src/lib.rs index b27f3048439..0333a78d3fe 100644 --- a/clients/sled-agent-client/src/lib.rs +++ b/clients/sled-agent-client/src/lib.rs @@ -91,6 +91,7 @@ progenitor::generate_api!( TypedUuidForSupportBundleKind = omicron_uuid_kinds::SupportBundleUuid, TypedUuidForZpoolKind = omicron_uuid_kinds::ZpoolUuid, Vni = omicron_common::api::external::Vni, + VpcFirewallIcmpFilter = omicron_common::api::external::VpcFirewallIcmpFilter, ZpoolKind = omicron_common::zpool_name::ZpoolKind, ZpoolName = omicron_common::zpool_name::ZpoolName, } @@ -309,7 +310,7 @@ impl From match s { Tcp => Self::Tcp, Udp => Self::Udp, - Icmp => Self::Icmp, + Icmp(v) => Self::Icmp(v), } } } diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index a7eef17b2fb..624cba3b411 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -43,6 +43,7 @@ use std::fmt::Result as FormatResult; use std::net::IpAddr; use std::net::Ipv4Addr; use std::num::{NonZeroU16, NonZeroU32}; +use std::ops::RangeInclusive; use std::str::FromStr; use tufaceous_artifact::ArtifactHash; use uuid::Uuid; @@ -1832,11 +1833,107 @@ pub struct VpcFirewallRuleFilter { /// The protocols that may be specified in a firewall rule's filter #[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize, JsonSchema)] -#[serde(rename_all = "UPPERCASE")] +#[serde(rename_all = "snake_case")] +#[serde(tag = "type", content = "value")] pub enum VpcFirewallRuleProtocol { Tcp, Udp, - Icmp, + Icmp(Option), + // TODO: IPv6 not supported by instances. + // Icmpv6(Option), + // TODO: OPTE does not yet permit further L4 protocols. (opte#609) + // Other(u16), +} + +impl FromStr for VpcFirewallRuleProtocol { + type Err = String; + + fn from_str(proto: &str) -> Result { + let (ty_str, content_str) = match proto.split_once(':') { + None => (proto, None), + Some((lhs, rhs)) => (lhs, Some(rhs)), + }; + + match (ty_str.to_lowercase().as_str(), content_str) { + ("tcp", None) => Ok(Self::Tcp), + ("udp", None) => Ok(Self::Udp), + ("icmp", None) => Ok(Self::Icmp(None)), + ("icmp", Some(rhs)) => Ok(Self::Icmp(Some(rhs.parse()?))), + (lhs, None) => Err(format!("unrecognized protocol \"{lhs}\"")), + (lhs, Some(_)) => Err(format!( + "cannot specify extra filters for protocol \"{lhs}\"" + )), + } + } +} + +impl TryFrom for VpcFirewallRuleProtocol { + type Error = ::Err; + + fn try_from(proto: String) -> Result { + proto.parse() + } +} + +impl Display for VpcFirewallRuleProtocol { + fn fmt(&self, f: &mut Formatter<'_>) -> FormatResult { + match self { + VpcFirewallRuleProtocol::Tcp => write!(f, "tcp"), + VpcFirewallRuleProtocol::Udp => write!(f, "udp"), + VpcFirewallRuleProtocol::Icmp(None) => write!(f, "icmp"), + VpcFirewallRuleProtocol::Icmp(Some(v)) => write!(f, "icmp:{v}"), + } + } +} + +#[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize, JsonSchema)] +pub struct VpcFirewallIcmpFilter { + pub ty: u8, + pub code: Option, +} + +impl Display for VpcFirewallIcmpFilter { + fn fmt(&self, f: &mut Formatter<'_>) -> FormatResult { + write!(f, "{}", self.ty)?; + if let Some(code) = self.code { + write!(f, ",{code}")?; + } + Ok(()) + } +} + +impl FromStr for VpcFirewallIcmpFilter { + type Err = String; + + fn from_str(filter: &str) -> Result { + let (ty_str, code_str) = match filter.split_once(',') { + None => (filter, None), + Some((lhs, rhs)) => (lhs, Some(rhs)), + }; + + Ok(Self { + ty: ty_str.parse::().map_err(|e| e.to_string())?, + code: code_str.map(|v| v.parse()).transpose()?, + }) + } +} + +impl From for IcmpParamRange { + fn from(value: u8) -> Self { + Self { first: value, last: value } + } +} + +impl From> for IcmpParamRange { + fn from(value: RangeInclusive) -> Self { + Self { first: *value.start(), last: *value.end() } + } +} + +impl From for RangeInclusive { + fn from(value: IcmpParamRange) -> Self { + value.first..=value.last + } } #[derive(Clone, Copy, Debug, Deserialize, Serialize, PartialEq, JsonSchema)] @@ -2045,6 +2142,94 @@ impl JsonSchema for L4PortRange { } } +/// A range of ICMP(v6) types or codes. This range is inclusive on both ends. +#[derive( + Clone, Copy, Debug, DeserializeFromStr, SerializeDisplay, PartialEq, +)] +pub struct IcmpParamRange { + /// The first number in the range + pub first: u8, + /// The last number in the range + pub last: u8, +} + +impl FromStr for IcmpParamRange { + type Err = String; + fn from_str(range: &str) -> Result { + const INVALID_NUMBER_MSG: &str = "invalid 8-bit number"; + + match range.split_once('-') { + None => { + let port = range + .parse::() + .map_err(|_| INVALID_NUMBER_MSG.to_string())?; + Ok(IcmpParamRange { first: port, last: port }) + } + Some((left, right)) => { + let first = left + .parse::() + .map_err(|_| INVALID_NUMBER_MSG.to_string())?; + let last = right + .parse::() + .map_err(|_| INVALID_NUMBER_MSG.to_string())?; + Ok(IcmpParamRange { first, last }) + } + } + } +} + +impl TryFrom for IcmpParamRange { + type Error = ::Err; + + fn try_from(range: String) -> Result { + range.parse() + } +} + +impl Display for IcmpParamRange { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + if self.first == self.last { + write!(f, "{}", self.first) + } else { + write!(f, "{}-{}", self.first, self.last) + } + } +} + +impl JsonSchema for IcmpParamRange { + fn schema_name() -> String { + "IcmpParamRange".to_string() + } + + fn json_schema( + _: &mut schemars::gen::SchemaGenerator, + ) -> schemars::schema::Schema { + schemars::schema::SchemaObject { + metadata: Some(Box::new(schemars::schema::Metadata { + title: Some("A range of ICMP(v6) types or codes".to_string()), + description: Some( + "An inclusive-inclusive range of ICMP(v6) types or codes. \ + The second value may be omitted to represent a single parameter." + .to_string(), + ), + examples: vec!["3".into(), "20-120".into()], + ..Default::default() + })), + instance_type: Some( + schemars::schema::InstanceType::String.into() + ), + string: Some(Box::new(schemars::schema::StringValidation { + max_length: Some(7), // 3 digits for each port and the dash + min_length: Some(1), + pattern: Some( + r#"^[0-9]{1,3}(-[0-9]{1,3})?$"#.to_string(), + ), + })), + ..Default::default() + }.into() + } +} + /// The `MacAddr` represents a Media Access Control (MAC) address, used to uniquely identify /// hardware devices on a network. // NOTE: We're using the `macaddr` crate for the internal representation. But as with the `ipnet`, diff --git a/illumos-utils/src/opte/firewall_rules.rs b/illumos-utils/src/opte/firewall_rules.rs index 26ab4d6218f..517c2c63bfa 100644 --- a/illumos-utils/src/opte/firewall_rules.rs +++ b/illumos-utils/src/opte/firewall_rules.rs @@ -21,7 +21,6 @@ use oxide_vpc::api::FirewallRule; use oxide_vpc::api::IpAddr; use oxide_vpc::api::Ports; use oxide_vpc::api::ProtoFilter; -use oxide_vpc::api::Protocol; use oxnet::IpNet; trait FromVpcFirewallRule { @@ -100,12 +99,17 @@ impl FromVpcFirewallRule for ResolvedVpcFirewallRule { match self.filter_protocols { Some(ref protos) if !protos.is_empty() => protos .iter() - .map(|proto| { - ProtoFilter::Proto(match proto { - VpcFirewallRuleProtocol::Tcp => Protocol::TCP, - VpcFirewallRuleProtocol::Udp => Protocol::UDP, - VpcFirewallRuleProtocol::Icmp => Protocol::ICMP, - }) + .map(|proto| match proto { + VpcFirewallRuleProtocol::Tcp => ProtoFilter::Tcp, + VpcFirewallRuleProtocol::Udp => ProtoFilter::Udp, + VpcFirewallRuleProtocol::Icmp(v) => { + ProtoFilter::Icmp(v.map(|v| { + oxide_vpc::api::IcmpFilter { + ty: v.ty, + codes: v.code.map(Into::into), + } + })) + } }) .collect(), _ => vec![ProtoFilter::Any], @@ -151,10 +155,19 @@ pub fn opte_firewall_rules( direction, filters: { let mut filters = Filters::new(); + + // Port assignments are incompatible with non + // TCP/UDP protocols. + if matches!( + proto, + ProtoFilter::Tcp | ProtoFilter::Udp + ) { + filters.set_ports(ports.clone()); + } + filters .set_hosts(*hosts) - .set_protocol(*proto) - .set_ports(ports.clone()); + .set_protocol(proto.clone()); filters }, }) diff --git a/illumos-utils/src/opte/illumos.rs b/illumos-utils/src/opte/illumos.rs index 7feaa84af4c..3d1f0c8c707 100644 --- a/illumos-utils/src/opte/illumos.rs +++ b/illumos-utils/src/opte/illumos.rs @@ -141,7 +141,7 @@ pub struct Handle { impl Handle { /// Construct a new handle to the OPTE kernel driver. pub fn new() -> Result { - OpteHdl::open(OpteHdl::XDE_CTL).map(|inner| Self { inner }) + OpteHdl::open().map(|inner| Self { inner }) } } diff --git a/nexus/db-fixed-data/src/vpc_firewall_rule.rs b/nexus/db-fixed-data/src/vpc_firewall_rule.rs index d59e1c556e6..eda549f1a98 100644 --- a/nexus/db-fixed-data/src/vpc_firewall_rule.rs +++ b/nexus/db-fixed-data/src/vpc_firewall_rule.rs @@ -4,9 +4,10 @@ use nexus_types::identity::Resource; use omicron_common::api::external::{ - L4PortRange, VpcFirewallRuleAction, VpcFirewallRuleDirection, - VpcFirewallRuleFilter, VpcFirewallRulePriority, VpcFirewallRuleProtocol, - VpcFirewallRuleStatus, VpcFirewallRuleTarget, VpcFirewallRuleUpdate, + L4PortRange, VpcFirewallIcmpFilter, VpcFirewallRuleAction, + VpcFirewallRuleDirection, VpcFirewallRuleFilter, VpcFirewallRulePriority, + VpcFirewallRuleProtocol, VpcFirewallRuleStatus, VpcFirewallRuleTarget, + VpcFirewallRuleUpdate, }; use std::sync::LazyLock; @@ -69,3 +70,48 @@ pub static NEXUS_VPC_FW_RULE: LazyLock = action: VpcFirewallRuleAction::Allow, priority: VpcFirewallRulePriority(65534), }); + +/// The name for the built-in VPC firewall rule for Nexus. +pub const NEXUS_ICMP_FW_RULE_NAME: &str = "nexus-icmp"; + +/// Built-in VPC firewall rule for Nexus. +/// +/// This rule allows *arbitrary forwarding nodes* on the network to inform the +/// Nexus zone that packets have been explicitly dropped. This is a key part in +/// enabling path MTU discovery, such as when customers are accessing Necus +/// over a VPN. +/// +/// Note that we currently rely on this being exactly one rule to implement the +/// system-level enable/disable endpoint. See `nexus/networking/src/firewall_rules.rs` +/// for more details. +pub static NEXUS_ICMP_FW_RULE: LazyLock = + LazyLock::new(|| VpcFirewallRuleUpdate { + name: NEXUS_VPC_FW_RULE_NAME.parse().unwrap(), + description: + "allow typical inbound ICMP error codes for outbound flows" + .to_string(), + status: VpcFirewallRuleStatus::Enabled, + direction: VpcFirewallRuleDirection::Inbound, + targets: vec![VpcFirewallRuleTarget::Subnet( + super::vpc_subnet::NEXUS_VPC_SUBNET.name().clone(), + )], + filters: VpcFirewallRuleFilter { + hosts: None, + protocols: Some(vec![ + VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter { + // Type 3 -- Destination Unreachable + ty: 3, + // Code 4 -- Fragmentation needed + code: Some(4.into()), + })), + VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter { + // Type 11 -- Time Exceeded + ty: 11, + code: None, + })), + ]), + ports: None, + }, + action: VpcFirewallRuleAction::Allow, + priority: VpcFirewallRulePriority(65534), + }); diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 1582493e309..5450049427c 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock> = LazyLock::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(143, "vpc-firewall-icmp"), KnownVersion::new(142, "bp-add-remove-mupdate-override"), KnownVersion::new(141, "caboose-sign-value"), KnownVersion::new(140, "instance-intended-state"), diff --git a/nexus/db-model/src/vpc_firewall_rule.rs b/nexus/db-model/src/vpc_firewall_rule.rs index faeec7d93f4..a7614f1f675 100644 --- a/nexus/db-model/src/vpc_firewall_rule.rs +++ b/nexus/db-model/src/vpc_firewall_rule.rs @@ -54,19 +54,41 @@ impl_enum_wrapper!( NewtypeFrom! { () pub struct VpcFirewallRuleAction(external::VpcFirewallRuleAction); } NewtypeDeref! { () pub struct VpcFirewallRuleAction(external::VpcFirewallRuleAction); } -impl_enum_wrapper!( - VpcFirewallRuleProtocolEnum: - - #[derive(Clone, Debug, AsExpression, FromSqlRow, Serialize, Deserialize)] - pub struct VpcFirewallRuleProtocol(pub external::VpcFirewallRuleProtocol); - - Tcp => b"TCP" - Udp => b"UDP" - Icmp => b"ICMP" -); +/// Newtype wrapper around [`external::VpcFirewallRuleProtocol`] so we can derive +/// diesel traits for it +#[derive(Clone, Debug, AsExpression, FromSqlRow, Serialize, Deserialize)] +#[diesel(sql_type = sql_types::Text)] +#[repr(transparent)] +pub struct VpcFirewallRuleProtocol(pub external::VpcFirewallRuleProtocol); NewtypeFrom! { () pub struct VpcFirewallRuleProtocol(external::VpcFirewallRuleProtocol); } NewtypeDeref! { () pub struct VpcFirewallRuleProtocol(external::VpcFirewallRuleProtocol); } +impl ToSql for VpcFirewallRuleProtocol { + fn to_sql<'a>( + &'a self, + out: &mut serialize::Output<'a, '_, Pg>, + ) -> serialize::Result { + >::to_sql( + &self.0.to_string(), + &mut out.reborrow(), + ) + } +} + +// Deserialize the "VpcFirewallRuleProtocol" object from SQL TEXT. +impl FromSql for VpcFirewallRuleProtocol +where + DB: Backend, + String: FromSql, +{ + fn from_sql(bytes: DB::RawValue<'_>) -> deserialize::Result { + Ok(VpcFirewallRuleProtocol( + String::from_sql(bytes)? + .parse::()?, + )) + } +} + /// Newtype wrapper around [`external::VpcFirewallRuleTarget`] so we can derive /// diesel traits for it #[derive(Clone, Debug, AsExpression, FromSqlRow, Serialize, Deserialize)] diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index c35ae5f76ab..6a41ab9f2a5 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -235,6 +235,7 @@ impl DataStore { opctx: &OpContext, ) -> Result<(), Error> { use nexus_db_fixed_data::vpc_firewall_rule::DNS_VPC_FW_RULE; + use nexus_db_fixed_data::vpc_firewall_rule::NEXUS_ICMP_FW_RULE; use nexus_db_fixed_data::vpc_firewall_rule::NEXUS_VPC_FW_RULE; debug!(opctx.log, "attempting to create built-in VPC firewall rules"); @@ -273,6 +274,15 @@ impl DataStore { fw_rules.insert(NEXUS_VPC_FW_RULE.name.clone(), rule); } + if !fw_rules.contains_key(&NEXUS_ICMP_FW_RULE.name) { + let rule = VpcFirewallRule::new( + Uuid::new_v4(), + *SERVICES_VPC_ID, + &NEXUS_ICMP_FW_RULE, + )?; + fw_rules.insert(NEXUS_ICMP_FW_RULE.name.clone(), rule); + } + let rules = fw_rules .into_values() .map(|mut rule| { diff --git a/nexus/db-schema/src/enums.rs b/nexus/db-schema/src/enums.rs index 79d312c8b64..cc03776c8d0 100644 --- a/nexus/db-schema/src/enums.rs +++ b/nexus/db-schema/src/enums.rs @@ -82,7 +82,6 @@ define_enums! { VolumeResourceUsageTypeEnum => "volume_resource_usage_type", VpcFirewallRuleActionEnum => "vpc_firewall_rule_action", VpcFirewallRuleDirectionEnum => "vpc_firewall_rule_direction", - VpcFirewallRuleProtocolEnum => "vpc_firewall_rule_protocol", VpcFirewallRuleStatusEnum => "vpc_firewall_rule_status", VpcRouterKindEnum => "vpc_router_kind", WebhookEventClassEnum => "webhook_event_class", diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index 89d92585ce0..f268fe93645 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -1297,7 +1297,7 @@ table! { targets -> Array, filter_hosts -> Nullable>, filter_ports -> Nullable>, - filter_protocols -> Nullable>, + filter_protocols -> Nullable>, action -> crate::enums::VpcFirewallRuleActionEnum, priority -> Int4, } diff --git a/nexus/tests/integration_tests/vpc_firewall.rs b/nexus/tests/integration_tests/vpc_firewall.rs index 83bc5599ba8..ea7abc6c024 100644 --- a/nexus/tests/integration_tests/vpc_firewall.rs +++ b/nexus/tests/integration_tests/vpc_firewall.rs @@ -90,7 +90,7 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { filters: VpcFirewallRuleFilter { hosts: None, ports: None, - protocols: Some(vec![VpcFirewallRuleProtocol::Icmp]), + protocols: Some(vec![VpcFirewallRuleProtocol::Icmp(None)]), }, direction: VpcFirewallRuleDirection::Inbound, priority: VpcFirewallRulePriority(10), @@ -191,7 +191,7 @@ fn is_default_firewall_rules( )], filters: VpcFirewallRuleFilter { hosts: None, - protocols: Some(vec![VpcFirewallRuleProtocol::Icmp]), + protocols: Some(vec![VpcFirewallRuleProtocol::Icmp(None)]), ports: None, }, action: VpcFirewallRuleAction::Allow, diff --git a/openapi/nexus.json b/openapi/nexus.json index 8ba6882ed3b..910f6b83130 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -18255,6 +18255,15 @@ "minLength": 1, "maxLength": 253 }, + "IcmpParamRange": { + "example": "3", + "title": "A range of ICMP(v6) types or codes", + "description": "An inclusive-inclusive range of ICMP(v6) types or codes. The second value may be omitted to represent a single parameter.", + "type": "string", + "pattern": "^[0-9]{1,3}(-[0-9]{1,3})?$", + "minLength": 1, + "maxLength": 7 + }, "IdentityProvider": { "description": "View of an Identity Provider", "type": "object", @@ -24824,6 +24833,27 @@ "name" ] }, + "VpcFirewallIcmpFilter": { + "type": "object", + "properties": { + "code": { + "nullable": true, + "allOf": [ + { + "$ref": "#/components/schemas/IcmpParamRange" + } + ] + }, + "ty": { + "type": "integer", + "format": "uint8", + "minimum": 0 + } + }, + "required": [ + "ty" + ] + }, "VpcFirewallRule": { "description": "A single rule in a VPC firewall", "type": "object", @@ -25071,11 +25101,58 @@ }, "VpcFirewallRuleProtocol": { "description": "The protocols that may be specified in a firewall rule's filter", - "type": "string", - "enum": [ - "TCP", - "UDP", - "ICMP" + "oneOf": [ + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "tcp" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "udp" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "icmp" + ] + }, + "value": { + "nullable": true, + "allOf": [ + { + "$ref": "#/components/schemas/VpcFirewallIcmpFilter" + } + ] + } + }, + "required": [ + "type", + "value" + ] + } ] }, "VpcFirewallRuleStatus": { diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index 3aeac93c776..67e836f97c8 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -4566,6 +4566,15 @@ ], "additionalProperties": false }, + "IcmpParamRange": { + "example": "3", + "title": "A range of ICMP(v6) types or codes", + "description": "An inclusive-inclusive range of ICMP(v6) types or codes. The second value may be omitted to represent a single parameter.", + "type": "string", + "pattern": "^[0-9]{1,3}(-[0-9]{1,3})?$", + "minLength": 1, + "maxLength": 7 + }, "IdMapDatasetConfig": { "type": "object", "additionalProperties": { @@ -7501,6 +7510,27 @@ "format": "uint32", "minimum": 0 }, + "VpcFirewallIcmpFilter": { + "type": "object", + "properties": { + "code": { + "nullable": true, + "allOf": [ + { + "$ref": "#/components/schemas/IcmpParamRange" + } + ] + }, + "ty": { + "type": "integer", + "format": "uint8", + "minimum": 0 + } + }, + "required": [ + "ty" + ] + }, "VpcFirewallRuleAction": { "type": "string", "enum": [ @@ -7517,11 +7547,58 @@ }, "VpcFirewallRuleProtocol": { "description": "The protocols that may be specified in a firewall rule's filter", - "type": "string", - "enum": [ - "TCP", - "UDP", - "ICMP" + "oneOf": [ + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "tcp" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "udp" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "icmp" + ] + }, + "value": { + "nullable": true, + "allOf": [ + { + "$ref": "#/components/schemas/VpcFirewallIcmpFilter" + } + ] + } + }, + "required": [ + "type", + "value" + ] + } ] }, "VpcFirewallRuleStatus": { diff --git a/package-manifest.toml b/package-manifest.toml index 35d762b2814..2166517e224 100644 --- a/package-manifest.toml +++ b/package-manifest.toml @@ -638,10 +638,10 @@ source.repo = "maghemite" # `tools/maghemite_openapi_version`. Failing to do so will cause a failure when # building `ddm-admin-client` (which will instruct you to update # `tools/maghemite_openapi_version`). -source.commit = "872aae7d76493f2a4f59711b24dde55523536b40" +source.commit = "ec985de970b616ee9977ac7474ab68b91b6e7614" # The SHA256 digest is automatically posted to: # https://buildomat.eng.oxide.computer/public/file/oxidecomputer/maghemite/image//mg-ddm-gz.sha256.txt -source.sha256 = "1b045d2d8868f4ff2f4d8b2d7b2832c90439048a7b7445cb683ce98dd6960e63" +source.sha256 = "5c3697573a883f4d99b141e0395fc4f68c5402b88018469b334a2ef8cf4e9a02" output.type = "tarball" [package.mg-ddm] @@ -654,10 +654,10 @@ source.repo = "maghemite" # `tools/maghemite_openapi_version`. Failing to do so will cause a failure when # building `ddm-admin-client` (which will instruct you to update # `tools/maghemite_openapi_version`). -source.commit = "872aae7d76493f2a4f59711b24dde55523536b40" +source.commit = "ec985de970b616ee9977ac7474ab68b91b6e7614" # The SHA256 digest is automatically posted to: # https://buildomat.eng.oxide.computer/public/file/oxidecomputer/maghemite/image//mg-ddm.sha256.txt -source.sha256 = "df6e08e78c2f593e52d5730d53442f4e179129e93bc3771fbc85c5643d9ad47f" +source.sha256 = "737227644e49d1b82fabbdc3a05cf9a994a85c0d4966afefd1d8d6504d429295" output.type = "zone" output.intermediate_only = true @@ -669,10 +669,10 @@ source.repo = "maghemite" # `tools/maghemite_openapi_version`. Failing to do so will cause a failure when # building `ddm-admin-client` (which will instruct you to update # `tools/maghemite_openapi_version`). -source.commit = "872aae7d76493f2a4f59711b24dde55523536b40" +source.commit = "ec985de970b616ee9977ac7474ab68b91b6e7614" # The SHA256 digest is automatically posted to: # https://buildomat.eng.oxide.computer/public/file/oxidecomputer/maghemite/image//mgd.sha256.txt -source.sha256 = "b917e4952f459fe8aae00736cf10f28ab75a71eac6d7df8b1835da5ddae794ba" +source.sha256 = "6de8eaf660293212785ed5d233c3c66c41f8852a5a7ac44d56fe997fe6d27495" output.type = "zone" output.intermediate_only = true diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 9185123b558..0221ec1b620 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -1846,12 +1846,6 @@ CREATE TYPE IF NOT EXISTS omicron.public.vpc_firewall_rule_action AS ENUM ( 'deny' ); -CREATE TYPE IF NOT EXISTS omicron.public.vpc_firewall_rule_protocol AS ENUM ( - 'TCP', - 'UDP', - 'ICMP' -); - CREATE TABLE IF NOT EXISTS omicron.public.vpc_firewall_rule ( /* Identity metadata (resource) */ id UUID PRIMARY KEY, @@ -1871,7 +1865,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.vpc_firewall_rule ( /* Also an array of targets */ filter_hosts STRING(128)[], filter_ports STRING(11)[], - filter_protocols omicron.public.vpc_firewall_rule_protocol[], + filter_protocols STRING(32)[], action omicron.public.vpc_firewall_rule_action NOT NULL, priority INT4 CHECK (priority BETWEEN 0 AND 65535) NOT NULL ); @@ -5518,7 +5512,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '142.0.0', NULL) + (TRUE, NOW(), NOW(), '143.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/vpc-firewall-icmp/up01.sql b/schema/crdb/vpc-firewall-icmp/up01.sql new file mode 100644 index 00000000000..d6e5e452e23 --- /dev/null +++ b/schema/crdb/vpc-firewall-icmp/up01.sql @@ -0,0 +1,6 @@ +-- We need to support more specific filters on given protocol families +-- (e.g., optional ICMP code/type specifiers). Move to have Nexus +-- parse/deparse these. This also opens the door for matches by IpProtocol +-- ID (numeric) in future. +ALTER TABLE omicron.public.vpc_firewall_rule + ALTER COLUMN filter_protocols TYPE STRING(32)[]; diff --git a/schema/crdb/vpc-firewall-icmp/up02.sql b/schema/crdb/vpc-firewall-icmp/up02.sql new file mode 100644 index 00000000000..24f1b68b45a --- /dev/null +++ b/schema/crdb/vpc-firewall-icmp/up02.sql @@ -0,0 +1 @@ +DROP TYPE IF EXISTS omicron.public.vpc_firewall_rule_protocol; diff --git a/tools/maghemite_ddm_openapi_version b/tools/maghemite_ddm_openapi_version index 94c5d48afd0..6df1023c4fa 100644 --- a/tools/maghemite_ddm_openapi_version +++ b/tools/maghemite_ddm_openapi_version @@ -1,2 +1,2 @@ -COMMIT="872aae7d76493f2a4f59711b24dde55523536b40" +COMMIT="ec985de970b616ee9977ac7474ab68b91b6e7614" SHA2="9146aaf60a52ecd138139708e4019e4496f330fb81a2c5a7a70cd3436a6a1318" diff --git a/tools/maghemite_mg_openapi_version b/tools/maghemite_mg_openapi_version index bae6d404dbf..c14c358fda2 100644 --- a/tools/maghemite_mg_openapi_version +++ b/tools/maghemite_mg_openapi_version @@ -1,2 +1,2 @@ -COMMIT="872aae7d76493f2a4f59711b24dde55523536b40" -SHA2="b24ab6aacc2a40421ff1bbf4983121209d8079f1656a6f986e8f92d55e0dade1" +COMMIT="ec985de970b616ee9977ac7474ab68b91b6e7614" +SHA2="7af1675e2e93e395185f8d3676db972db0123714c4c5640608f3e3570f3ce3a8" diff --git a/tools/maghemite_mgd_checksums b/tools/maghemite_mgd_checksums index 7fb57c60cca..13c1761de11 100644 --- a/tools/maghemite_mgd_checksums +++ b/tools/maghemite_mgd_checksums @@ -1,2 +1,2 @@ -CIDL_SHA256="b917e4952f459fe8aae00736cf10f28ab75a71eac6d7df8b1835da5ddae794ba" -MGD_LINUX_SHA256="c7eb61321ac5a46c962c8cb8b90fb3616cae56ba3098feaa510a191a7344d27f" \ No newline at end of file +CIDL_SHA256="6de8eaf660293212785ed5d233c3c66c41f8852a5a7ac44d56fe997fe6d27495" +MGD_LINUX_SHA256="0131bc10fe0011a9a39deab1cd2e41a1c98c0c1b7611421e44d5e976ac14f951" \ No newline at end of file From 8a3317069128cca3040eb7ab7cf478d2d4dc59eb Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Wed, 21 May 2025 13:32:57 +0100 Subject: [PATCH 02/27] More OpteHdl fixups. --- illumos-utils/src/opte/port_manager.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/illumos-utils/src/opte/port_manager.rs b/illumos-utils/src/opte/port_manager.rs index d3b1c14b5a8..97eba85e621 100644 --- a/illumos-utils/src/opte/port_manager.rs +++ b/illumos-utils/src/opte/port_manager.rs @@ -355,7 +355,7 @@ impl PortManager { "port_name" => &port_name, "rules" => ?&rules, ); - hdl.set_fw_rules(&oxide_vpc::api::SetFwRulesReq { + hdl.set_firewall_rules(&oxide_vpc::api::SetFwRulesReq { port_name: port_name.clone(), rules, })?; @@ -780,7 +780,7 @@ impl PortManager { "port" => ?&port_name, "rules" => ?&rules, ); - hdl.set_fw_rules(&oxide_vpc::api::SetFwRulesReq { + hdl.set_firewall_rules(&oxide_vpc::api::SetFwRulesReq { port_name, rules, })?; @@ -792,8 +792,7 @@ impl PortManager { &self, ) -> Result, Error> { let hdl = Handle::new()?; - let v2p = - hdl.dump_v2p(&oxide_vpc::api::DumpVirt2PhysReq { unused: 99 })?; + let v2p = hdl.dump_v2p()?; let mut mappings: Vec<_> = vec![]; for mapping in v2p.mappings { From 38bd2ffef109c764b454e377f8f58a741a580745 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Wed, 21 May 2025 13:43:30 +0100 Subject: [PATCH 03/27] Parity. --- illumos-utils/src/opte/non_illumos.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/illumos-utils/src/opte/non_illumos.rs b/illumos-utils/src/opte/non_illumos.rs index f878efc0678..3624a63547b 100644 --- a/illumos-utils/src/opte/non_illumos.rs +++ b/illumos-utils/src/opte/non_illumos.rs @@ -11,7 +11,6 @@ use oxide_vpc::api::ClearVirt2PhysReq; use oxide_vpc::api::DelRouterEntryReq; use oxide_vpc::api::DhcpCfg; use oxide_vpc::api::Direction; -use oxide_vpc::api::DumpVirt2PhysReq; use oxide_vpc::api::DumpVirt2PhysResp; use oxide_vpc::api::IpCfg; use oxide_vpc::api::IpCidr; @@ -230,7 +229,10 @@ impl Handle { } /// Set new firewall rules. - pub fn set_fw_rules(&self, _: &SetFwRulesReq) -> Result { + pub fn set_firewall_rules( + &self, + _: &SetFwRulesReq, + ) -> Result { Ok(NO_RESPONSE) } @@ -289,10 +291,7 @@ impl Handle { } /// Dump a mapping from a virtual NIC to a physical host. - pub fn dump_v2p( - &self, - _: &DumpVirt2PhysReq, - ) -> Result { + pub fn dump_v2p(&self) -> Result { unimplemented!("Not yet used in tests") } From 283fc56ff75ccfdde6eb45dd98c3ef7ed8d23e2e Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Wed, 21 May 2025 20:21:44 +0100 Subject: [PATCH 04/27] CRDB didn't like *that* migration. --- nexus/db-model/src/schema_versions.rs | 2 +- nexus/db-schema/src/schema.rs | 2 +- schema/crdb/dbinit.sql | 4 ++-- schema/crdb/vpc-firewall-icmp/up01.sql | 6 +++++- schema/crdb/vpc-firewall-icmp/up02.sql | 8 +++++++- schema/crdb/vpc-firewall-icmp/up03.sql | 2 ++ schema/crdb/vpc-firewall-icmp/up04.sql | 2 ++ schema/crdb/vpc-firewall-icmp/up05.sql | 1 + schema/crdb/vpc-firewall-icmp/up06.sql | 20 ++++++++++++++++++++ 9 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 schema/crdb/vpc-firewall-icmp/up03.sql create mode 100644 schema/crdb/vpc-firewall-icmp/up04.sql create mode 100644 schema/crdb/vpc-firewall-icmp/up05.sql create mode 100644 schema/crdb/vpc-firewall-icmp/up06.sql diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 5450049427c..e4bc4fff6d3 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock}; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: Version = Version::new(142, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(143, 0, 0); /// List of all past database schema versions, in *reverse* order /// diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index f268fe93645..d9593bd7362 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -1297,9 +1297,9 @@ table! { targets -> Array, filter_hosts -> Nullable>, filter_ports -> Nullable>, - filter_protocols -> Nullable>, action -> crate::enums::VpcFirewallRuleActionEnum, priority -> Int4, + filter_protocols -> Nullable>, } } diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 0221ec1b620..f91e8671c3a 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -1865,9 +1865,9 @@ CREATE TABLE IF NOT EXISTS omicron.public.vpc_firewall_rule ( /* Also an array of targets */ filter_hosts STRING(128)[], filter_ports STRING(11)[], - filter_protocols STRING(32)[], action omicron.public.vpc_firewall_rule_action NOT NULL, - priority INT4 CHECK (priority BETWEEN 0 AND 65535) NOT NULL + priority INT4 CHECK (priority BETWEEN 0 AND 65535) NOT NULL, + filter_protocols STRING(32)[] ); CREATE UNIQUE INDEX IF NOT EXISTS lookup_firewall_by_vpc ON omicron.public.vpc_firewall_rule ( diff --git a/schema/crdb/vpc-firewall-icmp/up01.sql b/schema/crdb/vpc-firewall-icmp/up01.sql index d6e5e452e23..a83680b5ed2 100644 --- a/schema/crdb/vpc-firewall-icmp/up01.sql +++ b/schema/crdb/vpc-firewall-icmp/up01.sql @@ -2,5 +2,9 @@ -- (e.g., optional ICMP code/type specifiers). Move to have Nexus -- parse/deparse these. This also opens the door for matches by IpProtocol -- ID (numeric) in future. +-- +-- However, ALTER COLUMN type from ENUM[] to STRING[] is 'experimental', given +-- the error messages. So we'll be roundtripping through a new column. + ALTER TABLE omicron.public.vpc_firewall_rule - ALTER COLUMN filter_protocols TYPE STRING(32)[]; + ADD COLUMN IF NOT EXISTS filter_protocols_2 STRING(32)[]; diff --git a/schema/crdb/vpc-firewall-icmp/up02.sql b/schema/crdb/vpc-firewall-icmp/up02.sql index 24f1b68b45a..e9a82932215 100644 --- a/schema/crdb/vpc-firewall-icmp/up02.sql +++ b/schema/crdb/vpc-firewall-icmp/up02.sql @@ -1 +1,7 @@ -DROP TYPE IF EXISTS omicron.public.vpc_firewall_rule_protocol; +SET LOCAL disallow_full_table_scans = off; + +-- Convert to lower-case strings. Existing filters like "ICMP" +-- will still be interpreted as ;all ICMP;. +UPDATE omicron.public.vpc_firewall_rule +SET + filter_protocols_2 = lower(filter_protocols::STRING)::STRING(32)[]; diff --git a/schema/crdb/vpc-firewall-icmp/up03.sql b/schema/crdb/vpc-firewall-icmp/up03.sql new file mode 100644 index 00000000000..e25cdc0c228 --- /dev/null +++ b/schema/crdb/vpc-firewall-icmp/up03.sql @@ -0,0 +1,2 @@ +ALTER TABLE omicron.public.vpc_firewall_rule + DROP COLUMN IF EXISTS filter_protocols; diff --git a/schema/crdb/vpc-firewall-icmp/up04.sql b/schema/crdb/vpc-firewall-icmp/up04.sql new file mode 100644 index 00000000000..70237260d53 --- /dev/null +++ b/schema/crdb/vpc-firewall-icmp/up04.sql @@ -0,0 +1,2 @@ +ALTER TABLE omicron.public.vpc_firewall_rule + RENAME COLUMN filter_protocols_2 TO filter_protocols; diff --git a/schema/crdb/vpc-firewall-icmp/up05.sql b/schema/crdb/vpc-firewall-icmp/up05.sql new file mode 100644 index 00000000000..24f1b68b45a --- /dev/null +++ b/schema/crdb/vpc-firewall-icmp/up05.sql @@ -0,0 +1 @@ +DROP TYPE IF EXISTS omicron.public.vpc_firewall_rule_protocol; diff --git a/schema/crdb/vpc-firewall-icmp/up06.sql b/schema/crdb/vpc-firewall-icmp/up06.sql new file mode 100644 index 00000000000..87bcf68419e --- /dev/null +++ b/schema/crdb/vpc-firewall-icmp/up06.sql @@ -0,0 +1,20 @@ +-- Add a well-known rule to services VPC to allow limited forms +-- of ICMP traffic. Inserting this rule is conditional on the +-- '' VPC existing. +INSERT INTO omicron.public.vpc_firewall_rule ( + id, + name, description, + time_created, time_modified, vpc_id, status, direction, + targets, filter_protocols, action, priority +) +SELECT + gen_random_uuid(), + -- Hardcoded name/description, see nexus/db-fixed-data/src/vpc_firewall_rule.rs. + 'nexus-icmp', 'allow typical inbound ICMP error codes for outbound flows', + NOW(), NOW(), vpc.id, 'enabled', 'inbound', + -- Allow inbound ICMP Destination Unreachable (Too Large) and Time Exceeded + ARRAY['subnet:nexus'], ARRAY['icmp:3,4','icmp:11'], 'allow', 65534 +FROM omicron.public.vpc + WHERE vpc.id = '001de000-074c-4000-8000-000000000000' + AND vpc.name = 'oxide-services' +ON CONFLICT DO NOTHING; From 3d137757894951218ebd011524e00babb2afa0b8 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Wed, 21 May 2025 23:03:29 +0100 Subject: [PATCH 05/27] TODO: migration test. --- nexus/tests/integration_tests/schema.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/nexus/tests/integration_tests/schema.rs b/nexus/tests/integration_tests/schema.rs index 9bdc3125219..40ab4213a76 100644 --- a/nexus/tests/integration_tests/schema.rs +++ b/nexus/tests/integration_tests/schema.rs @@ -2187,6 +2187,14 @@ fn after_140_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { }) } +fn before_143_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { + todo!() +} + +fn after_143_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { + todo!() +} + // Lazily initializes all migration checks. The combination of Rust function // pointers and async makes defining a static table fairly painful, so we're // using lazy initialization instead. @@ -2251,6 +2259,10 @@ fn get_migration_checks() -> BTreeMap { Version::new(140, 0, 0), DataMigrationFns::new().before(before_140_0_0).after(after_140_0_0), ); + map.insert( + Version::new(143, 0, 0), + DataMigrationFns::new().before(before_143_0_0).after(after_143_0_0), + ); map } From 29db3dd960cb7803b960e744a01c4b6f66b94688 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Thu, 22 May 2025 10:26:47 +0100 Subject: [PATCH 06/27] Iterating on launch failures. --- nexus/db-fixed-data/src/vpc_firewall_rule.rs | 2 +- schema/crdb/vpc-firewall-icmp/up06.sql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/db-fixed-data/src/vpc_firewall_rule.rs b/nexus/db-fixed-data/src/vpc_firewall_rule.rs index eda549f1a98..44d463b9d81 100644 --- a/nexus/db-fixed-data/src/vpc_firewall_rule.rs +++ b/nexus/db-fixed-data/src/vpc_firewall_rule.rs @@ -86,7 +86,7 @@ pub const NEXUS_ICMP_FW_RULE_NAME: &str = "nexus-icmp"; /// for more details. pub static NEXUS_ICMP_FW_RULE: LazyLock = LazyLock::new(|| VpcFirewallRuleUpdate { - name: NEXUS_VPC_FW_RULE_NAME.parse().unwrap(), + name: NEXUS_ICMP_FW_RULE_NAME.parse().unwrap(), description: "allow typical inbound ICMP error codes for outbound flows" .to_string(), diff --git a/schema/crdb/vpc-firewall-icmp/up06.sql b/schema/crdb/vpc-firewall-icmp/up06.sql index 87bcf68419e..00d6aab11e1 100644 --- a/schema/crdb/vpc-firewall-icmp/up06.sql +++ b/schema/crdb/vpc-firewall-icmp/up06.sql @@ -1,6 +1,6 @@ -- Add a well-known rule to services VPC to allow limited forms -- of ICMP traffic. Inserting this rule is conditional on the --- '' VPC existing. +-- 'oxide-services' VPC existing. INSERT INTO omicron.public.vpc_firewall_rule ( id, name, description, From 01f0f4ca693f17d050a589676b64818a11ad1497 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Thu, 22 May 2025 11:49:14 +0100 Subject: [PATCH 07/27] =?UTF-8?q?`serde=5Fjson::from=5Fstr`=20=F0=9F=AB=A0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit That seems to be be the primary reason for the entire test suite having a Very Bad Time. --- Cargo.lock | 12 ++-- Cargo.toml | 4 +- common/src/api/external/mod.rs | 13 ++++ nexus/defaults/src/lib.rs | 107 +++++++++++++++++++++------------ 4 files changed, 90 insertions(+), 46 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7884351d590..3d959ad2b15 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4608,7 +4608,7 @@ dependencies = [ [[package]] name = "illumos-sys-hdrs" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=a2e580038ebbb4aa78afa8b6078c420a2043a2c7#a2e580038ebbb4aa78afa8b6078c420a2043a2c7" +source = "git+https://github.com/oxidecomputer/opte?rev=792ec8a3816ba8c7c4268f65cd19dbd946a9027d#792ec8a3816ba8c7c4268f65cd19dbd946a9027d" dependencies = [ "bitflags 2.9.0", ] @@ -5181,7 +5181,7 @@ dependencies = [ [[package]] name = "kstat-macro" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=a2e580038ebbb4aa78afa8b6078c420a2043a2c7#a2e580038ebbb4aa78afa8b6078c420a2043a2c7" +source = "git+https://github.com/oxidecomputer/opte?rev=792ec8a3816ba8c7c4268f65cd19dbd946a9027d#792ec8a3816ba8c7c4268f65cd19dbd946a9027d" dependencies = [ "quote", "syn 2.0.101", @@ -8215,7 +8215,7 @@ dependencies = [ [[package]] name = "opte" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=a2e580038ebbb4aa78afa8b6078c420a2043a2c7#a2e580038ebbb4aa78afa8b6078c420a2043a2c7" +source = "git+https://github.com/oxidecomputer/opte?rev=792ec8a3816ba8c7c4268f65cd19dbd946a9027d#792ec8a3816ba8c7c4268f65cd19dbd946a9027d" dependencies = [ "bitflags 2.9.0", "dyn-clone", @@ -8233,7 +8233,7 @@ dependencies = [ [[package]] name = "opte-api" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=a2e580038ebbb4aa78afa8b6078c420a2043a2c7#a2e580038ebbb4aa78afa8b6078c420a2043a2c7" +source = "git+https://github.com/oxidecomputer/opte?rev=792ec8a3816ba8c7c4268f65cd19dbd946a9027d#792ec8a3816ba8c7c4268f65cd19dbd946a9027d" dependencies = [ "illumos-sys-hdrs", "ingot", @@ -8246,7 +8246,7 @@ dependencies = [ [[package]] name = "opte-ioctl" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=a2e580038ebbb4aa78afa8b6078c420a2043a2c7#a2e580038ebbb4aa78afa8b6078c420a2043a2c7" +source = "git+https://github.com/oxidecomputer/opte?rev=792ec8a3816ba8c7c4268f65cd19dbd946a9027d#792ec8a3816ba8c7c4268f65cd19dbd946a9027d" dependencies = [ "libc", "libnet", @@ -8315,7 +8315,7 @@ dependencies = [ [[package]] name = "oxide-vpc" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=a2e580038ebbb4aa78afa8b6078c420a2043a2c7#a2e580038ebbb4aa78afa8b6078c420a2043a2c7" +source = "git+https://github.com/oxidecomputer/opte?rev=792ec8a3816ba8c7c4268f65cd19dbd946a9027d#792ec8a3816ba8c7c4268f65cd19dbd946a9027d" dependencies = [ "cfg-if", "illumos-sys-hdrs", diff --git a/Cargo.toml b/Cargo.toml index fcbcb996cbd..2d379143044 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -561,7 +561,7 @@ omicron-test-utils = { path = "test-utils" } omicron-workspace-hack = "0.1.0" omicron-zone-package = "0.12.2" oxide-client = { path = "clients/oxide-client" } -oxide-vpc = { git = "https://github.com/oxidecomputer/opte", rev = "a2e580038ebbb4aa78afa8b6078c420a2043a2c7", features = [ "api", "std" ] } +oxide-vpc = { git = "https://github.com/oxidecomputer/opte", rev = "792ec8a3816ba8c7c4268f65cd19dbd946a9027d", features = [ "api", "std" ] } oxlog = { path = "dev-tools/oxlog" } oxnet = "0.1.2" once_cell = "1.21.3" @@ -571,7 +571,7 @@ openapiv3 = "2.0.0" # must match samael's crate! openssl = "0.10" openssl-sys = "0.9" -opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "a2e580038ebbb4aa78afa8b6078c420a2043a2c7" } +opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "792ec8a3816ba8c7c4268f65cd19dbd946a9027d" } oso = "0.27" owo-colors = "4.2.0" oximeter = { path = "oximeter/oximeter" } diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index b7ad4058f1f..1ed40bb949c 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -2142,6 +2142,19 @@ impl JsonSchema for L4PortRange { } } +impl From for L4PortRange { + fn from(value: L4Port) -> Self { + Self { first: value, last: value } + } +} + +impl From> for L4PortRange { + fn from(value: RangeInclusive) -> Self { + let (first, last) = value.into_inner(); + Self { first, last } + } +} + /// A range of ICMP(v6) types or codes. This range is inclusive on both ends. #[derive( Clone, Copy, Debug, DeserializeFromStr, SerializeDisplay, PartialEq, diff --git a/nexus/defaults/src/lib.rs b/nexus/defaults/src/lib.rs index 73c090a4d1b..6f6afe171f5 100644 --- a/nexus/defaults/src/lib.rs +++ b/nexus/defaults/src/lib.rs @@ -5,6 +5,18 @@ //! Default values for data in the Nexus API, when not provided explicitly in a request. use omicron_common::api::external; +use omicron_common::api::external::L4Port; +use omicron_common::api::external::Name; +use omicron_common::api::external::VpcFirewallRuleAction; +use omicron_common::api::external::VpcFirewallRuleDirection; +use omicron_common::api::external::VpcFirewallRuleFilter; +use omicron_common::api::external::VpcFirewallRuleHostFilter; +use omicron_common::api::external::VpcFirewallRulePriority; +use omicron_common::api::external::VpcFirewallRuleProtocol; +use omicron_common::api::external::VpcFirewallRuleStatus; +use omicron_common::api::external::VpcFirewallRuleTarget; +use omicron_common::api::external::VpcFirewallRuleUpdate; +use omicron_common::api::external::VpcFirewallRuleUpdateParams; use oxnet::Ipv4Net; use oxnet::Ipv6Net; use std::net::Ipv4Addr; @@ -21,44 +33,63 @@ pub const DEFAULT_PRIMARY_NIC_NAME: &str = "net0"; pub static DEFAULT_VPC_SUBNET_IPV4_BLOCK: LazyLock = LazyLock::new(|| Ipv4Net::new(Ipv4Addr::new(172, 30, 0, 0), 22).unwrap()); -pub static DEFAULT_FIREWALL_RULES: LazyLock< - external::VpcFirewallRuleUpdateParams, -> = LazyLock::new(|| { - serde_json::from_str(r#"{ - "rules": [ - { - "name": "allow-internal-inbound", - "status": "enabled", - "direction": "inbound", - "targets": [ { "type": "vpc", "value": "default" } ], - "filters": { "hosts": [ { "type": "vpc", "value": "default" } ] }, - "action": "allow", - "priority": 65534, - "description": "allow inbound traffic to all instances within the VPC if originated within the VPC" - }, - { - "name": "allow-ssh", - "status": "enabled", - "direction": "inbound", - "targets": [ { "type": "vpc", "value": "default" } ], - "filters": { "ports": [ "22" ], "protocols": [ "TCP" ] }, - "action": "allow", - "priority": 65534, - "description": "allow inbound TCP connections on port 22 from anywhere" - }, - { - "name": "allow-icmp", - "status": "enabled", - "direction": "inbound", - "targets": [ { "type": "vpc", "value": "default" } ], - "filters": { "protocols": [ "ICMP" ] }, - "action": "allow", - "priority": 65534, - "description": "allow inbound ICMP traffic from anywhere" - } - ] - }"#).unwrap() -}); +pub static DEFAULT_FIREWALL_RULES: LazyLock = + LazyLock::new(|| { + let default: Name = "default".parse().unwrap(); + let targets = vec![VpcFirewallRuleTarget::Vpc(default.clone())]; + VpcFirewallRuleUpdateParams { + rules: vec![ + VpcFirewallRuleUpdate { + name: "allow-internal-inbound".parse().unwrap(), + description: + "allow inbound traffic to all instances within the VPC if originated within the VPC" + .to_string(), + status: VpcFirewallRuleStatus::Enabled, + direction: VpcFirewallRuleDirection::Inbound, + targets: targets.clone(), + filters: VpcFirewallRuleFilter { + hosts: Some(vec![VpcFirewallRuleHostFilter::Vpc(default)]), + protocols: None, + ports: None, + }, + action: VpcFirewallRuleAction::Allow, + priority: VpcFirewallRulePriority(65534), + }, + VpcFirewallRuleUpdate { + name: "allow-ssh".parse().unwrap(), + description: + "allow inbound TCP connections on port 22 from anywhere" + .to_string(), + status: VpcFirewallRuleStatus::Enabled, + direction: VpcFirewallRuleDirection::Inbound, + targets: targets.clone(), + filters: VpcFirewallRuleFilter { + hosts: None, + protocols: Some(vec![VpcFirewallRuleProtocol::Tcp]), + ports: Some(vec![L4Port::try_from(22u16).unwrap().into()]), + }, + action: VpcFirewallRuleAction::Allow, + priority: VpcFirewallRulePriority(65534), + }, + VpcFirewallRuleUpdate { + name: "allow-icmp".parse().unwrap(), + description: + "allow inbound ICMP traffic from anywhere" + .to_string(), + status: VpcFirewallRuleStatus::Enabled, + direction: VpcFirewallRuleDirection::Inbound, + targets, + filters: VpcFirewallRuleFilter { + hosts: None, + protocols: Some(vec![VpcFirewallRuleProtocol::Icmp(None)]), + ports: None, + }, + action: VpcFirewallRuleAction::Allow, + priority: VpcFirewallRulePriority(65534), + }, + ] + } + }); /// Generate a random VPC IPv6 prefix, in the range `fd00::/48`. pub fn random_vpc_ipv6_prefix() -> Result { From 14804cb4f34412edac422a4af9f05a2f9fb4a992 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Thu, 22 May 2025 13:29:41 +0100 Subject: [PATCH 08/27] Simple migration query. --- nexus/tests/integration_tests/schema.rs | 106 +++++++++++++++++++++++- 1 file changed, 104 insertions(+), 2 deletions(-) diff --git a/nexus/tests/integration_tests/schema.rs b/nexus/tests/integration_tests/schema.rs index eee6b9ead62..1db92e4a72a 100644 --- a/nexus/tests/integration_tests/schema.rs +++ b/nexus/tests/integration_tests/schema.rs @@ -918,6 +918,12 @@ const VOLUME2: Uuid = Uuid::from_u128(0x2222566f_5c3d_4647_83b0_8f3515da7be1); const VOLUME3: Uuid = Uuid::from_u128(0x3333566f_5c3d_4647_83b0_8f3515da7be1); const VOLUME4: Uuid = Uuid::from_u128(0x4444566f_5c3d_4647_83b0_8f3515da7be1); +// "566C" -> "VPC". See above on "V". +const VPC: Uuid = Uuid::from_u128(0x1111566c_5c3d_4647_83b0_8f3515da7be1); + +// "7213" -> Firewall "Rule" +const FW_RULE: Uuid = Uuid::from_u128(0x11117213_5c3d_4647_83b0_8f3515da7be1); + fn before_23_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { Box::pin(async move { // Create two silos @@ -2199,11 +2205,107 @@ fn after_140_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { } fn before_145_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { - todo!() + Box::pin(async { + use nexus_db_queries::db::fixed_data::project::*; + use nexus_db_queries::db::fixed_data::vpc::*; + use omicron_common::address::SERVICE_VPC_IPV6_PREFIX; + + // First, create the oxide-services vpc. The migration will only add + // a new rule if it detects the presence of *this* VPC entry (i.e., + // RSS has run before). + let svc_vpc = *SERVICES_VPC_ID; + let svc_proj = *SERVICES_PROJECT_ID; + let svc_router = *SERVICES_VPC_ROUTER_ID; + let svc_vpc_name = &SERVICES_DB_NAME; + ctx.client + .batch_execute(&format!( + "INSERT INTO vpc ( + id, name, description, time_created, time_modified, + project_id, system_router_id, dns_name, + vni, ipv6_prefix, firewall_gen, subnet_gen + ) + VALUES ( + '{svc_vpc}', '{svc_vpc_name}', '', now(), now(), + '{svc_proj}', '{svc_router}', '{svc_vpc_name}', + 100, '{}', 0, 0 + );", + *SERVICE_VPC_IPV6_PREFIX + )) + .await + .expect("failed to create services vpc record"); + + // Second, create a firewall rule in a dummied-out VPC to validate + // that we correctly convert from ENUM[] to STRING[]. + ctx.client + .batch_execute(&format!( + "INSERT INTO vpc_firewall_rule ( + id, + name, description, + time_created, time_modified, vpc_id, status, direction, + targets, filter_protocols, + action, priority + ) + VALUES ( + '{FW_RULE}', + 'test-fw', '', + now(), now(), '{VPC}', 'enabled', 'outbound', + ARRAY['vpc:fiction'], ARRAY['ICMP', 'TCP', 'UDP'], + 'allow', 1234 + );" + )) + .await + .expect("failed to create firewall rule"); + }) } fn after_145_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { - todo!() + Box::pin(async { + use nexus_db_queries::db::fixed_data::vpc::*; + // Firstly -- has the new firewall rule been added? + let rows = ctx + .client + .query( + &format!( + "SELECT id, description + FROM vpc_firewall_rule + WHERE name = 'nexus-icmp' and vpc_id = '{}'", + *SERVICES_VPC_ID + ), + &[], + ) + .await + .expect("failed to load instance auto-restart policies"); + let records = process_rows(&rows); + + assert_eq!(records.len(), 1); + + // Secondly, have FW_RULE's filter_protocols been correctly stringified? + let rows = ctx + .client + .query( + &format!( + "SELECT filter_protocols + FROM vpc_firewall_rule + WHERE id = '{FW_RULE}'" + ), + &[], + ) + .await + .expect("failed to load instance auto-restart policies"); + let records = process_rows(&rows); + assert_eq!(records.len(), 1); + assert_eq!( + records[0].values, + vec![ColumnValue::new( + "filter_protocols", + AnySqlType::TextArray(vec![ + "icmp".into(), + "tcp".into(), + "udp".into(), + ]) + )], + ); + }) } // Lazily initializes all migration checks. The combination of Rust function From 983e6b75ef11dd262b43696af34ff54724b64c09 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Thu, 22 May 2025 16:12:37 +0100 Subject: [PATCH 09/27] An idempotent column rename Thanks, CRDB. --- common/src/api/external/mod.rs | 2 +- schema/crdb/vpc-firewall-icmp/README.adoc | 16 ++++++++++++++++ schema/crdb/vpc-firewall-icmp/up02.sql | 2 +- schema/crdb/vpc-firewall-icmp/up04.sql | 2 +- schema/crdb/vpc-firewall-icmp/up05.sql | 6 +++++- schema/crdb/vpc-firewall-icmp/up06.sql | 22 ++-------------------- schema/crdb/vpc-firewall-icmp/up07.sql | 1 + schema/crdb/vpc-firewall-icmp/up08.sql | 20 ++++++++++++++++++++ 8 files changed, 47 insertions(+), 24 deletions(-) create mode 100644 schema/crdb/vpc-firewall-icmp/README.adoc create mode 100644 schema/crdb/vpc-firewall-icmp/up07.sql create mode 100644 schema/crdb/vpc-firewall-icmp/up08.sql diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 1ed40bb949c..0740eaec6ad 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -3878,7 +3878,7 @@ mod test { "status": "disabled", "direction": "outbound", "targets": [ { "type": "vpc", "value": "default" } ], - "filters": {"ports": [ "22-25", "27" ], "protocols": [ "UDP" ]}, + "filters": {"ports": [ "22-25", "27" ], "protocols": [ { "type": "udp" } ]}, "action": "deny", "priority": 65533, "description": "second rule" diff --git a/schema/crdb/vpc-firewall-icmp/README.adoc b/schema/crdb/vpc-firewall-icmp/README.adoc new file mode 100644 index 00000000000..91499a1bbc4 --- /dev/null +++ b/schema/crdb/vpc-firewall-icmp/README.adoc @@ -0,0 +1,16 @@ +# Overview + +This schema change alters the definition of a firewall rule's protocol filters from an `ENUM[]` to a `STRING[]`. +The reason we're going through this is to be able to express more complex filters on protocol families, such as type/code restrictions on ICMP(v6) flows. +This also allows us, in future, to let customers express arbitrary L4 protocols (once OPTE allows them to be sent/received). + +However, as with many other well-intentioned schema type changes, we need to go through a bit of a circuitous route to get there since a) `ALTER COLUMN xxx TYPE` is experimental, and b) we cannot idempotently rename a column: + +. Add a new `filter_protocols_2` column with the correct type. (`up01.sql`) +. Populate this with the lower case contents of `filter_protocols`, then drop that column. (`up02.sql`, `up03.sql`) +. Recreate `filter_protocols` with the new target type. Copy `filter_protocols_2` into it for all rows. (`up04.sql`, `up05.sql`) +. Drop `filter_columns_2`. (`up06.sql`) +. Drop the now unused `vpc_firewall_rule_protocol` enum type. (`up07.sql`) + +Finally, we add in a new builtin firewall rule to the services VPC to enable Path MTU discovery in Nexus (`up08.sql`). +The presence of this rule is replied upon by the `/v1/system/xxx/xxx` edpoints to enable/disable it. diff --git a/schema/crdb/vpc-firewall-icmp/up02.sql b/schema/crdb/vpc-firewall-icmp/up02.sql index e9a82932215..4bd1ccef500 100644 --- a/schema/crdb/vpc-firewall-icmp/up02.sql +++ b/schema/crdb/vpc-firewall-icmp/up02.sql @@ -1,7 +1,7 @@ SET LOCAL disallow_full_table_scans = off; -- Convert to lower-case strings. Existing filters like "ICMP" --- will still be interpreted as ;all ICMP;. +-- will still be interpreted as 'all ICMP'. UPDATE omicron.public.vpc_firewall_rule SET filter_protocols_2 = lower(filter_protocols::STRING)::STRING(32)[]; diff --git a/schema/crdb/vpc-firewall-icmp/up04.sql b/schema/crdb/vpc-firewall-icmp/up04.sql index 70237260d53..189a4dc3ce7 100644 --- a/schema/crdb/vpc-firewall-icmp/up04.sql +++ b/schema/crdb/vpc-firewall-icmp/up04.sql @@ -1,2 +1,2 @@ ALTER TABLE omicron.public.vpc_firewall_rule - RENAME COLUMN filter_protocols_2 TO filter_protocols; + ADD COLUMN IF NOT EXISTS filter_protocols STRING(32)[]; diff --git a/schema/crdb/vpc-firewall-icmp/up05.sql b/schema/crdb/vpc-firewall-icmp/up05.sql index 24f1b68b45a..4542874ef20 100644 --- a/schema/crdb/vpc-firewall-icmp/up05.sql +++ b/schema/crdb/vpc-firewall-icmp/up05.sql @@ -1 +1,5 @@ -DROP TYPE IF EXISTS omicron.public.vpc_firewall_rule_protocol; +SET LOCAL disallow_full_table_scans = off; + +UPDATE omicron.public.vpc_firewall_rule +SET + filter_protocols = filter_protocols_2; diff --git a/schema/crdb/vpc-firewall-icmp/up06.sql b/schema/crdb/vpc-firewall-icmp/up06.sql index 00d6aab11e1..bf42ef80166 100644 --- a/schema/crdb/vpc-firewall-icmp/up06.sql +++ b/schema/crdb/vpc-firewall-icmp/up06.sql @@ -1,20 +1,2 @@ --- Add a well-known rule to services VPC to allow limited forms --- of ICMP traffic. Inserting this rule is conditional on the --- 'oxide-services' VPC existing. -INSERT INTO omicron.public.vpc_firewall_rule ( - id, - name, description, - time_created, time_modified, vpc_id, status, direction, - targets, filter_protocols, action, priority -) -SELECT - gen_random_uuid(), - -- Hardcoded name/description, see nexus/db-fixed-data/src/vpc_firewall_rule.rs. - 'nexus-icmp', 'allow typical inbound ICMP error codes for outbound flows', - NOW(), NOW(), vpc.id, 'enabled', 'inbound', - -- Allow inbound ICMP Destination Unreachable (Too Large) and Time Exceeded - ARRAY['subnet:nexus'], ARRAY['icmp:3,4','icmp:11'], 'allow', 65534 -FROM omicron.public.vpc - WHERE vpc.id = '001de000-074c-4000-8000-000000000000' - AND vpc.name = 'oxide-services' -ON CONFLICT DO NOTHING; +ALTER TABLE omicron.public.vpc_firewall_rule + DROP COLUMN IF EXISTS filter_protocols_2; diff --git a/schema/crdb/vpc-firewall-icmp/up07.sql b/schema/crdb/vpc-firewall-icmp/up07.sql new file mode 100644 index 00000000000..24f1b68b45a --- /dev/null +++ b/schema/crdb/vpc-firewall-icmp/up07.sql @@ -0,0 +1 @@ +DROP TYPE IF EXISTS omicron.public.vpc_firewall_rule_protocol; diff --git a/schema/crdb/vpc-firewall-icmp/up08.sql b/schema/crdb/vpc-firewall-icmp/up08.sql new file mode 100644 index 00000000000..00d6aab11e1 --- /dev/null +++ b/schema/crdb/vpc-firewall-icmp/up08.sql @@ -0,0 +1,20 @@ +-- Add a well-known rule to services VPC to allow limited forms +-- of ICMP traffic. Inserting this rule is conditional on the +-- 'oxide-services' VPC existing. +INSERT INTO omicron.public.vpc_firewall_rule ( + id, + name, description, + time_created, time_modified, vpc_id, status, direction, + targets, filter_protocols, action, priority +) +SELECT + gen_random_uuid(), + -- Hardcoded name/description, see nexus/db-fixed-data/src/vpc_firewall_rule.rs. + 'nexus-icmp', 'allow typical inbound ICMP error codes for outbound flows', + NOW(), NOW(), vpc.id, 'enabled', 'inbound', + -- Allow inbound ICMP Destination Unreachable (Too Large) and Time Exceeded + ARRAY['subnet:nexus'], ARRAY['icmp:3,4','icmp:11'], 'allow', 65534 +FROM omicron.public.vpc + WHERE vpc.id = '001de000-074c-4000-8000-000000000000' + AND vpc.name = 'oxide-services' +ON CONFLICT DO NOTHING; From 3d1a58e011e0a4a7204217bae9f04fc7a7ab6fa5 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Thu, 22 May 2025 18:50:01 +0100 Subject: [PATCH 10/27] Some control over ICMP allowance. --- nexus/db-queries/src/db/datastore/vpc.rs | 52 +++++++++++++++++ nexus/external-api/output/nexus_tags.txt | 2 + nexus/external-api/src/lib.rs | 26 +++++++++ nexus/src/app/vpc.rs | 53 +++++++++++++++++ nexus/src/external_api/http_entrypoints.rs | 44 +++++++++++++++ openapi/nexus.json | 66 ++++++++++++++++++++++ schema/crdb/vpc-firewall-icmp/README.adoc | 2 +- 7 files changed, 244 insertions(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index 6a41ab9f2a5..40d62c4d0a5 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -55,6 +55,7 @@ use nexus_db_fixed_data::vpc::SERVICES_INTERNET_GATEWAY_DEFAULT_ROUTE_V4; use nexus_db_fixed_data::vpc::SERVICES_INTERNET_GATEWAY_DEFAULT_ROUTE_V6; use nexus_db_fixed_data::vpc::SERVICES_INTERNET_GATEWAY_ID; use nexus_db_fixed_data::vpc::SERVICES_VPC_ID; +use nexus_db_fixed_data::vpc_firewall_rule::NEXUS_ICMP_FW_RULE_NAME; use nexus_db_lookup::DbConnection; use nexus_db_model::DbBpZoneDisposition; use nexus_db_model::ExternalIp; @@ -79,6 +80,7 @@ use omicron_common::api::external::RouterRouteKind as ExternalRouteKind; use omicron_common::api::external::UpdateResult; use omicron_common::api::external::Vni as ExternalVni; use omicron_common::api::external::http_pagination::PaginatedBy; +use omicron_common::api::external::VpcFirewallRuleStatus; use omicron_common::api::internal::shared::InternetGatewayRouterTarget; use omicron_common::api::internal::shared::ResolvedVpcRoute; use omicron_common::api::internal::shared::RouterTarget; @@ -750,6 +752,56 @@ impl DataStore { }) } + pub async fn nexus_inbound_icmp_view( + &self, + opctx: &OpContext, + ) -> Result { + opctx.authorize(authz::Action::Read, &authz::FLEET).await?; + use nexus_db_schema::schema::vpc_firewall_rule::dsl; + + let conn = self.pool_connection_authorized(opctx).await?; + + let rule = dsl::vpc_firewall_rule + .filter(dsl::time_deleted.is_null()) + .filter(dsl::vpc_id.eq(*SERVICES_VPC_ID)) + .filter(dsl::name.eq(NEXUS_ICMP_FW_RULE_NAME.to_string())) + .limit(1) + .select(VpcFirewallRule::as_select()) + .get_result_async::(&*conn) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + Ok(rule.status.0 == VpcFirewallRuleStatus::Enabled) + } + + pub async fn nexus_inbound_icmp_update( + &self, + opctx: &OpContext, + enabled: bool, + ) -> Result { + opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; + use nexus_db_schema::schema::vpc_firewall_rule::dsl; + + let conn = self.pool_connection_authorized(opctx).await?; + + let status = nexus_db_model::VpcFirewallRuleStatus(if enabled {VpcFirewallRuleStatus::Enabled} else { + VpcFirewallRuleStatus::Disabled + }); + + let now = Utc::now(); + let rule = diesel::update(dsl::vpc_firewall_rule) + .filter(dsl::time_deleted.is_null()) + .filter(dsl::vpc_id.eq(*SERVICES_VPC_ID)) + .filter(dsl::name.eq(NEXUS_ICMP_FW_RULE_NAME.to_string())) + .set((dsl::time_modified.eq(now), dsl::status.eq(status))) + .returning(VpcFirewallRule::as_returning()) + .get_result_async(&*conn) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + Ok(rule.status.0 == VpcFirewallRuleStatus::Enabled) + } + /// Return the list of `Sled`s hosting instances or control plane services /// with network interfaces on the provided VPC. pub async fn vpc_resolve_to_sleds( diff --git a/nexus/external-api/output/nexus_tags.txt b/nexus/external-api/output/nexus_tags.txt index 60545e1d308..82d7000cfe8 100644 --- a/nexus/external-api/output/nexus_tags.txt +++ b/nexus/external-api/output/nexus_tags.txt @@ -245,6 +245,8 @@ networking_bgp_exported GET /v1/system/networking/bgp-expo networking_bgp_imported_routes_ipv4 GET /v1/system/networking/bgp-routes-ipv4 networking_bgp_message_history GET /v1/system/networking/bgp-message-history networking_bgp_status GET /v1/system/networking/bgp-status +networking_inbound_icmp_update PUT /v1/system/networking/inbound-icmp +networking_inbound_icmp_view GET /v1/system/networking/inbound-icmp networking_loopback_address_create POST /v1/system/networking/loopback-address networking_loopback_address_delete DELETE /v1/system/networking/loopback-address/{rack_id}/{switch_location}/{address}/{subnet_mask} networking_loopback_address_list GET /v1/system/networking/loopback-address diff --git a/nexus/external-api/src/lib.rs b/nexus/external-api/src/lib.rs index 7689d47748d..950149d12ef 100644 --- a/nexus/external-api/src/lib.rs +++ b/nexus/external-api/src/lib.rs @@ -1971,6 +1971,32 @@ pub trait NexusExternalApi { params: TypedBody, ) -> Result, HttpError>; + /// Return whether API services can receive limited ICMP traffic + #[endpoint { + method = GET, + path = "/v1/system/networking/inbound-icmp", + tags = ["system/networking"], + }] + async fn networking_inbound_icmp_view( + rqctx: RequestContext, + ) -> Result, HttpError>; + + /// Set whether API services can receive limited ICMP traffic + /// + /// When enabled, Nexus is able to receive ICMP Destination Unreachable + /// (type 4, fragmentation needed) and Time Exceeded messages. These + /// enable Nexus to perform Path MTU discovery and better cope with + /// fragmentation issues. Otherwise all ICMP traffic will be dropped. + #[endpoint { + method = PUT, + path = "/v1/system/networking/inbound-icmp", + tags = ["system/networking"], + }] + async fn networking_inbound_icmp_update( + rqctx: RequestContext, + params: TypedBody, + ) -> Result, HttpError>; + // Images /// List images diff --git a/nexus/src/app/vpc.rs b/nexus/src/app/vpc.rs index 23a03bb3574..1788f9a58c7 100644 --- a/nexus/src/app/vpc.rs +++ b/nexus/src/app/vpc.rs @@ -272,4 +272,57 @@ impl super::Nexus { ) .await } + + pub async fn nexus_firewall_inbound_icmp_view( + &self, + opctx: &OpContext, + ) -> Result { + self.datastore().nexus_inbound_icmp_view(opctx).await + } + + pub async fn nexus_firewall_inbound_icmp_update( + &self, + opctx: &OpContext, + enabled: bool, + ) -> Result { + let out = self.datastore().nexus_inbound_icmp_update(opctx, enabled).await?; + + // Notify the sled-agents of the updated firewall rules. + // + // This code comes directly from `Nexus::allow_list_upsert`, where + // there is substantial commentary on the impact of changing the logger, + // actor, etc. + info!( + opctx.log, + "updated user-facing services ICMP status, switching to \ + internal opcontext to plumb rules to sled-agents"; + "icmp-allowed" => ?enabled, + ); + let new_opctx = self.opctx_for_internal_api(); + match nexus_networking::plumb_service_firewall_rules( + self.datastore(), + &new_opctx, + &[], + &new_opctx, + &new_opctx.log, + ) + .await + { + Ok(_) => { + info!(self.log, "plumbed updated ICMP status to sled-agents"); + Ok(out) + } + Err(e) => { + error!( + self.log, + "failed to update sled-agents with new ICMP status"; + "error" => ?e + ); + let message = "Failed to plumb ICMP status as firewall rules \ + to relevant sled agents. The request must be retried for them \ + to take effect."; + Err(Error::unavail(message)) + } + } + } } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 4f12035f42a..6fdfcc69fdf 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -4243,6 +4243,50 @@ impl NexusExternalApi for NexusExternalApiImpl { .await } + async fn networking_inbound_icmp_view( + rqctx: RequestContext, + ) -> Result, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.context.nexus; + let opctx = + crate::context::op_context_for_external_api(&rqctx).await?; + nexus + .nexus_firewall_inbound_icmp_view(&opctx) + .await + .map(HttpResponseOk) + .map_err(HttpError::from) + }; + apictx + .context + .external_latencies + .instrument_dropshot_handler(&rqctx, handler) + .await + } + + async fn networking_inbound_icmp_update( + rqctx: RequestContext, + params: TypedBody, + ) -> Result, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.context.nexus; + let params = params.into_inner(); + let opctx = + crate::context::op_context_for_external_api(&rqctx).await?; + nexus + .nexus_firewall_inbound_icmp_update(&opctx, params) + .await + .map(HttpResponseOk) + .map_err(HttpError::from) + }; + apictx + .context + .external_latencies + .instrument_dropshot_handler(&rqctx, handler) + .await + } + // Images async fn image_list( diff --git a/openapi/nexus.json b/openapi/nexus.json index 6aaeb75ee6c..b2ddec52dda 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -9647,6 +9647,72 @@ } } }, + "/v1/system/networking/inbound-icmp": { + "get": { + "tags": [ + "system/networking" + ], + "summary": "Return whether API services can receive limited ICMP traffic", + "operationId": "networking_inbound_icmp_view", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "Boolean", + "type": "boolean" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "put": { + "tags": [ + "system/networking" + ], + "summary": "Set whether API services can receive limited ICMP traffic", + "description": "When enabled, Nexus is able to receive ICMP Destination Unreachable (type 4, fragmentation needed) and Time Exceeded messages. These enable Nexus to perform Path MTU discovery and better cope with fragmentation issues. Otherwise all ICMP traffic will be dropped.", + "operationId": "networking_inbound_icmp_update", + "requestBody": { + "content": { + "application/json": { + "schema": { + "title": "Boolean", + "type": "boolean" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "Boolean", + "type": "boolean" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/v1/system/networking/loopback-address": { "get": { "tags": [ diff --git a/schema/crdb/vpc-firewall-icmp/README.adoc b/schema/crdb/vpc-firewall-icmp/README.adoc index 91499a1bbc4..71a0957b54b 100644 --- a/schema/crdb/vpc-firewall-icmp/README.adoc +++ b/schema/crdb/vpc-firewall-icmp/README.adoc @@ -13,4 +13,4 @@ However, as with many other well-intentioned schema type changes, we need to go . Drop the now unused `vpc_firewall_rule_protocol` enum type. (`up07.sql`) Finally, we add in a new builtin firewall rule to the services VPC to enable Path MTU discovery in Nexus (`up08.sql`). -The presence of this rule is replied upon by the `/v1/system/xxx/xxx` edpoints to enable/disable it. +The presence of this rule is replied upon by the `/v1/system/networking/inbound-icmp` endpoint to enable/disable this access. From 62a8b3d9c34ecbba7ce32ca1938d03ec5f627334 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Thu, 22 May 2025 19:00:56 +0100 Subject: [PATCH 11/27] Help the deploy job along, a little. --- .github/buildomat/jobs/deploy.sh | 2 +- nexus/db-queries/src/db/datastore/vpc.rs | 6 ++++-- nexus/src/app/vpc.rs | 3 ++- tools/opte_version | 2 +- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/.github/buildomat/jobs/deploy.sh b/.github/buildomat/jobs/deploy.sh index 1fa0caf38e0..c25963a713a 100755 --- a/.github/buildomat/jobs/deploy.sh +++ b/.github/buildomat/jobs/deploy.sh @@ -136,7 +136,7 @@ z_swadm () { # only set this if you want to override the version of opte/xde installed by the # install_opte.sh script -OPTE_COMMIT="" +OPTE_COMMIT="792ec8a3816ba8c7c4268f65cd19dbd946a9027d" if [[ "x$OPTE_COMMIT" != "x" ]]; then curl -sSfOL https://buildomat.eng.oxide.computer/public/file/oxidecomputer/opte/module/$OPTE_COMMIT/xde pfexec rem_drv xde || true diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index 40d62c4d0a5..5383c8f594e 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -79,8 +79,8 @@ use omicron_common::api::external::RouteTarget; use omicron_common::api::external::RouterRouteKind as ExternalRouteKind; use omicron_common::api::external::UpdateResult; use omicron_common::api::external::Vni as ExternalVni; -use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::VpcFirewallRuleStatus; +use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::internal::shared::InternetGatewayRouterTarget; use omicron_common::api::internal::shared::ResolvedVpcRoute; use omicron_common::api::internal::shared::RouterTarget; @@ -784,7 +784,9 @@ impl DataStore { let conn = self.pool_connection_authorized(opctx).await?; - let status = nexus_db_model::VpcFirewallRuleStatus(if enabled {VpcFirewallRuleStatus::Enabled} else { + let status = nexus_db_model::VpcFirewallRuleStatus(if enabled { + VpcFirewallRuleStatus::Enabled + } else { VpcFirewallRuleStatus::Disabled }); diff --git a/nexus/src/app/vpc.rs b/nexus/src/app/vpc.rs index 1788f9a58c7..6944bf517cb 100644 --- a/nexus/src/app/vpc.rs +++ b/nexus/src/app/vpc.rs @@ -285,7 +285,8 @@ impl super::Nexus { opctx: &OpContext, enabled: bool, ) -> Result { - let out = self.datastore().nexus_inbound_icmp_update(opctx, enabled).await?; + let out = + self.datastore().nexus_inbound_icmp_update(opctx, enabled).await?; // Notify the sled-agents of the updated firewall rules. // diff --git a/tools/opte_version b/tools/opte_version index d1429fe0279..229feaac8ce 100644 --- a/tools/opte_version +++ b/tools/opte_version @@ -1 +1 @@ -0.35.364 +0.37.395 From 564424f9631ae6583614b222eef879586fc5047f Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Thu, 22 May 2025 21:25:05 +0100 Subject: [PATCH 12/27] Last test fixup. --- nexus/tests/integration_tests/endpoints.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 0a97ba12d83..fa5d913cf77 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -2825,6 +2825,18 @@ pub static VERIFY_ENDPOINTS: LazyLock> = ), ], }, + // User-facing services inbound ICMP allow/block + VerifyEndpoint { + url: "/v1/system/networking/inbound-icmp", + visibility: Visibility::Public, + unprivileged_access: UnprivilegedAccess::None, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Put( + serde_json::to_value(true).unwrap(), + ), + ], + }, // Alerts VerifyEndpoint { url: &WEBHOOK_RECEIVERS_URL, From 0c19f5f205620b7856309763e8675b7fec0f9f37 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Thu, 22 May 2025 21:32:28 +0100 Subject: [PATCH 13/27] `fmt`. --- nexus/tests/integration_tests/endpoints.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index fa5d913cf77..5790b445c4f 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -2832,9 +2832,7 @@ pub static VERIFY_ENDPOINTS: LazyLock> = unprivileged_access: UnprivilegedAccess::None, allowed_methods: vec![ AllowedMethod::Get, - AllowedMethod::Put( - serde_json::to_value(true).unwrap(), - ), + AllowedMethod::Put(serde_json::to_value(true).unwrap()), ], }, // Alerts From 32a49141a5214f44208933cf7ab9d42da2310b90 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Fri, 23 May 2025 20:20:20 +0100 Subject: [PATCH 14/27] Renaming, and some serde/integration tests. --- common/src/api/external/mod.rs | 49 ++++++++++- illumos-utils/src/opte/firewall_rules.rs | 2 +- nexus/db-fixed-data/src/vpc_firewall_rule.rs | 4 +- nexus/tests/integration_tests/vpc_firewall.rs | 88 +++++++++++++++++++ openapi/nexus.json | 4 +- openapi/sled-agent.json | 4 +- 6 files changed, 140 insertions(+), 11 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 0740eaec6ad..0656c085772 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1888,13 +1888,13 @@ impl Display for VpcFirewallRuleProtocol { #[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize, JsonSchema)] pub struct VpcFirewallIcmpFilter { - pub ty: u8, + pub icmp_type: u8, pub code: Option, } impl Display for VpcFirewallIcmpFilter { fn fmt(&self, f: &mut Formatter<'_>) -> FormatResult { - write!(f, "{}", self.ty)?; + write!(f, "{}", self.icmp_type)?; if let Some(code) = self.code { write!(f, ",{code}")?; } @@ -1912,7 +1912,7 @@ impl FromStr for VpcFirewallIcmpFilter { }; Ok(Self { - ty: ty_str.parse::().map_err(|e| e.to_string())?, + icmp_type: ty_str.parse::().map_err(|e| e.to_string())?, code: code_str.map(|v| v.parse()).transpose()?, }) } @@ -1926,7 +1926,8 @@ impl From for IcmpParamRange { impl From> for IcmpParamRange { fn from(value: RangeInclusive) -> Self { - Self { first: *value.start(), last: *value.end() } + let (first, last) = value.into_inner(); + Self { first, last } } } @@ -3471,6 +3472,7 @@ mod test { use super::Generation; use super::RouteDestination; use super::RouteTarget; + use super::VpcFirewallIcmpFilter; use super::VpcFirewallRuleHostFilter; use super::VpcFirewallRuleTarget; use super::{ @@ -4069,6 +4071,45 @@ mod test { assert!("foo".parse::().is_err()); } + #[test] + fn test_firewall_rule_proto_filter_parse() { + assert_eq!(VpcFirewallRuleProtocol::Tcp, "tcp".parse().unwrap()); + assert_eq!(VpcFirewallRuleProtocol::Udp, "udp".parse().unwrap()); + + assert_eq!( + VpcFirewallRuleProtocol::Icmp(None), + "icmp".parse().unwrap() + ); + assert_eq!( + VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter { + icmp_type: 4, + code: None + })), + "icmp:4".parse().unwrap() + ); + assert_eq!( + VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter { + icmp_type: 60, + code: Some(0.into()) + })), + "icmp:60,0".parse().unwrap() + ); + assert_eq!( + VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter { + icmp_type: 60, + code: Some((0..=10).into()) + })), + "icmp:60,0-10".parse().unwrap() + ); + assert!("icmp:".parse::().is_err()); + assert!("icmp:20-30".parse::().is_err()); + assert!("icmp:10,".parse::().is_err()); + assert!("icmp:257".parse::().is_err()); + assert!( + "icmp:0,1000-1001".parse::().is_err() + ); + } + #[test] fn test_digest() { // No prefix diff --git a/illumos-utils/src/opte/firewall_rules.rs b/illumos-utils/src/opte/firewall_rules.rs index 517c2c63bfa..948808832a6 100644 --- a/illumos-utils/src/opte/firewall_rules.rs +++ b/illumos-utils/src/opte/firewall_rules.rs @@ -105,7 +105,7 @@ impl FromVpcFirewallRule for ResolvedVpcFirewallRule { VpcFirewallRuleProtocol::Icmp(v) => { ProtoFilter::Icmp(v.map(|v| { oxide_vpc::api::IcmpFilter { - ty: v.ty, + ty: v.icmp_type, codes: v.code.map(Into::into), } })) diff --git a/nexus/db-fixed-data/src/vpc_firewall_rule.rs b/nexus/db-fixed-data/src/vpc_firewall_rule.rs index 44d463b9d81..a2d8a649f84 100644 --- a/nexus/db-fixed-data/src/vpc_firewall_rule.rs +++ b/nexus/db-fixed-data/src/vpc_firewall_rule.rs @@ -100,13 +100,13 @@ pub static NEXUS_ICMP_FW_RULE: LazyLock = protocols: Some(vec![ VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter { // Type 3 -- Destination Unreachable - ty: 3, + icmp_type: 3, // Code 4 -- Fragmentation needed code: Some(4.into()), })), VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter { // Type 11 -- Time Exceeded - ty: 11, + icmp_type: 11, code: None, })), ]), diff --git a/nexus/tests/integration_tests/vpc_firewall.rs b/nexus/tests/integration_tests/vpc_firewall.rs index ea7abc6c024..610e86450db 100644 --- a/nexus/tests/integration_tests/vpc_firewall.rs +++ b/nexus/tests/integration_tests/vpc_firewall.rs @@ -4,6 +4,12 @@ use http::StatusCode; use http::method::Method; +use nexus_auth::authn; +use nexus_auth::context::OpContext; +use nexus_db_lookup::LookupPath; +use nexus_db_queries::db::fixed_data::vpc_firewall_rule::NEXUS_ICMP_FW_RULE_NAME; +use nexus_db_queries::db::{self, DataStore}; +use nexus_networking::vpc_list_firewall_rules; use nexus_test_utils::http_testing::{AuthnMode, NexusRequest}; use nexus_test_utils::resource_helpers::{ create_project, create_vpc, object_get, object_put, object_put_error, @@ -17,7 +23,9 @@ use omicron_common::api::external::{ VpcFirewallRuleProtocol, VpcFirewallRuleStatus, VpcFirewallRuleTarget, VpcFirewallRuleUpdate, VpcFirewallRuleUpdateParams, VpcFirewallRules, }; +use omicron_nexus::Nexus; use std::convert::TryFrom; +use std::sync::Arc; use uuid::Uuid; type ControlPlaneTestContext = @@ -582,3 +590,83 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { ) ); } + +async fn icmp_rule_is_enabled( + enabled: bool, + datastore: &DataStore, + nexus: &Nexus, + opctx: &OpContext, +) -> bool { + // (Partly) reimplementing opctx_for_internal_api. + // This is necessary to *reach* the services VPC. + let opctx = OpContext::for_background( + opctx.log.clone(), + Arc::clone(nexus.authz()), + authn::Context::internal_api(), + Arc::clone(nexus.datastore()) as Arc, + ); + let svcs_vpc = LookupPath::new(&opctx, datastore) + .vpc_id(*db::fixed_data::vpc::SERVICES_VPC_ID); + + let fw_rules = + vpc_list_firewall_rules(&datastore, &opctx, &svcs_vpc).await.unwrap(); + let the_rule = fw_rules + .iter() + .find(|v| v.identity.name.as_str() == NEXUS_ICMP_FW_RULE_NAME) + .unwrap(); + + the_rule.status.0 + == if enabled { + VpcFirewallRuleStatus::Enabled + } else { + VpcFirewallRuleStatus::Disabled + } +} + +#[nexus_test] +async fn test_nexus_firewall_icmp(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + let apictx = &cptestctx.server.server_context(); + let nexus = &apictx.nexus; + let datastore = nexus.datastore(); + let opctx = + OpContext::for_tests(cptestctx.logctx.log.new(o!()), datastore.clone()); + + const NEXUS_ICMP_URL: &str = "/v1/system/networking/inbound-icmp"; + + // ICMP access should be enabled by default. + let icmp_allowed: bool = NexusRequest::object_get(client, NEXUS_ICMP_URL) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + + assert!(icmp_allowed); + assert!(icmp_rule_is_enabled(true, datastore, nexus, &opctx).await); + + // Disabling this should propagate to the rule. + let icmp_allowed = + NexusRequest::object_put(client, NEXUS_ICMP_URL, Some(&false)) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body::() + .unwrap(); + assert!(!icmp_allowed); + assert!(icmp_rule_is_enabled(false, datastore, nexus, &opctx).await); + + // Now, re-anable the rule. + let icmp_allowed = + NexusRequest::object_put(client, NEXUS_ICMP_URL, Some(&true)) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body::() + .unwrap(); + assert!(icmp_allowed); + assert!(icmp_rule_is_enabled(true, datastore, nexus, &opctx).await); +} diff --git a/openapi/nexus.json b/openapi/nexus.json index b2ddec52dda..5111e306647 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -25247,14 +25247,14 @@ } ] }, - "ty": { + "icmp_type": { "type": "integer", "format": "uint8", "minimum": 0 } }, "required": [ - "ty" + "icmp_type" ] }, "VpcFirewallRule": { diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index 579b1b12b39..3345f6186b5 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -7649,14 +7649,14 @@ } ] }, - "ty": { + "icmp_type": { "type": "integer", "format": "uint8", "minimum": 0 } }, "required": [ - "ty" + "icmp_type" ] }, "VpcFirewallRuleAction": { From 6b9fa7a330d49e9601c8f208a18f07af9d26b136 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Tue, 27 May 2025 11:34:57 +0100 Subject: [PATCH 15/27] Self-review --- common/src/api/external/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 0656c085772..9c03eea915d 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -2233,7 +2233,7 @@ impl JsonSchema for IcmpParamRange { schemars::schema::InstanceType::String.into() ), string: Some(Box::new(schemars::schema::StringValidation { - max_length: Some(7), // 3 digits for each port and the dash + max_length: Some(7), // 3 digits for each value and the dash min_length: Some(1), pattern: Some( r#"^[0-9]{1,3}(-[0-9]{1,3})?$"#.to_string(), From 5c7caed2fe349222ef1ebf9937e1bbc42acaf005 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Wed, 28 May 2025 18:06:44 +0100 Subject: [PATCH 16/27] Review feedback --- common/src/api/external/mod.rs | 60 ++++++++++++------- nexus/db-queries/src/db/datastore/vpc.rs | 17 ++++-- nexus/external-api/src/lib.rs | 11 +--- nexus/src/app/vpc.rs | 19 +++--- nexus/src/external_api/http_entrypoints.rs | 9 +-- nexus/tests/integration_tests/vpc_firewall.rs | 55 ++++++++--------- openapi/nexus.json | 32 +++++----- 7 files changed, 114 insertions(+), 89 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 9c03eea915d..e1593e04fa5 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -2064,7 +2064,7 @@ pub struct L4PortRange { } impl FromStr for L4PortRange { - type Err = String; + type Err = &'static str; fn from_str(range: &str) -> Result { const INVALID_PORT_NUMBER_MSG: &str = "invalid port number"; @@ -2072,18 +2072,18 @@ impl FromStr for L4PortRange { None => { let port = range .parse::() - .map_err(|_| INVALID_PORT_NUMBER_MSG.to_string())? + .map_err(|_| INVALID_PORT_NUMBER_MSG)? .into(); Ok(L4PortRange { first: port, last: port }) } Some((left, right)) => { let first = left .parse::() - .map_err(|_| INVALID_PORT_NUMBER_MSG.to_string())? + .map_err(|_| INVALID_PORT_NUMBER_MSG)? .into(); let last = right .parse::() - .map_err(|_| INVALID_PORT_NUMBER_MSG.to_string())? + .map_err(|_| INVALID_PORT_NUMBER_MSG)? .into(); Ok(L4PortRange { first, last }) } @@ -2168,24 +2168,21 @@ pub struct IcmpParamRange { } impl FromStr for IcmpParamRange { - type Err = String; + type Err = &'static str; fn from_str(range: &str) -> Result { const INVALID_NUMBER_MSG: &str = "invalid 8-bit number"; match range.split_once('-') { None => { - let port = range - .parse::() - .map_err(|_| INVALID_NUMBER_MSG.to_string())?; + let port = + range.parse::().map_err(|_| INVALID_NUMBER_MSG)?; Ok(IcmpParamRange { first: port, last: port }) } Some((left, right)) => { - let first = left - .parse::() - .map_err(|_| INVALID_NUMBER_MSG.to_string())?; - let last = right - .parse::() - .map_err(|_| INVALID_NUMBER_MSG.to_string())?; + let first = + left.parse::().map_err(|_| INVALID_NUMBER_MSG)?; + let last = + right.parse::().map_err(|_| INVALID_NUMBER_MSG)?; Ok(IcmpParamRange { first, last }) } } @@ -3339,6 +3336,27 @@ pub enum BfdMode { MultiHop, } +/// Configuration of inbound ICMP allowed by API services. +#[derive( + Clone, + Copy, + Debug, + Deserialize, + Serialize, + JsonSchema, + PartialEq, + Eq, + Ord, + PartialOrd, +)] +pub struct ServiceIcmpConfig { + /// When enabled, Nexus is able to receive ICMP Destination Unreachable + /// (type 4, fragmentation needed) and Time Exceeded messages. These + /// enable Nexus to perform Path MTU discovery and better cope with + /// fragmentation issues. Otherwise all ICMP traffic will be dropped. + pub enabled: bool, +} + /// A description of an uploaded TUF repository. #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize, JsonSchema)] pub struct TufRepoDescription { @@ -3816,31 +3834,31 @@ mod test { assert_eq!( L4PortRange::try_from("".to_string()), - Err("invalid port number".to_string()) + Err("invalid port number") ); assert_eq!( L4PortRange::try_from("65536".to_string()), - Err("invalid port number".to_string()) + Err("invalid port number") ); assert_eq!( L4PortRange::try_from("65535-65536".to_string()), - Err("invalid port number".to_string()) + Err("invalid port number") ); assert_eq!( L4PortRange::try_from("0x23".to_string()), - Err("invalid port number".to_string()) + Err("invalid port number") ); assert_eq!( L4PortRange::try_from("0".to_string()), - Err("invalid port number".to_string()) + Err("invalid port number") ); assert_eq!( L4PortRange::try_from("0-20".to_string()), - Err("invalid port number".to_string()) + Err("invalid port number") ); assert_eq!( L4PortRange::try_from("-20".to_string()), - Err("invalid port number".to_string()) + Err("invalid port number") ); } diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index 5383c8f594e..7da34f0d777 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -77,6 +77,7 @@ use omicron_common::api::external::ResourceType; use omicron_common::api::external::RouteDestination; use omicron_common::api::external::RouteTarget; use omicron_common::api::external::RouterRouteKind as ExternalRouteKind; +use omicron_common::api::external::ServiceIcmpConfig; use omicron_common::api::external::UpdateResult; use omicron_common::api::external::Vni as ExternalVni; use omicron_common::api::external::VpcFirewallRuleStatus; @@ -755,7 +756,7 @@ impl DataStore { pub async fn nexus_inbound_icmp_view( &self, opctx: &OpContext, - ) -> Result { + ) -> Result { opctx.authorize(authz::Action::Read, &authz::FLEET).await?; use nexus_db_schema::schema::vpc_firewall_rule::dsl; @@ -771,17 +772,21 @@ impl DataStore { .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; - Ok(rule.status.0 == VpcFirewallRuleStatus::Enabled) + Ok(ServiceIcmpConfig { + enabled: rule.status.0 == VpcFirewallRuleStatus::Enabled, + }) } pub async fn nexus_inbound_icmp_update( &self, opctx: &OpContext, - enabled: bool, - ) -> Result { + config: ServiceIcmpConfig, + ) -> Result { opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; use nexus_db_schema::schema::vpc_firewall_rule::dsl; + let ServiceIcmpConfig { enabled } = config; + let conn = self.pool_connection_authorized(opctx).await?; let status = nexus_db_model::VpcFirewallRuleStatus(if enabled { @@ -801,7 +806,9 @@ impl DataStore { .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; - Ok(rule.status.0 == VpcFirewallRuleStatus::Enabled) + Ok(ServiceIcmpConfig { + enabled: rule.status.0 == VpcFirewallRuleStatus::Enabled, + }) } /// Return the list of `Sled`s hosting instances or control plane services diff --git a/nexus/external-api/src/lib.rs b/nexus/external-api/src/lib.rs index 950149d12ef..860b8276490 100644 --- a/nexus/external-api/src/lib.rs +++ b/nexus/external-api/src/lib.rs @@ -1979,14 +1979,9 @@ pub trait NexusExternalApi { }] async fn networking_inbound_icmp_view( rqctx: RequestContext, - ) -> Result, HttpError>; + ) -> Result, HttpError>; /// Set whether API services can receive limited ICMP traffic - /// - /// When enabled, Nexus is able to receive ICMP Destination Unreachable - /// (type 4, fragmentation needed) and Time Exceeded messages. These - /// enable Nexus to perform Path MTU discovery and better cope with - /// fragmentation issues. Otherwise all ICMP traffic will be dropped. #[endpoint { method = PUT, path = "/v1/system/networking/inbound-icmp", @@ -1994,8 +1989,8 @@ pub trait NexusExternalApi { }] async fn networking_inbound_icmp_update( rqctx: RequestContext, - params: TypedBody, - ) -> Result, HttpError>; + params: TypedBody, + ) -> Result; // Images diff --git a/nexus/src/app/vpc.rs b/nexus/src/app/vpc.rs index 6944bf517cb..32582f0d05b 100644 --- a/nexus/src/app/vpc.rs +++ b/nexus/src/app/vpc.rs @@ -23,6 +23,7 @@ use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::LookupType; use omicron_common::api::external::NameOrId; +use omicron_common::api::external::ServiceIcmpConfig; use omicron_common::api::external::UpdateResult; use omicron_common::api::external::VpcFirewallRuleUpdateParams; use omicron_common::api::external::http_pagination::PaginatedBy; @@ -276,17 +277,17 @@ impl super::Nexus { pub async fn nexus_firewall_inbound_icmp_view( &self, opctx: &OpContext, - ) -> Result { + ) -> Result { self.datastore().nexus_inbound_icmp_view(opctx).await } pub async fn nexus_firewall_inbound_icmp_update( &self, opctx: &OpContext, - enabled: bool, - ) -> Result { + config: ServiceIcmpConfig, + ) -> Result { let out = - self.datastore().nexus_inbound_icmp_update(opctx, enabled).await?; + self.datastore().nexus_inbound_icmp_update(opctx, config).await?; // Notify the sled-agents of the updated firewall rules. // @@ -297,7 +298,7 @@ impl super::Nexus { opctx.log, "updated user-facing services ICMP status, switching to \ internal opcontext to plumb rules to sled-agents"; - "icmp-allowed" => ?enabled, + "icmp-allowed" => ?config.enabled, ); let new_opctx = self.opctx_for_internal_api(); match nexus_networking::plumb_service_firewall_rules( @@ -314,14 +315,14 @@ impl super::Nexus { Ok(out) } Err(e) => { + let message = "Failed to plumb ICMP status as firewall rules \ + to relevant sled agents. The request must be retried for them \ + to take effect."; error!( self.log, "failed to update sled-agents with new ICMP status"; - "error" => ?e + "error" => format!("{message}: {e:?}"), ); - let message = "Failed to plumb ICMP status as firewall rules \ - to relevant sled agents. The request must be retried for them \ - to take effect."; Err(Error::unavail(message)) } } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 6fdfcc69fdf..d922f5d3ccd 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -78,6 +78,7 @@ use omicron_common::api::external::NameOrId; use omicron_common::api::external::Probe; use omicron_common::api::external::RouterRoute; use omicron_common::api::external::RouterRouteKind; +use omicron_common::api::external::ServiceIcmpConfig; use omicron_common::api::external::SwitchPort; use omicron_common::api::external::SwitchPortSettings; use omicron_common::api::external::SwitchPortSettingsView; @@ -4245,7 +4246,7 @@ impl NexusExternalApi for NexusExternalApiImpl { async fn networking_inbound_icmp_view( rqctx: RequestContext, - ) -> Result, HttpError> { + ) -> Result, HttpError> { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.context.nexus; @@ -4266,8 +4267,8 @@ impl NexusExternalApi for NexusExternalApiImpl { async fn networking_inbound_icmp_update( rqctx: RequestContext, - params: TypedBody, - ) -> Result, HttpError> { + params: TypedBody, + ) -> Result { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.context.nexus; @@ -4277,7 +4278,7 @@ impl NexusExternalApi for NexusExternalApiImpl { nexus .nexus_firewall_inbound_icmp_update(&opctx, params) .await - .map(HttpResponseOk) + .map(|_| HttpResponseUpdatedNoContent()) .map_err(HttpError::from) }; apictx diff --git a/nexus/tests/integration_tests/vpc_firewall.rs b/nexus/tests/integration_tests/vpc_firewall.rs index 610e86450db..91280d8d95e 100644 --- a/nexus/tests/integration_tests/vpc_firewall.rs +++ b/nexus/tests/integration_tests/vpc_firewall.rs @@ -10,14 +10,14 @@ use nexus_db_lookup::LookupPath; use nexus_db_queries::db::fixed_data::vpc_firewall_rule::NEXUS_ICMP_FW_RULE_NAME; use nexus_db_queries::db::{self, DataStore}; use nexus_networking::vpc_list_firewall_rules; -use nexus_test_utils::http_testing::{AuthnMode, NexusRequest}; +use nexus_test_utils::http_testing::{AuthnMode, NexusRequest, RequestBuilder}; use nexus_test_utils::resource_helpers::{ create_project, create_vpc, object_get, object_put, object_put_error, }; use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::views::Vpc; use omicron_common::api::external::{ - IdentityMetadata, L4Port, L4PortRange, VpcFirewallRule, + IdentityMetadata, L4Port, L4PortRange, ServiceIcmpConfig, VpcFirewallRule, VpcFirewallRuleAction, VpcFirewallRuleDirection, VpcFirewallRuleFilter, VpcFirewallRuleHostFilter, VpcFirewallRulePriority, VpcFirewallRuleProtocol, VpcFirewallRuleStatus, VpcFirewallRuleTarget, @@ -635,38 +635,39 @@ async fn test_nexus_firewall_icmp(cptestctx: &ControlPlaneTestContext) { const NEXUS_ICMP_URL: &str = "/v1/system/networking/inbound-icmp"; // ICMP access should be enabled by default. - let icmp_allowed: bool = NexusRequest::object_get(client, NEXUS_ICMP_URL) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body() - .unwrap(); - - assert!(icmp_allowed); - assert!(icmp_rule_is_enabled(true, datastore, nexus, &opctx).await); - - // Disabling this should propagate to the rule. - let icmp_allowed = - NexusRequest::object_put(client, NEXUS_ICMP_URL, Some(&false)) + let icmp_allowed: ServiceIcmpConfig = + NexusRequest::object_get(client, NEXUS_ICMP_URL) .authn_as(AuthnMode::PrivilegedUser) .execute() .await .unwrap() - .parsed_body::() + .parsed_body() .unwrap(); - assert!(!icmp_allowed); + + assert!(icmp_allowed.enabled); + assert!(icmp_rule_is_enabled(true, datastore, nexus, &opctx).await); + + // Disabling this should propagate to the rule. + NexusRequest::new( + RequestBuilder::new(client, http::Method::PUT, NEXUS_ICMP_URL) + .body(Some(&ServiceIcmpConfig { enabled: false })) + .expect_status(Some(http::StatusCode::NO_CONTENT)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); assert!(icmp_rule_is_enabled(false, datastore, nexus, &opctx).await); // Now, re-anable the rule. - let icmp_allowed = - NexusRequest::object_put(client, NEXUS_ICMP_URL, Some(&true)) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body::() - .unwrap(); - assert!(icmp_allowed); + NexusRequest::new( + RequestBuilder::new(client, http::Method::PUT, NEXUS_ICMP_URL) + .body(Some(&ServiceIcmpConfig { enabled: true })) + .expect_status(Some(http::StatusCode::NO_CONTENT)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); assert!(icmp_rule_is_enabled(true, datastore, nexus, &opctx).await); } diff --git a/openapi/nexus.json b/openapi/nexus.json index 5111e306647..f14eca76055 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -9660,8 +9660,7 @@ "content": { "application/json": { "schema": { - "title": "Boolean", - "type": "boolean" + "$ref": "#/components/schemas/ServiceIcmpConfig" } } } @@ -9679,30 +9678,20 @@ "system/networking" ], "summary": "Set whether API services can receive limited ICMP traffic", - "description": "When enabled, Nexus is able to receive ICMP Destination Unreachable (type 4, fragmentation needed) and Time Exceeded messages. These enable Nexus to perform Path MTU discovery and better cope with fragmentation issues. Otherwise all ICMP traffic will be dropped.", "operationId": "networking_inbound_icmp_update", "requestBody": { "content": { "application/json": { "schema": { - "title": "Boolean", - "type": "boolean" + "$ref": "#/components/schemas/ServiceIcmpConfig" } } }, "required": true }, "responses": { - "200": { - "description": "successful operation", - "content": { - "application/json": { - "schema": { - "title": "Boolean", - "type": "boolean" - } - } - } + "204": { + "description": "resource updated" }, "4XX": { "$ref": "#/components/responses/Error" @@ -22438,6 +22427,19 @@ "technical_contact_email" ] }, + "ServiceIcmpConfig": { + "description": "Configuration of inbound ICMP allowed by API services.", + "type": "object", + "properties": { + "enabled": { + "description": "When enabled, Nexus is able to receive ICMP Destination Unreachable (type 4, fragmentation needed) and Time Exceeded messages. These enable Nexus to perform Path MTU discovery and better cope with fragmentation issues. Otherwise all ICMP traffic will be dropped.", + "type": "boolean" + } + }, + "required": [ + "enabled" + ] + }, "ServiceUsingCertificate": { "description": "The service intended to use this certificate.", "oneOf": [ From 1fc7b2bb4de42bc58582c0e76d9bca5271a2a7f2 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Thu, 29 May 2025 10:22:30 +0100 Subject: [PATCH 17/27] Review feedback, missed test fixup. --- common/src/api/external/mod.rs | 124 +++++++++++++++------ nexus/tests/integration_tests/endpoints.rs | 14 ++- 2 files changed, 105 insertions(+), 33 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index e1593e04fa5..a893ac1f365 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1846,7 +1846,7 @@ pub enum VpcFirewallRuleProtocol { } impl FromStr for VpcFirewallRuleProtocol { - type Err = String; + type Err = Error; fn from_str(proto: &str) -> Result { let (ty_str, content_str) = match proto.split_once(':') { @@ -1854,14 +1854,21 @@ impl FromStr for VpcFirewallRuleProtocol { Some((lhs, rhs)) => (lhs, Some(rhs)), }; - match (ty_str.to_lowercase().as_str(), content_str) { - ("tcp", None) => Ok(Self::Tcp), - ("udp", None) => Ok(Self::Udp), - ("icmp", None) => Ok(Self::Icmp(None)), - ("icmp", Some(rhs)) => Ok(Self::Icmp(Some(rhs.parse()?))), - (lhs, None) => Err(format!("unrecognized protocol \"{lhs}\"")), - (lhs, Some(_)) => Err(format!( - "cannot specify extra filters for protocol \"{lhs}\"" + match (ty_str, content_str) { + (lhs, None) if lhs.eq_ignore_ascii_case("tcp") => Ok(Self::Tcp), + (lhs, None) if lhs.eq_ignore_ascii_case("udp") => Ok(Self::Udp), + (lhs, None) if lhs.eq_ignore_ascii_case("icmp") => { + Ok(Self::Icmp(None)) + } + (lhs, Some(rhs)) if lhs.eq_ignore_ascii_case("icmp") => { + Ok(Self::Icmp(Some(rhs.parse()?))) + } + (lhs, None) => { + Err(Error::invalid_value(lhs, "unrecognized protocol")) + } + (lhs, Some(rhs)) => Err(Error::invalid_value( + rhs, + format!("cannot specify extra filters for protocol \"{lhs}\""), )), } } @@ -1903,7 +1910,7 @@ impl Display for VpcFirewallIcmpFilter { } impl FromStr for VpcFirewallIcmpFilter { - type Err = String; + type Err = Error; fn from_str(filter: &str) -> Result { let (ty_str, code_str) = match filter.split_once(',') { @@ -1912,8 +1919,15 @@ impl FromStr for VpcFirewallIcmpFilter { }; Ok(Self { - icmp_type: ty_str.parse::().map_err(|e| e.to_string())?, - code: code_str.map(|v| v.parse()).transpose()?, + icmp_type: ty_str + .parse::() + .map_err(|e| Error::invalid_value(ty_str, e.to_string()))?, + code: code_str + .map(|v| { + v.parse::() + .map_err(|e| Error::invalid_value(v, e.to_string())) + }) + .transpose()?, }) } } @@ -2064,7 +2078,7 @@ pub struct L4PortRange { } impl FromStr for L4PortRange { - type Err = &'static str; + type Err = Error; fn from_str(range: &str) -> Result { const INVALID_PORT_NUMBER_MSG: &str = "invalid port number"; @@ -2072,18 +2086,33 @@ impl FromStr for L4PortRange { None => { let port = range .parse::() - .map_err(|_| INVALID_PORT_NUMBER_MSG)? + .map_err(|e| { + Error::invalid_value( + range, + format!("{INVALID_PORT_NUMBER_MSG}: {e}"), + ) + })? .into(); Ok(L4PortRange { first: port, last: port }) } Some((left, right)) => { let first = left .parse::() - .map_err(|_| INVALID_PORT_NUMBER_MSG)? + .map_err(|e| { + Error::invalid_value( + left, + format!("{INVALID_PORT_NUMBER_MSG}: {e}"), + ) + })? .into(); let last = right .parse::() - .map_err(|_| INVALID_PORT_NUMBER_MSG)? + .map_err(|e| { + Error::invalid_value( + right, + format!("{INVALID_PORT_NUMBER_MSG}: {e}"), + ) + })? .into(); Ok(L4PortRange { first, last }) } @@ -2168,21 +2197,33 @@ pub struct IcmpParamRange { } impl FromStr for IcmpParamRange { - type Err = &'static str; + type Err = Error; fn from_str(range: &str) -> Result { const INVALID_NUMBER_MSG: &str = "invalid 8-bit number"; match range.split_once('-') { None => { - let port = - range.parse::().map_err(|_| INVALID_NUMBER_MSG)?; - Ok(IcmpParamRange { first: port, last: port }) + let param = range.parse::().map_err(|e| { + Error::invalid_value( + range, + format!("{INVALID_NUMBER_MSG}: {e}"), + ) + })?; + Ok(IcmpParamRange { first: param, last: param }) } Some((left, right)) => { - let first = - left.parse::().map_err(|_| INVALID_NUMBER_MSG)?; - let last = - right.parse::().map_err(|_| INVALID_NUMBER_MSG)?; + let first = left.parse::().map_err(|e| { + Error::invalid_value( + left, + format!("{INVALID_NUMBER_MSG}: {e}"), + ) + })?; + let last = right.parse::().map_err(|e| { + Error::invalid_value( + right, + format!("{INVALID_NUMBER_MSG}: {e}"), + ) + })?; Ok(IcmpParamRange { first, last }) } } @@ -3834,31 +3875,52 @@ mod test { assert_eq!( L4PortRange::try_from("".to_string()), - Err("invalid port number") + Err(Error::invalid_value( + "", + "invalid port number: cannot parse integer from empty string" + )) ); assert_eq!( L4PortRange::try_from("65536".to_string()), - Err("invalid port number") + Err(Error::invalid_value( + "65536", + "invalid port number: number too large to fit in target type" + )) ); assert_eq!( L4PortRange::try_from("65535-65536".to_string()), - Err("invalid port number") + Err(Error::invalid_value( + "65536", + "invalid port number: number too large to fit in target type" + )) ); assert_eq!( L4PortRange::try_from("0x23".to_string()), - Err("invalid port number") + Err(Error::invalid_value( + "0x23", + "invalid port number: invalid digit found in string" + )) ); assert_eq!( L4PortRange::try_from("0".to_string()), - Err("invalid port number") + Err(Error::invalid_value( + "0", + "invalid port number: number would be zero for non-zero type" + )) ); assert_eq!( L4PortRange::try_from("0-20".to_string()), - Err("invalid port number") + Err(Error::invalid_value( + "0", + "invalid port number: number would be zero for non-zero type" + )) ); assert_eq!( L4PortRange::try_from("-20".to_string()), - Err("invalid port number") + Err(Error::invalid_value( + "", + "invalid port number: cannot parse integer from empty string" + )) ); } diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 5790b445c4f..bff02a9118e 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -36,6 +36,7 @@ use omicron_common::api::external::Name; use omicron_common::api::external::NameOrId; use omicron_common::api::external::RouteDestination; use omicron_common::api::external::RouteTarget; +use omicron_common::api::external::ServiceIcmpConfig; use omicron_common::api::external::UserId; use omicron_common::api::external::VpcFirewallRuleUpdateParams; use omicron_test_utils::certificates::CertificateChain; @@ -1242,6 +1243,12 @@ pub static DEMO_WEBHOOK_SECRET_CREATE: LazyLock = secret: "TRUSTNO1".to_string(), }); +pub static DEMO_INBOUND_ICMP_URL: &'static str = + "/v1/system/networking/inbound-icmp"; + +pub static DEMO_INBOUND_ICMP_UPDATE: LazyLock = + LazyLock::new(|| ServiceIcmpConfig { enabled: true }); + /// Describes an API endpoint to be verified by the "unauthorized" test /// /// These structs are also used to check whether we're covering all endpoints in @@ -2827,12 +2834,15 @@ pub static VERIFY_ENDPOINTS: LazyLock> = }, // User-facing services inbound ICMP allow/block VerifyEndpoint { - url: "/v1/system/networking/inbound-icmp", + url: DEMO_INBOUND_ICMP_URL, visibility: Visibility::Public, unprivileged_access: UnprivilegedAccess::None, allowed_methods: vec![ AllowedMethod::Get, - AllowedMethod::Put(serde_json::to_value(true).unwrap()), + AllowedMethod::Put( + serde_json::to_value(&*DEMO_INBOUND_ICMP_UPDATE) + .unwrap(), + ), ], }, // Alerts From 3d9011b9e36a71a1add4ba229ad3da31055ab9b1 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Fri, 30 May 2025 23:39:44 +0100 Subject: [PATCH 18/27] Further review feedback. --- common/src/api/external/mod.rs | 201 ++++++++++++++--------- nexus/db-queries/src/db/datastore/vpc.rs | 32 +++- 2 files changed, 146 insertions(+), 87 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index a893ac1f365..66621de8aa5 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -42,6 +42,7 @@ use std::fmt::Formatter; use std::fmt::Result as FormatResult; use std::net::IpAddr; use std::net::Ipv4Addr; +use std::num::ParseIntError; use std::num::{NonZeroU16, NonZeroU32}; use std::ops::RangeInclusive; use std::str::FromStr; @@ -1863,12 +1864,15 @@ impl FromStr for VpcFirewallRuleProtocol { (lhs, Some(rhs)) if lhs.eq_ignore_ascii_case("icmp") => { Ok(Self::Icmp(Some(rhs.parse()?))) } - (lhs, None) => { - Err(Error::invalid_value(lhs, "unrecognized protocol")) - } + (lhs, None) => Err(Error::invalid_value( + "vpc_firewall_rule_protocol", + format!("unrecognized protocol: {lhs}"), + )), (lhs, Some(rhs)) => Err(Error::invalid_value( - rhs, - format!("cannot specify extra filters for protocol \"{lhs}\""), + "vpc_firewall_rule_protocol", + format!( + "cannot specify extra filters ({rhs}) for protocol \"{lhs}\"" + ), )), } } @@ -1919,13 +1923,17 @@ impl FromStr for VpcFirewallIcmpFilter { }; Ok(Self { - icmp_type: ty_str - .parse::() - .map_err(|e| Error::invalid_value(ty_str, e.to_string()))?, + icmp_type: ty_str.parse::().map_err(|e| { + Error::invalid_value( + "icmp_type", + format!("{ty_str:?} unparsable for type: {e}"), + ) + })?, code: code_str .map(|v| { - v.parse::() - .map_err(|e| Error::invalid_value(v, e.to_string())) + v.parse::().map_err(|e| { + Error::invalid_value("code", e.to_string()) + }) }) .transpose()?, }) @@ -2078,41 +2086,26 @@ pub struct L4PortRange { } impl FromStr for L4PortRange { - type Err = Error; + type Err = L4PortRangeError; fn from_str(range: &str) -> Result { - const INVALID_PORT_NUMBER_MSG: &str = "invalid port number"; - match range.split_once('-') { None => { let port = range .parse::() - .map_err(|e| { - Error::invalid_value( - range, - format!("{INVALID_PORT_NUMBER_MSG}: {e}"), - ) - })? + .map_err(|e| L4PortRangeError::Value(range.into(), e))? .into(); Ok(L4PortRange { first: port, last: port }) } + Some(("", _)) => Err(L4PortRangeError::MissingStart), + Some((_, "")) => Err(L4PortRangeError::MissingEnd), Some((left, right)) => { let first = left .parse::() - .map_err(|e| { - Error::invalid_value( - left, - format!("{INVALID_PORT_NUMBER_MSG}: {e}"), - ) - })? + .map_err(|e| L4PortRangeError::Value(left.into(), e))? .into(); let last = right .parse::() - .map_err(|e| { - Error::invalid_value( - right, - format!("{INVALID_PORT_NUMBER_MSG}: {e}"), - ) - })? + .map_err(|e| L4PortRangeError::Value(right.into(), e))? .into(); Ok(L4PortRange { first, last }) } @@ -2120,6 +2113,22 @@ impl FromStr for L4PortRange { } } +#[derive(Clone, Debug, Eq, PartialEq, thiserror::Error)] +pub enum L4PortRangeError { + #[error("range has no start value")] + MissingStart, + #[error("range has no end value")] + MissingEnd, + #[error("{0:?} unparsable for type: {1}")] + Value(String, ParseIntError), +} + +impl From for Error { + fn from(value: L4PortRangeError) -> Self { + Error::invalid_value("l4_port_range", value.to_string()) + } +} + impl TryFrom for L4PortRange { type Error = ::Err; @@ -2197,39 +2206,40 @@ pub struct IcmpParamRange { } impl FromStr for IcmpParamRange { - type Err = Error; + type Err = IcmpParamRangeError; fn from_str(range: &str) -> Result { - const INVALID_NUMBER_MSG: &str = "invalid 8-bit number"; - match range.split_once('-') { None => { - let param = range.parse::().map_err(|e| { - Error::invalid_value( - range, - format!("{INVALID_NUMBER_MSG}: {e}"), - ) - })?; + let param = range + .parse::() + .map_err(|e| IcmpParamRangeError::Value(range.into(), e))?; Ok(IcmpParamRange { first: param, last: param }) } + Some(("", _)) => Err(IcmpParamRangeError::MissingStart), + Some((_, "")) => Err(IcmpParamRangeError::MissingEnd), Some((left, right)) => { - let first = left.parse::().map_err(|e| { - Error::invalid_value( - left, - format!("{INVALID_NUMBER_MSG}: {e}"), - ) - })?; - let last = right.parse::().map_err(|e| { - Error::invalid_value( - right, - format!("{INVALID_NUMBER_MSG}: {e}"), - ) - })?; + let first = left + .parse::() + .map_err(|e| IcmpParamRangeError::Value(left.into(), e))?; + let last = right + .parse::() + .map_err(|e| IcmpParamRangeError::Value(right.into(), e))?; Ok(IcmpParamRange { first, last }) } } } } +#[derive(Clone, Debug, Eq, PartialEq, thiserror::Error)] +pub enum IcmpParamRangeError { + #[error("range has no start value")] + MissingStart, + #[error("range has no end value")] + MissingEnd, + #[error("{0:?} unparsable for type: {1}")] + Value(String, ParseIntError), +} + impl TryFrom for IcmpParamRange { type Error = ::Err; @@ -3874,52 +3884,53 @@ mod test { ); assert_eq!( - L4PortRange::try_from("".to_string()), + L4PortRange::try_from("".to_string()).map_err(Into::into), Err(Error::invalid_value( - "", - "invalid port number: cannot parse integer from empty string" + "l4_port_range", + "\"\" unparsable for type: cannot parse integer from empty string" )) ); assert_eq!( - L4PortRange::try_from("65536".to_string()), + L4PortRange::try_from("65536".to_string()).map_err(Into::into), Err(Error::invalid_value( - "65536", - "invalid port number: number too large to fit in target type" + "l4_port_range", + "\"65536\" unparsable for type: number too large to fit in target type" )) ); assert_eq!( - L4PortRange::try_from("65535-65536".to_string()), + L4PortRange::try_from("65535-65536".to_string()) + .map_err(Into::into), Err(Error::invalid_value( - "65536", - "invalid port number: number too large to fit in target type" + "l4_port_range", + "\"65536\" unparsable for type: number too large to fit in target type" )) ); assert_eq!( - L4PortRange::try_from("0x23".to_string()), + L4PortRange::try_from("0x23".to_string()).map_err(Into::into), Err(Error::invalid_value( - "0x23", - "invalid port number: invalid digit found in string" + "l4_port_range", + "\"0x23\" unparsable for type: invalid digit found in string" )) ); assert_eq!( - L4PortRange::try_from("0".to_string()), + L4PortRange::try_from("0".to_string()).map_err(Into::into), Err(Error::invalid_value( - "0", - "invalid port number: number would be zero for non-zero type" + "l4_port_range", + "\"0\" unparsable for type: number would be zero for non-zero type" )) ); assert_eq!( - L4PortRange::try_from("0-20".to_string()), + L4PortRange::try_from("0-20".to_string()).map_err(Into::into), Err(Error::invalid_value( - "0", - "invalid port number: number would be zero for non-zero type" + "l4_port_range", + "\"0\" unparsable for type: number would be zero for non-zero type" )) ); assert_eq!( - L4PortRange::try_from("-20".to_string()), + L4PortRange::try_from("-20".to_string()).map_err(Into::into), Err(Error::invalid_value( - "", - "invalid port number: cannot parse integer from empty string" + "l4_port_range", + "range has no start value" )) ); } @@ -4181,12 +4192,44 @@ mod test { })), "icmp:60,0-10".parse().unwrap() ); - assert!("icmp:".parse::().is_err()); - assert!("icmp:20-30".parse::().is_err()); - assert!("icmp:10,".parse::().is_err()); - assert!("icmp:257".parse::().is_err()); - assert!( - "icmp:0,1000-1001".parse::().is_err() + assert_eq!( + "icmp:".parse::(), + Err(Error::invalid_value( + "icmp_type", + "\"\" unparsable for type: cannot parse integer from empty string" + )) + ); + assert_eq!( + "icmp:20-30".parse::(), + Err(Error::invalid_value( + "icmp_type", + "\"20-30\" unparsable for type: invalid digit found in string" + )) + ); + assert_eq!( + "icmp:10,".parse::(), + Err(Error::invalid_value( + "code", + "\"\" unparsable for type: cannot parse integer from empty string" + )) + ); + assert_eq!( + "icmp:257,".parse::(), + Err(Error::invalid_value( + "icmp_type", + "\"257\" unparsable for type: number too large to fit in target type" + )) + ); + assert_eq!( + "icmp:0,1000-1001".parse::(), + Err(Error::invalid_value( + "code", + "\"1000\" unparsable for type: number too large to fit in target type" + )) + ); + assert_eq!( + "icmp:0,30-".parse::(), + Err(Error::invalid_value("code", "range has no end value")) ); } diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index 7da34f0d777..d54496c35d1 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -765,16 +765,24 @@ impl DataStore { let rule = dsl::vpc_firewall_rule .filter(dsl::time_deleted.is_null()) .filter(dsl::vpc_id.eq(*SERVICES_VPC_ID)) - .filter(dsl::name.eq(NEXUS_ICMP_FW_RULE_NAME.to_string())) + .filter(dsl::name.eq(NEXUS_ICMP_FW_RULE_NAME)) .limit(1) .select(VpcFirewallRule::as_select()) .get_result_async::(&*conn) .await + .optional() .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; - Ok(ServiceIcmpConfig { - enabled: rule.status.0 == VpcFirewallRuleStatus::Enabled, - }) + if let Some(rule) = rule { + Ok(ServiceIcmpConfig { + enabled: rule.status.0 == VpcFirewallRuleStatus::Enabled, + }) + } else { + Err(Error::internal_error(&format!( + "services VPC is missing the builtin firewall rule \ + {NEXUS_ICMP_FW_RULE_NAME}" + ))) + } } pub async fn nexus_inbound_icmp_update( @@ -799,16 +807,24 @@ impl DataStore { let rule = diesel::update(dsl::vpc_firewall_rule) .filter(dsl::time_deleted.is_null()) .filter(dsl::vpc_id.eq(*SERVICES_VPC_ID)) - .filter(dsl::name.eq(NEXUS_ICMP_FW_RULE_NAME.to_string())) + .filter(dsl::name.eq(NEXUS_ICMP_FW_RULE_NAME)) .set((dsl::time_modified.eq(now), dsl::status.eq(status))) .returning(VpcFirewallRule::as_returning()) .get_result_async(&*conn) .await + .optional() .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; - Ok(ServiceIcmpConfig { - enabled: rule.status.0 == VpcFirewallRuleStatus::Enabled, - }) + if let Some(rule) = rule { + Ok(ServiceIcmpConfig { + enabled: rule.status.0 == VpcFirewallRuleStatus::Enabled, + }) + } else { + Err(Error::internal_error(&format!( + "services VPC is missing the builtin firewall rule \ + {NEXUS_ICMP_FW_RULE_NAME}" + ))) + } } /// Return the list of `Sled`s hosting instances or control plane services From 21efccf9bce205e8239d7ca4156732255fc3971d Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Fri, 13 Jun 2025 12:21:42 +0100 Subject: [PATCH 19/27] Feedback: allow limited ICMP into external DNS zones. --- nexus/db-fixed-data/src/vpc_firewall_rule.rs | 20 ++++++++++++-------- nexus/tests/integration_tests/schema.rs | 1 - schema/crdb/vpc-firewall-icmp/up08.sql | 13 +++++++++---- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/nexus/db-fixed-data/src/vpc_firewall_rule.rs b/nexus/db-fixed-data/src/vpc_firewall_rule.rs index a2d8a649f84..bb0ab69e4bc 100644 --- a/nexus/db-fixed-data/src/vpc_firewall_rule.rs +++ b/nexus/db-fixed-data/src/vpc_firewall_rule.rs @@ -77,13 +77,12 @@ pub const NEXUS_ICMP_FW_RULE_NAME: &str = "nexus-icmp"; /// Built-in VPC firewall rule for Nexus. /// /// This rule allows *arbitrary forwarding nodes* on the network to inform the -/// Nexus zone that packets have been explicitly dropped. This is a key part in -/// enabling path MTU discovery, such as when customers are accessing Necus +/// Nexus/DNS zones that packets have been explicitly dropped. This is a key part +/// in enabling path MTU discovery, such as when customers are accessing Nexus /// over a VPN. /// -/// Note that we currently rely on this being exactly one rule to implement the -/// system-level enable/disable endpoint. See `nexus/networking/src/firewall_rules.rs` -/// for more details. +/// We currently rely on this being exactly one rule to implement the system-level +/// enable/disable endpoint. See `nexus/networking/src/firewall_rules.rs`. pub static NEXUS_ICMP_FW_RULE: LazyLock = LazyLock::new(|| VpcFirewallRuleUpdate { name: NEXUS_ICMP_FW_RULE_NAME.parse().unwrap(), @@ -92,9 +91,14 @@ pub static NEXUS_ICMP_FW_RULE: LazyLock = .to_string(), status: VpcFirewallRuleStatus::Enabled, direction: VpcFirewallRuleDirection::Inbound, - targets: vec![VpcFirewallRuleTarget::Subnet( - super::vpc_subnet::NEXUS_VPC_SUBNET.name().clone(), - )], + targets: vec![ + VpcFirewallRuleTarget::Subnet( + super::vpc_subnet::NEXUS_VPC_SUBNET.name().clone(), + ), + VpcFirewallRuleTarget::Subnet( + super::vpc_subnet::DNS_VPC_SUBNET.name().clone(), + ), + ], filters: VpcFirewallRuleFilter { hosts: None, protocols: Some(vec![ diff --git a/nexus/tests/integration_tests/schema.rs b/nexus/tests/integration_tests/schema.rs index 9ff40eb3d69..d3564f6b392 100644 --- a/nexus/tests/integration_tests/schema.rs +++ b/nexus/tests/integration_tests/schema.rs @@ -2429,7 +2429,6 @@ fn after_151_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { }) } - // Lazily initializes all migration checks. The combination of Rust function // pointers and async makes defining a static table fairly painful, so we're // using lazy initialization instead. diff --git a/schema/crdb/vpc-firewall-icmp/up08.sql b/schema/crdb/vpc-firewall-icmp/up08.sql index 00d6aab11e1..9793aac75b8 100644 --- a/schema/crdb/vpc-firewall-icmp/up08.sql +++ b/schema/crdb/vpc-firewall-icmp/up08.sql @@ -4,16 +4,21 @@ INSERT INTO omicron.public.vpc_firewall_rule ( id, name, description, - time_created, time_modified, vpc_id, status, direction, - targets, filter_protocols, action, priority + time_created, time_modified, vpc_id, status, + targets, + filter_protocols, + direction, action, priority ) SELECT gen_random_uuid(), -- Hardcoded name/description, see nexus/db-fixed-data/src/vpc_firewall_rule.rs. 'nexus-icmp', 'allow typical inbound ICMP error codes for outbound flows', - NOW(), NOW(), vpc.id, 'enabled', 'inbound', + NOW(), NOW(), vpc.id, 'enabled', + -- Apply to the Nexus and External DNS zones... + ARRAY['subnet:nexus','subnet:external-dns'], -- Allow inbound ICMP Destination Unreachable (Too Large) and Time Exceeded - ARRAY['subnet:nexus'], ARRAY['icmp:3,4','icmp:11'], 'allow', 65534 + ARRAY['icmp:3,4','icmp:11'], + 'inbound', 'allow', 65534 FROM omicron.public.vpc WHERE vpc.id = '001de000-074c-4000-8000-000000000000' AND vpc.name = 'oxide-services' From 63e1f70e9c65c65487d9488501f97ca73f99cd15 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Fri, 13 Jun 2025 12:56:17 +0100 Subject: [PATCH 20/27] Extend ICMP allow to Redirect and Port Unreachable Redirect came out of our earlier discussion, port unreachable should be useful in the limit too. --- common/src/api/external/mod.rs | 7 ++++--- nexus/db-fixed-data/src/vpc_firewall_rule.rs | 9 +++++++-- nexus/networking/src/firewall_rules.rs | 5 +++++ openapi/nexus.json | 2 +- schema/crdb/vpc-firewall-icmp/up08.sql | 9 +++++++-- 5 files changed, 24 insertions(+), 8 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 027633a81eb..e5a11c6a034 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -3389,9 +3389,10 @@ pub enum BfdMode { )] pub struct ServiceIcmpConfig { /// When enabled, Nexus is able to receive ICMP Destination Unreachable - /// (type 4, fragmentation needed) and Time Exceeded messages. These - /// enable Nexus to perform Path MTU discovery and better cope with - /// fragmentation issues. Otherwise all ICMP traffic will be dropped. + /// type 3 (port unreachable) and type 4 (fragmentation needed), + /// Redirect, and Time Exceeded messages. These enable Nexus to perform Path + /// MTU discovery and better cope with fragmentation issues. Otherwise all + /// inbound ICMP traffic will be dropped. pub enabled: bool, } diff --git a/nexus/db-fixed-data/src/vpc_firewall_rule.rs b/nexus/db-fixed-data/src/vpc_firewall_rule.rs index bb0ab69e4bc..ccd0fc2d539 100644 --- a/nexus/db-fixed-data/src/vpc_firewall_rule.rs +++ b/nexus/db-fixed-data/src/vpc_firewall_rule.rs @@ -105,8 +105,13 @@ pub static NEXUS_ICMP_FW_RULE: LazyLock = VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter { // Type 3 -- Destination Unreachable icmp_type: 3, - // Code 4 -- Fragmentation needed - code: Some(4.into()), + // Codes 3,4 -- Port Unreachable, Fragmentation needed + code: Some((3..=4).into()), + })), + VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter { + // Type 5 -- Redirect + icmp_type: 5, + code: None, })), VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter { // Type 11 -- Time Exceeded diff --git a/nexus/networking/src/firewall_rules.rs b/nexus/networking/src/firewall_rules.rs index 5dba6d3bfbf..95fbb921d2f 100644 --- a/nexus/networking/src/firewall_rules.rs +++ b/nexus/networking/src/firewall_rules.rs @@ -316,6 +316,11 @@ pub async fn resolve_firewall_rules_for_sled_agent( // `nexus_db_queries::fixed_data::vpc_firewall_rule` for those // rules.) If those rules change to include any filter hosts, this // logic needs to change as well. + // + // Note that inbound ICMP is not currently governed by this filter, + // as error-type ICMP messages can arrive from any host which a + // Nexus/DNS zone reaches out to *or* a gateway/router on the path to + // that destination. (None, Some(allowed_ips)) => { if allowlist_applies_to_firewall_rule(rule) { match allowed_ips { diff --git a/openapi/nexus.json b/openapi/nexus.json index bd1889fd190..216744207d5 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -22845,7 +22845,7 @@ "type": "object", "properties": { "enabled": { - "description": "When enabled, Nexus is able to receive ICMP Destination Unreachable (type 4, fragmentation needed) and Time Exceeded messages. These enable Nexus to perform Path MTU discovery and better cope with fragmentation issues. Otherwise all ICMP traffic will be dropped.", + "description": "When enabled, Nexus is able to receive ICMP Destination Unreachable type 3 (port unreachable) and type 4 (fragmentation needed), Redirect, and Time Exceeded messages. These enable Nexus to perform Path MTU discovery and better cope with fragmentation issues. Otherwise all inbound ICMP traffic will be dropped.", "type": "boolean" } }, diff --git a/schema/crdb/vpc-firewall-icmp/up08.sql b/schema/crdb/vpc-firewall-icmp/up08.sql index 9793aac75b8..79e713468ad 100644 --- a/schema/crdb/vpc-firewall-icmp/up08.sql +++ b/schema/crdb/vpc-firewall-icmp/up08.sql @@ -16,8 +16,13 @@ SELECT NOW(), NOW(), vpc.id, 'enabled', -- Apply to the Nexus and External DNS zones... ARRAY['subnet:nexus','subnet:external-dns'], - -- Allow inbound ICMP Destination Unreachable (Too Large) and Time Exceeded - ARRAY['icmp:3,4','icmp:11'], + -- Allow inbound ICMP: + -- * Destination Unreachable + -- * Port Unreachable + -- * Fragmentation Needed + -- * Redirect + -- * Time Exceeded + ARRAY['icmp:3,3-4','icmp:5','icmp:11'], 'inbound', 'allow', 65534 FROM omicron.public.vpc WHERE vpc.id = '001de000-074c-4000-8000-000000000000' From 232389284559ce881482b366730f8996320f7c86 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Fri, 13 Jun 2025 13:04:56 +0100 Subject: [PATCH 21/27] Bump Deploy job image version (pre-emptively). --- .github/buildomat/jobs/deploy.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/buildomat/jobs/deploy.sh b/.github/buildomat/jobs/deploy.sh index fa41d797c43..5ad8e018b5d 100755 --- a/.github/buildomat/jobs/deploy.sh +++ b/.github/buildomat/jobs/deploy.sh @@ -2,7 +2,7 @@ #: #: name = "helios / deploy" #: variety = "basic" -#: target = "lab-2.0-opte-0.36" +#: target = "lab-2.0-opte-0.37" #: output_rules = [ #: "%/var/svc/log/oxide-*.log*", #: "%/zone/oxz_*/root/var/svc/log/oxide-*.log*", From fccba56883a10b3da83212e4c5fcaaffa33ae063 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Mon, 16 Jun 2025 17:34:57 +0100 Subject: [PATCH 22/27] Update VpcFirewallRuleProtocol to benefit from alloc-less FromSql --- nexus/db-model/src/vpc_firewall_rule.rs | 30 ++++++++----------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/nexus/db-model/src/vpc_firewall_rule.rs b/nexus/db-model/src/vpc_firewall_rule.rs index 42b332d3f2e..f8183c036d4 100644 --- a/nexus/db-model/src/vpc_firewall_rule.rs +++ b/nexus/db-model/src/vpc_firewall_rule.rs @@ -67,32 +67,20 @@ pub struct VpcFirewallRuleProtocol(pub external::VpcFirewallRuleProtocol); NewtypeFrom! { () pub struct VpcFirewallRuleProtocol(external::VpcFirewallRuleProtocol); } NewtypeDeref! { () pub struct VpcFirewallRuleProtocol(external::VpcFirewallRuleProtocol); } -impl ToSql for VpcFirewallRuleProtocol { - fn to_sql<'a>( - &'a self, - out: &mut serialize::Output<'a, '_, Pg>, - ) -> serialize::Result { - >::to_sql( - &self.0.to_string(), - &mut out.reborrow(), - ) +impl DatabaseString for VpcFirewallRuleProtocol { + type Error = ::Err; + + fn to_database_string(&self) -> Cow { + self.0.to_string().into() } -} -// Deserialize the "VpcFirewallRuleProtocol" object from SQL TEXT. -impl FromSql for VpcFirewallRuleProtocol -where - DB: Backend, - String: FromSql, -{ - fn from_sql(bytes: DB::RawValue<'_>) -> deserialize::Result { - Ok(VpcFirewallRuleProtocol( - String::from_sql(bytes)? - .parse::()?, - )) + fn from_database_string(s: &str) -> Result { + s.parse::().map(Self) } } +impl_from_sql_text!(VpcFirewallRuleProtocol); + /// Newtype wrapper around [`external::VpcFirewallRuleTarget`] so we can derive /// diesel traits for it #[derive(Clone, Debug, AsExpression, FromSqlRow, Serialize, Deserialize)] From 0d2fa6e0d645a29692afae4af5160796c552e3ad Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Wed, 25 Jun 2025 17:31:18 +0100 Subject: [PATCH 23/27] Remove OPTE_COMMIT override --- .github/buildomat/jobs/deploy.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/buildomat/jobs/deploy.sh b/.github/buildomat/jobs/deploy.sh index 5ad8e018b5d..4c670925f54 100755 --- a/.github/buildomat/jobs/deploy.sh +++ b/.github/buildomat/jobs/deploy.sh @@ -136,7 +136,7 @@ z_swadm () { # only set this if you want to override the version of opte/xde installed by the # install_opte.sh script -OPTE_COMMIT="792ec8a3816ba8c7c4268f65cd19dbd946a9027d" +OPTE_COMMIT="" if [[ "x$OPTE_COMMIT" != "x" ]]; then curl -sSfOL https://buildomat.eng.oxide.computer/public/file/oxidecomputer/opte/module/$OPTE_COMMIT/xde pfexec rem_drv xde || true From 6eec7a00ea889cb74f8e0ecb5777935998580a87 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Fri, 27 Jun 2025 11:20:23 +0100 Subject: [PATCH 24/27] Incredibly silly missed schema versions --- nexus/db-model/src/schema_versions.rs | 2 +- nexus/tests/integration_tests/schema.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 94be809bc90..b753f94424b 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock}; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: Version = Version::new(153, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(154, 0, 0); /// List of all past database schema versions, in *reverse* order /// diff --git a/nexus/tests/integration_tests/schema.rs b/nexus/tests/integration_tests/schema.rs index 7fca4c0303e..5e2fac0e8ae 100644 --- a/nexus/tests/integration_tests/schema.rs +++ b/nexus/tests/integration_tests/schema.rs @@ -2582,7 +2582,7 @@ fn get_migration_checks() -> BTreeMap { DataMigrationFns::new().before(before_151_0_0).after(after_151_0_0), ); map.insert( - Version::new(153, 0, 0), + Version::new(154, 0, 0), DataMigrationFns::new().before(before_154_0_0).after(after_154_0_0), ); From 5a5308fbb9010931129d2c5981fddb43043e486e Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Sat, 28 Jun 2025 08:59:01 +0100 Subject: [PATCH 25/27] Bump OPTE to latest patch. --- Cargo.lock | 17 +++++++++-------- Cargo.toml | 4 ++-- tools/opte_version | 2 +- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 76f763a2ad7..5e20a3dd2e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4738,7 +4738,7 @@ dependencies = [ [[package]] name = "illumos-sys-hdrs" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=3f2dfe36f156b486e60e7a08263ad6227be1e969#3f2dfe36f156b486e60e7a08263ad6227be1e969" +source = "git+https://github.com/oxidecomputer/opte?rev=3d1263ced8177893d46da54a914e4c510dc2bfc8#3d1263ced8177893d46da54a914e4c510dc2bfc8" dependencies = [ "bitflags 2.9.0", ] @@ -5317,7 +5317,7 @@ dependencies = [ [[package]] name = "kstat-macro" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=3f2dfe36f156b486e60e7a08263ad6227be1e969#3f2dfe36f156b486e60e7a08263ad6227be1e969" +source = "git+https://github.com/oxidecomputer/opte?rev=3d1263ced8177893d46da54a914e4c510dc2bfc8#3d1263ced8177893d46da54a914e4c510dc2bfc8" dependencies = [ "quote", "syn 2.0.104", @@ -8471,7 +8471,7 @@ dependencies = [ [[package]] name = "opte" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=3f2dfe36f156b486e60e7a08263ad6227be1e969#3f2dfe36f156b486e60e7a08263ad6227be1e969" +source = "git+https://github.com/oxidecomputer/opte?rev=3d1263ced8177893d46da54a914e4c510dc2bfc8#3d1263ced8177893d46da54a914e4c510dc2bfc8" dependencies = [ "bitflags 2.9.0", "dyn-clone", @@ -8489,7 +8489,7 @@ dependencies = [ [[package]] name = "opte-api" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=3f2dfe36f156b486e60e7a08263ad6227be1e969#3f2dfe36f156b486e60e7a08263ad6227be1e969" +source = "git+https://github.com/oxidecomputer/opte?rev=3d1263ced8177893d46da54a914e4c510dc2bfc8#3d1263ced8177893d46da54a914e4c510dc2bfc8" dependencies = [ "illumos-sys-hdrs", "ingot", @@ -8502,7 +8502,7 @@ dependencies = [ [[package]] name = "opte-ioctl" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=3f2dfe36f156b486e60e7a08263ad6227be1e969#3f2dfe36f156b486e60e7a08263ad6227be1e969" +source = "git+https://github.com/oxidecomputer/opte?rev=3d1263ced8177893d46da54a914e4c510dc2bfc8#3d1263ced8177893d46da54a914e4c510dc2bfc8" dependencies = [ "libc", "libnet", @@ -8585,10 +8585,11 @@ dependencies = [ [[package]] name = "oxide-tokio-rt" -version = "0.1.1" +version = "0.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4b64cff9722f687d5ba4c954d9a28e18d3e26c8861d588098cfe82871b3e31d1" +checksum = "84bd87abf37c68d414e4df90a545857542140e07206f75039b8f63b244da87b8" dependencies = [ + "anyhow", "tokio", "tokio-dtrace", ] @@ -8596,7 +8597,7 @@ dependencies = [ [[package]] name = "oxide-vpc" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=3f2dfe36f156b486e60e7a08263ad6227be1e969#3f2dfe36f156b486e60e7a08263ad6227be1e969" +source = "git+https://github.com/oxidecomputer/opte?rev=3d1263ced8177893d46da54a914e4c510dc2bfc8#3d1263ced8177893d46da54a914e4c510dc2bfc8" dependencies = [ "cfg-if", "illumos-sys-hdrs", diff --git a/Cargo.toml b/Cargo.toml index 3778bee1cb1..f149fc11942 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -565,7 +565,7 @@ omicron-workspace-hack = "0.1.0" omicron-zone-package = "0.12.2" oxide-client = { path = "clients/oxide-client" } oxide-tokio-rt = "0.1.1" -oxide-vpc = { git = "https://github.com/oxidecomputer/opte", rev = "3f2dfe36f156b486e60e7a08263ad6227be1e969", features = [ "api", "std" ] } +oxide-vpc = { git = "https://github.com/oxidecomputer/opte", rev = "3d1263ced8177893d46da54a914e4c510dc2bfc8", features = [ "api", "std" ] } oxlog = { path = "dev-tools/oxlog" } oxnet = "0.1.2" once_cell = "1.21.3" @@ -575,7 +575,7 @@ openapiv3 = "2.2.0" # must match samael's crate! openssl = "0.10" openssl-sys = "0.9" -opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "3f2dfe36f156b486e60e7a08263ad6227be1e969" } +opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "3d1263ced8177893d46da54a914e4c510dc2bfc8" } oso = "0.27" owo-colors = "4.2.2" oximeter = { path = "oximeter/oximeter" } diff --git a/tools/opte_version b/tools/opte_version index b0a9ceb5cb6..e99d1304310 100644 --- a/tools/opte_version +++ b/tools/opte_version @@ -1 +1 @@ -0.37.381 +0.37.383 From f652de1e7b87d10cfa6a6e3bf29bd2680be465cd Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Sat, 28 Jun 2025 13:15:21 +0100 Subject: [PATCH 26/27] Revert "Bump OPTE to latest patch." This reverts commit 5a5308fbb9010931129d2c5981fddb43043e486e. It looks as though something is gummed up in pkg, since 0.37.381 is the last available version there. --- Cargo.lock | 17 ++++++++--------- Cargo.toml | 4 ++-- tools/opte_version | 2 +- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5e20a3dd2e4..76f763a2ad7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4738,7 +4738,7 @@ dependencies = [ [[package]] name = "illumos-sys-hdrs" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=3d1263ced8177893d46da54a914e4c510dc2bfc8#3d1263ced8177893d46da54a914e4c510dc2bfc8" +source = "git+https://github.com/oxidecomputer/opte?rev=3f2dfe36f156b486e60e7a08263ad6227be1e969#3f2dfe36f156b486e60e7a08263ad6227be1e969" dependencies = [ "bitflags 2.9.0", ] @@ -5317,7 +5317,7 @@ dependencies = [ [[package]] name = "kstat-macro" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=3d1263ced8177893d46da54a914e4c510dc2bfc8#3d1263ced8177893d46da54a914e4c510dc2bfc8" +source = "git+https://github.com/oxidecomputer/opte?rev=3f2dfe36f156b486e60e7a08263ad6227be1e969#3f2dfe36f156b486e60e7a08263ad6227be1e969" dependencies = [ "quote", "syn 2.0.104", @@ -8471,7 +8471,7 @@ dependencies = [ [[package]] name = "opte" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=3d1263ced8177893d46da54a914e4c510dc2bfc8#3d1263ced8177893d46da54a914e4c510dc2bfc8" +source = "git+https://github.com/oxidecomputer/opte?rev=3f2dfe36f156b486e60e7a08263ad6227be1e969#3f2dfe36f156b486e60e7a08263ad6227be1e969" dependencies = [ "bitflags 2.9.0", "dyn-clone", @@ -8489,7 +8489,7 @@ dependencies = [ [[package]] name = "opte-api" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=3d1263ced8177893d46da54a914e4c510dc2bfc8#3d1263ced8177893d46da54a914e4c510dc2bfc8" +source = "git+https://github.com/oxidecomputer/opte?rev=3f2dfe36f156b486e60e7a08263ad6227be1e969#3f2dfe36f156b486e60e7a08263ad6227be1e969" dependencies = [ "illumos-sys-hdrs", "ingot", @@ -8502,7 +8502,7 @@ dependencies = [ [[package]] name = "opte-ioctl" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=3d1263ced8177893d46da54a914e4c510dc2bfc8#3d1263ced8177893d46da54a914e4c510dc2bfc8" +source = "git+https://github.com/oxidecomputer/opte?rev=3f2dfe36f156b486e60e7a08263ad6227be1e969#3f2dfe36f156b486e60e7a08263ad6227be1e969" dependencies = [ "libc", "libnet", @@ -8585,11 +8585,10 @@ dependencies = [ [[package]] name = "oxide-tokio-rt" -version = "0.1.2" +version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "84bd87abf37c68d414e4df90a545857542140e07206f75039b8f63b244da87b8" +checksum = "4b64cff9722f687d5ba4c954d9a28e18d3e26c8861d588098cfe82871b3e31d1" dependencies = [ - "anyhow", "tokio", "tokio-dtrace", ] @@ -8597,7 +8596,7 @@ dependencies = [ [[package]] name = "oxide-vpc" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/opte?rev=3d1263ced8177893d46da54a914e4c510dc2bfc8#3d1263ced8177893d46da54a914e4c510dc2bfc8" +source = "git+https://github.com/oxidecomputer/opte?rev=3f2dfe36f156b486e60e7a08263ad6227be1e969#3f2dfe36f156b486e60e7a08263ad6227be1e969" dependencies = [ "cfg-if", "illumos-sys-hdrs", diff --git a/Cargo.toml b/Cargo.toml index f149fc11942..3778bee1cb1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -565,7 +565,7 @@ omicron-workspace-hack = "0.1.0" omicron-zone-package = "0.12.2" oxide-client = { path = "clients/oxide-client" } oxide-tokio-rt = "0.1.1" -oxide-vpc = { git = "https://github.com/oxidecomputer/opte", rev = "3d1263ced8177893d46da54a914e4c510dc2bfc8", features = [ "api", "std" ] } +oxide-vpc = { git = "https://github.com/oxidecomputer/opte", rev = "3f2dfe36f156b486e60e7a08263ad6227be1e969", features = [ "api", "std" ] } oxlog = { path = "dev-tools/oxlog" } oxnet = "0.1.2" once_cell = "1.21.3" @@ -575,7 +575,7 @@ openapiv3 = "2.2.0" # must match samael's crate! openssl = "0.10" openssl-sys = "0.9" -opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "3d1263ced8177893d46da54a914e4c510dc2bfc8" } +opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "3f2dfe36f156b486e60e7a08263ad6227be1e969" } oso = "0.27" owo-colors = "4.2.2" oximeter = { path = "oximeter/oximeter" } diff --git a/tools/opte_version b/tools/opte_version index e99d1304310..b0a9ceb5cb6 100644 --- a/tools/opte_version +++ b/tools/opte_version @@ -1 +1 @@ -0.37.383 +0.37.381 From 299aca0d13ebdd6b8cff0a8f1139485c33f26421 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Mon, 30 Jun 2025 10:18:38 +0100 Subject: [PATCH 27/27] Bump Maghemite to main. --- Cargo.lock | 4 ++-- Cargo.toml | 4 ++-- package-manifest.toml | 12 ++++++------ tools/maghemite_ddm_openapi_version | 2 +- tools/maghemite_mg_openapi_version | 2 +- tools/maghemite_mgd_checksums | 4 ++-- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 76f763a2ad7..cf2b577b3fd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2311,7 +2311,7 @@ dependencies = [ [[package]] name = "ddm-admin-client" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/maghemite?rev=c1546d033d078ecd636951191393be1edcf37544#c1546d033d078ecd636951191393be1edcf37544" +source = "git+https://github.com/oxidecomputer/maghemite?rev=638c897d5ed1e5d3f2de0c1cb9dfaa4d77a35dea#638c897d5ed1e5d3f2de0c1cb9dfaa4d77a35dea" dependencies = [ "oxnet", "percent-encoding", @@ -5882,7 +5882,7 @@ dependencies = [ [[package]] name = "mg-admin-client" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/maghemite?rev=c1546d033d078ecd636951191393be1edcf37544#c1546d033d078ecd636951191393be1edcf37544" +source = "git+https://github.com/oxidecomputer/maghemite?rev=638c897d5ed1e5d3f2de0c1cb9dfaa4d77a35dea#638c897d5ed1e5d3f2de0c1cb9dfaa4d77a35dea" dependencies = [ "anyhow", "chrono", diff --git a/Cargo.toml b/Cargo.toml index 3778bee1cb1..ed3013cc097 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -509,8 +509,8 @@ lldp_protocol = { git = "https://github.com/oxidecomputer/lldp", package = "prot macaddr = { version = "1.0.1", features = ["serde_std"] } maplit = "1.0.2" newtype_derive = "0.1.6" -mg-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "c1546d033d078ecd636951191393be1edcf37544" } -ddm-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "c1546d033d078ecd636951191393be1edcf37544" } +mg-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "638c897d5ed1e5d3f2de0c1cb9dfaa4d77a35dea" } +ddm-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "638c897d5ed1e5d3f2de0c1cb9dfaa4d77a35dea" } multimap = "0.10.1" nexus-auth = { path = "nexus/auth" } nexus-background-task-interface = { path = "nexus/background-task-interface" } diff --git a/package-manifest.toml b/package-manifest.toml index a8fa40af9b6..60c78f2ce1e 100644 --- a/package-manifest.toml +++ b/package-manifest.toml @@ -638,10 +638,10 @@ source.repo = "maghemite" # `tools/maghemite_openapi_version`. Failing to do so will cause a failure when # building `ddm-admin-client` (which will instruct you to update # `tools/maghemite_openapi_version`). -source.commit = "c1546d033d078ecd636951191393be1edcf37544" +source.commit = "638c897d5ed1e5d3f2de0c1cb9dfaa4d77a35dea" # The SHA256 digest is automatically posted to: # https://buildomat.eng.oxide.computer/public/file/oxidecomputer/maghemite/image//mg-ddm-gz.sha256.txt -source.sha256 = "49138e76b59871b3ff9731b63f03b4c265f0e3f707d3fb9c1ccce766efdfe618" +source.sha256 = "9a528e6af530c8a5dcd180a41e17ebb1adb05e336550c1ac2134acca1d6557ab" output.type = "tarball" [package.mg-ddm] @@ -654,10 +654,10 @@ source.repo = "maghemite" # `tools/maghemite_openapi_version`. Failing to do so will cause a failure when # building `ddm-admin-client` (which will instruct you to update # `tools/maghemite_openapi_version`). -source.commit = "c1546d033d078ecd636951191393be1edcf37544" +source.commit = "638c897d5ed1e5d3f2de0c1cb9dfaa4d77a35dea" # The SHA256 digest is automatically posted to: # https://buildomat.eng.oxide.computer/public/file/oxidecomputer/maghemite/image//mg-ddm.sha256.txt -source.sha256 = "256cf5b29e50570970c577925f6c067ebc40ce051f08d7b24e178284efdd3bca" +source.sha256 = "275bf1088c548784e6a5eea7dd91db1878be1c44696a89f506b2a0515a5138ef" output.type = "zone" output.intermediate_only = true @@ -669,10 +669,10 @@ source.repo = "maghemite" # `tools/maghemite_openapi_version`. Failing to do so will cause a failure when # building `ddm-admin-client` (which will instruct you to update # `tools/maghemite_openapi_version`). -source.commit = "c1546d033d078ecd636951191393be1edcf37544" +source.commit = "638c897d5ed1e5d3f2de0c1cb9dfaa4d77a35dea" # The SHA256 digest is automatically posted to: # https://buildomat.eng.oxide.computer/public/file/oxidecomputer/maghemite/image//mgd.sha256.txt -source.sha256 = "c029265300e6f64cda2e23d75667e180d12941a7ab8049f6848c9c575275d1c5" +source.sha256 = "6451cdc1473e555676bc33eb3e94f96205a2d839120318b3dfd4600232f95626" output.type = "zone" output.intermediate_only = true diff --git a/tools/maghemite_ddm_openapi_version b/tools/maghemite_ddm_openapi_version index d8ba445e8d5..ba3584bd99e 100644 --- a/tools/maghemite_ddm_openapi_version +++ b/tools/maghemite_ddm_openapi_version @@ -1,2 +1,2 @@ -COMMIT="c1546d033d078ecd636951191393be1edcf37544" +COMMIT="638c897d5ed1e5d3f2de0c1cb9dfaa4d77a35dea" SHA2="9146aaf60a52ecd138139708e4019e4496f330fb81a2c5a7a70cd3436a6a1318" diff --git a/tools/maghemite_mg_openapi_version b/tools/maghemite_mg_openapi_version index ba71ae6c86c..a21dd3b71f3 100644 --- a/tools/maghemite_mg_openapi_version +++ b/tools/maghemite_mg_openapi_version @@ -1,2 +1,2 @@ -COMMIT="c1546d033d078ecd636951191393be1edcf37544" +COMMIT="638c897d5ed1e5d3f2de0c1cb9dfaa4d77a35dea" SHA2="7af1675e2e93e395185f8d3676db972db0123714c4c5640608f3e3570f3ce3a8" diff --git a/tools/maghemite_mgd_checksums b/tools/maghemite_mgd_checksums index 34089c7da63..89303c1d36d 100644 --- a/tools/maghemite_mgd_checksums +++ b/tools/maghemite_mgd_checksums @@ -1,2 +1,2 @@ -CIDL_SHA256="c029265300e6f64cda2e23d75667e180d12941a7ab8049f6848c9c575275d1c5" -MGD_LINUX_SHA256="ebbe6317bde0a4716e17ed4c5cf7b33765d7807066b1f743b1ec1db2dc274bd3" \ No newline at end of file +CIDL_SHA256="6451cdc1473e555676bc33eb3e94f96205a2d839120318b3dfd4600232f95626" +MGD_LINUX_SHA256="7f0490149777538f6c7d4b074a6ed5c973f07f96bdf74e672f93b17062d8ffc1" \ No newline at end of file