From 04543c2d990c54bb804f551440fa61f08d624b7a Mon Sep 17 00:00:00 2001 From: Bill Minckler Date: Mon, 17 Aug 2026 14:42:41 +0000 Subject: [PATCH] Fix SKU validation allocation bypass --- crates/api-core/src/cfg/README.md | 2 +- crates/api-core/src/tests/sku.rs | 144 +++++++++++++----- .../src/config/bom_validation.rs | 5 +- crates/machine-controller/src/handler/sku.rs | 41 ++--- docs/provisioning/sku-validation.md | 14 +- 5 files changed, 138 insertions(+), 68 deletions(-) diff --git a/crates/api-core/src/cfg/README.md b/crates/api-core/src/cfg/README.md index 0584db0967..1a676cd073 100644 --- a/crates/api-core/src/cfg/README.md +++ b/crates/api-core/src/cfg/README.md @@ -812,7 +812,7 @@ be propagated there by DPF. | ------- | ------ | --------- | ------------- | | `enabled` | `bool` | `false` | Enable BOM/SKU validation. | | `ignore_unassigned_machines` | `bool` | `false` | Let machines without a SKU bypass validation. | -| `allow_allocation_on_validation_failure` | `bool` | `false` | Keep machines allocatable even when validation fails. | +| `allow_allocation_on_validation_failure` | `bool` | `false` | Keep machines with assigned SKUs allocatable on validation failure; does not bypass unassigned machines. | | `find_match_interval` | `Duration` | `5m` | Interval between SKU match attempts. | | `auto_generate_missing_sku` | `bool` | `false` | Auto-create missing SKUs from expected machines. | | `auto_generate_missing_sku_interval` | `Duration` | `5m` | Interval between auto-generate attempts. | diff --git a/crates/api-core/src/tests/sku.rs b/crates/api-core/src/tests/sku.rs index 6446a68abd..fe633e143a 100644 --- a/crates/api-core/src/tests/sku.rs +++ b/crates/api-core/src/tests/sku.rs @@ -50,8 +50,8 @@ pub(in crate::tests) mod tests { // A = allow_allocation_on_validation_failure (F=block on failure, T=allow allocation despite failures) // B = auto_generate_missing_sku (F=manual, T=auto-generate) // - // When allow_allocation_on_validation_failure=true, machines in failed states - // (SkuVerificationFailed, SkuMissing, WaitingForSkuAssignment) can transition + // When allow_allocation_on_validation_failure=true, machines with assigned + // SKUs in failed states (SkuVerificationFailed or SkuMissing) can transition // back to Ready/MachineValidation instead of staying blocked. // // ================================================================================ @@ -72,7 +72,7 @@ pub(in crate::tests) mod tests { // │2.1 │ F │ F │ WaitingForSkuAssignment │ Stays in Waiting │ test_stays_in_waiting_state_when_not_assigned │ // │2.2 │ F │ F │ Waiting → Test assigns SKU │ → UpdatingInventory │ test_leave_waiting_when_assigned │ // │2.3 │ F │ F │ No SKU assigned │ Stuck in Waiting │ test_stuck_in_waiting_without_sku │ - // │2.4 │ T │ F │ No SKU (skips Waiting) │ Panic (never enters) │ test_escapes_waiting_with_allow_allocation │ + // │2.4 │ T │ F │ No SKU assigned │ Stays in Waiting │ test_allow_allocation_does_not_bypass_unassigned_sku │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ // │ 3. SkuMissing State │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ @@ -97,7 +97,7 @@ pub(in crate::tests) mod tests { // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ // │6.1 │ F │ * │ Second machine, same HW │ Auto-match existing → Ready│ test_auto_match_sku │ // │6.2 │ F │ * │ SKU replacement triggers │ Re-verify (to BomValidating)│ test_replace_triggers_verify │ - // │6.3 │ F │ * │ Unassign + clear status │ Immediate re-match → Ready │ test_auto_match_after_unassign │ + // │6.3 │ F │ * │ Unassign + retained status │ Immediate re-match → Ready │ test_auto_match_after_unassign │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ // │ 7. SKU Version Compatibility │ // ├────┼───┼───┼─────────────────────────────┼────────────────────────────┼──────────────────────────────────────────────────────────┤ @@ -294,6 +294,17 @@ pub(in crate::tests) mod tests { test_env } + async fn create_test_env_ignoring_unassigned_machines(db_pool: sqlx::PgPool) -> TestEnv { + let mut overrides = TestEnvOverrides::default(); + let mut config = get_config(); + config.bom_validation.enabled = true; + config.bom_validation.ignore_unassigned_machines = true; + config.bom_validation.allow_allocation_on_validation_failure = false; + overrides.config = Some(config); + + create_test_env_with_overrides(db_pool, overrides).await + } + async fn get_machine_state(pool: &sqlx::PgPool, mh: &TestManagedHost) -> ManagedHostState { let mut txn = pool.begin().await.unwrap(); let machine = mh.host().db_machine(&mut txn).await; @@ -337,23 +348,6 @@ pub(in crate::tests) mod tests { .ok_or_else(|| eyre::eyre!("machine not found: {}", machine_id)) } - /// Helper: Clear the SKU status/timestamp on a machine to allow re-matching - /// Used in tests to reset the SKU matching state and test re-match behavior - pub(in crate::tests) async fn clear_sku_status( - txn: &mut PgConnection, - machine_id: &MachineId, - ) -> Result<(), DatabaseError> { - let query = "UPDATE machines SET hw_sku_status=null WHERE id=$1 RETURNING id"; - - let _: () = sqlx::query_as(query) - .bind(machine_id) - .fetch_one(txn) - .await - .map_err(|e| DatabaseError::new("clear sku last match attempt", e))?; - - Ok(()) - } - /// Helper: Test SKU version compatibility and backward compatibility /// Creates a machine with latest SKU version, then generates an older version SKU /// from the same hardware to verify version handling and diff behavior @@ -804,21 +798,18 @@ pub(in crate::tests) mod tests { Ok(()) } - /// Test 2.4: Never enters WaitingForSkuAssignment with allow_allocation_on_validation_failure=true + /// Test 2.4: WaitingForSkuAssignment with allow_allocation_on_validation_failure=true /// Conditions: /// - allow_allocation_on_validation_failure = true /// - auto_generate_missing_sku = false /// - auto_assign_sku_in_fixture = false - /// - expected_state = WaitingForSkuAssignment - /// Expected: fixture panics because machine never enters WaitingForSkuAssignment - /// (machine directly skips to MachineValidation, proving allow_allocation logic works) + /// Expected: machine enters WaitingForSkuAssignment because no SKU validation occurred #[crate::sqlx_test] - #[should_panic(expected = "Expected Machine state condition not hit after")] - async fn test_escapes_waiting_with_allow_allocation(pool: sqlx::PgPool) { + async fn test_allow_allocation_does_not_bypass_unassigned_sku( + pool: sqlx::PgPool, + ) -> Result<(), eyre::Error> { let env = create_test_env_for_bom_validation(pool.clone(), true, None, false).await; - // Expect WaitingForSkuAssignment, but machine will never enter that state - // because allow_allocation=true makes it skip directly to MachineValidation let mut config = ManagedHostConfig::default().with_expected_state(ManagedHostState::BomValidating { bom_validating_state: BomValidating::WaitingForSkuAssignment( @@ -830,9 +821,50 @@ pub(in crate::tests) mod tests { }); config.auto_assign_sku_in_fixture = false; - // This will panic because machine never enters WaitingForSkuAssignment - // (it goes directly from MatchingSku to MachineValidation) - let _mh = create_managed_host_with_config(&env, config).await; + let mh = create_managed_host_with_config(&env, config).await; + let mut txn = pool.begin().await?; + let machine = mh.host().db_machine(&mut txn).await; + + assert!(matches!( + machine.current_state(), + ManagedHostState::BomValidating { + bom_validating_state: BomValidating::WaitingForSkuAssignment(_) + } + )); + assert!(machine.config.hw_sku.is_none()); + + Ok(()) + } + + /// Test 2.5: Machine creation with ignore_unassigned_machines=true + /// Conditions: + /// - ignore_unassigned_machines = true + /// - no SKU is assigned + /// Expected: machine reaches Ready state without an assigned SKU + #[crate::sqlx_test] + async fn test_ignore_unassigned_machines_bypasses_unassigned_sku( + pool: sqlx::PgPool, + ) -> Result<(), eyre::Error> { + let env = create_test_env_ignoring_unassigned_machines(pool.clone()).await; + let config = ManagedHostConfig { + auto_assign_sku_in_fixture: false, + ..ManagedHostConfig::default() + }; + + let mh = create_managed_host_with_config(&env, config).await; + let mut txn = pool.begin().await?; + let machine = mh.host().db_machine(&mut txn).await; + + assert_eq!(machine.current_state(), &ManagedHostState::Ready); + assert!(machine.config.hw_sku.is_none()); + assert!( + machine + .status + .hw_sku + .is_some_and(|status| status.last_match_attempt.is_some()) + ); + + Ok(()) } /// Test 3.1: SkuMissing State → stays in SkuMissing @@ -1490,6 +1522,44 @@ pub(in crate::tests) mod tests { Ok(()) } + /// Test 5.1a: Assigned SKU verification failure with ignore_unassigned_machines=true + /// Conditions: + /// - ignore_unassigned_machines = true + /// - allow_allocation_on_validation_failure = false + /// - machine has an assigned SKU that does not match hardware + /// Expected: machine enters SkuVerificationFailed and remains unallocatable + #[crate::sqlx_test] + async fn test_ignore_unassigned_machines_does_not_bypass_assigned_sku_verification( + pool: sqlx::PgPool, + ) -> Result<(), eyre::Error> { + let env = create_test_env_ignoring_unassigned_machines(pool.clone()).await; + + let mh = create_managed_host(&env).await; + let machine_id = mh.host().id; + + let mut txn = pool.begin().await?; + let current_sku = db::sku::generate_sku_from_machine(txn.as_mut(), &machine_id).await?; + db::sku::create(&mut txn, ¤t_sku).await?; + assign_mismatched_sku(&mut txn, &machine_id, ¤t_sku.id).await?; + db::machine::update_sku_status_verify_request_time(&mut txn, &machine_id).await?; + txn.commit().await?; + + handle_inventory_update(&pool, &env, &mh).await; + env.run_machine_state_controller_iteration().await; + env.run_machine_state_controller_iteration().await; + + let mut txn = pool.begin().await?; + let machine = get_machine_by_id(&mut txn, &machine_id).await?; + assert!(matches!( + machine.current_state(), + ManagedHostState::BomValidating { + bom_validating_state: BomValidating::SkuVerificationFailed(_) + } + )); + + Ok(()) + } + /// Test 5.2: SKU replaced but no re-verification triggered → continues to Ready /// Conditions: /// - allow_allocation_on_validation_failure = false (standard mode) @@ -1731,16 +1801,13 @@ pub(in crate::tests) mod tests { Ok(()) } - /// Test 6.3: Auto-matching SKU after unassign and status clear + /// Test 6.3: Auto-matching SKU after unassign with retained SKU status /// Conditions: /// - allow_allocation_on_validation_failure = false (standard mode) /// - auto_generate_missing_sku = false /// - Machine reaches Ready with auto-matched SKU - /// - SKU is unassigned and hw_sku_status is cleared (simulating fresh start) + /// - SKU is unassigned while hw_sku_status retains a verification request /// Expected: Machine re-enters BOM validation flow and successfully re-matches the same SKU - /// Note: Status is cleared to bypass find_match_interval for immediate re-matching. The - /// find_match_interval config throttles retry attempts when NO matching SKU is found, - /// not when a matching SKU exists (which matches immediately). #[crate::sqlx_test] async fn test_auto_match_after_unassign(pool: sqlx::PgPool) -> Result<(), eyre::Error> { let env = create_test_env_for_bom_validation( @@ -1797,8 +1864,7 @@ pub(in crate::tests) mod tests { assert_eq!(machine.config.hw_sku, Some(expected_sku.id.clone())); assert_eq!(machine.current_state(), &ManagedHostState::Ready); - clear_sku_status(&mut txn, &machine_id).await?; - // test that an unassigned can find and assign a machine. + db::machine::update_sku_status_verify_request_time(&mut txn, &machine_id).await?; db::machine::unassign_sku(&mut txn, &machine_id).await?; txn.commit().await?; diff --git a/crates/machine-controller/src/config/bom_validation.rs b/crates/machine-controller/src/config/bom_validation.rs index 46b2cd29a9..4bbb51d712 100644 --- a/crates/machine-controller/src/config/bom_validation.rs +++ b/crates/machine-controller/src/config/bom_validation.rs @@ -32,10 +32,11 @@ pub struct BomValidationConfig { #[serde(default)] pub ignore_unassigned_machines: bool, - /// Allow machines to stay in Ready state and remain allocatable even when SKU validation fails + /// Allow machines with assigned SKUs to stay in Ready state and remain allocatable even when SKU validation fails /// When false (default): Standard mode - validation failures block allocation (machine enters failed state) /// When true: Allow allocation mode - validation still occurs and health reports are recorded, but machines do not transition - /// into failed states (SkuVerificationFailed, SkuMissing, WaitingForSkuAssignment) and can proceed to Ready/MachineValidation + /// into failed states (SkuVerificationFailed, SkuMissing) and can proceed to Ready/MachineValidation + /// This does not bypass machines without an assigned SKU; use ignore_unassigned_machines for that. #[serde(default)] pub allow_allocation_on_validation_failure: bool, diff --git a/crates/machine-controller/src/handler/sku.rs b/crates/machine-controller/src/handler/sku.rs index 70acc75c91..55f36e0627 100644 --- a/crates/machine-controller/src/handler/sku.rs +++ b/crates/machine-controller/src/handler/sku.rs @@ -73,13 +73,11 @@ async fn match_sku_for_machine( mh_snapshot: &ManagedHostStateSnapshot, ) -> Result, StateHandlerError> { let sku_status = mh_snapshot.host_snapshot.status.hw_sku.as_ref(); - if sku_status.is_none() - || sku_status.is_some_and(|ss| { - ss.last_match_attempt.is_some_and(|t| { - t < (Utc::now() - host_handler_params.bom_validation.find_match_interval) - }) + if sku_status.is_none_or(|ss| { + ss.last_match_attempt.is_none_or(|t| { + t < (Utc::now() - host_handler_params.bom_validation.find_match_interval) }) - { + }) { let machine_sku = db::sku::generate_sku_from_machine(&mut *txn, &mh_snapshot.host_snapshot.id).await?; let matching_sku = db::sku::find_matching(txn, &machine_sku).await?; @@ -188,16 +186,18 @@ async fn generate_missing_sku_for_machine( true } -/// Helper function to determine if machine should be allowed to allocate even when validation fails -/// This is useful to avoid blocking allocation when SKU validation issues occur +/// Determine whether a machine with an assigned SKU can allocate despite validation failure. fn should_allow_allocation_on_validation_failure(host_handler_params: &HostHandlerParams) -> bool { - // TODO:tmp solution to consider ignore_unassigned_machines for compatibale with some running sites + host_handler_params + .bom_validation + .allow_allocation_on_validation_failure +} + +/// Determine whether a machine without an assigned SKU can bypass BOM validation. +fn should_ignore_unassigned_machine(host_handler_params: &HostHandlerParams) -> bool { host_handler_params .bom_validation .ignore_unassigned_machines - || host_handler_params - .bom_validation - .allow_allocation_on_validation_failure } pub(crate) async fn handle_bom_validation_requested( @@ -234,12 +234,13 @@ pub(crate) async fn handle_bom_validation_requested( "Cannot find a matching SKU for machine" ); - if should_allow_allocation_on_validation_failure(host_handler_params) { - // Case 1.2.1: Allow allocation despite no SKU match + if should_ignore_unassigned_machine(host_handler_params) { + // Case 1.2.1: Allow allocation without an assigned SKU tracing::info!( machine_id=%mh_snapshot.host_snapshot.id, - "allow_allocation_on_validation_failure is true, staying in Ready state" + "ignore_unassigned_machines is true, staying in Ready state" ); + txn.commit().await?; return Ok(None); } else { // Case 1.2.2: Block allocation, wait for SKU assignment @@ -354,14 +355,14 @@ async fn advance_to_waiting_for_sku_assignment( mh_snapshot: &ManagedHostStateSnapshot, host_handler_params: &HostHandlerParams, ) -> Result, StateHandlerError> { - if should_allow_allocation_on_validation_failure(host_handler_params) + if should_ignore_unassigned_machine(host_handler_params) && mh_snapshot.host_snapshot.config.hw_sku.is_none() { skip_bom_validation_and_advance( txn, host_handler_params, mh_snapshot, - "allow_allocation_when_sku_unassigned", + "ignore_unassigned_machine_before_waiting", ) .await } else { @@ -414,7 +415,7 @@ async fn advance_to_machine_validating( } /// Skip BOM validation and proceed to machine validation (or Ready if machine validation is disabled) -/// Used when BOM validation is disabled or when allow_allocation_on_validation_failure is enabled +/// Used when BOM validation is disabled or a configured policy bypasses BOM validation async fn skip_bom_validation_and_advance( txn: PgTransaction<'static>, host_handler_params: &HostHandlerParams, @@ -645,13 +646,13 @@ pub(crate) async fn handle_bom_validation_state( .is_some() { advance_to_updating_inventory(txn, mh_snapshot).await - } else if should_allow_allocation_on_validation_failure(host_handler_params) { + } else if should_ignore_unassigned_machine(host_handler_params) { // Allow machine to proceed without SKU assignment skip_bom_validation_and_advance( txn, host_handler_params, mh_snapshot, - "allow_allocation_on_validation_failure", + "ignore_unassigned_machine_while_waiting", ) .await } else { diff --git a/docs/provisioning/sku-validation.md b/docs/provisioning/sku-validation.md index 10aafaf5af..1ae905a37e 100644 --- a/docs/provisioning/sku-validation.md +++ b/docs/provisioning/sku-validation.md @@ -66,12 +66,14 @@ auto_generate_missing_sku_interval = "300s" - `enabled` - Enables or disables the entire bom validation process. When disabled, machines will skip bom validation and proceed as if all validation has passed. - - `allow_allocation_on_validation_failure` - When true, machines are allowed to stay in Ready state and remain allocatable - even when SKU validation fails. Validation still occurs but only logs are recorded - health reports are cleared instead - of recording validation failures. Machines do not transition into failed states (SkuVerificationFailed, SkuMissing, - WaitingForSkuAssignment). When false (default), standard mode applies where validation failures are recorded in health - reports and machines enter failed states and become unallocatable until fixed. This is useful for avoiding machine - allocation blockage due to SKU validation issues when you only need logging without health report alerts. + - `allow_allocation_on_validation_failure` - When true, machines with an assigned SKU are allowed to stay in Ready state + and remain allocatable even when SKU validation fails. Validation still occurs but only logs are recorded - health reports + are cleared instead of recording validation failures. Machines do not transition into failed states + (SkuVerificationFailed or SkuMissing). When false (default), standard mode applies where validation failures are recorded + in health reports and machines enter failed states and become unallocatable until fixed. This is useful for avoiding machine + allocation blockage due to SKU validation issues when you only need logging without health report alerts. This option does + not bypass a machine without an assigned SKU; with `ignore_unassigned_machines = false`, that machine waits for SKU + assignment. - `ignore_unassigned_machines` - When true and BOM validation encounters a machine that does not have an associated SKU, it will proceed as if all validation has passed. Only machines with an associated SKU will be validated. This allows existing sites to be upgraded and BOM Validation enabled as SKUs are added to the system without impacting site operation.