Reject restores that reference a backup in the Deleting phase#9868
Closed
RoySerbi wants to merge 2 commits into
Closed
Reject restores that reference a backup in the Deleting phase#9868RoySerbi wants to merge 2 commits into
RoySerbi wants to merge 2 commits into
Conversation
When a backup's Status.Phase is Deleting, its underlying data (object store contents and volume snapshots) may be concurrently removed by the backup deletion controller. Previously validateAndComplete only checked that the backup existed and its BSL was available, so a restore created against a being-deleted backup was accepted and could fail mid-execution or produce incomplete output. Add a phase check after fetchBackupInfo so such restores fail validation with a clear error. The schedule path is unaffected because it already selects only Completed backups. Fixes velero-io#9790 Assisted-by: Claude (Anthropic) Signed-off-by: RoyS <229624717+RoySerbi@users.noreply.github.com>
👷 Deploy request for velero pending review.Visit the deploys page to approve it
|
Signed-off-by: RoyS <229624717+RoySerbi@users.noreply.github.com>
Author
|
Closing as a duplicate of #9792, which already addresses #9790 (and more comprehensively, covering all non-usable backup phases rather than only |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please add a summary of your change
When a
Backup'sStatus.PhaseisDeleting, its underlying data (object store contents, PV snapshots, pod volume snapshots) may be concurrently removed by the backup deletion controller. PreviouslyvalidateAndComplete(pkg/controller/restore_controller.go) only verified that the backup existed and that itsBackupStorageLocationwas available — it never checkedBackup.Status.Phase. As a result, aRestorecreated viaspec.backupNameagainst a being-deleted backup was accepted and could begin executing against data that is mid-deletion, leading to a restore that fails part-way through or produces incomplete/corrupt output while still being treated as valid.This PR adds a phase check immediately after
fetchBackupInfosucceeds: if the referenced backup is in theDeletingphase, the restore is rejected with a clear validation error and set toFailedValidation:The
spec.scheduleNamepath is unaffected, becausemostRecentCompletedBackupalready filters toCompletedbackups.A table-driven test case was added to
TestRestoreReconcilecovering this scenario (restore referencing aDeleting-phase backup →FailedValidationwith the expected error).Does your change fix a particular issue?
Fixes #9790
Please indicate you've done the following:
make new-changelog) or comment/kind changelog-not-requiredon this PR.site/content/docs/main. — Not applicable; this is an internal restore-validation rule with no user-facing documentation surface.AI assistance disclosure: This change was prepared with AI assistance (Claude, Anthropic). I have personally reviewed and tested it, and I take full responsibility for the contribution. The fix was developed test-first: the new test fails on
main(the deleting-phase backup is wrongly accepted and the restore proceeds to download backup contents) and passes after the validation check is added.go vetandgofmtare clean and the existingpkg/controllerunit tests continue to pass.