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

feat: allow using existing secret for backup and restore #199

Closed

Conversation

DASPRiD
Copy link
Contributor

@DASPRiD DASPRiD commented Feb 29, 2024

This PR allows to specify S3 credentials through an existing secret instead of having to commit the secrets with custom helm chart values.

Closes #197

@DASPRiD DASPRiD requested a review from itay-grudev as a code owner February 29, 2024 22:35
@itay-grudev
Copy link
Collaborator

Can you apply the same functionality to the google and azure secrets as well for consistency?

@itay-grudev
Copy link
Collaborator

Also please make sure to signoff your commits and amend your existing commit(s) to have a signoff as well, for the DCO check.

Copy link
Collaborator

@phisco phisco left a comment

Choose a reason for hiding this comment

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

You might want to have secret.create (default true) and secret.name (default empty) so that one can both specify a custom name for the secret. It's pretty common.
I wonder if it could be moved to an upper level so that we don't have to repeat it for all flavors.

@DASPRiD
Copy link
Contributor Author

DASPRiD commented Mar 1, 2024

Can you apply the same functionality to the google and azure secrets as well for consistency?

I did look at that initially, but I'm not sure how exactly to handle that with Azure, since it has if statements depending on the auto-created secret.

@DASPRiD DASPRiD force-pushed the feat/s3-secret-credentials branch 2 times, most recently from 125eb98 to 0af6d06 Compare March 1, 2024 18:24
key: ACCESS_SECRET_KEY
{{- else if eq .scope.provider "azure" }}
{{- if empty .scope.destinationPath }}
destinationPath: "https://{{ required "You need to specify Azure storageAccount if destinationPath is not specified." .scope.azure.storageAccount }}.{{ .scope.azure.serviceName }}.core.windows.net/{{ .scope.azure.containerName }}{{ .scope.azure.path }}"
{{- end }}
azureCredentials:
{{ $secretName := coalesce .scope.secret.name (printf "%s-backup-azure%s-creds" .chartFullname .secretSuffix) }}
{{- if .scope.azure.connectionString }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need some guidance here. Since the credentials are not defined in the values anymore, how would you suggest to handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just an idea, since I don't know how this is handled in the operator:

Are the if statements there actually required, or will the operator be fine when specific variables leading to empty secret values are defined?

@DASPRiD
Copy link
Contributor Author

DASPRiD commented Mar 1, 2024

Alright, I signed off the commit and implemented the suggested parent secret property for both recovery and backups. I'm still stumbled on the azure config, see my inline comment.

@DASPRiD DASPRiD force-pushed the feat/s3-secret-credentials branch from 0af6d06 to c0e0fd9 Compare March 1, 2024 18:57
@DASPRiD DASPRiD changed the title feat: allow using existing secret for S3 credentials feat: allow using existing secret for backup and restore Mar 1, 2024
@DASPRiD DASPRiD requested a review from phisco March 7, 2024 19:56
@itay-grudev
Copy link
Collaborator

I reworked your proposal in #239.

Closing in favor of #239.

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.

Add support for existing backup credentials
3 participants