Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop abusing OPTE's source NAT configuration for external IPs #1479

Merged
merged 1 commit into from
Jul 25, 2022
Merged

Stop abusing OPTE's source NAT configuration for external IPs #1479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Cargo.lock
View file
Open in desktop

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

18 changes: 9 additions & 9 deletions nexus/src/app/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ use omicron_common::api::external::ListResultVec;
use omicron_common::api::external::LookupResult;
use omicron_common::api::external::UpdateResult;
use omicron_common::api::internal::nexus;
use sled_agent_client::types::ExternalIp;
use sled_agent_client::types::InstanceRuntimeStateMigrateParams;
use sled_agent_client::types::InstanceRuntimeStateRequested;
use sled_agent_client::types::InstanceStateRequested;
use sled_agent_client::types::SourceNatConfig;
use sled_agent_client::Client as SledAgentClient;
use std::sync::Arc;
use uuid::Uuid;
Expand Down Expand Up @@ -538,6 +538,8 @@ impl super::Nexus {
.partition(|ip| ip.kind == IpKind::SNat);

// Sanity checks on the number and kind of each IP address.
// TODO-correctness: Handle multiple IP addresses, see
// https://github.com/oxidecomputer/omicron/issues/1467
if external_ips.len() > MAX_EXTERNAL_IPS_PER_INSTANCE {
return Err(Error::internal_error(
format!(
Expand All @@ -549,18 +551,15 @@ impl super::Nexus {
.as_str(),
));
}
let external_ips =
external_ips.into_iter().map(|model| model.ip.ip()).collect();
if snat_ip.len() != 1 {
return Err(Error::internal_error(
"Expected exactly one SNAT IP address for an instance",
));
}

// For now, we take the Ephemeral IP, if it exists, or the SNAT IP if not.
// TODO-correctness: Handle multiple IP addresses, see
// https://github.com/oxidecomputer/omicron/issues/1467
let external_ip = ExternalIp::from(
external_ips.into_iter().chain(snat_ip).next().unwrap(),
);
let source_nat =
SourceNatConfig::from(snat_ip.into_iter().next().unwrap());

// Gather the SSH public keys of the actor make the request so
// that they may be injected into the new image via cloud-init.
Expand Down Expand Up @@ -600,7 +599,8 @@ impl super::Nexus {
db_instance.runtime().clone(),
),
nics,
external_ip,
source_nat,
external_ips,
disks: disk_reqs,
cloud_init_bytes: Some(base64::encode(
db_instance.generate_cidata(&public_keys)?,
Expand Down
15 changes: 6 additions & 9 deletions nexus/src/app/sagas/instance_migrate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@ use omicron_common::api::external::Error;
use omicron_common::api::internal::nexus::InstanceRuntimeState;
use serde::Deserialize;
use serde::Serialize;
use sled_agent_client::types::ExternalIp;
use sled_agent_client::types::InstanceEnsureBody;
use sled_agent_client::types::InstanceHardware;
use sled_agent_client::types::InstanceMigrateParams;
use sled_agent_client::types::InstanceRuntimeStateMigrateParams;
use sled_agent_client::types::InstanceRuntimeStateRequested;
use sled_agent_client::types::InstanceStateRequested;
use sled_agent_client::types::SourceNatConfig;
use std::net::Ipv6Addr;
use std::sync::Arc;
use steno::new_action_noop_undo;
Expand Down Expand Up @@ -189,24 +189,21 @@ async fn sim_instance_migrate(
.as_str(),
)));
}
let external_ips =
external_ips.into_iter().map(|model| model.ip.ip()).collect();
if snat_ip.len() != 1 {
return Err(ActionError::action_failed(Error::internal_error(
"Expected exactly one SNAT IP address for an instance",
)));
}

// For now, we take the Ephemeral IP, if it exists, or the SNAT IP if not.
// TODO-correctness: Handle multiple IP addresses, see
// https://github.com/oxidecomputer/omicron/issues/1467
let external_ip = ExternalIp::from(
external_ips.into_iter().chain(snat_ip).next().unwrap(),
);
let source_nat = SourceNatConfig::from(snat_ip.into_iter().next().unwrap());

let instance_hardware = InstanceHardware {
runtime: runtime.into(),
// TODO: populate NICs
nics: vec![],
external_ip,
source_nat,
external_ips,
// TODO: populate disks
disks: vec![],
// TODO: populate cloud init bytes
Expand Down
2 changes: 1 addition & 1 deletion nexus/src/db/model/external_ip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ pub struct InstanceExternalIp {
pub last_port: SqlU16,
}

impl From<InstanceExternalIp> for sled_agent_client::types::ExternalIp {
impl From<InstanceExternalIp> for sled_agent_client::types::SourceNatConfig {
fn from(eip: InstanceExternalIp) -> Self {
Self {
ip: eip.ip.ip(),
Expand Down
6 changes: 1 addition & 5 deletions nexus/src/db/queries/external_ip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,7 @@ const INSTANCE_EXTERNAL_IP_FROM_CLAUSE: InstanceExternalIpFromClause =
// instead to check if a candidate port range has any overlap with an existing
// port range, which is more complicated. That's deferred until we actually have
// that situation (which may be as soon as allocating ephemeral IPs).
//
// TODO-correctness: We're currently providing the entire port range, even for
// source NAT. We'd like to chunk this up in to something more like quadrants,
// e.g., ranges `[0, 16384)`, `[16384, 32768)`, etc.
const NUM_SOURCE_NAT_PORTS: usize = 1 << 16;
const NUM_SOURCE_NAT_PORTS: usize = 1 << 14;
const MAX_PORT: i32 = u16::MAX as _;

/// Select the next available IP address and port range for an instance's
Expand Down
73 changes: 41 additions & 32 deletions openapi/sled-agent.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,34 +709,6 @@
"request_id"
]
},
"ExternalIp": {
"description": "An external IP address used for external connectivity for an instance.",
"type": "object",
"properties": {
"first_port": {
"description": "The first port used for instance NAT, inclusive.",
"type": "integer",
"format": "uint16",
"minimum": 0
},
"ip": {
"description": "The external address provided to the instance",
"type": "string",
"format": "ip"
},
"last_port": {
"description": "The last port used for instance NAT, also inclusive.",
"type": "integer",
"format": "uint16",
"minimum": 0
}
},
"required": [
"first_port",
"ip",
"last_port"
]
},
"Generation": {
"description": "Generation numbers stored in the database, used for optimistic concurrency control",
"type": "integer",
Expand Down Expand Up @@ -798,8 +770,13 @@
"$ref": "#/components/schemas/DiskRequest"
}
},
"external_ip": {
"$ref": "#/components/schemas/ExternalIp"
"external_ips": {
"description": "Zero or more external IP addresses (either floating or ephemeral), provided to an instance to allow inbound connectivity.",
"type": "array",
"items": {
"type": "string",
"format": "ip"
}
},
"nics": {
"type": "array",
Expand All @@ -809,13 +786,17 @@
},
"runtime": {
"$ref": "#/components/schemas/InstanceRuntimeState"
},
"source_nat": {
"$ref": "#/components/schemas/SourceNatConfig"
}
},
"required": [
"disks",
"external_ip",
"external_ips",
"nics",
"runtime"
"runtime",
"source_nat"
]
},
"InstanceMigrateParams": {
Expand Down Expand Up @@ -1236,6 +1217,34 @@
"format": "uint8",
"minimum": 0
},
"SourceNatConfig": {
"description": "An IP address and port range used for instance source NAT, i.e., making outbound network connections from guests.",
"type": "object",
"properties": {
"first_port": {
"description": "The first port used for instance NAT, inclusive.",
"type": "integer",
"format": "uint16",
"minimum": 0
},
"ip": {
"description": "The external address provided to the instance",
"type": "string",
"format": "ip"
},
"last_port": {
"description": "The last port used for instance NAT, also inclusive.",
"type": "integer",
"format": "uint16",
"minimum": 0
}
},
"required": [
"first_port",
"ip",
"last_port"
]
},
"UpdateArtifact": {
"description": "Description of a single update artifact.",
"type": "object",
Expand Down
4 changes: 2 additions & 2 deletions sled-agent/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ vsss-rs = { version = "2.0.0-pre2", default-features = false, features = ["std"]
zone = "0.1"

[target.'cfg(target_os = "illumos")'.dependencies]
opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "eb9e0c687e3c072dbc7d4782475f4ba5a6f50258" }
opte = { git = "https://github.com/oxidecomputer/opte", rev = "eb9e0c687e3c072dbc7d4782475f4ba5a6f50258", features = [ "api", "std" ] }
opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "23884d35aa7908e23accaa77f125a370ddf5c606" }
opte = { git = "https://github.com/oxidecomputer/opte", rev = "23884d35aa7908e23accaa77f125a370ddf5c606", features = [ "api", "std" ] }

[dev-dependencies]
expectorate = "1.0.5"
Expand Down
25 changes: 16 additions & 9 deletions sled-agent/src/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ use crate::instance_manager::InstanceTicket;
use crate::nexus::LazyNexusClient;
use crate::opte::PortManager;
use crate::opte::PortTicket;
use crate::params::ExternalIp;
use crate::params::NetworkInterface;
use crate::params::SourceNatConfig;
use crate::params::{
InstanceHardware, InstanceMigrateParams, InstanceRuntimeStateRequested,
InstanceSerialConsoleData,
Expand Down Expand Up @@ -219,7 +219,8 @@ struct InstanceInner {

// Guest NIC and OPTE port information
requested_nics: Vec<NetworkInterface>,
external_ip: ExternalIp,
source_nat: SourceNatConfig,
external_ips: Vec<IpAddr>,

// Disk related properties
requested_disks: Vec<DiskRequest>,
Expand Down Expand Up @@ -480,7 +481,8 @@ impl Instance {
vnic_allocator,
port_manager,
requested_nics: initial.nics,
external_ip: initial.external_ip,
source_nat: initial.source_nat,
external_ips: initial.external_ips,
requested_disks: initial.disks,
cloud_init_bytes: initial.cloud_init_bytes,
state: InstanceStates::new(initial.runtime),
Expand All @@ -502,12 +504,16 @@ impl Instance {
let mut opte_ports = Vec::with_capacity(inner.requested_nics.len());
let mut port_tickets = Vec::with_capacity(inner.requested_nics.len());
for nic in inner.requested_nics.iter() {
let external_ip =
if nic.primary { Some(inner.external_ip) } else { None };
let (snat, external_ips) = if nic.primary {
(Some(inner.source_nat), Some(inner.external_ips.clone()))
} else {
(None, None)
};
let port = inner.port_manager.create_port(
*inner.id(),
nic,
external_ip,
snat,
external_ips,
)?;
port_tickets.push(port.ticket());
opte_ports.push(port);
Expand Down Expand Up @@ -793,8 +799,8 @@ mod test {
use crate::illumos::dladm::Etherstub;
use crate::nexus::LazyNexusClient;
use crate::opte::PortManager;
use crate::params::ExternalIp;
use crate::params::InstanceStateRequested;
use crate::params::SourceNatConfig;
use chrono::Utc;
use macaddr::MacAddr6;
use omicron_common::api::external::{
Expand Down Expand Up @@ -839,11 +845,12 @@ mod test {
time_updated: Utc::now(),
},
nics: vec![],
external_ip: ExternalIp {
source_nat: SourceNatConfig {
ip: IpAddr::from(Ipv4Addr::new(10, 0, 0, 1)),
first_port: 0,
last_port: 1 << 14 - 1,
last_port: 16_384,
},
external_ips: vec![],
disks: vec![],
cloud_init_bytes: None,
}
Expand Down
5 changes: 3 additions & 2 deletions sled-agent/src/instance_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,8 @@ mod test {
use crate::illumos::{dladm::MockDladm, zone::MockZones};
use crate::instance::MockInstance;
use crate::nexus::LazyNexusClient;
use crate::params::ExternalIp;
use crate::params::InstanceStateRequested;
use crate::params::SourceNatConfig;
use chrono::Utc;
use macaddr::MacAddr6;
use omicron_common::api::external::{
Expand Down Expand Up @@ -271,11 +271,12 @@ mod test {
time_updated: Utc::now(),
},
nics: vec![],
external_ip: ExternalIp {
source_nat: SourceNatConfig {
ip: IpAddr::from(Ipv4Addr::new(10, 0, 0, 1)),
first_port: 0,
last_port: 1 << 14 - 1,
},
external_ips: vec![],
disks: vec![],
cloud_init_bytes: None,
}
Expand Down
Loading