Support Bundle: Add user comment support

smklein · smklein · commit eda9b97cb5b8 · 2025-07-11T12:46:20.000-07:00
diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs
@@ -4009,6 +4009,7 @@ async fn cmd_nexus_support_bundles_list(
         reason_for_creation: String,
         reason_for_failure: String,
         state: String,
+        user_comment: String,
     }
     let rows = support_bundles.into_iter().map(|sb| SupportBundleInfo {
         id: *sb.id,
@@ -4018,6 +4019,7 @@ async fn cmd_nexus_support_bundles_list(
             .reason_for_failure
             .unwrap_or_else(|| "-".to_string()),
         state: format!("{:?}", sb.state),
+        user_comment: sb.user_comment.unwrap_or_else(|| "-".to_string()),
     });
     let table = tabled::Table::new(rows)
         .with(tabled::settings::Style::empty())
diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs
@@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock};
 ///
 /// This must be updated when you change the database schema.  Refer to
 /// schema/crdb/README.adoc in the root of this repository for details.
-pub const SCHEMA_VERSION: Version = Version::new(161, 0, 0);
+pub const SCHEMA_VERSION: Version = Version::new(162, 0, 0);
 
 /// List of all past database schema versions, in *reverse* order
 ///
@@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock<Vec<KnownVersion>> = LazyLock::new(|| {
         // |  leaving the first copy as an example for the next person.
         // v
         // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
+        KnownVersion::new(162, "bundle-user-comment"),
         KnownVersion::new(161, "inv_cockroachdb_status"),
         KnownVersion::new(160, "tuf-trust-root"),
         KnownVersion::new(159, "sled-config-desired-host-phase-2"),
diff --git a/nexus/db-model/src/support_bundle.rs b/nexus/db-model/src/support_bundle.rs
@@ -95,6 +95,7 @@ pub struct SupportBundle {
     pub zpool_id: DbTypedUuid<ZpoolKind>,
     pub dataset_id: DbTypedUuid<DatasetKind>,
     pub assigned_nexus: Option<DbTypedUuid<OmicronZoneKind>>,
+    pub user_comment: Option<String>,
 }
 
 impl SupportBundle {
@@ -113,6 +114,7 @@ impl SupportBundle {
             zpool_id: zpool_id.into(),
             dataset_id: dataset_id.into(),
             assigned_nexus: Some(nexus_id.into()),
+            user_comment: None,
         }
     }
 
@@ -128,6 +130,7 @@ impl From<SupportBundle> for SupportBundleView {
             time_created: bundle.time_created,
             reason_for_creation: bundle.reason_for_creation,
             reason_for_failure: bundle.reason_for_failure,
+            user_comment: bundle.user_comment,
             state: bundle.state.into(),
         }
     }
diff --git a/nexus/db-queries/src/db/datastore/support_bundle.rs b/nexus/db-queries/src/db/datastore/support_bundle.rs
@@ -433,6 +433,42 @@ impl DataStore {
         }
     }
 
+    /// Updates the user comment of a support bundle.
+    ///
+    /// Returns:
+    /// - "Ok" if the bundle was updated successfully.
+    /// - "Err::InvalidRequest" if the comment exceeds the maximum length.
+    pub async fn support_bundle_update_user_comment(
+        &self,
+        opctx: &OpContext,
+        authz_bundle: &authz::SupportBundle,
+        user_comment: Option<String>,
+    ) -> Result<(), Error> {
+        opctx.authorize(authz::Action::Modify, authz_bundle).await?;
+
+        if let Some(ref comment) = user_comment {
+            if comment.len() > 4096 {
+                return Err(Error::invalid_request(
+                    "User comment cannot exceed 4096 bytes",
+                ));
+            }
+        }
+
+        use nexus_db_schema::schema::support_bundle::dsl;
+
+        let id = authz_bundle.id().into_untyped_uuid();
+        let conn = self.pool_connection_authorized(opctx).await?;
+        diesel::update(dsl::support_bundle)
+            .filter(dsl::id.eq(id))
+            .set(dsl::user_comment.eq(user_comment))
+            .execute_async(&*conn)
+            .await
+            .map(|_rows_modified| ())
+            .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?;
+
+        Ok(())
+    }
+
     /// Deletes a support bundle.
     ///
     /// This should only be invoked after all storage for the support bundle has
diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs
@@ -1470,6 +1470,7 @@ table! {
         dataset_id -> Uuid,
 
         assigned_nexus -> Nullable<Uuid>,
+        user_comment -> Nullable<Text>,
     }
 }
 
diff --git a/nexus/external-api/output/nexus_tags.txt b/nexus/external-api/output/nexus_tags.txt
@@ -62,6 +62,7 @@ support_bundle_head                      HEAD     /experimental/v1/system/suppor
 support_bundle_head_file                 HEAD     /experimental/v1/system/support-bundles/{bundle_id}/download/{file}
 support_bundle_index                     GET      /experimental/v1/system/support-bundles/{bundle_id}/index
 support_bundle_list                      GET      /experimental/v1/system/support-bundles
+support_bundle_update                    PUT      /experimental/v1/system/support-bundles/{bundle_id}
 support_bundle_view                      GET      /experimental/v1/system/support-bundles/{bundle_id}
 system_update_get_repository             GET      /v1/system/update/repository/{system_version}
 system_update_put_repository             PUT      /v1/system/update/repository
diff --git a/nexus/external-api/src/lib.rs b/nexus/external-api/src/lib.rs
@@ -3335,6 +3335,18 @@ pub trait NexusExternalApi {
         path_params: Path<params::SupportBundlePath>,
     ) -> Result<HttpResponseDeleted, HttpError>;
 
+    /// Update a support bundle
+    #[endpoint {
+        method = PUT,
+        path = "/experimental/v1/system/support-bundles/{bundle_id}",
+        tags = ["experimental"], // system/support-bundles: only one tag is allowed
+    }]
+    async fn support_bundle_update(
+        rqctx: RequestContext<Self::Context>,
+        path_params: Path<params::SupportBundlePath>,
+        body: TypedBody<params::SupportBundleUpdate>,
+    ) -> Result<HttpResponseOk<shared::SupportBundleInfo>, HttpError>;
+
     // Probes (experimental)
 
     /// List instrumentation probes
diff --git a/nexus/internal-api/src/lib.rs b/nexus/internal-api/src/lib.rs
@@ -677,6 +677,17 @@ pub trait NexusInternalApi {
         path_params: Path<params::SupportBundlePath>,
     ) -> Result<HttpResponseDeleted, HttpError>;
 
+    /// Update a support bundle
+    #[endpoint {
+        method = PUT,
+        path = "/experimental/v1/system/support-bundles/{bundle_id}",
+    }]
+    async fn support_bundle_update(
+        rqctx: RequestContext<Self::Context>,
+        path_params: Path<params::SupportBundlePath>,
+        body: TypedBody<params::SupportBundleUpdate>,
+    ) -> Result<HttpResponseOk<shared::SupportBundleInfo>, HttpError>;
+
     /// Get all the probes associated with a given sled.
     #[endpoint {
         method = GET,
diff --git a/nexus/src/app/support_bundles.rs b/nexus/src/app/support_bundles.rs
@@ -224,4 +224,27 @@ impl super::Nexus {
             .await?;
         Ok(())
     }
+
+    pub async fn support_bundle_update_user_comment(
+        &self,
+        opctx: &OpContext,
+        id: SupportBundleUuid,
+        user_comment: Option<String>,
+    ) -> LookupResult<SupportBundle> {
+        let (authz_bundle, ..) = LookupPath::new(opctx, &self.db_datastore)
+            .support_bundle(id)
+            .lookup_for(authz::Action::Modify)
+            .await?;
+
+        self.db_datastore
+            .support_bundle_update_user_comment(
+                &opctx,
+                &authz_bundle,
+                user_comment,
+            )
+            .await?;
+
+        // Return the updated bundle
+        self.support_bundle_view(opctx, id).await
+    }
 }
diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs
@@ -7493,6 +7493,37 @@ impl NexusExternalApi for NexusExternalApiImpl {
             .await
     }
 
+    async fn support_bundle_update(
+        rqctx: RequestContext<Self::Context>,
+        path_params: Path<params::SupportBundlePath>,
+        body: TypedBody<params::SupportBundleUpdate>,
+    ) -> Result<HttpResponseOk<shared::SupportBundleInfo>, HttpError> {
+        let apictx = rqctx.context();
+        let handler = async {
+            let nexus = &apictx.context.nexus;
+            let path = path_params.into_inner();
+            let update = body.into_inner();
+
+            let opctx =
+                crate::context::op_context_for_external_api(&rqctx).await?;
+
+            let bundle = nexus
+                .support_bundle_update_user_comment(
+                    &opctx,
+                    SupportBundleUuid::from_untyped_uuid(path.bundle_id),
+                    update.user_comment,
+                )
+                .await?;
+
+            Ok(HttpResponseOk(bundle.into()))
+        };
+        apictx
+            .context
+            .external_latencies
+            .instrument_dropshot_handler(&rqctx, handler)
+            .await
+    }
+
     async fn probe_list(
         rqctx: RequestContext<ApiContext>,
         query_params: Query<PaginatedByNameOrId<params::ProjectSelector>>,
diff --git a/nexus/src/internal_api/http_entrypoints.rs b/nexus/src/internal_api/http_entrypoints.rs
@@ -36,6 +36,7 @@ use nexus_types::external_api::params::PhysicalDiskPath;
 use nexus_types::external_api::params::SledSelector;
 use nexus_types::external_api::params::SupportBundleFilePath;
 use nexus_types::external_api::params::SupportBundlePath;
+use nexus_types::external_api::params::SupportBundleUpdate;
 use nexus_types::external_api::params::UninitializedSledId;
 use nexus_types::external_api::shared::ProbeInfo;
 use nexus_types::external_api::shared::UninitializedSled;
@@ -1319,6 +1320,37 @@ impl NexusInternalApi for NexusInternalApiImpl {
             .await
     }
 
+    async fn support_bundle_update(
+        rqctx: RequestContext<Self::Context>,
+        path_params: Path<SupportBundlePath>,
+        body: TypedBody<SupportBundleUpdate>,
+    ) -> Result<HttpResponseOk<shared::SupportBundleInfo>, HttpError> {
+        let apictx = rqctx.context();
+        let handler = async {
+            let nexus = &apictx.context.nexus;
+            let path = path_params.into_inner();
+            let update = body.into_inner();
+
+            let opctx =
+                crate::context::op_context_for_internal_api(&rqctx).await;
+
+            let bundle = nexus
+                .support_bundle_update_user_comment(
+                    &opctx,
+                    SupportBundleUuid::from_untyped_uuid(path.bundle_id),
+                    update.user_comment,
+                )
+                .await?;
+
+            Ok(HttpResponseOk(bundle.into()))
+        };
+        apictx
+            .context
+            .external_latencies
+            .instrument_dropshot_handler(&rqctx, handler)
+            .await
+    }
+
     async fn probes_get(
         rqctx: RequestContext<Self::Context>,
         path_params: Path<ProbePathParam>,
diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs
@@ -2474,6 +2474,12 @@ pub static VERIFY_ENDPOINTS: LazyLock<Vec<VerifyEndpoint>> =
                 allowed_methods: vec![
                     AllowedMethod::Get,
                     AllowedMethod::Delete,
+                    AllowedMethod::Put(
+                        serde_json::to_value(&params::SupportBundleUpdate {
+                            user_comment: None,
+                        })
+                        .unwrap(),
+                    ),
                 ],
             },
             /* Updates */
diff --git a/nexus/tests/integration_tests/support_bundles.rs b/nexus/tests/integration_tests/support_bundles.rs
@@ -279,6 +279,29 @@ async fn bundle_download_expect_fail(
     Ok(())
 }
 
+async fn bundle_update_comment(
+    client: &ClientTestContext,
+    id: SupportBundleUuid,
+    comment: Option<String>,
+) -> Result<SupportBundleInfo> {
+    use nexus_types::external_api::params::SupportBundleUpdate;
+
+    let url = format!("{BUNDLES_URL}/{id}");
+    let update = SupportBundleUpdate { user_comment: comment };
+
+    NexusRequest::new(
+        RequestBuilder::new(client, Method::PUT, &url)
+            .body(Some(&update))
+            .expect_status(Some(StatusCode::OK)),
+    )
+    .authn_as(AuthnMode::PrivilegedUser)
+    .execute()
+    .await
+    .context("failed to update bundle comment")?
+    .parsed_body()
+    .context("failed to parse 'update bundle comment' response")
+}
+
 // -- Background Task --
 //
 // The following logic helps us trigger and observe the output of the support
@@ -591,3 +614,73 @@ async fn test_support_bundle_range_requests(
     .unwrap();
     assert_eq!(second_half, full_contents[first_half.len()..]);
 }
+
+// Test updating bundle comments
+#[nexus_test]
+async fn test_support_bundle_update_comment(
+    cptestctx: &ControlPlaneTestContext,
+) {
+    let client = &cptestctx.external_client;
+
+    let _disk_test =
+        DiskTestBuilder::new(&cptestctx).with_zpool_count(1).build().await;
+
+    // Create a bundle
+    let bundle = bundle_create(&client).await.unwrap();
+    assert_eq!(bundle.user_comment, None);
+
+    // Update the comment
+    let comment = Some("Test comment".to_string());
+    let updated_bundle =
+        bundle_update_comment(&client, bundle.id, comment.clone())
+            .await
+            .unwrap();
+    assert_eq!(updated_bundle.user_comment, comment);
+
+    // Update with a different comment
+    let new_comment = Some("Updated comment".to_string());
+    let updated_bundle =
+        bundle_update_comment(&client, bundle.id, new_comment.clone())
+            .await
+            .unwrap();
+    assert_eq!(updated_bundle.user_comment, new_comment);
+
+    // Clear the comment
+    let updated_bundle =
+        bundle_update_comment(&client, bundle.id, None).await.unwrap();
+    assert_eq!(updated_bundle.user_comment, None);
+
+    // Test maximum length validation (4096 bytes)
+    let max_comment = "a".repeat(4096);
+    let updated_bundle =
+        bundle_update_comment(&client, bundle.id, Some(max_comment.clone()))
+            .await
+            .unwrap();
+    assert_eq!(updated_bundle.user_comment, Some(max_comment));
+
+    // Test exceeding maximum length (4097 bytes)
+    let too_long_comment = "a".repeat(4097);
+    let url = format!("{BUNDLES_URL}/{}", bundle.id);
+    let update = nexus_types::external_api::params::SupportBundleUpdate {
+        user_comment: Some(too_long_comment),
+    };
+
+    let error = NexusRequest::new(
+        RequestBuilder::new(client, Method::PUT, &url)
+            .body(Some(&update))
+            .expect_status(Some(StatusCode::BAD_REQUEST)),
+    )
+    .authn_as(AuthnMode::PrivilegedUser)
+    .execute()
+    .await
+    .context("failed to update bundle comment")
+    .unwrap()
+    .parsed_body::<HttpErrorResponseBody>()
+    .context("failed to parse error response")
+    .unwrap();
+
+    assert!(error.message.contains("cannot exceed 4096 bytes"));
+
+    // Clean up
+    bundle_delete(&client, bundle.id).await.unwrap();
+}
diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs
@@ -158,6 +158,12 @@ pub struct SupportBundleFilePath {
     pub file: String,
 }
 
+#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)]
+pub struct SupportBundleUpdate {
+    /// User comment for the support bundle
+    pub user_comment: Option<String>,
+}
+
 #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)]
 pub struct OptionalSiloSelector {
     /// Name or ID of the silo
diff --git a/nexus/types/src/external_api/shared.rs b/nexus/types/src/external_api/shared.rs
diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json
diff --git a/openapi/nexus.json b/openapi/nexus.json
diff --git a/schema/crdb/bundle-user-comment/up01.sql b/schema/crdb/bundle-user-comment/up01.sql
diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql

Original file line number	Diff line number	Diff line change
`@@ -95,6 +95,7 @@ pub struct SupportBundle {`
`95`	`95`	`pub zpool_id: DbTypedUuid<ZpoolKind>,`
`96`	`96`	`pub dataset_id: DbTypedUuid<DatasetKind>,`
`97`	`97`	`pub assigned_nexus: Option<DbTypedUuid<OmicronZoneKind>>,`
	`98`	`+ pub user_comment: Option<String>,`
`98`	`99`	`}`
`99`	`100`
`100`	`101`	`impl SupportBundle {`
`@@ -113,6 +114,7 @@ impl SupportBundle {`
`113`	`114`	`zpool_id: zpool_id.into(),`
`114`	`115`	`dataset_id: dataset_id.into(),`
`115`	`116`	`assigned_nexus: Some(nexus_id.into()),`
	`117`	`+ user_comment: None,`
`116`	`118`	`}`
`117`	`119`	`}`
`118`	`120`
`@@ -128,6 +130,7 @@ impl From<SupportBundle> for SupportBundleView {`
`128`	`130`	`time_created: bundle.time_created,`
`129`	`131`	`reason_for_creation: bundle.reason_for_creation,`
`130`	`132`	`reason_for_failure: bundle.reason_for_failure,`
	`133`	`+ user_comment: bundle.user_comment,`
`131`	`134`	`state: bundle.state.into(),`
`132`	`135`	`}`
`133`	`136`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1470,6 +1470,7 @@ table! {`
`1470`	`1470`	`dataset_id -> Uuid,`
`1471`	`1471`
`1472`	`1472`	`assigned_nexus -> Nullable<Uuid>,`
	`1473`	`+ user_comment -> Nullable<Text>,`
`1473`	`1474`	`}`
`1474`	`1475`	`}`
`1475`	`1476`