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

charts: backup: flexible credentials #1248

Merged
merged 6 commits into from
Dec 4, 2019

Conversation

gregwebs
Copy link
Contributor

allow retrieving credentials from metadata on AWS and GCP

on GCP, stream the backup upload

What problem does this PR solve?

The current backup chart does not allow the recommended approach to getting credentials on AWS and GCP

What is changed and how does it work?

Don't require a k8s secret for credentials

Check List

  • Manually tested on GKE. A non-final version of this was tested on AWS.

Code changes

  • Has Helm charts change

Side effects

  • Should maintain backwards compatibility

Related changes

  • Need to cherry-pick to the release branch ?
  • docs: update the docs to indicate that k8s secrets are unnecessary.

Does this PR introduce a user-facing change?:

backup chart: allow retrieving credentials from metadata on AWS and GCP

allow retrieving credentials from the environment

on GCP, stream the backup upload
@tennix
Copy link
Member

tennix commented Nov 28, 2019

We have the backup controller which makes it much easier to do with backup and restore. Better to add this feature to the controller. /cc @onlymellb @DanielZhangQD

@gregwebs
Copy link
Contributor Author

yes, the backup controller has all these same issues. If we are going to have the chart in this repo, this PR should be accepted. Otherwise we should delete the chart from the repo.

@gregwebs
Copy link
Contributor Author

I suggest though that we may want to keep a chart for mydumper if we move the controller to the new br.

@tennix
Copy link
Member

tennix commented Nov 28, 2019

We aren't going to remove mydumper backup from the backup controller even if we have integrated the new BR.

Copy link
Contributor

@DanielZhangQD DanielZhangQD left a comment

Choose a reason for hiding this comment

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

I suggest though that we may want to keep a chart for mydumper if we move the controller to the new br.

The backup controller does not use this chart.
It seems that no need to keep this chart after the backup controller introduced? @onlymellb

@@ -94,6 +106,7 @@ s3: {}
# region: ""
# bucket: ""
# secretName is the name of the secret which stores s3 object store access key and secret key
# This is not necessary on AWS. Instead you should be able to get the credentials from the EKS service IAM role.
Copy link
Contributor

Choose a reason for hiding this comment

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

Then how to use it in the job container?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The AWS SDK will find the credentials, it just needs to run with a service account. You have to be using EKS or properly running something like iam-authenticator in the k8s cluster.

Copy link
Contributor

Choose a reason for hiding this comment

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

We just need to attach the service account, no need to install the SDK or call any command to run with it?

Copy link
Member

Choose a reason for hiding this comment

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

Does iam-authenticator need to be packaged in the Docker image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, these changes are tested as working. Just set the service account.
On GCP the association with the k8s service account to the cloud service account is done with

kubectl -n operations annotate serviceaccount ${svcacct} iam.gke.io/gcp-service-account=${svcacct}@${PROJECT}.iam.gserviceaccount.com

On EKS I believe one just needs to attach a policy. If not using EKS you run an authenticator program for the entire cluster. https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts-technical-overview.html

charts/tidb-backup/values.yaml Show resolved Hide resolved
@onlymellb
Copy link
Contributor

I suggest though that we may want to keep a chart for mydumper if we move the controller to the new br.

The backup controller does not use this chart.
It seems that no need to keep this chart after the backup controller introduced? @onlymellb

In order to be compatible with the old version of tidb-operator, I think it can't be deleted in a short time.

@tennix
Copy link
Member

tennix commented Nov 29, 2019

/run-e2e-test

Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

@gregwebs
Copy link
Contributor Author

gregwebs commented Dec 4, 2019

/run-e2e-test

@gregwebs gregwebs merged commit 8714a53 into pingcap:master Dec 4, 2019
@gregwebs
Copy link
Contributor Author

gregwebs commented Dec 4, 2019

This PR was also tested on AWS now.

@gregwebs gregwebs deleted the charts/backup-fix-credentials branch December 4, 2019 04:50
@gregwebs gregwebs restored the charts/backup-fix-credentials branch December 4, 2019 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants