[4/n] [ls-apis] component graph should also filter out IDU edges

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.

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<String> {
-        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)>),
 }
 

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)