Pass configured BSL credential to plugin via config #3442

zubron · 2021-02-10T18:13:18Z

Please add a summary of your change

Update NewObjectBackupStore to take a CredentialsGetter which can be
used to get the credentials for a BackupStorageLocation if it has been
configured with a Credential. If the BSL has a credential, use that
SecretKeySelector to fetch the secret, write the contents to a temp file
and then pass that file through to the plugin via the config map using
the key credentialsFile. This relies on the plugin being able to use
this new config field.

This does not yet handle ResticRepositories.

The documentation for this feature will be updated in the follow up
PR that adds support for Restic.

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

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.

nrb

On a first pass, this looks reasonable to me, but I want to re-review when I have a bit more dedicated time. I have a couple comments, but I don't think they're huge.

nrb · 2021-02-22T18:45:20Z

pkg/credentials/file_store.go

+	}
+
+	keyFilePath := filepath.Join(n.fsRoot, fmt.Sprintf("%s-%s", selector.Name, selector.Key))
+	file, err := n.fs.OpenFile(keyFilePath, os.O_RDWR|os.O_CREATE, 0644)


Since this is key data and folks may use sidecars with Velero, is it possible to set the permissions to 0600?

I'm going to do some further tests this afternoon so I'll try that out, but I initially opted to use 0644 as those are the permissions of the file mounted in from the cloud-credentials secret.

nrb · 2021-02-22T18:46:22Z

pkg/credentials/file_store.go

+}
+
+func (n *namespacedFileStore) Get(selector *corev1.SecretKeySelector) (string, error) {
+	creds, err := kube.GetSecretKey(n.client, n.namespace, selector)


Would it make sense to look for an existing file on disk before fetching the key? Or do we always fetch in order to ensure that the key doesn't get stale in case of updates?

I thought about doing that but opted to always fetch to avoid the credentials being stale. Thankfully we're using the kubebuilder client here so there is caching built in to avoid spamming the API server.

My intention was that we could use this approach for now but could always optimise it in future. I experimented with creating a controller that would watch for and serialize any secrets from the velero namespace and update them upon change but I didn't think that I could guarantee that the latest version of a secret would always be available. It seemed safest to always refresh it before use.

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 is 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]>

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]>

carlisia

Excellent job with this @zubron. I looked at this closely and have no comment on the logic, only style change requests. 👍

pkg/controller/backup_storage_location_controller.go

pkg/controller/download_request_controller.go

pkg/credentials/file_store.go

pkg/credentials/file_store_test.go

pkg/persistence/object_store.go

pkg/persistence/object_store_test.go

pkg/cmd/server/server.go

zubron · 2021-02-26T16:25:53Z

Thanks for the review and feedback, @carlisia! I've addressed your comments, so this is ready for review again :)

carlisia

Great work! 👍

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]>

Update NewObjectBackupStore to take a CredentialsGetter which can be used to get the credentials for a BackupStorageLocation if it has been configured with a Credential. If the BSL has a credential, use that SecretKeySelector to fetch the secret, write the contents to a temp file and then pass that file through to the plugin via the config map using the key `credentialsFile`. This relies on the plugin being able to use this new config field. This does not yet handle VolumeSnapshotLocations or ResticRepositories. Signed-off-by: Bridget McErlean <[email protected]>

Add godocs and comments. Improve formatting and test names. Signed-off-by: Bridget McErlean <[email protected]>

jenting

The rest LGTM

pkg/restic/common.go

jenting · 2021-03-03T04:27:33Z

pkg/persistence/object_store_test.go

@@ -678,6 +696,73 @@ func TestNewObjectBackupStoreGetter(t *testing.T) {
 	}
 }

+// TestNewObjectBackupStoreGetterConfig runs the NewObjectBackupStoreGetter constructor and ensures
+// that it initializes the ObjectBackupStore with the correct config.
+func TestNewObjectBackupStoreGetterConfig(t *testing.T) {


Would it be good to put these test cases under TestNewObjectBackupStoreGetter?

I thought about that but the logic and set up for these checks seemed different enough that I opted to have a separate test.

internal/credentials/file_store.go

internal/credentials/file_store_test.go

pkg/builder/backup_storage_location_builder.go

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

nrb

Looks good! 🎉

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]>

* 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 #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]>

* Load credentials and pass to ObjectStorage plugins Update NewObjectBackupStore to take a CredentialsGetter which can be used to get the credentials for a BackupStorageLocation if it has been configured with a Credential. If the BSL has a credential, use that SecretKeySelector to fetch the secret, write the contents to a temp file and then pass that file through to the plugin via the config map using the key `credentialsFile`. This relies on the plugin being able to use this new config field. This does not yet handle VolumeSnapshotLocations or ResticRepositories. Signed-off-by: Bridget McErlean <[email protected]> * Address code reviews Add godocs and comments. Improve formatting and test names. Signed-off-by: Bridget McErlean <[email protected]> * Address code reviews 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]>

* Load credentials and pass to ObjectStorage plugins Update NewObjectBackupStore to take a CredentialsGetter which can be used to get the credentials for a BackupStorageLocation if it has been configured with a Credential. If the BSL has a credential, use that SecretKeySelector to fetch the secret, write the contents to a temp file and then pass that file through to the plugin via the config map using the key `credentialsFile`. This relies on the plugin being able to use this new config field. This does not yet handle VolumeSnapshotLocations or ResticRepositories. Signed-off-by: Bridget McErlean <[email protected]> * Address code reviews Add godocs and comments. Improve formatting and test names. Signed-off-by: Bridget McErlean <[email protected]> * Address code reviews 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]>

* Load credentials and pass to ObjectStorage plugins Update NewObjectBackupStore to take a CredentialsGetter which can be used to get the credentials for a BackupStorageLocation if it has been configured with a Credential. If the BSL has a credential, use that SecretKeySelector to fetch the secret, write the contents to a temp file and then pass that file through to the plugin via the config map using the key `credentialsFile`. This relies on the plugin being able to use this new config field. This does not yet handle VolumeSnapshotLocations or ResticRepositories. Signed-off-by: Bridget McErlean <[email protected]> * Address code reviews Add godocs and comments. Improve formatting and test names. Signed-off-by: Bridget McErlean <[email protected]> * Address code reviews 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 the has-unit-tests label Feb 10, 2021

zubron force-pushed the pass-credentials-file-config-for-bsls branch 3 times, most recently from 3d29528 to bb31046 Compare February 10, 2021 18:20

zubron changed the title ~~Pass credentials file config for bsls~~ Pass configured credential to plugin via config Feb 10, 2021

zubron changed the title ~~Pass configured credential to plugin via config~~ Pass configured BSL credential to plugin via config Feb 10, 2021

zubron force-pushed the pass-credentials-file-config-for-bsls branch 2 times, most recently from 44accb3 to c1bba0e Compare February 20, 2021 20:51

github-actions bot added the has-changelog label Feb 20, 2021

zubron marked this pull request as ready for review February 20, 2021 20:52

github-actions bot requested review from ashish-amarnath, carlisia, dsu-igeek, jenting and nrb February 20, 2021 20:52

github-actions bot assigned zubron Feb 20, 2021

zubron force-pushed the pass-credentials-file-config-for-bsls branch 2 times, most recently from 36ac9ef to 5c9fe63 Compare February 20, 2021 21:13

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

zubron added this to the v1.6.0 milestone Feb 22, 2021

nrb reviewed Feb 22, 2021

View reviewed changes

zubron mentioned this pull request Feb 22, 2021

Use Credential from BSL for restic commands #3489

Merged

3 tasks

carlisia suggested changes Feb 25, 2021

View reviewed changes

carlisia previously approved these changes Feb 26, 2021

View reviewed changes

zubron dismissed carlisia’s stale review via 1a5be9e March 2, 2021 17:12

zubron force-pushed the pass-credentials-file-config-for-bsls branch from b2ab816 to 1a5be9e Compare March 2, 2021 17:12

github-actions bot requested a review from carlisia March 2, 2021 17:12

Address code reviews

fbe4c8c

Add godocs and comments. Improve formatting and test names. Signed-off-by: Bridget McErlean <[email protected]>

zubron force-pushed the pass-credentials-file-config-for-bsls branch from 1a5be9e to fbe4c8c Compare March 2, 2021 21:34

jenting reviewed Mar 3, 2021

View reviewed changes

internal/credentials/file_store.go Outdated Show resolved Hide resolved

internal/credentials/file_store_test.go Outdated Show resolved Hide resolved

pkg/builder/backup_storage_location_builder.go Outdated Show resolved Hide resolved

Address code reviews

8d68c89

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

github-actions bot requested a review from jenting March 4, 2021 19:14

nrb approved these changes Mar 4, 2021

View reviewed changes

carlisia approved these changes Mar 4, 2021

View reviewed changes

carlisia merged commit b9a8c0b into vmware-tanzu:main Mar 4, 2021

zubron mentioned this pull request Mar 12, 2021

Allow Velero to support multiple credentials per storage provider #3304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass configured BSL credential to plugin via config #3442

Pass configured BSL credential to plugin via config #3442

zubron commented Feb 10, 2021 •

edited

Loading

nrb left a comment

nrb Feb 22, 2021

zubron Feb 22, 2021

nrb Feb 22, 2021

zubron Feb 22, 2021

zubron Feb 22, 2021

carlisia left a comment

zubron commented Feb 26, 2021

carlisia left a comment

jenting left a comment

jenting Mar 3, 2021

zubron Mar 4, 2021 •

edited

Loading

nrb left a comment

Pass configured BSL credential to plugin via config #3442

Pass configured BSL credential to plugin via config #3442

Conversation

zubron commented Feb 10, 2021 • edited Loading

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

nrb left a comment

Choose a reason for hiding this comment

nrb Feb 22, 2021

Choose a reason for hiding this comment

zubron Feb 22, 2021

Choose a reason for hiding this comment

nrb Feb 22, 2021

Choose a reason for hiding this comment

zubron Feb 22, 2021

Choose a reason for hiding this comment

zubron Feb 22, 2021

Choose a reason for hiding this comment

carlisia left a comment

Choose a reason for hiding this comment

zubron commented Feb 26, 2021

carlisia left a comment

Choose a reason for hiding this comment

jenting left a comment

Choose a reason for hiding this comment

jenting Mar 3, 2021

Choose a reason for hiding this comment

zubron Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

nrb left a comment

Choose a reason for hiding this comment

zubron commented Feb 10, 2021 •

edited

Loading

zubron Mar 4, 2021 •

edited

Loading