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 design doc for Custom CA support for S3 BSLs and Velero Installation #2259

Merged
merged 3 commits into from Feb 27, 2020

Conversation

dymurray
Copy link
Contributor

Signed-off-by: Dylan Murray dymurray@redhat.com

Design for #1027

Signed-off-by: Dylan Murray <dymurray@redhat.com>
additional field to the `BackupStorageLocation`'s provider `Config` resource to
provide a secretRef which will contain the coordinates to a secret containing
the relevant cert file for object storage. Then, in order for Restic to be able
to consume and use this cert, Velero will need to modify the Restic daemonset
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an accepted path for Velero? It feels odd to me to update the daemonset for Restic on the fly when creating BSLs as this could interrupt some activity Restic is undertaking.

However, without modifying the restic daemonset I don't see a feasible way to get the custom CA bundle into Restic at runtime. In our PoC we had to use an initcontainer on Restic which would symlink a secret that is mounted into the Restic container. This would allow us to modify a secret from Velero and have the associated changes populated in the Restic container. I do not think that's a viable approach moving forward though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One idea: take a look at Velero's code for managing restic repo credentials, i.e. the encryption key. The way this works today is that the key is stored in a secret in the velero namespace, and each time we exec a restic command, we read the contents of the secret, write it out to a temp file, pass the path of the temp file to restic, and then remove the temp file afterwards. This has worked pretty well in practice*.

We might be able to take a similar approach for the CA bundles, where they're stored in secrets, the BSL has a reference to them, and they're projected into a temp file just-in-time for restic invocations.

Here are a few links to relevant sections of the current code:

https://github.com/vmware-tanzu/velero/blob/master/pkg/restic/repository_manager.go#L238-L245
https://github.com/vmware-tanzu/velero/blob/master/pkg/restic/common.go#L168-L203

  • as an aside, yes, this is over-complicated given that we're currently using a static/non-secret encryption key for restic repos, but this was implemented before we decided to go with that approach instead of using unique, secret keys per repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skriss thanks a lot for the links I definitely think we could follow a similar pattern for the CA bundle here since restic allows for a --cacert command flag which we can specify like we do the password file here: https://github.com/vmware-tanzu/velero/blob/master/pkg/restic/command.go#L86.

as an aside, yes, this is over-complicated given that we're currently using a static/non-secret encryption key for restic repos

Makes sense. I have faced similar problems like this in the past and it seems like this is a common pattern used to solve the issue of needing data stored somewhere in memory of a pod that a controller will manage.

I will try something like this out and update the document.

@carlisia carlisia added the Area/Design Design Documents label Feb 11, 2020
Custom Resource to allow a user to specify a custom CA bundle. This mechanism
needs to also allow Restic to access and use this custom CA bundle.

## Goals
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this support adding the certs on the client side? I know velero client-side commands such as velero (backup|restore) (describe|logs) on AWS/GCP/Azure get a 1-time use URL for retrieving files from object store, and on Minio they use a BSL's Spec.Config.publicUrl value in order to get non-encrypted values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It currently does not but I wouldn't rule that out. Are you thinking that the velero install command would install the cert on the client's machine? Or that the velero client commands would have a new flag --cacert which a user can optionally specify?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure, honestly. I think either would work, so long as it's documented. A --cacert flag would probably be easier since certs can be managed in several different ways depending on client OS.

Mostly, I'd like to avoid users hitting issues like this one often. #2256

@dymurray dymurray changed the title Add design doc for Custom CA support for S3 BSLs Add design doc for Custom CA support for S3 BSLs and Velero Installation Feb 17, 2020
@skriss
Copy link
Member

skriss commented Feb 24, 2020

@dymurray just checking in to see if you were able to try out the restic approach per #2259 (comment), and if you need any other input!

@dymurray
Copy link
Contributor Author

@skriss I was on PTO for the better part of last week. Have begun putting together the PoC for this this morning. Hoping to have it confirmed by the community meeting tomorrow and can update the design doc.

@dymurray
Copy link
Contributor Author

Can confirm that the approach of using the methods shown in the repositoryManager code will work for injecting the CA bundles into the restic pod. Updating the design doc to reflect this and will poke for review.

Update design to write CA bundle to a temp file
Include information about client flag

Signed-off-by: Dylan Murray <dymurray@redhat.com>
@dymurray
Copy link
Contributor Author

@skriss @nrb Please take a look when you get a free chance. I have added information about including a way to configure a cert in the client and to include the updates about using a temp file similar to the restic repostiory credentials.

Thanks!

Since the Velero client is responsible for gathering logs and information about
the Object Storage, this implementation should include a new flag `--cacert`
which can be used when communicating with the Object Storage. Additionally, the
user should be able to set this in their client configuration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To put more detail on it, I would probably put it under velero client config set cacert PATH if someone would like to permanently set the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx will add this as a note


### Install command changes

The installation flags should be updated to include the ability to pass in a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carlisia This is relevant to #2202. My guess would probably be part of the plugin add command there, with a new option for certs that would create the secret and do the updating on the deployment, and restic DaemonSet. I think this is only valid for ObjectStore plugins, at least the restic DaemonSet part. I'm envisioning the command would take a path to the cert file, create the Secret in the cluster, then put a secretRef in the Velero Deployment/restic DaemonSet.

We'd also need to have the backup-location (set|create) commands have an argument to take a reference to a secret name (which is the cert) in the same namespace. Possibly the snapshot-location (set|create) commands, if it's relevant to them. The command would create or edit the BSL (or VSL) to have a reference to the Secret within the same namespace.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy that! I'll add it to the doc, thank you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it will work the same way as with the credentials for the provider (as far as adding it to the cluster).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think it'll work the same way, it's just a Secret with different contents.

Signed-off-by: Dylan Murray <dymurray@redhat.com>
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dymurray no further comments/questions, this LGTM! Thanks again for putting this together.

Copy link
Contributor

@nrb nrb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but going to give @carlisia one more chance to review as I know she was interested in this, too.

@ron1
Copy link

ron1 commented Feb 27, 2020

Note that on Linux it appears the restic cacert can be set using golang standard pki environment variables as described here: https://go-review.googlesource.com/c/go/+/36093/. In particular env var SSL_CERT_FILE is known to work in the daemonset.

Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 🎉

I will include this new flag/info in the new CLI design.

@carlisia carlisia merged commit a9f0726 into vmware-tanzu:master Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area/Design Design Documents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants