diff --git a/dev-tools/ls-apis/api-manifest.toml b/dev-tools/ls-apis/api-manifest.toml index ad60c615397..041750593fb 100644 --- a/dev-tools/ls-apis/api-manifest.toml +++ b/dev-tools/ls-apis/api-manifest.toml @@ -586,37 +586,15 @@ of which are deployed. # future false negatives. # -[[dependency_filter_rules]] -ancestor = "dice-verifier" -client = "sled-agent-client" -evaluation = "bogus" -note = """ -propolis-server does actually talk to sled-agent-client, but by virtue of both -APIs being server-side versioned, trips a check in the component graph that -requires server-side versioned APIs to not form a cycle. This is a deficiency -in the server-side API checking, since this cycle is acceptable on the basis of -propolis-server and sled-agent being in the same deployment unit. - -This filter should be removed when the intra-deployment relationship between -propolis-server and sled-agent can be tolerated in the component graph check. -""" - [[dependency_filter_rules]] ancestor = "sled-agent-client" client = "propolis-client" evaluation = "bogus" note = """ -Same as the bogus rule between sled-agent-types and propolis-client. sled-agent-client re-exports some (Crucible!) types from propolis-client for -sled-agent-client's consumers. - -This bogus rule would be fine enough on its own, though ideally we should not -need sled-agent-client to depend on propolis-client even just for types. But, -perhaps surprisingly, this was necessary to add even though the -dice-verifier->sled-agent-client rule above "broke" the dependency path from -propolis-server through to propolis-client. Once we've fixed the issue requiring -the dice-verifier rule above, we should probably keep this dependency filter -rule. +sled-agent-client's consumers. Ideally sled-agent-client would not need to +depend on propolis-client even just for types, but until that's untangled, this +Cargo dependency does not represent a real API relationship. """ [[dependency_filter_rules]] @@ -827,6 +805,22 @@ permalinks = [ "https://github.com/oxidecomputer/omicron/blob/6cef874/sled-agent/src/instance.rs#L2283", ] +[[intra_deployment_unit_only_edges]] +server = "propolis-server" +client = "sled-agent-client" +note = """ +The Propolis attestation server uses dice-verifier's AttestSledAgent to call +RoT attestation endpoints on Sled Agent. The Sled Agent address is +derived from propolis-server's own IPv6 listen address by computing the sled +subnet and calling get_sled_address (propolis-server/src/main.rs:326), so it +can only ever resolve to the same sled (and therefore, the same Host OS +deployment unit). +""" +permalinks = [ + "https://github.com/oxidecomputer/propolis/blob/58ab73bde89ade637b0ca8118682ee9575da6c2a/bin/propolis-server/src/main.rs#L326-L337", + "https://github.com/oxidecomputer/dice-util/blob/d7472bfa91aee859c3fe0bdc1dbb1e320285228e/verifier/src/sled_agent.rs#L19-L25", +] + # NOTE: The following sled-agent edges are bugs: they cross deployment units. # They are kept here to avoid breaking the build, but need to be fixed, either # through client-side versioning or by some other means. diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index b52cc5271e9..1aef84952a3 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -713,6 +713,38 @@ impl SystemApis { .in_upgrade_dag() } + /// Returns whether an edge from `server` to `client` should be excluded + /// under `edge_filter`. + fn edge_filtered_out( + &self, + edge_filter: EdgeFilter<'_>, + server: &ServerComponentName, + client: &ClientPackageName, + ) -> bool { + match edge_filter { + EdgeFilter::All => false, + EdgeFilter::DagOnly(idu_edges) => { + // unwrap(): every `client` encountered here came from + // `component_apis_consumed`, which only yields known APIs. + let api = + self.api_metadata.client_pkgname_lookup(client).unwrap(); + if api.versioned_how != VersionedHow::Server { + return true; + } + + if !self.component_in_upgrade_dag(server) { + return true; + } + + // Intra-deployment-unit-only edges always connect components in + // the same *instance* of the same deployment unit, so they + // can't induce an update-ordering constraint and shouldn't + // count as cycles. + idu_edges.contains(&(server.clone(), client.clone())) + } + } + } + // 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)] @@ -741,34 +773,12 @@ impl SystemApis { for (client_pkg, _) in self.component_apis_consumed(server_pkg, dependency_filter)? { - if let EdgeFilter::DagOnly(idu_edges) = edge_filter { - let api = self - .api_metadata - .client_pkgname_lookup(client_pkg) - .unwrap(); - // Filtering DAG-only edges means ignoring everything - // that's not server-side-versioned. - if api.versioned_how != VersionedHow::Server { - continue; - } - - if !self.component_in_upgrade_dag(server_pkg) { - continue; - } - - // When filtering DAG-only edges, also skip edges that - // represent intra-deployment-unit-only communication - // (communication within one instance of one deployment - // unit). That's because intra-deployment-unit edges - // would look like a cycle in the dependency graph, but - // aren't one that we care about since they're always - // referring to the same *instance* of the same - // deployment unit. - if idu_edges - .contains(&(server_pkg.clone(), client_pkg.clone())) - { - continue; - } + if self.edge_filtered_out( + edge_filter, + server_pkg, + client_pkg, + ) { + continue; } // Multiple server components may produce an API. However, @@ -801,7 +811,8 @@ impl SystemApis { &self, filter: ApiDependencyFilter, ) -> Result { - let (graph, _nodes) = self.make_component_graph(filter, false)?; + let (graph, _nodes) = + self.make_component_graph(filter, EdgeFilter::All)?; Ok(Dot::new(&graph).to_string()) } @@ -811,7 +822,7 @@ impl SystemApis { fn make_component_graph( &self, dependency_filter: ApiDependencyFilter, - versioned_on_server_only: bool, + edge_filter: EdgeFilter<'_>, ) -> Result<( Graph<&ServerComponentName, &ClientPackageName>, BTreeMap<&ServerComponentName, NodeIndex>, @@ -833,17 +844,12 @@ impl SystemApis { let consumed_apis = self .component_apis_consumed(server_component, dependency_filter)?; for (client_pkg, _) in consumed_apis { - if versioned_on_server_only { - let api = self - .api_metadata - .client_pkgname_lookup(client_pkg) - .unwrap(); - if api.versioned_how != VersionedHow::Server { - continue; - } - if !self.component_in_upgrade_dag(server_component) { - continue; - } + if self.edge_filtered_out( + edge_filter, + server_component, + client_pkg, + ) { + continue; } for other_component in self.api_producers(client_pkg) { @@ -1047,7 +1053,10 @@ impl SystemApis { // APIs // // Check if this DAG is cyclic. This can't be made to work. - let (graph, nodes) = self.make_component_graph(filter, true)?; + let (graph, nodes) = self.make_component_graph( + filter, + EdgeFilter::DagOnly(&idu_only_edges), + )?; let reverse_nodes: BTreeMap<_, _> = nodes.iter().map(|(s_c, node)| (node, s_c)).collect(); if let Err(error) = petgraph::algo::toposort(&graph, None) { @@ -1238,8 +1247,18 @@ impl SystemApis { } } +#[derive(Clone, Copy)] enum EdgeFilter<'a> { + /// Include every edge. All, + /// Include only edges that participate in the upgrade DAG. This excludes: + /// + /// * Client-side versioned APIs + /// * Components for which `Lifecycle::in_upgrade_dag` returns `false` + /// * Intra-deployment-unit-only edges + /// + /// The carried data is the set of intra-deployment-unit-only + /// `(server_component, client_package)` pairs to exclude. DagOnly(&'a BTreeSet<(ServerComponentName, ClientPackageName)>), } diff --git a/dev-tools/ls-apis/tests/api_dependencies.out b/dev-tools/ls-apis/tests/api_dependencies.out index 4f1b3357041..adc3bdd8a96 100644 --- a/dev-tools/ls-apis/tests/api_dependencies.out +++ b/dev-tools/ls-apis/tests/api_dependencies.out @@ -106,6 +106,7 @@ Repo Depot API (client: repo-depot-client) Sled Agent (client: sled-agent-client) consumed by: omicron-nexus (omicron/nexus) via 9 paths + consumed by: propolis-server (propolis/bin/propolis-server) via 1 path consumed by: sled-agent-rack-setup (omicron/sled-agent/rack-setup) via 1 path [subcomponent of omicron-sled-agent; rack-init only] Wicketd (client: wicketd-client)