Use Credential from BSL for restic commands #3489

zubron · 2021-02-22T00:55:04Z

Please add a summary of your change

This change introduces support for restic to make use of per-BSL
credentials. It makes use of the credentials.FileStore introduced in
PR #3442 to write the BSL credentials to disk. To support per-BSL
credentials for restic, the environment for the restic commands needs to
be modified for each provider to ensure that the credentials are
provided via the correct provider specific environment variables.
This change introduces a new function restic.CmdEnv to check the BSL
provider and create the correct mapping of environment variables for
each provider.

Previously, AWS and GCP could rely on the environment variables in the
Velero deployments to obtain the credentials file, but now these
environment variables need to be set with the path to the serialized
credentials file if a credential is set on the BSL.

For Azure, the credentials file in the environment was loaded and parsed
to set the environment variables for restic. Now, we check if the BSL
has a credential, and if it does, load and parse that file instead.

This change also introduces a few other small improvements. Now that we
are fetching the BSL to check for the Credential field, we can use the
BSL directly to get the CACert which means that we can remove the
GetCACert function. Also, now that we have a way to serialize secrets
to disk, we can use the credentials.FileStore to get a temp file for
the restic repo password and remove the restic.TempCredentialsFile
function.

Does your change fix a particular issue?

Part of fixing #3304.

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.
Updated the corresponding documentation in site/content/docs/main.

zubron · 2021-02-24T00:31:53Z

This PR depends on #3442 but the restic specific commit can be reviewed: 9ccc9eb

I've tested this locally with a Tilt setup with a BSL with custom creds created for each provider (AWS, Azure, GCP) and was able to successfully backup and restore a pod with restic volumes from each. I'm going to focus on docs and getting E2E tests ready.

nrb · 2021-03-09T16:00:12Z

This PR depends on #3442 but the restic specific commit can be reviewed: 9ccc9eb

3442 has now merged; can this be taken out of draft?

zubron · 2021-03-09T16:06:48Z

I am working on an E2E test for this change, but can add that in a follow up PR or it can be reviewed as a separate commit on this PR.

I've marked this as ready for review now, so the main code changes and docs can be reviewed as those are ready.

zubron · 2021-03-09T16:28:47Z

@a-mccarthy Could you please review the docs changes in this PR? Thank you!

This change adds an E2E test which exercises the mulitple credentials feature added in vmware-tanzu#3489. The test creates a secret from the given credentials and creates a BackupStorageLocation which uses those credentials. A backup and restore is then performed to the default BSL and to the newly created BSL. This change adds new flags to the E2E test suite to configure the BSL created and used in the test. Signed-off-by: Bridget McErlean <[email protected]>

zubron · 2021-03-10T21:29:11Z

Thanks for the great feedback, @a-mccarthy! I think I've addressed most of your comments but this will likely need more improvements. If you have further comments, I'll make issues from those and address them in a follow up PR so that we can get the code changes in and start doing some testing for the 1.6 release.

This change adds an E2E test which exercises the mulitple credentials feature added in vmware-tanzu#3489. The test creates a secret from the given credentials and creates a BackupStorageLocation which uses those credentials. A backup and restore is then performed to the default BSL and to the newly created BSL. This change adds new flags to the E2E test suite to configure the BSL created and used in the test. Signed-off-by: Bridget McErlean <[email protected]>

carlisia

This looks solid and I appreciate the refactorings. I have only ridiculously minor requests.

Questions:

was this tested with the 3 providers we maintain? I didn't run manual tests.
did you think through any possible backwards compatibility issues?

Also, yes, any e2e test can be added in a new PR imo.

carlisia · 2021-03-11T02:00:58Z

site/content/docs/main/locations.md

- Velero only supports a single set of credentials *per provider*. It's not yet possible to use different credentials for different locations, if they're for the same provider.
+- Velero supports multiple credentials for `BackupStorageLocations`, allowing you to specify the credentials to use with any `BackupStorageLocation`.
+  However, use of this feature requires support within the plugin for the object storage provider you wish to use.
+  All [plugins provided by the Velero team][5] support this feature.


s/provided/maintained

carlisia · 2021-03-11T02:06:50Z

site/content/docs/main/locations.md

+
+#### Prerequisites
+- This feature requires support from the [object storage provider plugin][5] you wish to use.
+  All plugins provided by the Velero team support this feature.


s/provided/maintained

carlisia · 2021-03-11T02:12:17Z

site/content/docs/main/locations.md

+velero backup-location get
+```
+
+To use this new `BackupStorageLocation` when performing a backup, use the flag `--storage-location <bsl-name>` when running `velero backup create`.


It'd be good to also add something like:

Likewise, you may set this new BackupStorageLocation as the default BSL with the command velero backup-location set --default <bsl-name>.

Yes! I had forgotten about that aspect of it and remembered while preparing #3561. It's now on my list of things to add :)

carlisia · 2021-03-11T02:15:59Z

site/content/docs/main/troubleshooting.md

+
+   If the logs from the Velero deployment contain the error message `"config has invalid keys credentialsFile"`, the version of your object storage plugin does not yet support multiple credentials.
+
+   The object storage plugins [provided by the Velero team][11] support this feature, so please update your plugin to the latest version if you see the above error message.


/provided/maintained

carlisia · 2021-03-11T13:47:30Z

pkg/restic/common.go

-	if err != nil {
-		return nil, errors.Wrap(err, "error getting aws restic env vars")
+	switch backendType {
+	case AWSBackend:


I love this refactoring.

carlisia · 2021-03-11T14:34:03Z

pkg/restic/exec_commands.go

-	cmd.CACertFile = caCertFile
-
-	stdout, stderr, err := exec.RunCommand(cmd.Cmd())
+func GetSnapshotID(snapshotIdCmd *Command) (string, error) {


Minor: there's no longer tags being passed in and also this function signature changed enough that making a change to the godoc would probably be useful.

a-mccarthy · 2021-03-11T15:57:25Z

site/content/docs/main/locations.md

+[5]: /plugins
+[6]: overview-plugins.md
+[7]: https://kubernetes.io/docs/concepts/configuration/secret/
+[8]: #create-a-storage-location-that-uses-unique-credentials


I dont think the links with the # are working correctly, i can't tell if this is bc there is an issue with the preview, or they will also not work on the live site?

Ahhh, I totally thought these worked 🤦‍♀️ okay, let me figure out how to fix this!

site/content/docs/main/locations.md

a-mccarthy · 2021-03-11T16:09:52Z

@zubron LGTM! Great write up! i had a few comments, but they are relatively minor. Please let me know if you want to me look at anything else :)

Signed-off-by: Bridget McErlean <[email protected]>

carlisia

👍

zubron · 2021-03-11T23:45:22Z

Thanks, @carlisia, these are great questions.

was this tested with the 3 providers we maintain? I didn't run manual tests.

Yep, I tested this locally against all the 3 plugins that I made changes for. I did that testing as part of the testing for those plugins, and have run the newly added E2E test with Azure and AWS. I will do another test with GCP for completeness.

did you think through any possible backwards compatibility issues?

My focus with backwards compatibility was to ensure that we were continuing to set the same environment variables for each restic command. I compared the set of env vars before and after my change and checked they were the same (but with new values for the new credentials files). Previously, all restic commands used the environment from os.Environ with the provider specific vars for restic added. Here, we are also using os.Environ but replacing the provider specific vars that would usually be set within the Velero container spec.

For example, in the AWS case, AWS_SHARED_CREDENTIALS_FILE is already set, as it is defined in the container spec and wouldn't be modified as part of getting the S3 specific env vars. Now, we override that env var for the restic command based on the contents of the Credential field in the BSL. If it is not set, we rely on the original value defined in the container environment. Note that overriding these env vars is specific to that particular restic command instance. We do not override the env vars for the main Velero process. (There is an exception here with setting the env for Azure, as it involves overriding environment variables in the current process but that is existing behaviour and is not changed by this PR. It is something that will need to be fixed in future though if we move to parallel backups/restores.)

@carlisia Let me know if you have any further questions or concerns! Thanks for bringing these up.

carlisia · 2021-03-12T00:01:41Z

Thanks @zubron!

This change adds an E2E test which exercises the mulitple credentials feature added in vmware-tanzu#3489. The test creates a secret from the given credentials and creates a BackupStorageLocation which uses those credentials. A backup and restore is then performed to the default BSL and to the newly created BSL. This change adds new flags to the E2E test suite to configure the BSL created and used in the test. Signed-off-by: Bridget McErlean <[email protected]>

* Use Credential from BSL for restic commands This change introduces support for restic to make use of per-BSL credentials. It makes use of the `credentials.FileStore` introduced in PR vmware-tanzu#3442 to write the BSL credentials to disk. To support per-BSL credentials for restic, the environment for the restic commands needs to be modified for each provider to ensure that the credentials are provided via the correct provider specific environment variables. This change introduces a new function `restic.CmdEnv` to check the BSL provider and create the correct mapping of environment variables for each provider. Previously, AWS and GCP could rely on the environment variables in the Velero deployments to obtain the credentials file, but now these environment variables need to be set with the path to the serialized credentials file if a credential is set on the BSL. For Azure, the credentials file in the environment was loaded and parsed to set the environment variables for restic. Now, we check if the BSL has a credential, and if it does, load and parse that file instead. This change also introduces a few other small improvements. Now that we are fetching the BSL to check for the `Credential` field, we can use the BSL directly to get the `CACert` which means that we can remove the `GetCACert` function. Also, now that we have a way to serialize secrets to disk, we can use the `credentials.FileStore` to get a temp file for the restic repo password and remove the `restic.TempCredentialsFile` function. Signed-off-by: Bridget McErlean <[email protected]> * Add documentation for per-BSL credentials Signed-off-by: Bridget McErlean <[email protected]> * Address review feedback Signed-off-by: Bridget McErlean <[email protected]> * Address review comments Signed-off-by: Bridget McErlean <[email protected]>

This change adds an E2E test which exercises the mulitple credentials feature added in vmware-tanzu#3489. The test creates a secret from the given credentials and creates a BackupStorageLocation which uses those credentials. A backup and restore is then performed to the default BSL and to the newly created BSL. This change adds new flags to the E2E test suite to configure the BSL created and used in the test. Signed-off-by: Bridget McErlean <[email protected]>

* Use Credential from BSL for restic commands This change introduces support for restic to make use of per-BSL credentials. It makes use of the `credentials.FileStore` introduced in PR vmware-tanzu#3442 to write the BSL credentials to disk. To support per-BSL credentials for restic, the environment for the restic commands needs to be modified for each provider to ensure that the credentials are provided via the correct provider specific environment variables. This change introduces a new function `restic.CmdEnv` to check the BSL provider and create the correct mapping of environment variables for each provider. Previously, AWS and GCP could rely on the environment variables in the Velero deployments to obtain the credentials file, but now these environment variables need to be set with the path to the serialized credentials file if a credential is set on the BSL. For Azure, the credentials file in the environment was loaded and parsed to set the environment variables for restic. Now, we check if the BSL has a credential, and if it does, load and parse that file instead. This change also introduces a few other small improvements. Now that we are fetching the BSL to check for the `Credential` field, we can use the BSL directly to get the `CACert` which means that we can remove the `GetCACert` function. Also, now that we have a way to serialize secrets to disk, we can use the `credentials.FileStore` to get a temp file for the restic repo password and remove the `restic.TempCredentialsFile` function. Signed-off-by: Bridget McErlean <[email protected]> * Add documentation for per-BSL credentials Signed-off-by: Bridget McErlean <[email protected]> * Address review feedback Signed-off-by: Bridget McErlean <[email protected]> * Address review comments Signed-off-by: Bridget McErlean <[email protected]>

This change adds an E2E test which exercises the mulitple credentials feature added in vmware-tanzu#3489. The test creates a secret from the given credentials and creates a BackupStorageLocation which uses those credentials. A backup and restore is then performed to the default BSL and to the newly created BSL. This change adds new flags to the E2E test suite to configure the BSL created and used in the test. Signed-off-by: Bridget McErlean <[email protected]>

* Use Credential from BSL for restic commands This change introduces support for restic to make use of per-BSL credentials. It makes use of the `credentials.FileStore` introduced in PR vmware-tanzu#3442 to write the BSL credentials to disk. To support per-BSL credentials for restic, the environment for the restic commands needs to be modified for each provider to ensure that the credentials are provided via the correct provider specific environment variables. This change introduces a new function `restic.CmdEnv` to check the BSL provider and create the correct mapping of environment variables for each provider. Previously, AWS and GCP could rely on the environment variables in the Velero deployments to obtain the credentials file, but now these environment variables need to be set with the path to the serialized credentials file if a credential is set on the BSL. For Azure, the credentials file in the environment was loaded and parsed to set the environment variables for restic. Now, we check if the BSL has a credential, and if it does, load and parse that file instead. This change also introduces a few other small improvements. Now that we are fetching the BSL to check for the `Credential` field, we can use the BSL directly to get the `CACert` which means that we can remove the `GetCACert` function. Also, now that we have a way to serialize secrets to disk, we can use the `credentials.FileStore` to get a temp file for the restic repo password and remove the `restic.TempCredentialsFile` function. Signed-off-by: Bridget McErlean <[email protected]> * Add documentation for per-BSL credentials Signed-off-by: Bridget McErlean <[email protected]> * Address review feedback Signed-off-by: Bridget McErlean <[email protected]> * Address review comments Signed-off-by: Bridget McErlean <[email protected]>

github-actions bot added has-changelog has-unit-tests labels Feb 22, 2021

zubron force-pushed the provider-specific-env-for-restic branch 7 times, most recently from 4d39e6f to 9ccc9eb Compare February 23, 2021 12:35

zubron force-pushed the provider-specific-env-for-restic branch from 9ccc9eb to 5074e6d Compare March 1, 2021 18:32

zubron added P1 - Important kind/release-blocker Must fix issues for the coming release (milestone) labels Mar 2, 2021

zubron added this to the v1.6.0 milestone Mar 2, 2021

zubron force-pushed the provider-specific-env-for-restic branch from 5074e6d to f5e1027 Compare March 4, 2021 22:09

zubron force-pushed the provider-specific-env-for-restic branch from f5e1027 to 4fa1390 Compare March 9, 2021 16:05

zubron marked this pull request as ready for review March 9, 2021 16:05

github-actions bot added the Documentation label Mar 9, 2021

github-actions bot requested review from ashish-amarnath and jenting March 9, 2021 16:05

github-actions bot assigned zubron Mar 9, 2021

zubron force-pushed the provider-specific-env-for-restic branch from 4fa1390 to 0e31d1f Compare March 9, 2021 16:22

zubron requested a review from a-mccarthy March 9, 2021 16:28

github-actions bot requested review from carlisia, nrb and dsu-igeek March 9, 2021 17:09

zubron mentioned this pull request Mar 10, 2021

Add E2E test for multiple credentials #3559

Merged

3 tasks

carlisia requested a review from a-mccarthy March 11, 2021 01:44

carlisia suggested changes Mar 11, 2021

View reviewed changes

a-mccarthy reviewed Mar 11, 2021

View reviewed changes

site/content/docs/main/locations.md Outdated Show resolved Hide resolved

a-mccarthy previously approved these changes Mar 11, 2021

View reviewed changes

Address review comments

10f5789

Signed-off-by: Bridget McErlean <[email protected]>

zubron dismissed a-mccarthy’s stale review via 10f5789 March 11, 2021 17:11

zubron requested a review from carlisia March 11, 2021 17:12

carlisia approved these changes Mar 11, 2021

View reviewed changes

nrb approved these changes Mar 11, 2021

View reviewed changes

nrb merged commit 9ffffda into vmware-tanzu:main Mar 11, 2021

This was referenced Mar 12, 2021

Links with fragments are not rendered on Hugo site #3566

Closed

Allow Velero to support multiple credentials per storage provider #3304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Credential from BSL for restic commands #3489

Use Credential from BSL for restic commands #3489

zubron commented Feb 22, 2021 •

edited

Loading

zubron commented Feb 24, 2021

nrb commented Mar 9, 2021

zubron commented Mar 9, 2021

zubron commented Mar 9, 2021

zubron commented Mar 10, 2021

carlisia left a comment

carlisia Mar 11, 2021

carlisia Mar 11, 2021

carlisia Mar 11, 2021

zubron Mar 11, 2021

carlisia Mar 11, 2021

carlisia Mar 11, 2021

carlisia Mar 11, 2021

a-mccarthy Mar 11, 2021

zubron Mar 11, 2021

a-mccarthy commented Mar 11, 2021

carlisia left a comment

zubron commented Mar 11, 2021

carlisia commented Mar 12, 2021


		If the logs from the Velero deployment contain the error message `"config has invalid keys credentialsFile"`, the version of your object storage plugin does not yet support multiple credentials.

		The object storage plugins [provided by the Velero team][11] support this feature, so please update your plugin to the latest version if you see the above error message.

Use Credential from BSL for restic commands #3489

Use Credential from BSL for restic commands #3489

Conversation

zubron commented Feb 22, 2021 • edited Loading

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

zubron commented Feb 24, 2021

nrb commented Mar 9, 2021

zubron commented Mar 9, 2021

zubron commented Mar 9, 2021

zubron commented Mar 10, 2021

carlisia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-mccarthy commented Mar 11, 2021

carlisia left a comment

Choose a reason for hiding this comment

zubron commented Mar 11, 2021

carlisia commented Mar 12, 2021

zubron commented Feb 22, 2021 •

edited

Loading