-
Notifications
You must be signed in to change notification settings - Fork 242
fix(hubs): never lower retention on redeploy #2288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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' | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth spelling out the consequence in the docs: after this, stored retention can only ever grow. A customer who deliberately wants to lower retention to cut storage cost has no supported path — redeploying with a smaller value is now a no-op, silently. That's the correct default, but it needs an escape hatch or at least a documented manual one ("edit |
||
| } | ||
|
|
||
| # Set or update raw retention | ||
|
|
@@ -144,14 +147,14 @@ else | |
| $json.retention.raw.days = [Int32]::Parse($env:rawRetentionInDays) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I grepped
Two options: scope the title/description to the settings.json path, or open a follow-up for the raw policy. Either is fine, but it shouldn't go unrecorded. |
||
| } | ||
|
|
||
| # 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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two negative assertions (this one and the
finalequivalent on line 44) can never fail, so the regression they're guarding is unprotected.$here is end-of-string —-matchdoesn't setRegexOptions.Multiline— and the trailing\s*can't span the code that follows the assignment. So the pattern only matches if that assignment happens to be the last thing in the file. I checked against a sample with the regression pattern deliberately present mid-file:Both tests pass today, and they'd still pass if someone reverted the fix. Dropping
\s*$from both patterns fixes it.Minor, while you're here: these are source-text assertions, and the repo's convention for those is
Tests/Lint/(KqlJoinKinds.Tests.ps1,HubsKqlOperators.Tests.ps1) rather thanTests/Unit/.