diff --git a/dev-tools/ls-apis/api-manifest.toml b/dev-tools/ls-apis/api-manifest.toml index 5a9c64560d5..46adc8f0a70 100644 --- a/dev-tools/ls-apis/api-manifest.toml +++ b/dev-tools/ls-apis/api-manifest.toml @@ -593,3 +593,97 @@ note = """ Sled Agent uses the Crucible Agent client types only, and only in the simulated sled agent. """ + +################################################################################ +# Localhost-only edges +# +# These edges are excluded from the deployment unit dependency graph because +# they represent communication that only happens locally within a deployment +# unit, not across deployment unit boundaries. A single deployment unit is +# updated atomically, so there's no risk of version skew with localhost-only +# edges. +################################################################################ + +[[localhost_only_edges]] +server = "ddmd" +client = "dpd-client" +note = "verified: sled-agent configures ddmd with dpd_host=[::1] (services.rs:3127)" + +[[localhost_only_edges]] +server = "dpd" +client = "gateway-client" +note = "verified: dpd defaults to [::1]:MGS_PORT (dendrite switch_identifiers.rs:60)" + +[[localhost_only_edges]] +server = "lldpd" +client = "dpd-client" +note = "verified: lldpd defaults to localhost for dpd (lldp dendrite.rs:194)" + +[[localhost_only_edges]] +server = "mgd" +client = "ddm-admin-client" +note = "verified: mg-lower hardcodes localhost:8000 (mg-lower ddm.rs:110)" + +[[localhost_only_edges]] +server = "mgd" +client = "dpd-client" +note = "verified: mg-lower hardcodes localhost (mg-lower dendrite.rs:491)" + +[[localhost_only_edges]] +server = "mgd" +client = "gateway-client" +note = "verified: mgd defaults to [::1]:12225 (mgd main.rs:93)" + +# NOTE: The following sled-agent edges are BUGS - they use underlay IPs, not +# localhost. They are kept here to avoid breaking the build, but need to be +# fixed, either through client-side versioning or by some other means. + +[[localhost_only_edges]] +server = "omicron-sled-agent" +client = "gateway-client" +note = "BUG: uses underlay IP, not localhost (early_networking.rs:376)" + +[[localhost_only_edges]] +server = "omicron-sled-agent" +client = "ddm-admin-client" +note = "verified: uses Client::localhost() (ddm_reconciler.rs:56)" + +[[localhost_only_edges]] +server = "omicron-sled-agent" +client = "dpd-client" +note = "BUG: uses underlay IP, not localhost (services.rs:1090)" + +[[localhost_only_edges]] +server = "omicron-sled-agent" +client = "mg-admin-client" +note = "BUG: uses underlay IP, not localhost (early_networking.rs:483)" + +[[localhost_only_edges]] +server = "omicron-sled-agent" +client = "propolis-client" +note = "BUG: uses zone IP, not localhost (instance.rs:2283)" + +[[localhost_only_edges]] +server = "tfportd" +client = "dpd-client" +note = "verified: configured with dpd_host=[::1] (services.rs:2852)" + +[[localhost_only_edges]] +server = "tfportd" +client = "lldpd-client" +note = "verified: hardcodes localhost (tfportd tfport.rs:88)" + +[[localhost_only_edges]] +server = "wicketd" +client = "ddm-admin-client" +note = "verified: uses DdmAdminClient::localhost() (bootstrap_addrs.rs:162)" + +[[localhost_only_edges]] +server = "wicketd" +client = "dpd-client" +note = "verified: hardcodes [::1]:DENDRITE_PORT (preflight_check/uplink.rs:83)" + +[[localhost_only_edges]] +server = "wicketd" +client = "gateway-client" +note = "verified: --mgs-address CLI expects localhost (bin/wicketd.rs:35)" diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index b0e3dfa4247..3d0ff0052e2 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -30,6 +30,7 @@ pub struct AllApiMetadata { deployment_units: BTreeMap, dependency_rules: BTreeMap>, ignored_non_clients: BTreeSet, + localhost_only_edges: Vec, } impl AllApiMetadata { @@ -73,6 +74,11 @@ impl AllApiMetadata { &self.ignored_non_clients } + /// Returns the list of localhost-only edges + pub fn localhost_only_edges(&self) -> &[LocalhostOnlyEdge] { + &self.localhost_only_edges + } + /// Returns how we should filter the given dependency pub(crate) fn evaluate_dependency( &self, @@ -138,6 +144,7 @@ struct RawApiMetadata { deployment_units: Vec, dependency_filter_rules: Vec, ignored_non_clients: Vec, + localhost_only_edges: Vec, } impl TryFrom for AllApiMetadata { @@ -188,17 +195,35 @@ impl TryFrom for AllApiMetadata { for client_pkg in raw.ignored_non_clients { if !ignored_non_clients.insert(client_pkg.clone()) { bail!( - "entry in ignored_non_clients appearead twice: {:?}", + "entry in ignored_non_clients appeared twice: {:?}", &client_pkg ); } } + // Validate localhost_only_edges reference known server components and + // APIs. + let known_components: BTreeSet<_> = + deployment_units.values().flat_map(|u| u.packages.iter()).collect(); + for edge in &raw.localhost_only_edges { + if !known_components.contains(&edge.server) { + bail!( + "localhost_only_edges: unknown server component {:?}", + edge.server + ); + } + let client_name = edge.client.as_specific(); + if !apis.contains_key(client_name) { + bail!("localhost_only_edges: unknown client {:?}", client_name); + } + } + Ok(AllApiMetadata { apis, deployment_units, dependency_rules, ignored_non_clients, + localhost_only_edges: raw.localhost_only_edges, }) } } @@ -416,3 +441,61 @@ pub enum Evaluation { /// This dependency should be part of the update DAG Dag, } + +/// Specifies which client to match in a localhost-only edge rule. +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum ClientMatcher { + /// Match a specific client package. + Specific(ClientPackageName), +} + +impl<'de> Deserialize<'de> for ClientMatcher { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + Ok(ClientMatcher::Specific(ClientPackageName::from(s))) + } +} + +impl ClientMatcher { + /// Returns true if this matcher matches the given client package. + pub fn matches(&self, client: &ClientPackageName) -> bool { + match self { + ClientMatcher::Specific(name) => name == client, + } + } + + /// Returns the specific client name. + pub fn as_specific(&self) -> &ClientPackageName { + match self { + ClientMatcher::Specific(name) => name, + } + } +} + +/// An edge that should be excluded from the deployment unit dependency graph +/// because it represents communication that only happens locally within a +/// deployment unit. +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +pub struct LocalhostOnlyEdge { + /// The server component that consumes the API. + pub server: ServerComponentName, + /// The client package consumed. + pub client: ClientMatcher, + /// Explanation of why this edge is localhost-only. + pub note: String, +} + +impl LocalhostOnlyEdge { + /// Returns true if this rule matches the given (server, client) pair. + pub fn matches( + &self, + server: &ServerComponentName, + client: &ClientPackageName, + ) -> bool { + self.server == *server && self.client.matches(client) + } +} diff --git a/dev-tools/ls-apis/src/bin/ls-apis.rs b/dev-tools/ls-apis/src/bin/ls-apis.rs index df7fd9617a6..54169e076c0 100644 --- a/dev-tools/ls-apis/src/bin/ls-apis.rs +++ b/dev-tools/ls-apis/src/bin/ls-apis.rs @@ -252,7 +252,14 @@ fn print_server_components<'a>( ); } for (c, path) in apis.component_apis_consumed(s, filter)? { - println!("{} consumes: {}", prefix, c); + if let Some(note) = apis.localhost_only_edge_note(s, c) { + println!( + "{} consumes: {} (localhost-only: {})", + prefix, c, note + ); + } else { + println!("{} consumes: {}", prefix, c); + } if show_deps { for p in path.nodes() { println!("{} via: {}", prefix, p); diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index e61af0b7c50..56f0e179483 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -15,6 +15,7 @@ use crate::api_metadata::ApiConsumerStatus; use crate::api_metadata::ApiExpectedConsumer; use crate::api_metadata::ApiExpectedConsumers; use crate::api_metadata::ApiMetadata; +use crate::api_metadata::ClientMatcher; use crate::api_metadata::Evaluation; use crate::api_metadata::VersionedHow; use crate::cargo::DepPath; @@ -330,6 +331,19 @@ impl SystemApis { &self.api_metadata } + /// Returns the note for a localhost-only edge, if one matches. + pub fn localhost_only_edge_note( + &self, + server: &ServerComponentName, + client: &ClientPackageName, + ) -> Option<&str> { + self.api_metadata + .localhost_only_edges() + .iter() + .find(|edge| edge.matches(server, client)) + .map(|edge| edge.note.as_str()) + } + /// Given a server component, return the APIs consumed by this component pub fn component_apis_consumed( &self, @@ -474,6 +488,23 @@ impl SystemApis { /// Returns a string that can be passed to `dot(1)` to render a graph of /// API dependencies among deployment units pub fn dot_by_unit(&self, filter: ApiDependencyFilter) -> Result { + let (graph, _) = self.make_deployment_unit_graph(filter, None)?; + Ok(Dot::new(&graph).to_string()) + } + + // The complex type below is only used in this one place: the return value + // of this internal helper function. A type alias doesn't seem better. + #[allow(clippy::type_complexity)] + fn make_deployment_unit_graph( + &self, + dependency_filter: ApiDependencyFilter, + localhost_only_edges: Option< + &BTreeSet<(ServerComponentName, ClientPackageName)>, + >, + ) -> Result<( + petgraph::graph::Graph<&DeploymentUnitName, &ClientPackageName>, + BTreeMap<&DeploymentUnitName, NodeIndex>, + )> { let mut graph = petgraph::graph::Graph::new(); let nodes: BTreeMap<_, _> = self .deployment_units() @@ -489,8 +520,29 @@ impl SystemApis { let my_node = nodes.get(deployment_unit).unwrap(); for server_pkg in server_components { for (client_pkg, _) in - self.component_apis_consumed(server_pkg, filter)? + self.component_apis_consumed(server_pkg, dependency_filter)? { + // When localhost_only_edges is provided, we're building a + // graph of server-side-versioned API dependencies only. + if let Some(localhost_edges) = localhost_only_edges { + let api = self + .api_metadata + .client_pkgname_lookup(client_pkg) + .unwrap(); + if api.versioned_how != VersionedHow::Server { + continue; + } + + // Skip edges that represent localhost-only communication + // (communication within a deployment unit that doesn't + // cross deployment unit boundaries). + if localhost_edges + .contains(&(server_pkg.clone(), client_pkg.clone())) + { + continue; + } + } + // Multiple server components may produce an API. However, // if an API is produced by multiple server components // within the same deployment unit, we would like to only @@ -506,17 +558,13 @@ impl SystemApis { .collect(); for other_unit in other_units { let other_node = nodes.get(other_unit).unwrap(); - graph.update_edge( - *my_node, - *other_node, - client_pkg.clone(), - ); + graph.update_edge(*my_node, *other_node, client_pkg); } } } } - Ok(Dot::new(&graph).to_string()) + Ok((graph, nodes)) } /// Returns a string that can be passed to `dot(1)` to render a graph of @@ -530,7 +578,7 @@ impl SystemApis { } // The complex type below is only used in this one place: the return value - // of an internal helper function. A type alias doesn't seem better. + // of this internal helper function. A type alias doesn't seem better. #[allow(clippy::type_complexity)] fn make_component_graph( &self, @@ -577,6 +625,112 @@ impl SystemApis { Ok((graph, nodes)) } + /// Computes the set of (server, client) edges that must be excluded from + /// the deployment unit dependency graph because they represent intra-unit + /// communication for server-side-versioned APIs. + /// + /// These are edges where all of the below are true: + /// + /// 1. The server consumes the client. + /// 2. The API is server-side-versioned. + /// 3. The server and API producer are in the same deployment unit. + /// + /// Returns a set of (server, client) pairs. + fn compute_required_localhost_edges( + &self, + ) -> Result> { + let filter = ApiDependencyFilter::Default; + let mut required = BTreeSet::new(); + + for server in self.server_component_units.keys() { + let Some(server_unit) = self.server_component_units.get(server) + else { + continue; + }; + + for (client, _) in self.component_apis_consumed(server, filter)? { + // Only consider server-side-versioned APIs. + let Some(api) = self.api_metadata.client_pkgname_lookup(client) + else { + continue; + }; + if api.versioned_how != VersionedHow::Server { + continue; + } + + // Check if any producer is in the same deployment unit. + for producer in self.api_producers(client) { + let Some(producer_unit) = + self.server_component_units.get(producer) + else { + continue; + }; + + if server_unit == producer_unit { + // This edge would create an intra-unit dependency for + // a server-versioned API, so it must be in the + // localhost-only list. + required.insert((server.clone(), client.clone())); + break; + } + } + } + } + + Ok(required) + } + + /// Validates that the configured localhost_only_edges exactly match the + /// required set of edges that must be excluded. + /// + /// Returns the validated set of edges for use by make_deployment_unit_graph. + fn validate_localhost_only_edges( + &self, + ) -> Result> { + let required = self.compute_required_localhost_edges()?; + + // Build the configured set from the manifest. + let mut configured = BTreeSet::new(); + for edge in self.api_metadata.localhost_only_edges() { + let ClientMatcher::Specific(client) = &edge.client; + configured.insert((edge.server.clone(), client.clone())); + } + + // Compare the two sets. + let missing: BTreeSet<_> = required.difference(&configured).collect(); + let extra: BTreeSet<_> = configured.difference(&required).collect(); + + if !missing.is_empty() || !extra.is_empty() { + let mut msg = String::from( + "localhost_only_edges configuration does not match required edges:\n", + ); + + if !missing.is_empty() { + msg.push_str("\nMissing entries (these edges exist and need localhost-only exclusion):\n"); + for (server, client) in &missing { + msg.push_str(&format!( + " - server = {:?}, client = {:?}\n", + server, client + )); + } + } + + if !extra.is_empty() { + msg.push_str("\nExtra entries (these edges don't exist or don't need exclusion):\n"); + for (server, client) in &extra { + msg.push_str(&format!( + " - server = {:?}, client = {:?}\n", + server, client + )); + } + } + + bail!("{}", msg); + } + + Ok(required) + } + /// Verifies various important properties about the assignment of which APIs /// are server-managed vs. client-managed. /// @@ -631,6 +785,10 @@ impl SystemApis { // can't be part of a cycle. let filter = ApiDependencyFilter::Default; + // Validate that all configured localhost_only_edges are correct and + // match the required set exactly. + let localhost_only_edges = self.validate_localhost_only_edges()?; + // Construct a graph where: // // - nodes are all the API producer and consumer components @@ -643,8 +801,21 @@ impl SystemApis { nodes.iter().map(|(s_c, node)| (node, s_c)).collect(); if let Err(error) = petgraph::algo::toposort(&graph, None) { bail!( - "graph of server-managed components has a cycle (includes \ - node: {:?})", + "graph of server-managed API dependencies between components \ + has a cycle (includes node: {:?})", + reverse_nodes.get(&error.node_id()).unwrap() + ); + } + + // Do the same with a graph of deployment units. + let (graph, nodes) = self + .make_deployment_unit_graph(filter, Some(&localhost_only_edges))?; + let reverse_nodes: BTreeMap<_, _> = + nodes.iter().map(|(d_u, node)| (node, d_u)).collect(); + if let Err(error) = petgraph::algo::toposort(&graph, None) { + bail!( + "graph of server-managed API dependencies between deployment \ + units has a cycle (includes node: {:?})", reverse_nodes.get(&error.node_id()).unwrap() ); }