Skip to content
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

Add credential file store in the backup deletion controller. #5521

Conversation

blackpiglet
Copy link
Contributor

@blackpiglet blackpiglet commented Oct 31, 2022

Signed-off-by: Xun Jiang blackpiglet@gmail.com

Thank you for contributing to Velero!

Please add a summary of your change

Add credential store in backup deletion controller to support VSL credential.

I followed these steps of v1.10 Velero manual test to verify VSL credential feature.

  • Prepare workload as target to backup;
  • Modify the default credential cloud-credentials to wrong value.
  • Backup should fail.
  • Create a new secret with correct credentials, and patch BSL and VSL to use it.
# Handle VSL credential
kubectl create secret generic -n velero --from-file=/Users/jxun/Documents/credentials-velero-gcp vsl-credential
 
kubectl -n velero patch VolumeSnapshotLocation default -p '{"spec":{"credential":{"name":"vsl-credential", "key":"credentials-velero-gcp"}}}' --type=merge

# Handel BSL credential
kubectl create secret generic -n velero --from-file=/Users/jxun/Documents/credentials-velero-gcp bsl-credential
 
kubectl -n velero patch BackupStorageLocation default -p '{"spec":{"credential":{"name":"bsl-credential", "key":"credentials-velero-gcp"}}}' --type=merge

After BSL and VSL's credential modification, backup can be created successfully, but backup deletion would fail with something similar to "unable to read credential from environment variable GOOGLE_APPLICATION_CREDENTIALS".

This is caused by BackupDeletionController didn't have the secret credetial store to read secret. I didn't trigger this error during GCP supporting VSL credential PR, because BSL's credential is not modified during verification.

Does your change fix a particular issue?

Fixes #4862

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@blackpiglet blackpiglet self-assigned this Oct 31, 2022
@github-actions github-actions bot added Dependencies Pull requests that update a dependency file has-changelog has-unit-tests labels Oct 31, 2022
@blackpiglet blackpiglet requested review from ywk253100 and removed request for reasonerjt and qiuming-best October 31, 2022 08:58
Signed-off-by: Xun Jiang <blackpiglet@gmail.com>
@blackpiglet blackpiglet force-pushed the add-credential-store-in-back-deletion-controller branch from cf348f5 to 41fc641 Compare October 31, 2022 08:59
@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2022

Codecov Report

Merging #5521 (41fc641) into main (150570f) will decrease coverage by 0.06%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##             main    #5521      +/-   ##
==========================================
- Coverage   40.74%   40.68%   -0.07%     
==========================================
  Files         238      238              
  Lines       20553    20559       +6     
==========================================
- Hits         8375     8364      -11     
- Misses      11563    11578      +15     
- Partials      615      617       +2     
Impacted Files Coverage Δ
pkg/cmd/server/server.go 6.57% <0.00%> (-0.02%) ⬇️
pkg/controller/backup_deletion_controller.go 57.26% <42.85%> (-0.26%) ⬇️
pkg/uploader/provider/kopia.go 52.84% <0.00%> (-4.07%) ⬇️
pkg/restore/restore.go 64.16% <0.00%> (-0.57%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Lyndon-Li Lyndon-Li merged commit 502b058 into vmware-tanzu:main Nov 1, 2022
@blackpiglet blackpiglet deleted the add-credential-store-in-back-deletion-controller branch March 10, 2023 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dependencies Pull requests that update a dependency file has-changelog has-unit-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand BSL multiple credentials support to VolumeSnapshotLocations
4 participants