[ls-apis] check that only omicron-sled-agent depends on nexus-lockstep-client (#9413)

Sled Agent depends on nexus-lockstep-client via RSS, and we now ensure
that nothing else that's part of one of the deployment units depends on
it. (We don't treat omdb as part of a deployment unit because it doesn't
participate in upgrade in the typical sense.)

Also add an expectorate test for the output of `ls-apis check`, similar
to the existing one for `ls-apis apis`.

Closes #9057.

Author: sunshowers

Cargo.lock

@@ -12,6 +12,8 @@ anyhow.workspace = true
 camino.workspace = true
 cargo_metadata.workspace = true
 clap.workspace = true
+iddqd.workspace = true
+indent_write.workspace = true
 newtype_derive.workspace = true
 parse-display.workspace = true
 petgraph.workspace = true

dev-tools/ls-apis/Cargo.toml

@@ -347,6 +347,12 @@ server_package_name = "nexus-lockstep-api"
 # dependency with sled-agent-client, which is server-versioned.
 versioned_how = "client"
 versioned_how_reason = "Circular dependencies between Nexus and other services"
+# Exactly these consumers (Rust packages specified in one or more of the
+# `packages` arrays in the deployment_units table above) are allowed to use this
+# API. ls-apis check will error out if the list of consumers is incorrect.
+restricted_to_consumers = [
+     { name = "omicron-sled-agent", reason = "Only RSS uses this API and it is not part of upgrade" },
+]
 
 [[apis]]
 client_package_name = "oxide-client"

dev-tools/ls-apis/api-manifest.toml

@@ -11,6 +11,9 @@ use crate::ServerPackageName;
 use crate::cargo::DepPath;
 use crate::workspaces::Workspaces;
 use anyhow::{Result, bail};
+use iddqd::IdOrdItem;
+use iddqd::IdOrdMap;
+use iddqd::id_upcast;
 use serde::Deserialize;
 use std::borrow::Borrow;
 use std::collections::BTreeMap;
@@ -228,6 +231,13 @@ pub struct ApiMetadata {
     pub label: String,
     /// package name of the server that provides the corresponding API
     pub server_package_name: ServerPackageName,
+    /// expected consumers (Rust packages) that use this API
+    ///
+    /// By default, we don't make any assertions about expected consumers. But
+    /// some APIs must have a fixed list of consumers, and we assert on that
+    /// via this array.
+    #[serde(default)]
+    pub restricted_to_consumers: ApiExpectedConsumers,
     /// human-readable notes about this API
     pub notes: Option<String>,
     /// describes how we've decided this API will be versioned
@@ -248,6 +258,121 @@ impl ApiMetadata {
     }
 }
 
+/// Expected consumers (Rust packages) for an API.
+#[derive(Debug, Default)]
+pub enum ApiExpectedConsumers {
+    /// This API has no configured restrictions on which consumers can use it.
+    #[default]
+    Unrestricted,
+    /// This API is restricted to exactly these consumers.
+    Restricted(IdOrdMap<ApiExpectedConsumer>),
+}
+
+impl ApiExpectedConsumers {
+    pub fn status(
+        &self,
+        server_pkgname: &ServerComponentName,
+    ) -> ApiConsumerStatus {
+        match self {
+            ApiExpectedConsumers::Unrestricted => {
+                ApiConsumerStatus::NoAssertion
+            }
+            ApiExpectedConsumers::Restricted(consumers) => {
+                if let Some(consumer) =
+                    consumers.iter().find(|c| c.name == *server_pkgname)
+                {
+                    ApiConsumerStatus::Expected {
+                        reason: consumer.reason.clone(),
+                    }
+                } else {
+                    ApiConsumerStatus::Unexpected
+                }
+            }
+        }
+    }
+}
+
+impl<'de> Deserialize<'de> for ApiExpectedConsumers {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        use serde::de::Error;
+
+        struct ApiExpectedConsumersVisitor;
+
+        impl<'de> serde::de::Visitor<'de> for ApiExpectedConsumersVisitor {
+            type Value = ApiExpectedConsumers;
+
+            fn expecting(
+                &self,
+                formatter: &mut std::fmt::Formatter,
+            ) -> std::fmt::Result {
+                formatter.write_str(
+                    "null (for no assertions) or a list of Rust package names",
+                )
+            }
+
+            fn visit_unit<E>(self) -> Result<Self::Value, E>
+            where
+                E: Error,
+            {
+                Ok(ApiExpectedConsumers::Unrestricted)
+            }
+
+            fn visit_none<E>(self) -> Result<Self::Value, E>
+            where
+                E: Error,
+            {
+                Ok(ApiExpectedConsumers::Unrestricted)
+            }
+
+            fn visit_seq<A>(self, seq: A) -> Result<Self::Value, A::Error>
+            where
+                A: serde::de::SeqAccess<'de>,
+            {
+                // Note IdOrdMap deserializes as a sequence by default.
+                let consumers = IdOrdMap::<ApiExpectedConsumer>::deserialize(
+                    serde::de::value::SeqAccessDeserializer::new(seq),
+                )?;
+                Ok(ApiExpectedConsumers::Restricted(consumers))
+            }
+        }
+
+        deserializer.deserialize_any(ApiExpectedConsumersVisitor)
+    }
+}
+
+/// Describes a single allowed consumer for an API.
+#[derive(Clone, Debug, Deserialize)]
+#[serde(deny_unknown_fields)]
+pub struct ApiExpectedConsumer {
+    /// The name of the Rust package.
+    pub name: ServerComponentName,
+    /// The reason this consumer is allowed.
+    pub reason: String,
+}
+
+impl IdOrdItem for ApiExpectedConsumer {
+    type Key<'a> = &'a ServerComponentName;
+    fn key(&self) -> Self::Key<'_> {
+        &self.name
+    }
+    id_upcast!();
+}
+
+/// The status of an API consumer that was discovered by walking the Cargo
+/// metadata graph.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum ApiConsumerStatus {
+    /// No assertions were made about this API consumer.
+    NoAssertion,
+    /// The API consumer is expected to be used.
+    Expected { reason: String },
+    /// The API consumer was not expected. This is an error case.
+    Unexpected,
+}
+
 /// Describes a unit that combines one or more servers that get deployed
 /// together
 #[derive(Deserialize)]

dev-tools/ls-apis/src/api_metadata.rs

@@ -7,9 +7,11 @@
 use anyhow::{Context, Result, bail};
 use camino::Utf8PathBuf;
 use clap::{Args, Parser, Subcommand};
+use indent_write::indentable::Indentable;
 use omicron_ls_apis::{
-    AllApiMetadata, ApiDependencyFilter, ApiMetadata, LoadArgs,
-    ServerComponentName, SystemApis, VersionedHow,
+    AllApiMetadata, ApiConsumerStatus, ApiDependencyFilter, ApiMetadata,
+    FailedConsumerCheck, LoadArgs, ServerComponentName, SystemApis,
+    VersionedHow, plural,
 };
 use parse_display::{Display, FromStr};
 
@@ -119,11 +121,11 @@ fn run_adoc(apis: &SystemApis) -> Result<()> {
         println!("|{}", apis.adoc_label(&api.client_package_name)?);
 
         println!("|");
-        for (c, _) in apis.api_consumers(
+        for consumer in apis.api_consumers(
             &api.client_package_name,
             ApiDependencyFilter::default(),
         )? {
-            println!("* {}", apis.adoc_label(c)?);
+            println!("* {}", apis.adoc_label(&consumer.server_pkgname)?);
         }
 
         match &api.versioned_how {
@@ -145,24 +147,25 @@ fn run_adoc(apis: &SystemApis) -> Result<()> {
 }
 
 fn run_apis(apis: &SystemApis, args: ShowDepsArgs) -> Result<()> {
+    let mut error_count = 0;
+
     let metadata = apis.api_metadata();
     for api in metadata.apis() {
         println!("{} (client: {})", api.label, api.client_package_name);
-        for (s, dep_paths) in
-            apis.api_consumers(&api.client_package_name, args.filter)?
-        {
-            let (repo_name, package_path) = apis.package_label(s)?;
+        for c in apis.api_consumers(&api.client_package_name, args.filter)? {
+            let (repo_name, package_path) =
+                apis.package_label(c.server_pkgname)?;
             println!(
                 "    consumed by: {} ({}/{}) via {} path{}",
-                s,
+                c.server_pkgname,
                 repo_name,
                 package_path,
-                dep_paths.len(),
-                if dep_paths.len() == 1 { "" } else { "s" },
+                c.dep_paths.len(),
+                if c.dep_paths.len() == 1 { "" } else { "s" },
             );
             if args.show_deps {
-                for (i, dep_path) in dep_paths.iter().enumerate() {
-                    let label = if dep_paths.len() > 1 {
+                for (i, dep_path) in c.dep_paths.iter().enumerate() {
+                    let label = if c.dep_paths.len() > 1 {
                         format!(" path {}", i + 1)
                     } else {
                         String::new()
@@ -173,8 +176,33 @@ fn run_apis(apis: &SystemApis, args: ShowDepsArgs) -> Result<()> {
                     }
                 }
             }
+            match c.status {
+                ApiConsumerStatus::NoAssertion => {
+                    // We don't know whether it's okay for this API to be
+                    // present.
+                }
+                ApiConsumerStatus::Expected { reason } => {
+                    println!("        status: expected, reason: {reason}");
+                }
+                ApiConsumerStatus::Unexpected => {
+                    println!("        error: unexpected dependency");
+                    error_count += 1;
+                }
+            }
         }
-        println!("");
+        if let Some(missing) =
+            apis.missing_expected_consumers(&api.client_package_name)
+        {
+            println!("{}", missing.display(apis).indented("    "));
+            error_count += missing.error_count();
+        }
+        println!();
+    }
+    if error_count > 0 {
+        bail!(
+            "{error_count} {} reported (see above)",
+            plural::errors_str(error_count)
+        );
     }
     Ok(())
 }
@@ -311,6 +339,32 @@ fn run_check(apis: &SystemApis) -> Result<()> {
         println!(")");
     }
 
+    fn print_failed_consumer_check(
+        api: &ApiMetadata,
+        check: &FailedConsumerCheck,
+        apis: &SystemApis,
+    ) {
+        println!("    {} ({}):", api.label, api.client_package_name);
+        for consumer in &check.unexpected {
+            let deployment_unit =
+                apis.server_component_unit(consumer).unwrap_or_else(|| {
+                    panic!(
+                        "consumer {consumer} doesn't have an associated \
+                         deployment unit (this is checked at load time, so \
+                         if you're seeing this message, there's a bug in that \
+                         check)"
+                    );
+                });
+            println!(
+                "        error: unexpected dependency on {consumer} (part of {deployment_unit})"
+            );
+        }
+
+        if let Some(missing) = check.missing {
+            println!("{}", missing.display(apis).indented("        "));
+        }
+    }
+
     println!("\n");
     println!("Server-managed APIs:\n");
     for api in apis
@@ -330,6 +384,8 @@ fn run_check(apis: &SystemApis) -> Result<()> {
         }
     }
 
+    let mut error_count = 0;
+
     println!("\n");
     print!("APIs with unknown version management:");
     let unknown: Vec<_> = apis
@@ -343,8 +399,37 @@ fn run_check(apis: &SystemApis) -> Result<()> {
         println!("\n");
         for api in unknown {
             print_api_and_producers(api, apis);
+            error_count += 1;
         }
-        bail!("at least one API has unknown version strategy (see above)");
+    }
+
+    println!("\n");
+    print!("APIs that failed consumer checks:");
+    if dag_check.failed_consumers().is_empty() {
+        println!(" none");
+    } else {
+        println!("\n");
+        for c in dag_check.failed_consumers() {
+            let api = apis
+                .api_metadata()
+                .client_pkgname_lookup(c.client_pkgname)
+                .unwrap_or_else(|| {
+                    panic!(
+                        "client package name {} not found in API metadata",
+                        c.client_pkgname
+                    )
+                });
+            print_failed_consumer_check(api, c, apis);
+            error_count += c.error_count();
+        }
+    }
+
+    if error_count > 0 {
+        println!("\n");
+        bail!(
+            "{error_count} {} reported (see above)",
+            plural::errors_str(error_count)
+        );
     }
 
     Ok(())

dev-tools/ls-apis/src/bin/ls-apis.rs

@@ -6,13 +6,17 @@
 
 mod api_metadata;
 mod cargo;
+pub mod plural;
 mod system_apis;
 mod workspaces;
 
 pub use api_metadata::AllApiMetadata;
+pub use api_metadata::ApiConsumerStatus;
+pub use api_metadata::ApiExpectedConsumer;
 pub use api_metadata::ApiMetadata;
 pub use api_metadata::VersionedHow;
 pub use system_apis::ApiDependencyFilter;
+pub use system_apis::FailedConsumerCheck;
 pub use system_apis::SystemApis;
 
 use anyhow::{Context, Result};
@@ -25,7 +29,7 @@ use std::borrow::Borrow;
 #[macro_use]
 extern crate newtype_derive;
 
-#[derive(Clone, Deserialize, Ord, PartialOrd, Eq, PartialEq)]
+#[derive(Clone, Deserialize, Hash, Ord, PartialOrd, Eq, PartialEq)]
 #[serde(transparent)]
 pub struct ClientPackageName(String);
 NewtypeDebug! { () pub struct ClientPackageName(String); }
@@ -62,7 +66,7 @@ impl Borrow<str> for ServerPackageName {
     }
 }
 
-#[derive(Clone, Deserialize, Ord, PartialOrd, Eq, PartialEq)]
+#[derive(Clone, Deserialize, Hash, Ord, PartialOrd, Eq, PartialEq)]
 #[serde(transparent)]
 pub struct ServerComponentName(String);
 NewtypeDebug! { () pub struct ServerComponentName(String); }