diff --git a/docs-mslearn/toolkit/changelog.md b/docs-mslearn/toolkit/changelog.md index 92501d68d..a2cf3f5a3 100644 --- a/docs-mslearn/toolkit/changelog.md +++ b/docs-mslearn/toolkit/changelog.md @@ -34,6 +34,7 @@ The following section lists features and enhancements that are currently in deve - Replaced redundant `tolower()` comparisons in hub KQL with case-insensitive operators (`has`, `=~`, `!~`) so the engine can use the term index instead of scanning every row ([#2213](https://github.com/microsoft/finops-toolkit/issues/2213)). - Replaced whole-term `contains` matches with `has` across hub KQL and the query catalog (resource ID paths, licensing phrases, SKU description terms) and added a per-row operator-equivalence regression harness with unit test coverage ([#2220](https://github.com/microsoft/finops-toolkit/pull/2220)). - **Fixed** + - Fixed hub redeploys silently resetting ingestion and Data Explorer retention to the 13-month default when a custom value wasn't explicitly re-specified, which caused the purge pipeline to age out historical data on the next run ([#2206](https://github.com/microsoft/finops-toolkit/issues/2206)). - Fixed private-network deployments that Azure Policy blocked when `defaultOutboundAccess` was omitted. Private mode subnets now set it to `false`, while an Azure Files private endpoint supports deployment-script storage and the NAT Gateway provides required container egress ([#2258](https://github.com/microsoft/finops-toolkit/issues/2258), [#2259](https://github.com/microsoft/finops-toolkit/pull/2259)). - Fixed the `ContractedCost` recompute guard to compare with a null-safe tolerance instead of exact float equality, eliminating millions of no-op rewrites that polluted the `x_SourceValues` audit trail while preserving the null-cost backfill and no longer overwriting an existing cost when the unit price is missing ([#2216](https://github.com/microsoft/finops-toolkit/issues/2216)). - Fixed the SQL VMs without Azure Hybrid Benefit recommendation query to join on the SQL VM `virtualMachineResourceId` instead of a case-sensitive VM name match that skipped VMs with uppercase names and dropped duplicate names, and made all Azure Resource Graph join kinds explicit so no query relies on the `innerunique` default ([#2225](https://github.com/microsoft/finops-toolkit/pull/2225)). diff --git a/src/powershell/Tests/Unit/HubsRetentionGuard.Tests.ps1 b/src/powershell/Tests/Unit/HubsRetentionGuard.Tests.ps1 new file mode 100644 index 000000000..07713d185 --- /dev/null +++ b/src/powershell/Tests/Unit/HubsRetentionGuard.Tests.ps1 @@ -0,0 +1,61 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +<# + Regression coverage for the settings.json retention guard (#2206): + Copy-FileToAzureBlob.ps1 unconditionally overwrote retention.ingestion.months / retention.final.months + with whatever the deploymentScript's Bicep parameters passed in. Bicep always resolves a value for an + optional parameter (defaulting to 13 if the caller didn't specify one), so the script cannot tell an + explicit redeploy value from a silently-defaulted one. A redeploy that omitted a previously-customized + retention value therefore silently reset it to the toolkit default, and the next purge pipeline run + aged out historical data older than the new (lower) cutoff -- oldest data first. + + The fix: never lower stored retention on redeploy. Growing retention is always safe; shrinking it has a + destructive, hard-to-reverse consequence (data purge), so the script now takes the max of the stored + value and the incoming one instead of overwriting unconditionally. +#> + +Describe 'HubsRetentionGuard' { + + BeforeAll { + $repoRoot = (Resolve-Path "$PSScriptRoot/../../../..").Path + $scriptPath = Join-Path $repoRoot 'src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Core/Copy-FileToAzureBlob.ps1' + $content = Get-Content -Path $scriptPath -Raw + } + + Context 'Never-shrink guard' { + + It 'Should take the max of stored and incoming ingestion retention' { + $content | Should -Match '\$json\.retention\.ingestion\.months\s*=\s*\[Math\]::Max\(\$json\.retention\.ingestion\.months,\s*\[Int32\]::Parse\(\$env:ingestionRetentionInMonths\)\)' ` + -Because 'a redeploy that omits an explicit retention value must not silently shrink stored retention and purge historical data (#2206)' + } + + It 'Should take the max of stored and incoming final retention' { + $content | Should -Match '\$json\.retention\.final\.months\s*=\s*\[Math\]::Max\(\$json\.retention\.final\.months,\s*\[Int32\]::Parse\(\$env:finalRetentionInMonths\)\)' ` + -Because 'a redeploy that omits an explicit retention value must not silently shrink stored retention and purge historical data (#2206)' + } + + It 'Should not unconditionally overwrite ingestion retention' { + $content | Should -Not -Match '\$json\.retention\.ingestion\.months\s*=\s*\[Int32\]::Parse\(\$env:ingestionRetentionInMonths\)\s*$' ` + -Because 'a direct assignment (rather than a max guard) was the source of the #2206 regression' + } + + It 'Should not unconditionally overwrite final retention' { + $content | Should -Not -Match '\$json\.retention\.final\.months\s*=\s*\[Int32\]::Parse\(\$env:finalRetentionInMonths\)\s*$' ` + -Because 'a direct assignment (rather than a max guard) was the source of the #2206 regression' + } + } + + Context 'First-run behavior unchanged' { + + It 'Should still seed ingestion retention from the parameter when no retention object exists yet' { + $content | Should -Match 'Add-Member -Name ingestion -Value \(ConvertFrom-Json "\{""months"":\$\(\$env:ingestionRetentionInMonths\)\}"\)' ` + -Because 'a brand-new settings.json has no stored value to protect, so the first deploy must still honor the requested retention' + } + + It 'Should still seed final retention from the parameter when no retention object exists yet' { + $content | Should -Match 'Add-Member -Name final -Value \(ConvertFrom-Json "\{""months"":\$\(\$env:finalRetentionInMonths\)\}"\)' ` + -Because 'a brand-new settings.json has no stored value to protect, so the first deploy must still honor the requested retention' + } + } +} diff --git a/src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Core/Copy-FileToAzureBlob.ps1 b/src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Core/Copy-FileToAzureBlob.ps1 index ffea9d454..195c7236a 100644 --- a/src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Core/Copy-FileToAzureBlob.ps1 +++ b/src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Core/Copy-FileToAzureBlob.ps1 @@ -125,13 +125,16 @@ else } # Set or update ingestion retention +# NOTE: Bicep always passes a value here (defaulting to 13 if the caller didn't specify one), so this script +# cannot tell an explicit redeploy value from a silently-defaulted one. Never lower retention on redeploy -- +# shrinking silently ages out historical data the next time the purge pipeline runs (#2206); growing is safe. if (!($json.retention.ingestion)) { $json.retention | Add-Member -Name ingestion -Value (ConvertFrom-Json "{""months"":$($env:ingestionRetentionInMonths)}") -MemberType NoteProperty } else { - $json.retention.ingestion.months = [Int32]::Parse($env:ingestionRetentionInMonths) + $json.retention.ingestion.months = [Math]::Max($json.retention.ingestion.months, [Int32]::Parse($env:ingestionRetentionInMonths)) } # Set or update raw retention @@ -144,14 +147,14 @@ else $json.retention.raw.days = [Int32]::Parse($env:rawRetentionInDays) } -# Set or update final retention +# Set or update final retention (never lower on redeploy -- see note above) if (!($json.retention.final)) { $json.retention | Add-Member -Name final -Value (ConvertFrom-Json "{""months"":$($env:finalRetentionInMonths)}") -MemberType NoteProperty } else { - $json.retention.final.months = [Int32]::Parse($env:finalRetentionInMonths) + $json.retention.final.months = [Math]::Max($json.retention.final.months, [Int32]::Parse($env:finalRetentionInMonths)) } # Updating settings