Move the resourcemodifiers package from internal/ to pkg/ so that external
plugins, such as Restore Item Action (RIA) plugins, can import and reuse
Velero's built-in resource modifier implementation.
Due to Go's internal package visibility restrictions, external plugins
cannot import packages located under internal/. Relocating resourcemodifiers
to pkg/ allows plugins to evaluate and apply resource modifier rules to
in-memory resource objects prior to validation or restore execution,
ensuring consistent behavior with the Velero server without duplicate code.
Update all internal import paths and add an unreleased changelog entry.
Fixes#10668
Signed-off-by: Adam Zhang <adam.zhang@broadcom.com>
Comment why unix.BLKZEROOUT must be used instead of a literal, and add a
flushZeroBlocks test with a non-zero prefilled file proving the write
fallback zeroes only the requested range.
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-Authored-By: Claude Code (model: Claude Sonnet 5.5) <noreply@anthropic.com>
Other code in the kopia repository uses its logger without a nil check,
so drop the check and give the test a logger to cover the path.
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-Authored-By: Claude Code (model: Claude Opus 5.5) <noreply@anthropic.com>
Deleting a kopia snapshot refreshes the repo when the snapshot is not
found, and failed when it was still not found. When a backup deletion
was interrupted after the snapshot was deleted, e.g. by a Velero server
restart, every retry of the deletion failed with "snapshot not found",
so the backup could never be deleted.
A snapshot that is still not found after the refresh is already deleted,
so return success.
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
blkZeroOut hard coded BLKZEROOUT as 0x127b, which is BLKPBSZGET. The
ioctl therefore succeeded on block devices without zeroing anything, so
the conservative fallback that writes zero buffers never ran and block
data mover restores left the previous content in place for zero ranges
of the backup.
Take the request number from golang.org/x/sys/unix, which also gets the
arch specific value right (e.g. 0x2000127f on ppc64le).
Add a test that zeroes a range of a loop device (runs as root only) and
checks the type and number encoded in the request.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
velero backup logs and velero restore logs check the phase before they
create a DownloadRequest, and only go ahead for Completed,
PartiallyFailed, Failed, WaitingForPluginOperations and
WaitingForPluginOperationsPartiallyFailed. Finalizing and
FinalizingPartiallyFailed are not on either list, so both commands
answer "logs ... are not available until it's finished processing" for
a backup or restore in those phases.
The log is already in object storage by then. The backup controller
writes it in persistBackup (PutBackup uploads the log first) before the
Backup is patched out of InProgress, and the restore controller calls
PutRestoreLog before it sets the WaitingForPluginOperations or
Finalizing phase. Neither finalizer controller writes the log again,
and the DownloadRequest controller signs URLs in both phases. Finalizing
also comes after WaitingForPluginOperations, which both commands
already accept, so a log that could be read while plugin operations
were running becomes unreadable again until the finalizer is done.
For restores that window is not short. The restore finalizer waits up
to the resource timeout for restored PVCs to bind before patching their
PVs, and waits for post-restore exec hooks, so a restore can stay in
Finalizing for minutes, and a restore stuck there is when its log is
most needed.
Add the two Finalizing phases to both allowlists. The new table tests
create a backup or restore in each phase and check that the phases with
a log get as far as requesting it, while New, InProgress and
FailedValidation are still refused.
Signed-off-by: Max Freedom Pollard <272618364+MaxFreedomPollard@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Fix restore resource priorities aliasing that can skip restoring resources
Priorities.Set returned HighPriorities and LowPriorities as slices of
one backing array when the value had a "-" in the middle, leaving
HighPriorities with spare capacity over the separator and the low list.
getOrderedResources appended to HighPriorities, so a backup with between
2 and 1+len(low) unprioritized resource types overwrote the low list in
place. That could drop a backed-up resource type from the restore order,
and because the server's priorities are shared by every restore, the
corrupted order persisted for later restores.
- Set: cap HighPriorities with full slice expressions.
- getOrderedResources: build a new slice instead of appending to the
caller's.
- String: build a fresh slice instead of appending to HighPriorities.
Signed-off-by: abd-ulbasit <basit@basit.engineer>
* Add changelog for #10665
Signed-off-by: abd-ulbasit <basit@basit.engineer>
---------
Signed-off-by: abd-ulbasit <basit@basit.engineer>
The GCS Go client returns "storage: object doesn't exist" (ErrObjectNotExist)
when the backup tarball is already gone, but isTarballNotFoundError() only
checked for "objectnotexist". The error was treated as transient, so the
DeleteBackupRequest kept the error and the Backup stayed stuck in Deleting.
Add "doesn't exist" to the recognized not-found indicators, per the
maintainer's suggestion on the issue, and cover it with a unit test
including the wrapped gRPC-style error observed in practice.
Fixes#10659
Signed-off-by: sapnasingh4852-source <sapnasingh4852@gmail.com>
Introduce "AllowExistingVolumeMismatch" in restore to support bypassing same volume handle pre-flight checking
Signed-off-by: Wenkai Yin (尹文开) <wenkai.yin@broadcom.com>
This commit makes change to the code, scripts and some template files to
reflect the change of the org after donation to CNCF.
The files under "/site" "/changelogs" and "/design" are deliberately
excluded in this change. I'll make follow up changes in these
directories when needed.
Signed-off-by: Daniel Jiang <daniel.jiang@broadcom.com>
Update issue URLs in pkg/cmd/cli/bug/bug.go and GitHub issue templates from legacy vmware-tanzu/velero to velero-io/velero, and add unit test coverage for the bug CLI package.
Closes#10639
Signed-off-by: Abhayraj Jaiswal <abhayraj916146@gmail.com>
When an existing pod is still using the backed-up volumes, the in-place
restore pre-flight check reports an error but the restore of the pod
continued. Return right after reporting the error.
Signed-off-by: chlins <chlins.zhang@gmail.com>
Snapshot-based volume data is never restored into an existing volume and
the user only sees the generic "already exists" warning.
- CSI snapshot (with or without data movement): the volume data is
restored through the PVC, so warn when the PVC already exists.
- Native snapshot: the volume data is restored per PV, so warn when the
PV already exists and its snapshot is not restored.
Signed-off-by: chlins <chlins.zhang@gmail.com>
Pass caller-provided context.Context through cluster API queries during backup request preparation and snapshot location validation, ensuring context cancellation and deadlines are properly respected.
Closes#10620
Signed-off-by: Abhayraj Jaiswal <abhayraj916146@gmail.com>
* Fix Zip Slip path check accepting a sibling directory in archive extraction
sanitizeArchivePath confirmed an extracted tar entry stayed inside the
destination directory with strings.HasPrefix. HasPrefix has no separator
boundary, so a sibling directory whose name begins with the destination
directory's name - reached via a "../<destDir-name>-evil/x" entry -
satisfied the check and escaped the directory.
Verify containment with filepath.Rel instead, matching the check #10102
added to GetVersionedItemFilePath, rejecting a relative path equal to ".."
or under "../".
Fixes#10184
Signed-off-by: phatlc <phatle.hsd@gmail.com>
* Pin sanitizeArchivePath behaviour for entries naming destDir itself
An entry named "." or one with an empty name joins to destDir, so
filepath.Rel returns "." and the path is accepted. That is correct - it is
contained - but the table did not state it, and the returned path was not
asserted at all. Add the two rows and check targetPath on the accepted
cases so the containment boundary is pinned on both sides.
Signed-off-by: phatlc <phatle.hsd@gmail.com>
---------
Signed-off-by: phatlc <phatle.hsd@gmail.com>
* fix: defer VGS cleanup until backup finalization
The CSI PVC backup action deleted the VolumeGroupSnapshot and bound
VolumeGroupSnapshotContent after creating and waiting for all group
member VolumeSnapshots. At that point, Velero had not necessarily
completed processing the member VolumeSnapshots, CSI async operations,
or archive finalization.
Deleting the VGS triggered external-snapshotter member cleanup and could
remove VolumeSnapshot API objects still referenced by the Backup
workflow.
Remove immediate VGS/VGSC deletion from getVolumeSnapshotReference and
retain the group until the Backup reaches a terminal phase. Continue
processing VGS member VolumeSnapshots through the existing normal CSI
finalization path.
Add the velero.io/volume-group-snapshot-cleanup Backup finalizer before
VGS creation. Label VGSC resources with the Backup name and UID, enforce
DeletionPolicy=Retain before cleanup, and delete VGS/VGSC resources from
terminal Backup reconciliation only.
Make cleanup idempotent and retryable, including recovery of labeled
orphan VGSC resources. Remove the Backup finalizer only after VGS/VGSC
cleanup verification succeeds. Block Backup deletion while cleanup is
pending.
Add unit and controller integration tests covering finalizer
registration, VGS creation, terminal cleanup, failed-Backup cleanup,
orphan VGSC cleanup, and finalizer removal.
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
* add changelog
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
* Adress review feedback
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
* remove stale VGS finalizer during snapshot restore
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
* Improve codecoverage
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
* explain VGS member snapshot cleanup timing
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
* Second pass at review comment
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
* Third pass at review comment
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
* Dont swallow get errors
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
* Patch backup CR only when annotation is missing
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
---------
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
In invalidateBackupReposForBSL, pass the caller-provided context.Context to r.List and r.patchBackupRepository so that context cancellation and deadlines are properly respected during BackupRepository invalidation on BSL changes.
Closes#10604
Signed-off-by: Abhayraj Jaiswal <abhayraj916146@gmail.com>
* fix: regenerate CRD manifests for the status.activities field
PR #10552 ("Move the progress messages to activities") added a new
Activities []string field to PodVolumeBackupStatus, PodVolumeRestoreStatus,
DataUploadStatus, and DataDownloadStatus, but the CRD YAML manifests under
config/crd/{v1,v2alpha1}/bases were never regenerated to match -- so main's
installed CRDs have been silently out of sync with their own Go types since
that PR merged. Anything validating status writes against a strict/vendored
copy of these CRDs (e.g. an admission webhook, or the new --crd-schema-check
startup check in #9910) would reject a legitimate .status.activities write
as an unknown field.
Regenerated via: GOPATH=~/go bash hack/update-3generated-crd-code.sh
(controller-gen v0.16.5, matching go.mod's sigs.k8s.io/controller-tools pin).
No manual edits; the deepcopy diffs are the expected byproduct of the new
[]string field.
Co-authored-by: Hermes Agent <noreply@hermes-agent>
Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
* changelog: add entry for PR #10608
Co-authored-by: Hermes Agent <noreply@hermes-agent>
Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
---------
Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
Co-authored-by: Hermes Agent <noreply@hermes-agent>
* Migrate VolumeGroupSnapshot API from v1beta2 to v1
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
* Add changelog
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
* Detect VolumeGroupSnapshot API version at runtime
Velero hardcoded the VolumeGroupSnapshot (VGS) API version, so VGS backup and
restore failed on clusters that serve a different version than Velero expects.
For example OCP 5.0 serves only groupsnapshot.storage.k8s.io/v1 and OCP 4.22
serves only v1beta1, while Velero targeted a single version and failed with
"no matches for kind" on the others.
Detect the served VGS API version at runtime via the discovery client
(preferring v1 > v1beta2 > v1beta1) and perform all VGS, VGSClass and VGSContent
I/O through the dynamic client, converting to and from the canonical v1 typed
structs. The controller-runtime typed client cannot negotiate versions because
it sends the GVK of the Go type, so the dynamic client is used at the I/O
boundary instead. Owner-reference matching is now version-independent (Kind and
UID), since the CSI controller stamps refs with whatever version the cluster
serves.
This builds on the v1 migration and external-snapshotter client bump from the
previous commits.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
* Add changelog for VGS runtime version detection
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
* docs: VGS supports v1/v1beta2/v1beta1 via runtime detection
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
* Remove superseded changelog for #10600 (covered by #10601)
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
* Use crClient + unstructured for VGS access instead of a dynamic client
Address review feedback: rather than a dedicated VGSClient wrapping
dynamic.Interface + discovery.DiscoveryInterface, perform all VolumeGroupSnapshot
I/O through the existing controller-runtime client with *unstructured.Unstructured.
The object's GVK is stamped to the served version and controller-runtime routes it
via its RESTMapper, giving the same multi-version behavior (v1/v1beta2/v1beta1)
that the typed client cannot.
This removes the extra client plumbing: no constructor signature changes to
NewRestoreFinalizerReconciler or server.go, no Factory DynamicClient/DiscoveryClient
mock churn, and no separate fake client package. The version is resolved from the
client's RESTMapper (preferring v1 > v1beta2 > v1beta1), and the helpers convert
to and from the canonical v1 typed structs.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
---------
Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Co-authored-by: Prasad Joshi <prajoshi@redhat.com>
* Fix unchecked type assertion panic in ChangeImageNameAction
replaceImageName reads a restored container's image field out of the
unstructured object and asserts it to string without checking ok. The
comma-ok map lookup on the line above only confirms the "image" key is
present -- it says nothing about the value's type. A restored resource
whose image field is present but not a JSON string (e.g. a number,
bool, null, array, or object) causes an unrecovered
"interface conversion: interface {} is not string" panic in this
RestoreItemAction plugin whenever the optional image-remapping
ConfigMap feature is configured.
Switch to the comma-ok form of the assertion and skip (with a log
message) any container whose image field isn't a string, instead of
panicking.
Signed-off-by: Kaizhe Huang <derek0405@gmail.com>
* Add regression test for non-string image field panic
Covers the comma-ok assertion fix: replaceImageName operates on
generic unstructured content decoded from a backup tarball, which
isn't validated against the Pod schema before this code runs, so
"image" isn't guaranteed to be a string.
Signed-off-by: Kaizhe Huang <derek0405@gmail.com>
* Guard container-entry type assertion, use unstructured.NestedString
Addresses reviewer feedback: container.(map[string]any) was also an
unchecked assertion, and unstructured.NestedString gives safer,
more idiomatic type-checking than a manual comma-ok assertion. Also
switches the skip-path logging from Info to Warn per review.
Signed-off-by: Kaizhe Huang <derek0405@gmail.com>
---------
Signed-off-by: Kaizhe Huang <derek0405@gmail.com>
PodVolumeBackupStatus has no Node field, so the Node column was always
empty. The node is recorded in the spec.
Fixes#10444
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
This commit fixes the issue that when a namespace with label velero.io/exclude-from-backup: "true"
is added to the exludeNamespaces of a backup CR. There will be
duplicated entries of the namespace in the spec of the backup.
Signed-off-by: Daniel Jiang <daniel.jiang@broadcom.com>
* Fix backup-finalizer: do not set backup phase to Completed before PutBackupMetadata succeeds
Previously, the backup finalizer controller set backup.Status.Phase to
Completed/PartiallyFailed in-memory BEFORE calling PutBackupMetadata and
PutBackupContents. When these uploads failed (e.g., due to object lock
or immutability), the deferred patch function still wrote the terminal
phase to the Kubernetes API server, preventing the controller from
retrying the upload on the next reconcile.
This fix moves the phase assignment to AFTER both uploads succeed. A
DeepCopy of the backup is used to encode the JSON with the final phase
for object storage, while the in-memory backup object retains the
Finalizing phase until uploads complete.
Caveats:
- CompletionTimestamp is now captured before upload but only committed to
the API server after upload succeeds. On retry after a transient
failure, a new timestamp is generated, so the completion time reflects
when the upload finally succeeded rather than when finalization
processing completed.
- Metrics (RegisterBackupSuccess/RegisterBackupPartialFailure) are now
recorded after uploads succeed, so they accurately reflect only fully
persisted backups.
- The metadata uploaded to object storage contains the final phase and
completion timestamp via DeepCopy, so storage state is correct even
before the API server is patched.
Fixes#9645
Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Add changelog for #9646
Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Fix testifylint: use require.Error instead of assert.Error
Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Address review feedback on backup-finalizer fix
- Add default guard for unhandled phase values in finalPhase switch
- Add retry with DefaultBackoff for PutBackupMetadata per reviewer request
- Replace brittle framework.BackupItemActionResolverV2{} mock with mock.Anything
- Add FinalizingPartiallyFailed test case for PutBackupContents failure
Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Use bounded, object-storage-tuned backoff for backup-finalizer uploads
retry.DefaultBackoff is tuned for API server optimistic-concurrency
conflicts (4 steps, ~1.25s total) and gives up far too quickly for
object storage calls, which can see longer transient outages or
throttling (review feedback from blackpiglet). Replace it with a
dedicated, bounded backoff (1s base, 2x factor, 5 steps, ~31s total)
applied to both PutBackupMetadata and PutBackupContents.
Being bounded (rather than retrying forever) means a persistent
failure, e.g. an object-lock/immutability policy denying every write,
surfaces as an error within a bounded time instead of hanging the
reconcile indefinitely; controller-runtime requeues on error, so
retries continue across reconciles (review feedback from priyansh17).
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Fix PutBackupMetadata retry to re-read backupJSON each attempt
backupJSON is a bytes.Buffer, so passing it directly to
PutBackupMetadata drains it on the first read attempt. A retry after
a transient failure would then upload empty content instead of the
backup metadata. Wrap it in bytes.NewReader(backupJSON.Bytes()) inside
the retry closure so every attempt gets a fresh reader.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
---------
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Happy <yesreply@happy.engineering>
Update Request struct initialization in backup_test.go to use
SkippedVolumeTracker and NewSkipVolumeTracker following the rename
from SkippedPVTracker.
Signed-off-by: Adam Zhang <adam.zhang@broadcom.com>
lyndon-li
Adam Zhang
Lyndon-Li
Tiger KaovilaiandClaude Code
Max Freedom PollardandClaude Opus 5.5
Abdul Basit
Harshil-1603
sapnasingh4852-source
Wenkai Yin(尹文开)
Daniel Jiang
Abhayraj Jaiswal
Chlins Zhang
hsdfat
Prasad Joshi
Shubham PampattiwarandPrasad Joshi
Xun Jiang
Kaizhe Huang
Asish Kumar
Xun Jiang/Bruce Jiang
