Ensure quotas are non-negative (#8990)

- Adds constraints that all the `INT8` quota components cannot be
negative
- Adds a schema migration to ensure this is the case
- Adds tests validating that data migration, creation, and update cannot
create negative silo values

Fixes #8973

Author: smklein

nexus/db-model/src/quota.rs

@@ -80,9 +80,9 @@ impl From<SiloQuotas> for views::SiloQuotas {
 pub struct SiloQuotasUpdate {
     pub cpus: Option<i64>,
     #[diesel(column_name = memory_bytes)]
-    pub memory: Option<i64>,
+    pub memory: Option<ByteCount>,
     #[diesel(column_name = storage_bytes)]
-    pub storage: Option<i64>,
+    pub storage: Option<ByteCount>,
     pub time_modified: DateTime<Utc>,
 }
 

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(187, 0, 0);
+pub const SCHEMA_VERSION: Version = Version::new(188, 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(188, "positive-quotas"),
         KnownVersion::new(187, "no-default-pool-for-internal-silo"),
         KnownVersion::new(186, "nexus-generation"),
         KnownVersion::new(185, "populate-db-metadata-nexus"),

nexus/db-queries/src/db/datastore/quota.rs

@@ -4,6 +4,8 @@ use crate::context::OpContext;
 use crate::db::pagination::paginated;
 use async_bb8_diesel::AsyncRunQueryDsl;
 use diesel::prelude::*;
+use diesel::result::DatabaseErrorKind;
+use diesel::result::Error as DieselError;
 use nexus_db_errors::ErrorHandler;
 use nexus_db_errors::public_error_from_diesel;
 use nexus_db_lookup::DbConnection;
@@ -17,6 +19,15 @@ use omicron_common::api::external::ResourceType;
 use omicron_common::api::external::UpdateResult;
 use uuid::Uuid;
 
+fn constraint_to_error(constraint: &str) -> &str {
+    match constraint {
+        "cpus_not_negative" => "CPU quota must not be negative",
+        "memory_not_negative" => "Memory quota must not be negative",
+        "storage_not_negative" => "Storage quota must not be negative",
+        _ => "Unknown constraint",
+    }
+}
+
 impl DataStore {
     /// Creates new quotas for a silo. This is grouped with silo creation
     /// and shouldn't be called outside of that flow.
@@ -38,14 +49,26 @@ impl DataStore {
             .values(quotas)
             .execute_async(conn)
             .await
-            .map_err(|e| {
-                public_error_from_diesel(
+            .map_err(|e| match e {
+                DieselError::DatabaseError(
+                    DatabaseErrorKind::CheckViolation,
+                    ref info,
+                ) => {
+                    let msg = match info.constraint_name() {
+                        Some(constraint) => constraint_to_error(constraint),
+                        None => "Missing constraint name for Check Violation",
+                    };
+                    Error::invalid_request(&format!(
+                        "Cannot create silo quota: {msg}"
+                    ))
+                }
+                _ => public_error_from_diesel(
                     e,
                     ErrorHandler::Conflict(
                         ResourceType::SiloQuotas,
                         &silo_id.to_string(),
                     ),
-                )
+                ),
             })
             .map(|_| ())
     }
@@ -85,14 +108,26 @@ impl DataStore {
             .returning(SiloQuotas::as_returning())
             .get_result_async(&*self.pool_connection_authorized(opctx).await?)
             .await
-            .map_err(|e| {
-                public_error_from_diesel(
+            .map_err(|e| match e {
+                DieselError::DatabaseError(
+                    DatabaseErrorKind::CheckViolation,
+                    ref info,
+                ) => {
+                    let msg = match info.constraint_name() {
+                        Some(constraint) => constraint_to_error(constraint),
+                        None => "Missing constraint name for Check Violation",
+                    };
+                    Error::invalid_request(&format!(
+                        "Cannot update silo quota: {msg}"
+                    ))
+                }
+                _ => public_error_from_diesel(
                     e,
                     ErrorHandler::Conflict(
                         ResourceType::SiloQuotas,
                         &silo_id.to_string(),
                     ),
-                )
+                ),
             })
     }
 

nexus/db-queries/src/db/datastore/virtual_provisioning_collection.rs

@@ -407,8 +407,8 @@ mod test {
 
         let quotas_update = SiloQuotasUpdate {
             cpus: Some(24),
-            memory: Some(1 << 40),
-            storage: Some(1 << 50),
+            memory: Some((1 << 40).try_into().unwrap()),
+            storage: Some((1 << 50).try_into().unwrap()),
             time_modified: chrono::Utc::now(),
         };
         let authz_silo = LookupPath::new(&opctx, datastore)

nexus/test-utils/src/resource_helpers.rs

@@ -166,7 +166,7 @@ where
     .authn_as(AuthnMode::PrivilegedUser)
     .execute()
     .await
-    .unwrap()
+    .unwrap_or_else(|err| panic!("Error creating object with {path}: {err}"))
     .parsed_body::<HttpErrorResponseBody>()
     .unwrap()
 }

nexus/tests/integration_tests/quotas.rs

@@ -12,6 +12,7 @@ use nexus_test_utils::resource_helpers::create_local_user;
 use nexus_test_utils::resource_helpers::grant_iam;
 use nexus_test_utils::resource_helpers::link_ip_pool;
 use nexus_test_utils::resource_helpers::object_create;
+use nexus_test_utils::resource_helpers::object_create_error;
 use nexus_test_utils::resource_helpers::test_params;
 use nexus_test_utils_macros::nexus_test;
 use nexus_types::external_api::params;
@@ -50,6 +51,27 @@ impl ResourceAllocator {
         .await
     }
 
+    async fn set_quotas_expect_error(
+        &self,
+        client: &ClientTestContext,
+        quotas: params::SiloQuotasUpdate,
+        code: http::StatusCode,
+    ) -> HttpErrorResponseBody {
+        NexusRequest::expect_failure_with_body(
+            client,
+            code,
+            http::Method::PUT,
+            "/v1/system/silos/quota-test-silo/quotas",
+            &Some(&quotas),
+        )
+        .authn_as(self.auth.clone())
+        .execute()
+        .await
+        .expect("Expected failure updating quotas")
+        .parsed_body::<HttpErrorResponseBody>()
+        .expect("Failed to read response after setting quotas")
+    }
+
     async fn get_quotas(&self, client: &ClientTestContext) -> SiloQuotas {
         NexusRequest::object_get(
             client,
@@ -386,3 +408,68 @@ async fn test_quota_limits(cptestctx: &ControlPlaneTestContext) {
         assert_eq!(quotas.limits.storage, quota_limit.storage.unwrap());
     }
 }
+
+#[nexus_test]
+async fn test_negative_quota(cptestctx: &ControlPlaneTestContext) {
+    let client = &cptestctx.external_client;
+
+    // Can't make a silo with a negative quota
+    let mut quotas = params::SiloQuotasCreate::empty();
+    quotas.cpus = -1;
+    let response = object_create_error(
+        client,
+        "/v1/system/silos",
+        &params::SiloCreate {
+            identity: IdentityMetadataCreateParams {
+                name: "negative-cpus-not-allowed".parse().unwrap(),
+                description: "".into(),
+            },
+            quotas,
+            discoverable: true,
+            identity_mode: shared::SiloIdentityMode::LocalOnly,
+            admin_group_name: None,
+            tls_certificates: vec![],
+            mapped_fleet_roles: Default::default(),
+        },
+        http::StatusCode::BAD_REQUEST,
+    )
+    .await;
+
+    assert!(
+        response.message.contains(
+            "Cannot create silo quota: CPU quota must not be negative"
+        ),
+        "Unexpected response: {}",
+        response.message
+    );
+
+    // Make the silo with an empty quota
+    let system = setup_silo_with_quota(
+        &client,
+        "quota-test-silo",
+        params::SiloQuotasCreate::empty(),
+    )
+    .await;
+
+    // Can't update a silo with a negative quota
+    let quota_limit = params::SiloQuotasUpdate {
+        cpus: Some(-1),
+        memory: Some(0_u64.try_into().unwrap()),
+        storage: Some(0_u64.try_into().unwrap()),
+    };
+    let response = system
+        .set_quotas_expect_error(
+            client,
+            quota_limit.clone(),
+            http::StatusCode::BAD_REQUEST,
+        )
+        .await;
+
+    assert!(
+        response.message.contains(
+            "Cannot update silo quota: CPU quota must not be negative"
+        ),
+        "Unexpected response: {}",
+        response.message
+    );
+}

nexus/tests/integration_tests/schema.rs

@@ -3040,6 +3040,88 @@ fn after_185_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> {
     })
 }
 
+const NEGATIVE_QUOTA: Uuid =
+    Uuid::from_u128(0x00006001_5c3d_4647_83b0_8f351dda7ce1);
+const POSITIVE_QUOTA: Uuid =
+    Uuid::from_u128(0x00006001_5c3d_4647_83b0_8f351dda7ce2);
+const MIXED_QUOTA: Uuid =
+    Uuid::from_u128(0x00006001_5c3d_4647_83b0_8f351dda7ce3);
+
+fn before_188_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> {
+    Box::pin(async move {
+        ctx.client
+            .execute(
+                &format!(
+                    "INSERT INTO omicron.public.silo_quotas (
+                        silo_id,
+                        time_created,
+                        time_modified,
+                        cpus,
+                        memory_bytes,
+                        storage_bytes
+                    )
+                     VALUES
+                     ('{NEGATIVE_QUOTA}', now(), now(), -1, -2, -3),
+                     ('{POSITIVE_QUOTA}', now(), now(), 1, 2, 3),
+                     ('{MIXED_QUOTA}', now(), now(), -1, 0, 3);",
+                ),
+                &[],
+            )
+            .await
+            .expect("inserted silo_quotas");
+    })
+}
+
+fn after_188_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> {
+    Box::pin(async move {
+        let rows = ctx
+            .client
+            .query(
+                "SELECT
+                   silo_id,
+                   cpus,
+                   memory_bytes,
+                   storage_bytes
+                 FROM omicron.public.silo_quotas ORDER BY silo_id ASC;",
+                &[],
+            )
+            .await
+            .expect("queried post-migration positive-quotas");
+
+        fn check_quota(
+            row: &Row,
+            id: Uuid,
+            cpus: i64,
+            memory: i64,
+            storage: i64,
+        ) {
+            assert_eq!(
+                row.values[0].expect("silo_id").unwrap(),
+                &AnySqlType::Uuid(id)
+            );
+            assert_eq!(
+                row.values[1].expect("cpus").unwrap(),
+                &AnySqlType::Int8(cpus)
+            );
+            assert_eq!(
+                row.values[2].expect("memory_bytes").unwrap(),
+                &AnySqlType::Int8(memory)
+            );
+            assert_eq!(
+                row.values[3].expect("storage_bytes").unwrap(),
+                &AnySqlType::Int8(storage)
+            );
+        }
+
+        let rows = process_rows(&rows);
+        assert_eq!(rows.len(), 3);
+
+        check_quota(&rows[0], NEGATIVE_QUOTA, 0, 0, 0);
+        check_quota(&rows[1], POSITIVE_QUOTA, 1, 2, 3);
+        check_quota(&rows[2], MIXED_QUOTA, 0, 0, 3);
+    })
+}
+
 // Lazily initializes all migration checks. The combination of Rust function
 // pointers and async makes defining a static table fairly painful, so we're
 // using lazy initialization instead.
@@ -3142,6 +3224,10 @@ fn get_migration_checks() -> BTreeMap<Version, DataMigrationFns> {
         Version::new(185, 0, 0),
         DataMigrationFns::new().before(before_185_0_0).after(after_185_0_0),
     );
+    map.insert(
+        Version::new(188, 0, 0),
+        DataMigrationFns::new().before(before_188_0_0).after(after_188_0_0),
+    );
     map
 }
 

schema/crdb/dbinit.sql

@@ -1073,7 +1073,11 @@ CREATE TABLE IF NOT EXISTS omicron.public.silo_quotas (
     time_modified TIMESTAMPTZ NOT NULL,
     cpus INT8 NOT NULL,
     memory_bytes INT8 NOT NULL,
-    storage_bytes INT8 NOT NULL
+    storage_bytes INT8 NOT NULL,
+
+    CONSTRAINT cpus_not_negative CHECK (cpus >= 0),
+    CONSTRAINT memory_not_negative CHECK (memory_bytes >= 0),
+    CONSTRAINT storage_not_negative CHECK (storage_bytes >= 0)
 );
 
 /**
@@ -6613,7 +6617,7 @@ INSERT INTO omicron.public.db_metadata (
     version,
     target_version
 ) VALUES
-    (TRUE, NOW(), NOW(), '187.0.0', NULL)
+    (TRUE, NOW(), NOW(), '188.0.0', NULL)
 ON CONFLICT DO NOTHING;
 
 COMMIT;

schema/crdb/positive-quotas/up01.sql

@@ -0,0 +1,8 @@
+SET LOCAL disallow_full_table_scans = off;
+
+UPDATE omicron.public.silo_quotas
+SET
+    cpus = GREATEST(cpus, 0),
+    memory_bytes = GREATEST(memory_bytes, 0),
+    storage_bytes = GREATEST(storage_bytes, 0)
+WHERE cpus < 0 OR memory_bytes < 0 OR storage_bytes < 0;

schema/crdb/positive-quotas/up02.sql

@@ -0,0 +1,4 @@
+ALTER TABLE omicron.public.silo_quotas
+    ADD CONSTRAINT IF NOT EXISTS cpus_not_negative CHECK (cpus >= 0),
+    ADD CONSTRAINT IF NOT EXISTS memory_not_negative CHECK (memory_bytes >= 0),
+    ADD CONSTRAINT IF NOT EXISTS storage_not_negative CHECK (storage_bytes >= 0);