Repository navigation
Embed sush in sled-agent #11252
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.
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
base: main
Are you sure you want to change the base?
Embed sush in sled-agent #11252
Changes from 18 commits
01f98ba
552801f
1b65c0f
7f810f4
d44a838
3dc9c68
ae91da0
301e9d5
59f30ff
4e9c78e
5a65227
3c7792c
b1eefa9
4c7ccb1
ecaf96d
6ef1b06
e50003d
48ada88
e544dcd
898c661
7481505
3179577
37e4597
05b110c
a2b8a97
b0eef7f
f65febb
9e058e7
2640954
32741d6
f62cd42
e9ff322
e3068d5
5096283
c494933
fe80b99
e014b50
19d21a1
40d6e40
f3d856a
289dbec
aed0181
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ use super::server::StartError; | |
| use bootstore::schemes::v0 as bootstore; | ||
| use camino::Utf8PathBuf; | ||
| use omicron_common::address::BOOTSTORE_PORT; | ||
| use omicron_common::address::SUSH_GOSSIP_PORT; | ||
| use omicron_common::address::TRUST_QUORUM_PORT; | ||
| use omicron_ddm_admin_client::Client as DdmAdminClient; | ||
| use sled_hardware_types::Baseboard; | ||
|
|
@@ -22,6 +23,7 @@ use std::collections::BTreeSet; | |
| use std::net::Ipv6Addr; | ||
| use std::net::SocketAddrV6; | ||
| use std::time::Duration; | ||
| use tokio::sync::watch; | ||
|
|
||
| const BOOTSTORE_FSM_STATE_FILE: &str = "bootstore-fsm-state.json"; | ||
| const BOOTSTORE_NETWORK_CONFIG_FILE: &str = "bootstore-network-config.json"; | ||
|
|
@@ -75,10 +77,14 @@ fn bootstore_network_config_paths( | |
| Ok(paths) | ||
| } | ||
|
|
||
| pub async fn poll_ddmd_for_bootstore_and_tq_peer_update( | ||
| /// Poll ddmd for the set of peer sleds on the bootstrap network, and | ||
| /// publish it to the three consumers that each need it at their own | ||
| /// port: the bootstore, the trust quorum node, and sush gossip. | ||
|
plotnick marked this conversation as resolved.
Resolved
|
||
| pub async fn poll_ddmd_for_peer_updates( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now that this has a more generic name, perhaps it deserves a comment explaining what the "peer updates" it polls for (i.e. the bootstore data, trust quorum, and sush gossip proto)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, hmm...i kinda wonder if this really still belongs in a module called
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not disagree, but think it should be done as a follow-up in conjunction with someone that has (much) more sled agent experience than I do. Documented in d44a838, thanks. |
||
| log: Logger, | ||
| bootstore_node_handle: bootstore::NodeHandle, | ||
| trust_quorum_handle: trust_quorum::NodeTaskHandle, | ||
| sush_gossip_tx: watch::Sender<BTreeSet<SocketAddrV6>>, | ||
| ) { | ||
| let mut current_bootstore_peers: BTreeSet<SocketAddrV6> = BTreeSet::new(); | ||
| let mut current_tq_peers: BTreeSet<SocketAddrV6> = BTreeSet::new(); | ||
|
|
@@ -137,6 +143,18 @@ pub async fn poll_ddmd_for_bootstore_and_tq_peer_update( | |
| return; | ||
| } | ||
| } | ||
| // Inform sush gossip of all known peer addresses. | ||
| let sush_peers: BTreeSet<_> = addrs | ||
| .iter() | ||
| .map(|ip| SocketAddrV6::new(*ip, SUSH_GOSSIP_PORT, 0, 0)) | ||
| .collect(); | ||
| sush_gossip_tx.send_if_modified(|current| { | ||
| let changed = *current != sush_peers; | ||
| if changed { | ||
| *current = sush_peers; | ||
| } | ||
| changed | ||
| }); | ||
|
hawkw marked this conversation as resolved.
Resolved
|
||
| } | ||
| Err(err) => { | ||
| warn!( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,6 +120,70 @@ pub struct Config { | |
| /// root certificates and whether to use local certificate chain or | ||
| /// one over IPCC | ||
| pub sprockets: SprocketsConfig, | ||
|
|
||
| /// Settings for the Support Shell server (RFD 620). If this is absent, no | ||
| /// Support Shell server runs on this sled. | ||
| #[serde(default)] | ||
| pub sush: Option<SushConfig>, | ||
| } | ||
|
|
||
| /// Configuration for the Support Shell (`sush`) server that runs in the global | ||
| /// zone. See RFD 620. | ||
| #[derive(Clone, Debug, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct SushConfig { | ||
| /// PEM files holding the trusted root certificates for job requests, | ||
| /// one certificate per file. A job whose signature does not chain to | ||
| /// one of these is refused, so an empty list means no job could ever | ||
| /// run, and config parsing rejects it. To run a sled with no Support | ||
| /// Shell server, omit the `[sush]` section instead. | ||
|
plotnick marked this conversation as resolved.
Resolved
|
||
| #[serde(deserialize_with = "SushConfig::nonempty_roots")] | ||
| pub roots: Vec<Utf8PathBuf>, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. take it or leave it suggestion: alternatively, this could be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is a nice way to make the bad case unrepresentable, but I think failing at parse time is good enough for now. |
||
|
|
||
| /// Where to record job output before an encrypted dataset is available. | ||
| /// Must be on a ramdisk so that it is never persisted unencrypted. | ||
| #[serde(default = "SushConfig::default_ramdisk_dir")] | ||
| pub ramdisk_dir: Utf8PathBuf, | ||
|
|
||
| /// Maximum output a single job may record before an encrypted dataset is | ||
| /// available. The default is small because the ramdisk is global zone | ||
| /// memory. | ||
| #[serde(default = "SushConfig::default_ramdisk_max_output_mb")] | ||
| pub ramdisk_max_output_mb: u32, | ||
|
|
||
| /// Maximum output a single job may record once an encrypted dataset is | ||
| /// available. | ||
| #[serde(default = "SushConfig::default_max_output_mb")] | ||
| pub max_output_mb: u32, | ||
|
hawkw marked this conversation as resolved.
Resolved
|
||
| } | ||
|
|
||
| impl SushConfig { | ||
| // TODO: check that this is the right default. `/var/run/oxide` is tmpfs | ||
| // and is created during bootstrap, but it exists to hold ZFS key files | ||
| // (see `illumos_utils::zfs::KEYPATH_ROOT`), not bulk job output. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this TODO deserve to become TO-done before this merges?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, I am hoping this very review will settle the question!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What properties are you looking for? That the path will exist and be mounted and be a tmpfs? I think that's done by the which our images are built with.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you very much, that is exactly what I was looking for! Noted in 7481505. |
||
| fn default_ramdisk_dir() -> Utf8PathBuf { | ||
| "/var/run/oxide/sush".into() | ||
| } | ||
|
|
||
| fn default_ramdisk_max_output_mb() -> u32 { | ||
| 64 | ||
| } | ||
|
|
||
| fn default_max_output_mb() -> u32 { | ||
| 10 * 1024 | ||
| } | ||
|
|
||
| fn nonempty_roots<'de, D: serde::Deserializer<'de>>( | ||
| deserializer: D, | ||
| ) -> Result<Vec<Utf8PathBuf>, D::Error> { | ||
| let roots = Vec::deserialize(deserializer)?; | ||
| if roots.is_empty() { | ||
| return Err(serde::de::Error::custom( | ||
| "sush needs at least one root certificate to run", | ||
| )); | ||
| } | ||
| Ok(roots) | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
|
|
@@ -210,4 +274,37 @@ mod test { | |
| } | ||
| assert!(configs_seen > 0, "No sled-agent configs found"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_sush_config() { | ||
| // Only `roots` is required. The rest have defaults. | ||
| let sush: SushConfig = | ||
| toml::from_str(r#"roots = ["/pkg/sush-root.pem"]"#).unwrap(); | ||
| assert_eq!(sush.roots, vec![Utf8PathBuf::from("/pkg/sush-root.pem")]); | ||
| assert_eq!(sush.ramdisk_dir, SushConfig::default_ramdisk_dir()); | ||
| assert_eq!( | ||
| sush.ramdisk_max_output_mb, | ||
| SushConfig::default_ramdisk_max_output_mb() | ||
| ); | ||
| assert_eq!(sush.max_output_mb, SushConfig::default_max_output_mb()); | ||
|
|
||
| let sush: SushConfig = toml::from_str( | ||
| r#" | ||
| roots = ["/pkg/sush-root.pem"] | ||
| ramdisk_dir = "/var/run/sush" | ||
| ramdisk_max_output_mb = 8 | ||
| max_output_mb = 128 | ||
| "#, | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!(sush.ramdisk_dir, "/var/run/sush"); | ||
| assert_eq!(sush.ramdisk_max_output_mb, 8); | ||
| assert_eq!(sush.max_output_mb, 128); | ||
|
|
||
| // No roots would mean no jobs can ever run, so reject at parse time. | ||
| let err = toml::from_str::<SushConfig>("roots = []").unwrap_err(); | ||
| assert!( | ||
| err.to_string().contains("needs at least one root certificate") | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
instead? The caller can then decide what to do with the RecvError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent suggestion, thank you; applied in 898c661.