Support copying namespace-scoped secrets for backup PVC provisioning#9920
Support copying namespace-scoped secrets for backup PVC provisioning#9920shubham-pampattiwar wants to merge 7 commits into
Conversation
👷 Deploy request for velero pending review.Visit the deploys page to approve it
|
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Add a SecretNames field to the BackupPVC type to allow users to specify secrets that need to be copied from the source PVC namespace to the Velero namespace before creating the backup PVC. This is needed for CSI drivers that require namespace-scoped secrets for volume provisioning, such as encrypted volumes with KMS. Fixes velero-io#9879 Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Add CopySecret, DeleteSecretIfAny, and DeleteSecretsWithLabel utilities for copying namespace-scoped secrets to the Velero namespace during datamover backup PVC creation. CopySecret handles three cases: - Secret does not exist in target: copies it with a tracking label - Secret exists with same data: no-op (same source namespace) - Secret exists with different data: returns ErrSecretCollision so the caller can requeue Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Copy configured secrets from the source namespace to the Velero namespace in the New phase of the DataUpload reconcile loop, before calling Expose(). This is done in the controller rather than the exposer because Expose() errors are non-retryable (marked as permanent failure), while the controller can requeue on collision. On secret collision (same name, different data from another DataUpload), the controller requeues with a 5s delay, matching the existing pattern used for VGDP constraint checking. Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Add label-based secret cleanup in CleanUp() to delete any secrets that were copied to the Velero namespace for backup PVC provisioning. Uses the velero.io/backup-pvc-secret label to find secrets associated with the DataUpload being cleaned up. Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
0f91d56 to
7e046b9
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- Fix import ordering in test file (gofmt) - Add nolint:gosec for BackupPVCSecretLabel constant (not a credential) - Use assert.Error instead of assert.True(err != nil) (testifylint) Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
7e046b9 to
c4cba79
Compare
| // Copy secrets required for backup PVC provisioning (e.g., encrypted volumes with KMS). | ||
| // This must happen before Expose() since Expose() errors are non-retryable. | ||
| // On collision (same secret name, different data from another DataUpload), requeue. | ||
| if du.Spec.CSISnapshot != nil { |
There was a problem hiding this comment.
This will bring a contest among all the cluster nodes, because the DU is not taken by any nodes yet.
There was a problem hiding this comment.
@Lyndon-Li Good catch. I'll move the secret copy after acceptDataUpload() so only the accepting node handles it. Does that work for you?
There was a problem hiding this comment.
Once a DU is accepted, there is no way to return it back and retry.
There was a problem hiding this comment.
@Lyndon-Li Since the DU can't be retried after accept, for the collision case (same secret name, different data from concurrent cross-tenant backups) the DataUpload would fail with a clear error message and the user can retry.
The collision is a narrow edge case -- it requires concurrent encrypted backups from different namespaces using the same secret name with different values. For the common cases (single tenant, same-namespace concurrent, sequential backups), there's no collision.
If we want automatic retry for the collision case, we'd need to handle it in the Accepted phase. Would you prefer we keep it simple with a fail + retry for now, or should we add the Accepted phase handling?
There was a problem hiding this comment.
Also, if you have a different approach in mind for handling this, I'm open to suggestions.
There was a problem hiding this comment.
The collision is a narrow edge case -- it requires concurrent encrypted backups from different namespaces using the same secret name with different values. For the common cases (single tenant, same-namespace concurrent, sequential backups), there's no collision.
For sure, I prefer a simpler approach AS LONG AS it could meet the majority cases.
However, for this judgement, I am not fully convinced --- multiple tenant is a common case, concurrent backup of multiple volumes among multiple nodes are very common. The only thing I am not sure is whether it is a common case that different tenants/namespaces would use different secrets.
There was a problem hiding this comment.
@Lyndon-Li This could be a documented exception. Secrets for encrypted volumes should be unique cluster-wide to avoid possible failures with concurrent backups.
There was a problem hiding this comment.
@Lyndon-Li @sseago Agreed with Scott's suggestion. We can document that for concurrent multi-tenant encrypted backups, secret names should be unique per namespace. On the ceph-csi side, this is already supported via the tenantTokenName configuration in the KMS config.
So the approach would be:
- Secret copy after accept, fail on collision with a clear error message
- Document that users with concurrent cross-tenant encrypted backups should configure unique secret names per namespace
Does that work for you?
- Add ConfigMapNames field to BackupPVC config for copying tenant configmaps (e.g., ceph-csi-kms-config with Vault connection overrides) - Add CopyConfigMap, DeleteConfigMapIfAny, DeleteConfigMapsWithLabel utilities mirroring the secret copy functions - Move secret/configmap copy after acceptDataUpload() so only the accepting node handles it, avoiding multi-node contest - Clean up copied configmaps in CleanUp() alongside secrets Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Summary
Add support for copying namespace-scoped secrets and configmaps from the source PVC namespace to the Velero namespace during datamover backup PVC creation. This enables backup of CSI volumes that use encrypted StorageClasses requiring namespace-scoped secrets and configmaps (e.g., ODF/ceph-csi with Vault KMS).
New
secretNamesandconfigMapNamesfields inbackupPVCConfigallow users to specify which resources should be copied:{ "backupPVC": { "ocs-storagecluster-ceph-rbd-encrypted": { "secretNames": ["ceph-csi-kms-token"], "configMapNames": ["ceph-csi-kms-config"] } } }The copy happens in the DataUpload controller after
acceptDataUpload(), so only the accepting node handles it:velero.io/backup-pvc-secret)CleanUp()using label-based lookup after the DataUpload completesDoes your change fix a particular issue?
Fixes #9879
Please indicate you've done the following:
make new-changelog) or comment/kind changelog-not-requiredon this PR.site/content/docs/main.