From bcdd59cf4a962ca63bc2e2edb8553f4e43cb5646 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 10 Jul 2025 17:19:04 -0700 Subject: [PATCH] Support Bundle: Add user comment support --- dev-tools/omdb/src/bin/omdb/nexus.rs | 6 +- nexus/db-model/src/schema_versions.rs | 3 +- nexus/db-model/src/support_bundle.rs | 4 + .../src/db/datastore/support_bundle.rs | 61 ++++++-- nexus/db-schema/src/schema.rs | 1 + nexus/external-api/output/nexus_tags.txt | 1 + nexus/external-api/src/lib.rs | 13 ++ nexus/internal-api/src/lib.rs | 12 ++ .../tasks/support_bundle_collector.rs | 63 ++++++-- nexus/src/app/support_bundles.rs | 28 +++- nexus/src/external_api/http_entrypoints.rs | 39 ++++- nexus/src/internal_api/http_entrypoints.rs | 40 ++++- nexus/tests/integration_tests/endpoints.rs | 17 ++- .../integration_tests/support_bundles.rs | 144 ++++++++++++++++++ nexus/tests/integration_tests/unauthorized.rs | 7 +- nexus/types/src/external_api/params.rs | 12 ++ nexus/types/src/external_api/shared.rs | 1 + openapi/nexus-internal.json | 78 ++++++++++ openapi/nexus.json | 81 ++++++++++ schema/crdb/bundle-user-comment/up01.sql | 1 + schema/crdb/dbinit.sql | 7 +- 21 files changed, 588 insertions(+), 31 deletions(-) create mode 100644 schema/crdb/bundle-user-comment/up01.sql diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs index fca3415534e..6d63240b06f 100644 --- 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()) @@ -4033,7 +4035,9 @@ async fn cmd_nexus_support_bundles_create( _destruction_token: DestructiveOperationToken, ) -> Result<(), anyhow::Error> { let support_bundle_id = client - .support_bundle_create() + .support_bundle_create(&nexus_client::types::SupportBundleCreate { + user_comment: None, + }) .await .context("creating support bundle")? .into_inner() diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 2d2d48dfbbc..f7c94edc3a0 100644 --- 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(165, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(166, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock> = 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(166, "bundle-user-comment"), KnownVersion::new(165, "route-config-rib-priority"), KnownVersion::new(164, "fix-leaked-bp-oximeter-read-policy-rows"), KnownVersion::new(163, "bp-desired-host-phase-2"), diff --git a/nexus/db-model/src/support_bundle.rs b/nexus/db-model/src/support_bundle.rs index 6be06285126..b9dc9f55b17 100644 --- 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, pub dataset_id: DbTypedUuid, pub assigned_nexus: Option>, + pub user_comment: Option, } impl SupportBundle { @@ -103,6 +104,7 @@ impl SupportBundle { zpool_id: ZpoolUuid, dataset_id: DatasetUuid, nexus_id: OmicronZoneUuid, + user_comment: Option, ) -> Self { Self { id: SupportBundleUuid::new_v4().into(), @@ -113,6 +115,7 @@ impl SupportBundle { zpool_id: zpool_id.into(), dataset_id: dataset_id.into(), assigned_nexus: Some(nexus_id.into()), + user_comment, } } @@ -128,6 +131,7 @@ impl From 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 index 30e10aecc39..b6aaf5b4661 100644 --- a/nexus/db-queries/src/db/datastore/support_bundle.rs +++ b/nexus/db-queries/src/db/datastore/support_bundle.rs @@ -76,6 +76,7 @@ impl DataStore { opctx: &OpContext, reason_for_creation: &'static str, this_nexus_id: OmicronZoneUuid, + user_comment: Option, ) -> CreateResult { opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; let conn = self.pool_connection_authorized(opctx).await?; @@ -89,6 +90,7 @@ impl DataStore { self.transaction_retry_wrapper("support_bundle_create") .transaction(&conn, |conn| { let err = err.clone(); + let user_comment = user_comment.clone(); async move { use nexus_db_schema::schema::rendezvous_debug_dataset::dsl as dataset_dsl; @@ -129,6 +131,7 @@ impl DataStore { dataset.pool_id(), dataset.id(), this_nexus_id, + user_comment, ); diesel::insert_into(support_bundle_dsl::support_bundle) @@ -438,6 +441,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, + ) -> 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 @@ -628,7 +667,7 @@ mod test { this_nexus_id: OmicronZoneUuid, ) { let err = datastore - .support_bundle_create(&opctx, "for tests", this_nexus_id) + .support_bundle_create(&opctx, "for tests", this_nexus_id, None) .await .expect_err("Shouldn't provision bundle without datasets"); let Error::InsufficientCapacity { message } = err else { @@ -674,15 +713,15 @@ mod test { // Create two bundles on "nexus A", one bundle on "nexus B" let bundle_a1 = datastore - .support_bundle_create(&opctx, "for the test", nexus_a) + .support_bundle_create(&opctx, "for the test", nexus_a, None) .await .expect("Should be able to create bundle"); let bundle_a2 = datastore - .support_bundle_create(&opctx, "for the test", nexus_a) + .support_bundle_create(&opctx, "for the test", nexus_a, None) .await .expect("Should be able to create bundle"); let bundle_b1 = datastore - .support_bundle_create(&opctx, "for the test", nexus_b) + .support_bundle_create(&opctx, "for the test", nexus_b, None) .await .expect("Should be able to create bundle"); @@ -800,6 +839,7 @@ mod test { &opctx, "for the test", this_nexus_id, + None, ) .await .expect("Should be able to create bundle"), @@ -846,7 +886,7 @@ mod test { .await .expect("Should be able to destroy this bundle"); datastore - .support_bundle_create(&opctx, "for the test", this_nexus_id) + .support_bundle_create(&opctx, "for the test", this_nexus_id, None) .await .expect("Should be able to create bundle"); @@ -867,7 +907,7 @@ mod test { // Create the bundle, then observe it through the "getter" APIs let mut bundle = datastore - .support_bundle_create(&opctx, reason, this_nexus_id) + .support_bundle_create(&opctx, reason, this_nexus_id, None) .await .expect("Should be able to create bundle"); assert_eq!(bundle.reason_for_creation, reason); @@ -1031,7 +1071,7 @@ mod test { // When we create a bundle, it should exist on a dataset provisioned by // the blueprint. let bundle = datastore - .support_bundle_create(&opctx, "for the test", this_nexus_id) + .support_bundle_create(&opctx, "for the test", this_nexus_id, None) .await .expect("Should be able to create bundle"); assert_eq!(bundle.assigned_nexus, Some(this_nexus_id.into())); @@ -1136,7 +1176,7 @@ mod test { // When we create a bundle, it should exist on a dataset provisioned by // the blueprint. let bundle = datastore - .support_bundle_create(&opctx, "for the test", this_nexus_id) + .support_bundle_create(&opctx, "for the test", this_nexus_id, None) .await .expect("Should be able to create bundle"); assert_eq!(bundle.assigned_nexus, Some(this_nexus_id.into())); @@ -1237,7 +1277,7 @@ mod test { // When we create a bundle, it should exist on a dataset provisioned by // the blueprint. let bundle = datastore - .support_bundle_create(&opctx, "for the test", nexus_ids[0]) + .support_bundle_create(&opctx, "for the test", nexus_ids[0], None) .await .expect("Should be able to create bundle"); @@ -1358,7 +1398,7 @@ mod test { // When we create a bundle, it should exist on a dataset provisioned by // the blueprint. let bundle = datastore - .support_bundle_create(&opctx, "for the test", nexus_ids[0]) + .support_bundle_create(&opctx, "for the test", nexus_ids[0], None) .await .expect("Should be able to create bundle"); @@ -1442,6 +1482,7 @@ mod test { &opctx, "Bundle for time ordering test", this_nexus_id, + None, ) .await .expect("Should be able to create bundle"); diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index c5a6caae9e0..51dd5434c65 100644 --- 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, + user_comment -> Nullable, } } diff --git a/nexus/external-api/output/nexus_tags.txt b/nexus/external-api/output/nexus_tags.txt index 54262eb34a5..b073c650169 100644 --- 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 index 68f8db3e906..abf66566a7b 100644 --- a/nexus/external-api/src/lib.rs +++ b/nexus/external-api/src/lib.rs @@ -3368,6 +3368,7 @@ pub trait NexusExternalApi { }] async fn support_bundle_create( rqctx: RequestContext, + body: TypedBody, ) -> Result, HttpError>; /// Delete an existing support bundle @@ -3384,6 +3385,18 @@ pub trait NexusExternalApi { path_params: Path, ) -> Result; + /// 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, + path_params: Path, + body: TypedBody, + ) -> Result, HttpError>; + // Probes (experimental) /// List instrumentation probes diff --git a/nexus/internal-api/src/lib.rs b/nexus/internal-api/src/lib.rs index 69b4c214564..269582773ee 100644 --- a/nexus/internal-api/src/lib.rs +++ b/nexus/internal-api/src/lib.rs @@ -665,6 +665,7 @@ pub trait NexusInternalApi { }] async fn support_bundle_create( rqctx: RequestContext, + body: TypedBody, ) -> Result, HttpError>; /// Delete an existing support bundle @@ -680,6 +681,17 @@ pub trait NexusInternalApi { path_params: Path, ) -> Result; + /// Update a support bundle + #[endpoint { + method = PUT, + path = "/experimental/v1/system/support-bundles/{bundle_id}", + }] + async fn support_bundle_update( + rqctx: RequestContext, + path_params: Path, + body: TypedBody, + ) -> Result, HttpError>; + /// Get all the probes associated with a given sled. #[endpoint { method = GET, diff --git a/nexus/src/app/background/tasks/support_bundle_collector.rs b/nexus/src/app/background/tasks/support_bundle_collector.rs index f1088d8e67b..74459fc86a7 100644 --- a/nexus/src/app/background/tasks/support_bundle_collector.rs +++ b/nexus/src/app/background/tasks/support_bundle_collector.rs @@ -1454,7 +1454,12 @@ mod test { // Assign a bundle to ourselves. We expect to collect it on // the next call to "collect_bundle". let bundle = datastore - .support_bundle_create(&opctx, "For collection testing", nexus.id()) + .support_bundle_create( + &opctx, + "For collection testing", + nexus.id(), + None, + ) .await .expect("Couldn't allocate a support bundle"); assert_eq!(bundle.state, SupportBundleState::Collecting); @@ -1514,7 +1519,12 @@ mod test { TestDataset::setup(cptestctx, &datastore, &opctx, 1).await; let bundle = datastore - .support_bundle_create(&opctx, "For collection testing", nexus.id()) + .support_bundle_create( + &opctx, + "For collection testing", + nexus.id(), + None, + ) .await .expect("Couldn't allocate a support bundle"); assert_eq!(bundle.state, SupportBundleState::Collecting); @@ -1594,11 +1604,21 @@ mod test { // Assign two bundles to ourselves. let bundle1 = datastore - .support_bundle_create(&opctx, "For collection testing", nexus.id()) + .support_bundle_create( + &opctx, + "For collection testing", + nexus.id(), + None, + ) .await .expect("Couldn't allocate a support bundle"); let bundle2 = datastore - .support_bundle_create(&opctx, "For collection testing", nexus.id()) + .support_bundle_create( + &opctx, + "For collection testing", + nexus.id(), + None, + ) .await .expect("Couldn't allocate a second support bundle"); @@ -1679,7 +1699,12 @@ mod test { // If we delete the bundle before we start collection, we can delete it // immediately. let bundle = datastore - .support_bundle_create(&opctx, "For collection testing", nexus.id()) + .support_bundle_create( + &opctx, + "For collection testing", + nexus.id(), + None, + ) .await .expect("Couldn't allocate a support bundle"); assert_eq!(bundle.state, SupportBundleState::Collecting); @@ -1738,7 +1763,12 @@ mod test { // We can allocate a support bundle and collect it let bundle = datastore - .support_bundle_create(&opctx, "For collection testing", nexus.id()) + .support_bundle_create( + &opctx, + "For collection testing", + nexus.id(), + None, + ) .await .expect("Couldn't allocate a support bundle"); assert_eq!(bundle.state, SupportBundleState::Collecting); @@ -1811,7 +1841,12 @@ mod test { // We can allocate a support bundle, though we'll fail it before it gets // collected. let bundle = datastore - .support_bundle_create(&opctx, "For collection testing", nexus.id()) + .support_bundle_create( + &opctx, + "For collection testing", + nexus.id(), + None, + ) .await .expect("Couldn't allocate a support bundle"); assert_eq!(bundle.state, SupportBundleState::Collecting); @@ -1875,7 +1910,12 @@ mod test { // We can allocate a support bundle and collect it let bundle = datastore - .support_bundle_create(&opctx, "For collection testing", nexus.id()) + .support_bundle_create( + &opctx, + "For collection testing", + nexus.id(), + None, + ) .await .expect("Couldn't allocate a support bundle"); assert_eq!(bundle.state, SupportBundleState::Collecting); @@ -1955,7 +1995,12 @@ mod test { // We can allocate a support bundle and collect it let bundle = datastore - .support_bundle_create(&opctx, "For collection testing", nexus.id()) + .support_bundle_create( + &opctx, + "For collection testing", + nexus.id(), + None, + ) .await .expect("Couldn't allocate a support bundle"); assert_eq!(bundle.state, SupportBundleState::Collecting); diff --git a/nexus/src/app/support_bundles.rs b/nexus/src/app/support_bundles.rs index 69c5db77b07..57ae8ed7f27 100644 --- a/nexus/src/app/support_bundles.rs +++ b/nexus/src/app/support_bundles.rs @@ -61,8 +61,11 @@ impl super::Nexus { &self, opctx: &OpContext, reason: &'static str, + user_comment: Option, ) -> CreateResult { - self.db_datastore.support_bundle_create(&opctx, reason, self.id).await + self.db_datastore + .support_bundle_create(&opctx, reason, self.id, user_comment) + .await } pub async fn support_bundle_download( @@ -224,4 +227,27 @@ impl super::Nexus { .await?; Ok(()) } + + pub async fn support_bundle_update_user_comment( + &self, + opctx: &OpContext, + id: SupportBundleUuid, + user_comment: Option, + ) -> LookupResult { + 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 index 1bf6b9812f6..f03e6eb4483 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -7560,16 +7560,22 @@ impl NexusExternalApi for NexusExternalApiImpl { async fn support_bundle_create( rqctx: RequestContext, + body: TypedBody, ) -> Result, HttpError> { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.context.nexus; + let create_params = body.into_inner(); let opctx = crate::context::op_context_for_external_api(&rqctx).await?; let bundle = nexus - .support_bundle_create(&opctx, "Created by external API") + .support_bundle_create( + &opctx, + "Created by external API", + create_params.user_comment, + ) .await?; Ok(HttpResponseCreated(bundle.into())) }; @@ -7608,6 +7614,37 @@ impl NexusExternalApi for NexusExternalApiImpl { .await } + async fn support_bundle_update( + rqctx: RequestContext, + path_params: Path, + body: TypedBody, + ) -> Result, 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, query_params: Query>, diff --git a/nexus/src/internal_api/http_entrypoints.rs b/nexus/src/internal_api/http_entrypoints.rs index c0e2a5a29f0..eb9eb001faa 100644 --- 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; @@ -1273,16 +1274,22 @@ impl NexusInternalApi for NexusInternalApiImpl { async fn support_bundle_create( rqctx: RequestContext, + body: TypedBody, ) -> Result, HttpError> { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.context.nexus; + let create_params = body.into_inner(); let opctx = crate::context::op_context_for_internal_api(&rqctx).await; let bundle = nexus - .support_bundle_create(&opctx, "Created by internal API") + .support_bundle_create( + &opctx, + "Created by internal API", + create_params.user_comment, + ) .await?; Ok(HttpResponseCreated(bundle.into())) }; @@ -1321,6 +1328,37 @@ impl NexusInternalApi for NexusInternalApiImpl { .await } + async fn support_bundle_update( + rqctx: RequestContext, + path_params: Path, + body: TypedBody, + ) -> Result, 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, path_params: Path, diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index d2fa3cc3f43..01d03f94331 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -1421,8 +1421,8 @@ pub static URL_USERS_DB_INIT: LazyLock = LazyLock::new(|| { }); /// List of endpoints to be verified -pub static VERIFY_ENDPOINTS: LazyLock> = - LazyLock::new(|| { +pub static VERIFY_ENDPOINTS: LazyLock> = LazyLock::new( + || { vec![ // Global IAM policy VerifyEndpoint { @@ -2499,7 +2499,9 @@ pub static VERIFY_ENDPOINTS: LazyLock> = unprivileged_access: UnprivilegedAccess::None, allowed_methods: vec![ AllowedMethod::Get, - AllowedMethod::Post(serde_json::to_value(()).unwrap()), + AllowedMethod::Post(serde_json::to_value(&nexus_types::external_api::params::SupportBundleCreate { + user_comment: None, + }).unwrap()), ], }, VerifyEndpoint { @@ -2509,6 +2511,12 @@ pub static VERIFY_ENDPOINTS: LazyLock> = allowed_methods: vec![ AllowedMethod::Get, AllowedMethod::Delete, + AllowedMethod::Put( + serde_json::to_value(¶ms::SupportBundleUpdate { + user_comment: None, + }) + .unwrap(), + ), ], }, /* Updates */ @@ -3021,4 +3029,5 @@ pub static VERIFY_ENDPOINTS: LazyLock> = allowed_methods: vec![AllowedMethod::Get], }, ] - }); + }, +); diff --git a/nexus/tests/integration_tests/support_bundles.rs b/nexus/tests/integration_tests/support_bundles.rs index 878ae588350..5bd3f18faf0 100644 --- a/nexus/tests/integration_tests/support_bundles.rs +++ b/nexus/tests/integration_tests/support_bundles.rs @@ -143,8 +143,20 @@ async fn bundle_delete( async fn bundle_create( client: &ClientTestContext, ) -> Result { + bundle_create_with_comment(client, None).await +} + +async fn bundle_create_with_comment( + client: &ClientTestContext, + user_comment: Option, +) -> Result { + use nexus_types::external_api::params::SupportBundleCreate; + + let create_params = SupportBundleCreate { user_comment }; + NexusRequest::new( RequestBuilder::new(client, Method::POST, BUNDLES_URL) + .body(Some(&create_params)) .expect_status(Some(StatusCode::CREATED)), ) .authn_as(AuthnMode::PrivilegedUser) @@ -160,8 +172,12 @@ async fn bundle_create_expect_fail( expected_status: StatusCode, expected_message: &str, ) -> Result<()> { + use nexus_types::external_api::params::SupportBundleCreate; + + let create_params = SupportBundleCreate { user_comment: None }; let error = NexusRequest::new( RequestBuilder::new(client, Method::POST, BUNDLES_URL) + .body(Some(&create_params)) .expect_status(Some(expected_status)), ) .authn_as(AuthnMode::PrivilegedUser) @@ -279,6 +295,29 @@ async fn bundle_download_expect_fail( Ok(()) } +async fn bundle_update_comment( + client: &ClientTestContext, + id: SupportBundleUuid, + comment: Option, +) -> Result { + 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 @@ -640,3 +679,108 @@ async fn test_support_bundle_list_time_ordering( ); } } + +// 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::() + .context("failed to parse error response") + .unwrap(); + + assert!(error.message.contains("cannot exceed 4096 bytes")); + + // Clean up + bundle_delete(&client, bundle.id).await.unwrap(); +} + +// Test creating bundles with comments +#[nexus_test] +async fn test_support_bundle_create_with_comment( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + + let _disk_test = + DiskTestBuilder::new(&cptestctx).with_zpool_count(3).build().await; + + // Create a bundle without comment + let bundle_no_comment = + bundle_create_with_comment(&client, None).await.unwrap(); + assert_eq!(bundle_no_comment.user_comment, None); + + // Create a bundle with comment + let comment = Some("Test comment during creation".to_string()); + let bundle_with_comment = + bundle_create_with_comment(&client, comment.clone()).await.unwrap(); + assert_eq!(bundle_with_comment.user_comment, comment); + + // Create a bundle with empty comment + let empty_comment = Some("".to_string()); + let bundle_empty_comment = + bundle_create_with_comment(&client, empty_comment.clone()) + .await + .unwrap(); + assert_eq!(bundle_empty_comment.user_comment, empty_comment); + + // Clean up + bundle_delete(&client, bundle_no_comment.id).await.unwrap(); + bundle_delete(&client, bundle_with_comment.id).await.unwrap(); + bundle_delete(&client, bundle_empty_comment.id).await.unwrap(); +} diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 08a6b4c15c0..0969874fe7d 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -397,7 +397,12 @@ static SETUP_REQUESTS: LazyLock> = LazyLock::new(|| { // Create a Support Bundle SetupReq::Post { url: &SUPPORT_BUNDLES_URL, - body: serde_json::to_value(()).unwrap(), + body: serde_json::to_value( + &nexus_types::external_api::params::SupportBundleCreate { + user_comment: None, + }, + ) + .unwrap(), id_routes: vec!["/experimental/v1/system/support-bundles/{id}"], }, // Create a trusted root for updates diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 0ca082a094a..333a8e73cbe 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -159,6 +159,18 @@ pub struct SupportBundleFilePath { pub file: String, } +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct SupportBundleCreate { + /// User comment for the support bundle + pub user_comment: Option, +} + +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct SupportBundleUpdate { + /// User comment for the support bundle + pub user_comment: Option, +} + #[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 index 7688b969a34..6de268e6a45 100644 --- a/nexus/types/src/external_api/shared.rs +++ b/nexus/types/src/external_api/shared.rs @@ -645,6 +645,7 @@ pub struct SupportBundleInfo { pub time_created: DateTime, pub reason_for_creation: String, pub reason_for_failure: Option, + pub user_comment: Option, pub state: SupportBundleState, } diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 68a0f0b13d1..f401117f3ec 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -911,6 +911,16 @@ "post": { "summary": "Create a new support bundle", "operationId": "support_bundle_create", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SupportBundleCreate" + } + } + }, + "required": true + }, "responses": { "201": { "description": "successful creation", @@ -966,6 +976,50 @@ } } }, + "put": { + "summary": "Update a support bundle", + "operationId": "support_bundle_update", + "parameters": [ + { + "in": "path", + "name": "bundle_id", + "description": "ID of the support bundle", + "required": true, + "schema": { + "type": "string", + "format": "uuid" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SupportBundleUpdate" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SupportBundleInfo" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, "delete": { "summary": "Delete an existing support bundle", "description": "May also be used to cancel a support bundle which is currently being collected, or to remove metadata for a support bundle that has failed.", @@ -7419,6 +7473,16 @@ "weight" ] }, + "SupportBundleCreate": { + "type": "object", + "properties": { + "user_comment": { + "nullable": true, + "description": "User comment for the support bundle", + "type": "string" + } + } + }, "SupportBundleInfo": { "type": "object", "properties": { @@ -7438,6 +7502,10 @@ "time_created": { "type": "string", "format": "date-time" + }, + "user_comment": { + "nullable": true, + "type": "string" } }, "required": [ @@ -7500,6 +7568,16 @@ } ] }, + "SupportBundleUpdate": { + "type": "object", + "properties": { + "user_comment": { + "nullable": true, + "description": "User comment for the support bundle", + "type": "string" + } + } + }, "SwitchLocation": { "description": "Identifies switch physical location", "oneOf": [ diff --git a/openapi/nexus.json b/openapi/nexus.json index 646603564f3..bd91bcc6534 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -365,6 +365,16 @@ ], "summary": "Create a new support bundle", "operationId": "support_bundle_create", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SupportBundleCreate" + } + } + }, + "required": true + }, "responses": { "201": { "description": "successful creation", @@ -423,6 +433,53 @@ } } }, + "put": { + "tags": [ + "experimental" + ], + "summary": "Update a support bundle", + "operationId": "support_bundle_update", + "parameters": [ + { + "in": "path", + "name": "bundle_id", + "description": "ID of the support bundle", + "required": true, + "schema": { + "type": "string", + "format": "uuid" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SupportBundleUpdate" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SupportBundleInfo" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, "delete": { "tags": [ "experimental" @@ -24173,6 +24230,16 @@ "items" ] }, + "SupportBundleCreate": { + "type": "object", + "properties": { + "user_comment": { + "nullable": true, + "description": "User comment for the support bundle", + "type": "string" + } + } + }, "SupportBundleInfo": { "type": "object", "properties": { @@ -24192,6 +24259,10 @@ "time_created": { "type": "string", "format": "date-time" + }, + "user_comment": { + "nullable": true, + "type": "string" } }, "required": [ @@ -24254,6 +24325,16 @@ } ] }, + "SupportBundleUpdate": { + "type": "object", + "properties": { + "user_comment": { + "nullable": true, + "description": "User comment for the support bundle", + "type": "string" + } + } + }, "Switch": { "description": "An operator's view of a Switch.", "type": "object", diff --git a/schema/crdb/bundle-user-comment/up01.sql b/schema/crdb/bundle-user-comment/up01.sql new file mode 100644 index 00000000000..57b71841c52 --- /dev/null +++ b/schema/crdb/bundle-user-comment/up01.sql @@ -0,0 +1 @@ +ALTER TABLE omicron.public.support_bundle ADD COLUMN IF NOT EXISTS user_comment TEXT; diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 1a76f8b3422..21414557de1 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -2650,7 +2650,10 @@ CREATE TABLE IF NOT EXISTS omicron.public.support_bundle ( -- The Nexus which is in charge of collecting the support bundle, -- and later managing its storage. - assigned_nexus UUID + assigned_nexus UUID, + + user_comment TEXT + ); -- The "UNIQUE" part of this index helps enforce that we allow one support bundle @@ -6209,7 +6212,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '165.0.0', NULL) + (TRUE, NOW(), NOW(), '166.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT;