Add credentials to volume snapshot locations. #4864
Conversation
|
We'll target this to post-v1.9 |
|
@sseago I found the comment by Bridget:
So will it cause problems for some cloud providers if we allow them to set different credentials for BSL and VSL? |
I don't believe so. She's only saying that there's no need for multiple VSLs to coexist in the same cluster for the same provider. However, BSL and VSL can definitely have different credentials from each other. It could be that the default credentials are the wrong credentials for the VSL, so we want VSL credentials. Also, some users may want to manage all of their credentials in the same way, never creating the default secret but always setting credentials when they create a VSL or a BSL. |
| Config flag.Map | ||
| Labels flag.Map | ||
| Credential flag.Map | ||
| secretName string |
There was a problem hiding this comment.
Looks like VSL's create and set command's Options are different.
secretName and secretKey are used as internal values to keep Credential's secret name and key.
Could these two parameters also removed in create command?
There was a problem hiding this comment.
@blackpiglet I've gone through the BSL credentials commits after the initial implementation (some refactoring and validation changes) -- making similar changes here removes these Options fields, as they're not really necessary.
|
I've gone through the BSL credentials commits after the initial implementation (some refactoring and validation changes) -- making similar changes here. |
|
@sseago Thanks, the code looks good to me. |
pkg/volume/snapshotlocation.go
Outdated
|
|
||
| // If the VSL specifies a credential, add credential file path to config | ||
| func UpdateVolumeSnapshotLocationWithCredentialConfig(location *velerov1api.VolumeSnapshotLocation, credentialStore credentials.FileStore, logger logrus.FieldLogger) error { | ||
| // add the bucket name and prefix to the config map so that object stores |
pkg/volume/snapshotlocation.go
Outdated
| velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" | ||
| ) | ||
|
|
||
| // If the VSL specifies a credential, add credential file path to config |
There was a problem hiding this comment.
Could you follow the convention to change the comment to be like // UpdateVolumeSnapshotLocationWithCredentialConfig .....
sseago
force-pushed
the
vsl-creds-main
branch
from
28e8ff0
to
2a7a65c
Compare
|
@reasonerjt PR updated with comment fixes and doc updates. |
| @@ -175,9 +175,9 @@ Follow the below troubleshooting steps to confirm that Velero is using the corre | |||
| <Output should be your credentials> | |||
| ``` | |||
|
|
|||
| ### Troubleshooting `BackupStorageLocation` credentials | |||
| ### Troubleshooting `BackupStorageLocation` and `VolumeSnapshotLocation credentials | |||
There was a problem hiding this comment.
Missed right back-tick quota mark.
There was a problem hiding this comment.
Good catch. Will fix.
There was a problem hiding this comment.
@blackpiglet Updated
This is analogous to the BSL creds work that was done previously, but for VSLs instead. Signed-off-by: Scott Seago <sseago@redhat.com>
sseago
force-pushed
the
vsl-creds-main
branch
from
2a7a65c
to
596114b
Compare
This is analogous to the BSL creds work that was done previously, but for VSLs instead.
Originally the per-BSL credentials support was intended to be added to VolumeSnapshotLocations as well. Some initial work was already done for this but was reverted when the VSL work was de-scoped from a prior release (the AWS plugin work to support this remained in place, though). This issue is for going though with the enhancement.
Signed-off-by: Scott Seago sseago@redhat.com
Please add a summary of your change
Does your change fix a particular issue?
Fixes #4862
Please indicate you've done the following:
/kind changelog-not-requiredas a comment on this pull request.site/content/docs/main.