Add credential field to the bsl #3190

carlisia · 2020-12-15T21:13:35Z

Partially closes #2425.

Notes:

I made this optional, is this what we want? If not, we can instruct ppl to set it up with the currently hard coded info
I couldn't think of any validation to add, let me know if there's any you can think of

Signed-off-by: Carlisia carlisia@vmware.com

carlisia · 2020-12-15T21:21:47Z

Not sure if this PR should be scoped to what is in it right now (populating the BSL with the secret), or to push the secret info down to the plugin.

zubron · 2020-12-15T23:06:19Z

Not sure if this PR should be scoped to what is in it right now (populating the BSL with the secret), or to push the secret info down to the plugin.

I don't think we came to a decision on how to make the secret data available to the plugins (at least that's what I am updating the design doc with) so I think what is here is good for now!

zubron · 2020-12-15T23:11:04Z

I made this optional, is this what we want? If not, we can instruct ppl to set it up with the currently hard coded info

I think this will need to be optional as we currently support the --no-secret option for providers using node based auth (e.g. AWS kube2iam)

I couldn't think of any validation to add, let me know if there's any you can think of

If you try to create a BSL with a secret that doesn't exist, are any errors thrown when creating/updating the BSL? It's not clear to me if the API will do some validation by using that Secret selector type. I can't think of any other validation that would be needed at the point of adding it. I assume the credential will be checked for validity at the point when we try to use it like our current approach with credentials.

carlisia · 2020-12-15T23:36:40Z

If you try to create a BSL with a secret that doesn't exist, are any errors thrown when creating/updating the BSL?

This would be a good validation. But we are letting ppl create BSLs w/o any type of validation as to if it's a valid store, with it's connection and all. So I say let's add that validation to the BSL controller.

It's probably best to add that at the time when we add the code to use it (in the PR that will do the passing of the secret to the plugin). It will become part of the validation for the BSL, in the controller: does the secret exist? Yes? Then instantiate the store passing all the current info + the secret, if there is one (*passing TBD). If we get back an instance, then mark the BSL as valid.

pkg/apis/velero/v1/backupstoragelocation_types.go

pkg/cmd/cli/backuplocation/set.go

nrb · 2020-12-16T21:22:31Z

I made this optional, is this what we want? If not, we can instruct ppl to set it up with the currently hard coded info

Yes, new fields should be optional as much as possible in order to retain backwards compatibility.

pkg/cmd/cli/backuplocation/create.go

nrb · 2020-12-16T21:32:56Z

pkg/cmd/cli/backuplocation/set.go

 }

 func (o *SetOptions) BindFlags(flags *pflag.FlagSet) {
+	flags.Var(&o.Credential, "credential", "Sets the one credential to be used by this location in key-value pair, where key is the secret name, and value is the secret key name. Optional.")


Hm, I wonder if we could have shared string consts for these help messages. I wouldn't say redo them all in this PR, but maybe it's something we could work towards, as repeating this in 2 files seems prone to error.

nrb · 2020-12-16T21:40:58Z

If not, we can instruct ppl to set it up with the currently hard coded info

We could also populate this info at startup for 1-2 releases.

carlisia · 2020-12-18T21:18:08Z

Comments have been addressed, ready for review.

jenting

TBH, if the secret key is fixed to use cloud before, we could simplify passing the secret name to the --credential flag.

carlisia · 2021-01-20T19:49:38Z

TBH, if the secret key is fixed to use cloud before, we could simplify passing the secret name to the --credential flag.

@jenting that was before. There is a new change (#2403 and 8e96722#diff-1c259cca045064227710d0f341606e8ba73adda92dc126782acf62e2667af5baR44-R55) that will require the user to pass the secret name and the key name, this way we can have multiple secrets.

carlisia · 2021-01-20T19:51:46Z

This PR is still ready for review. This is a blocker for #3299 (multiple secrets).

pkg/cmd/cli/backuplocation/set.go

zubron

Thanks @carlisia! This is great, and has worked as expected for the POC 👍 I just have one suggestion based on the comments already made around the validation of the flags.

zubron · 2021-01-25T15:24:20Z

pkg/cmd/cli/backuplocation/create.go

@@ -148,6 +152,16 @@ func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error {
 		validationFrequency = &metav1.Duration{Duration: o.ValidationFrequency}
 	}

+	if len(o.Credential.Data()) > 1 {


I think it would be better if we could add this check into the Validate function above. We don't seem to have an equivalent function for the set command so that would be good to add and use as the place to check in the other file.

Since the set and create commands have separate Options types, we'll likely have to split the Credentials field out somehow to have a shared validation if that's what we want. But I otherwise agree on having a Validate function that does this!

When I made this comment, introducing an equivalent Validate function was all I had in mind so I'm happy with that approach!

Nolan and I paired and looking into merging some of the code from create and set, and it did not make sense.

config/crd/bases/velero.io_backupstoragelocations.yaml

nrb · 2021-02-05T22:04:42Z

#3409 is the same change on the volume snapshot side. We should make sure they're using the same approach aside from types. I think I modeled @zubron's comment with the request for checks in the Validate function, though I know I have some other things to change, still.

zubron

Thank you!

Signed-off-by: Carlisia <carlisia@vmware.com>

carlisia · 2021-02-09T21:27:10Z

@vmware-tanzu/velero-owners this release blocker is ready for review.

carlisia added the Area/CLI related to the command-line interface label Dec 15, 2020

github-actions bot assigned carlisia Dec 15, 2020

carlisia requested review from ashish-amarnath, zubron, nrb and dsu-igeek December 15, 2020 21:19

carlisia mentioned this pull request Dec 15, 2020

Improve the velero backup-location command #2425

Closed

3 tasks

jenting reviewed Dec 16, 2020

View reviewed changes

pkg/apis/velero/v1/backupstoragelocation_types.go Outdated Show resolved Hide resolved

pkg/cmd/cli/backuplocation/set.go Show resolved Hide resolved

nrb suggested changes Dec 16, 2020

View reviewed changes

github-actions bot requested review from jenting and nrb December 18, 2020 21:09

jenting reviewed Dec 19, 2020

View reviewed changes

zubron mentioned this pull request Jan 19, 2021

POC: Load and pass credentials to plugins #3299

Closed

jenting reviewed Jan 21, 2021

View reviewed changes

pkg/cmd/cli/backuplocation/set.go Show resolved Hide resolved

zubron suggested changes Jan 25, 2021

View reviewed changes

nrb added kind/release-blocker Must fix issues for the coming release (milestone) P3 - Wouldn't it be nice if... labels Feb 3, 2021

nrb reviewed Feb 5, 2021

View reviewed changes

config/crd/bases/velero.io_backupstoragelocations.yaml Show resolved Hide resolved

nrb added P1 - Important and removed P3 - Wouldn't it be nice if... labels Feb 9, 2021

github-actions bot requested review from jenting, nrb and zubron February 9, 2021 21:01

zubron previously approved these changes Feb 9, 2021

View reviewed changes

Carlisia added 5 commits February 9, 2021 13:11

Add credential field to the bsl

9dbb8b6

Signed-off-by: Carlisia <carlisia@vmware.com>

Add changelog

4b2aae3

Signed-off-by: Carlisia <carlisia@vmware.com>

Fix typo

60cbac1

Signed-off-by: Carlisia <carlisia@vmware.com>

Better help messages and validation check

b7c2f2d

Signed-off-by: Carlisia <carlisia@vmware.com>

Better validation

930508b

Signed-off-by: Carlisia <carlisia@vmware.com>

carlisia dismissed zubron’s stale review via 930508b February 9, 2021 21:19

carlisia force-pushed the c-bsl-credential branch from 5aa980a to 930508b Compare February 9, 2021 21:19

github-actions bot requested a review from zubron February 9, 2021 21:19

nrb approved these changes Feb 9, 2021

View reviewed changes

jenting approved these changes Feb 10, 2021

View reviewed changes

jenting merged commit 7a2fea0 into vmware-tanzu:main Feb 10, 2021

carlisia deleted the c-bsl-credential branch February 10, 2021 05:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add credential field to the bsl #3190

Add credential field to the bsl #3190

carlisia commented Dec 15, 2020 •

edited

Loading

carlisia commented Dec 15, 2020

zubron commented Dec 15, 2020

zubron commented Dec 15, 2020

carlisia commented Dec 15, 2020 •

edited

Loading

nrb commented Dec 16, 2020

nrb Dec 16, 2020

nrb commented Dec 16, 2020

carlisia commented Dec 18, 2020

jenting left a comment

carlisia commented Jan 20, 2021

carlisia commented Jan 20, 2021

zubron left a comment

zubron Jan 25, 2021

nrb Feb 9, 2021

zubron Feb 9, 2021

carlisia Feb 9, 2021

nrb commented Feb 5, 2021 •

edited

Loading

zubron left a comment

carlisia commented Feb 9, 2021

Add credential field to the bsl #3190

Add credential field to the bsl #3190

Conversation

carlisia commented Dec 15, 2020 • edited Loading

carlisia commented Dec 15, 2020

zubron commented Dec 15, 2020

zubron commented Dec 15, 2020

carlisia commented Dec 15, 2020 • edited Loading

nrb commented Dec 16, 2020

nrb Dec 16, 2020

Choose a reason for hiding this comment

nrb commented Dec 16, 2020

carlisia commented Dec 18, 2020

jenting left a comment

Choose a reason for hiding this comment

carlisia commented Jan 20, 2021

carlisia commented Jan 20, 2021

zubron left a comment

Choose a reason for hiding this comment

zubron Jan 25, 2021

Choose a reason for hiding this comment

nrb Feb 9, 2021

Choose a reason for hiding this comment

zubron Feb 9, 2021

Choose a reason for hiding this comment

carlisia Feb 9, 2021

Choose a reason for hiding this comment

nrb commented Feb 5, 2021 • edited Loading

zubron left a comment

Choose a reason for hiding this comment

carlisia commented Feb 9, 2021

carlisia commented Dec 15, 2020 •

edited

Loading

carlisia commented Dec 15, 2020 •

edited

Loading

nrb commented Feb 5, 2021 •

edited

Loading