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(cluster): allow using existing secret for backup and restore #239

Merged
merged 9 commits into from
May 25, 2024

Conversation

itay-grudev
Copy link
Collaborator

Closes #197

itay-grudev and others added 2 commits March 28, 2024 01:35
Also removes the ugly whitespace from the generated YAML
Signed-off-by: Itay Grudev <[email protected]>
Co-authored-by: Ben Scholzen (DASPRiD) <[email protected]>
Signed-off-by: Itay Grudev <[email protected]>
@itay-grudev
Copy link
Collaborator Author

@phisco I've made your review slightly more complicated as this is the base for my work on adding support for replica clusters and I've fixed and restructured the credential secrets.

@gpothier
Copy link
Contributor

gpothier commented Apr 8, 2024

Hi, I merged this branch into main in my fork, as I needed the feature, and I noticed a few issues:

  • There are problematic -}} at a few places places in _barman_object_store.tpl in the handling of destinationPath and $secretName; it doesn't work as is, and moreover it complicates things, like having to do {{ " destinationPath: \"s3://" }} instead of just destinationPath: "s3://.
  • In _backup.tpl you pass a secretPrefix to barmanObjectStoreConfig, but in _barman_object_store.tpl you expect a secretSuffix.

@itay-grudev
Copy link
Collaborator Author

@gpothier Have you fixed these in your fork? Can I take a look and/or steal your proposal?

@gpothier
Copy link
Contributor

gpothier commented Apr 8, 2024

Sure, I just committed it to my fork: gpothier@fbdb794

I only fixed the yaml formatting stuff (using }} instead of -}}, as whitespace is part of YAML syntax).

I don't know what your intent is regarding these prefix/suffix things so I did not touch that.

I also added quote to endpointURL as I thought this was part of the problem; it wasn't, but I think it's better to quote in general anyway, and in particular for URLs that usually contain : signs that are not very safe to use in unquoted YAML values. I think it might be a good idea to have a look at other values that are not quoted. See https://helm.sh/docs/chart_template_guide/functions_and_pipelines/:

Let's start with a best practice: When injecting strings from the .Values object into the template, we ought to quote these strings.

@gpothier
Copy link
Contributor

Hi, have you been able to take a look at my suggested fix for this? Do you want me to create another PR with my changes?

@itay-grudev
Copy link
Collaborator Author

I'm super overwhelmed with work right now, so yhea, I would really appreciate any help.

@sxd
Copy link
Member

sxd commented May 24, 2024

@phisco can you give us a hand here? :D

@itay-grudev
Copy link
Collaborator Author

@Cr4mble Thanks a lot! I would have totally missed that!

@Cr4mble
Copy link
Contributor

Cr4mble commented May 24, 2024

@itay-grudev can confirm that the backup to s3 is now working with the current state of the helm chart.
The wal files are created in the specific s3 bucket.
I used the following values for testing (some are default values):

backups:
  enabled: true
  provider: s3
  secret:
    create: false
    name: "cloudnative-pg-s3-creds"
  s3:
    region: "eu-central-1"
    bucket: "testing-cloudnative-pg-cluster-helm-chart"
    path: ""
  scheduledBackups:
    -
      name: daily-backup
      schedule: "0 0 0 * * *"
      backupOwnerReference: self
      method: barmanObjectStore

@itay-grudev
Copy link
Collaborator Author

@Cr4mble Thanks a lot.

@8ball030
Copy link

Thanks a bunch for this guys.
Really love the collaboration here :)

@itay-grudev itay-grudev added the chart( cluster ) Related to the cluster chart label May 25, 2024
@itay-grudev itay-grudev merged commit a123fb4 into main May 25, 2024
5 checks passed
@itay-grudev itay-grudev deleted the dev/197 branch May 25, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chart( cluster ) Related to the cluster chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for existing backup credentials
6 participants