Skip to content

Sync eng/common directory with azure-sdk-tools for PR 14973 #46199

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.

Sign up for GitHub

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

Open
azure-sdk wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Sync eng/common directory with azure-sdk-tools for PR 14973 #46199

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d71c231
adding support for soft deleted blobs and blob versions during resour…
ibrandes Apr 7, 2026
c4b3abc
Apply suggestions from code review
ibrandes Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 47 additions & 6 deletions eng/common/scripts/Helpers/Resource-Helpers.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,11 +434,17 @@ function RemoveStorageAccount($Account) {

try {
foreach ($container in $containers) {
$blobs = $container | Get-AzStorageBlob
foreach ($blob in $blobs) {
$shouldDelete = EnableBlobDeletion -Blob $blob -Container $container -StorageAccountName $Account.StorageAccountName -ResourceGroupName $Account.ResourceGroupName
if ($shouldDelete) {
$deleteNow += $blob
# VLW containers need version-aware cleanup: soft-delete causes deleted blobs to linger
# as non-current versions that block container deletion. See Remove-VlwContainerBlobs.
if (($container | Get-Member 'BlobContainerProperties') -and $container.BlobContainerProperties.HasImmutableStorageWithVersioning) {
Remove-VlwContainerBlobs -Container $container -StorageAccountName $Account.StorageAccountName -ResourceGroupName $Account.ResourceGroupName
} else {
Comment on lines +439 to +441
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get-Member 'BlobContainerProperties' will emit an error when that member doesn’t exist; in runs where $ErrorActionPreference = 'Stop' (common in eng scripts), this can terminate the cleanup unexpectedly. Use a non-throwing property-existence check (e.g., PSObject properties) or add -ErrorAction SilentlyContinue to Get-Member and ensure the condition remains reliable.

Copilot uses AI. Check for mistakes.
$blobs = $container | Get-AzStorageBlob
foreach ($blob in $blobs) {
$shouldDelete = EnableBlobDeletion -Blob $blob -Container $container -StorageAccountName $Account.StorageAccountName -ResourceGroupName $Account.ResourceGroupName
if ($shouldDelete) {
$deleteNow += $blob
}
}
}
}
Expand Down Expand Up @@ -524,6 +530,41 @@ function EnableBlobDeletion($Blob, $Container, $StorageAccountName, $ResourceGro
return $forceBlobDeletion
}

# In VLW (Versioned-Level WORM) containers with soft-delete enabled, deleting a blob creates a
# non-current version instead of truly removing it. A standard Get-AzStorageBlob listing can't
# see these leftovers, but they still block container deletion (409 Conflict on the management
# plane DELETE). Listing with -IncludeVersion -IncludeDeleted makes them visible so we can clear
# immutability policies / legal holds and delete each version individually. Multiple passes handle
# new non-current versions that surface after each round of deletions.
function Remove-VlwContainerBlobs($Container, $StorageAccountName, $ResourceGroupName) {
Write-Host "Cleaning VLW container '$($Container.Name)' versions and soft-deleted blobs in account '$StorageAccountName', group: $ResourceGroupName"

for ($round = 0; $round -lt 5; $round++) {
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry bound 5 is a magic number. Consider making it a named constant or a parameter (defaulting to 5) so callers can tune the behavior and future maintainers understand/adjust the limit without editing logic.

Suggested change
for ($round = 0; $round -lt 5; $round++) {
$maxCleanupPasses = 5
for ($round = 0; $round -lt $maxCleanupPasses; $round++) {

Copilot uses AI. Check for mistakes.
$found = $false
$blobs = @($Container | Get-AzStorageBlob -IncludeVersion -IncludeDeleted -ErrorAction SilentlyContinue)

foreach ($blob in $blobs) {
$found = $true

# Unconditionally clear legal holds and immutability policies. Errors are expected for
# soft-deleted blobs or blobs that don't have these set.
try { $blob | Set-AzStorageBlobLegalHold -DisableLegalHold | Out-Null } catch { }
try { $blob | Remove-AzStorageBlobImmutabilityPolicy | Out-Null } catch { }
Comment on lines +551 to +552
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set-AzStorageBlobLegalHold commonly does not support a -DisableLegalHold switch (many Az.Storage versions use a dedicated Clear-AzStorageBlobLegalHold cmdlet instead). Because the catch is empty, a parameter-binding failure here would be silently ignored, leaving legal holds in place and preventing deletions. Use the correct cmdlet/parameter set for clearing legal holds (and consider surfacing unexpected failures).

Copilot uses AI. Check for mistakes.
try {
$blob | Remove-AzStorageBlob -Force
} catch {
# Deleting the current version by version ID returns 403
# (OperationNotAllowedOnRootBlob); fall back to base blob deletion.
try {
Remove-AzStorageBlob -Container $Container.Name -Blob $blob.Name -Context $Container.Context -Force
} catch { }
}
Comment on lines +549 to +561
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple empty catch { } blocks make it very hard to distinguish expected/benign failures (e.g., missing immutability policy) from real problems (auth/permission issues, missing modules/parameters, API regressions). Consider narrowing what you suppress (e.g., only specific known error cases) and emitting at least Write-Verbose/Write-Warning for unexpected exceptions so failures don’t silently leave containers partially undeleted.

Copilot uses AI. Check for mistakes.
}

if (-not $found) { break }
}
}

function DoesSubnetOverlap([string]$ipOrCidr, [string]$overlapIp) {
[System.Net.IPAddress]$overlapIpAddress = $overlapIp
$parsed = $ipOrCidr -split '/'
Expand All @@ -543,4 +584,4 @@ function DoesSubnetOverlap([string]$ipOrCidr, [string]$overlapIp) {
}

return $baseIp.Address -eq ($overlapIpAddress.Address -band ([System.Net.IPAddress]$mask).Address)
}
}
Loading