feat: backups taken from a standby via pgBackRest multi-host TLS (draft PoC for #103)

[melancholictheory]

This is a draft proof of concept for #103 (support backups taken from a standby). It is meant to make the design concrete and get direction on the operational pieces before investing further. It is not proposed as merge-ready.

Problem

CloudNativePG's default backup target is prefer-standby, so backups are dispatched to a replica to keep I/O off the primary. Today the plugin configures pgBackRest for a single local instance, so a backup that lands on a standby fails at stanza-create with pgBackRest exit 56 (unable to find primary cluster). Multi-instance clusters are forced back to target: primary.

As discussed on #103, simply skipping stanza-create on a standby is not enough: pgbackrest backup itself needs a primary among its configured hosts to run pg_backup_start/pg_backup_stop, so the real fix is to give pgBackRest a connection to the current primary (multi-host).

What this PR does

• Adds an opt-in PgbackrestConfiguration.BackupStandby field (y/prefer/n, default disabled) that gates the feature.
• When a backup runs on a standby and the feature is enabled, configures the current primary as a second pgBackRest host over TLS (--pg2-host ... --pg2-host-type tls ... --pg2-host-cert-file/key-file/ca-file ... --pg2-path/user/socket-path) and passes --backup-standby=<mode>.
• Gives stanza-create the same primary peer (so it can initialise from a standby) without --backup-standby.
• Discovers the primary from cluster.Status.CurrentPrimary (already used in the WAL path) and resolves it to the primary pod's IP. The pod is read uncached (added to the manager cache DisableFor, so it needs only get pods, not a cluster-wide Pod informer), and a pods get RBAC rule is declared. Falls back to a normal local backup when this instance is the primary or no primary is known.
• Adds PgbackrestServerOptions (the pgbackrest server TLS-server invocation) as the client-visible half of the design.
• Unit tests for the generated backup / stanza-create options (including the disabled no-op path and both y and prefer modes), the server options, the primary-peer decision, and resolveStandbyTopology (via a fake client).

Deliberately out of scope (needs maintainer direction)

The operational half from #103 is not wired here, because it depends on decisions I did not want to pre-empt:

• Running the long-lived pgbackrest server process in each sidecar (lifecycle, port exposure).
• Provisioning the pgBackRest TLS certificates (a dedicated chain vs reusing existing material) and the tls-server-auth identity.

Because of that, this cannot yet run a real standby backup end to end without those certificates and the server in place, and there is no e2e test flipping test/e2e/.../backup/fixtures.go off target: primary yet. The cert and server paths are constants and a command builder, documented as the contract the plugin expects.

Testing

go build ./..., go vet, gofmt, and the unit tests (go test ./internal/..., with envtest for the controller package) all pass. New unit tests assert the exact generated pgBackRest invocations, including that the disabled path is byte-for-byte unchanged.

Open questions

Same as #103. I would appreciate direction on the TLS server lifecycle, certificate provisioning, and primary discovery (pod IP vs a dedicated headless service) before extending this.

[Agalin]

Do we actually need this field? CNPG defines in the Backup resource where it should run. If anything, we might want a simple boolean value that would explicitly enable/disable the feature and define additional dependencies.

I believe we already discussed that ideally there would be separate flags for enabling/disabling cusotm service and certificate SAN injection. That would give a yaml structure like this I believe:

spec:
  backupStandby:
    enabled: true
    injectSAN: true
    injectService: true

[melancholictheory]

Agreed on the shape. Booleans read better than mirroring pgBackRest's y/prefer/n, and it matches the opt-outs we landed on in #103.

On "do we need the field": I'd split it into two decisions that live in different places.

• Enabling the feature is cluster-level and ahead of time. The service and SAN injection happens at cluster reconcile (the mutate-cluster hook), before any Backup exists, so it needs an Archive-level switch. That's backupStandby.enabled, with injectSAN and injectService as per-piece opt-outs. This PoC doesn't do the injection yet (only the multi-host --backup-standby wiring), so that part lands with the restructure.
• Routing a backup to a standby is per-backup, and CNPG already owns it via the Backup/ScheduledBackup target. So you're right that the plugin shouldn't read y/prefer/n from the Archive. It can detect at backup time that it's running on a replica (because target: prefer-standby routed it there), configure the second host and pass --backup-standby, and skip it on the primary.

So I'd drop BackupStandby y/prefer/n for:

spec:
  configuration:
    backupStandby:
      enabled: true        # provision TLS multi-host support (service + SAN)
      injectSAN: true      # opt out to manage serverAltDNSNames yourself
      injectService: true  # opt out to manage the headless service yourself

and derive --backup-standby from whether the backup actually landed on a standby.

One thing to confirm: pgBackRest's prefer mode gets redundant if we derive from target. target: prefer-standby already means "standby if available, else primary" at the CNPG level, so we'd pass --backup-standby=y only when we land on a replica. Does that match what you want?

Happy to rework the PoC onto this, and fold in the service and SAN injection, unless you'd shape it differently.

[Agalin]

Yes, that seems correct. At pgbackrest level we either deal with running on primary (backup from primary) or running on standby (deploy from standby). We can assume CNPG operator routes the request to a correct node.

[Agalin]

PoC looks promising but critical pieces are still missing (extension of Cluster manifest, starting the server).

Are there any undecided things left? If not, what are the next steps, do you want to continue here or try to merge this partial implementation and work on that state? The former would require some kind of fail-fast code marking the feature as not ready (and likely a readme entry about it). so for now I'd rather see the later.

[Agalin]

If we go with a managed service, as discussed, I believe we don't need to retrieve pod at all. Whole flow would be:

• Check if standby backups enabled.
• Check if we're primary.
• If we're on standby - use the configured service's name (its name is either known because we manage it or configurable by user in the Archive object), same for port.

That also means we don't need permissions to read pods (all data comes directly from the Archive object). The only thing needed is to determine primary name from the Cluster to find if we're primary.

[Agalin]

I think we need a service name configuration here. While we can provide a sane default for the service name we manage, it might need an override support and we still need to know how is user-provided service named.

For port, operator currently provides us with a static setup so it's not configurable in the managed mode. I wonder if it should be configurable at all or the requirement for user-provided service should be to match the port configuration. Most likely the later.

[Agalin]

I believe we should raise an error if current primary is not set. That's not a healthy cluster state.

[melancholictheory]

Merging the partial implementation works for me, so I went with fail-fast plus a README note rather than holding the PR until everything is there.

Changes from your comments:

• Dropped the pod lookup. The topology now comes from the Archive (service name, fixed port) and the Cluster is only read for currentPrimary, so the pods get permission and the uncached Pod read are gone.
• Added serviceName to backupStandby, defaulting to <cluster>-pgbackrest. The port stays fixed, so a user-provided service has to expose it, which is the option you leaned towards.
• An empty currentPrimary is now an error instead of a silent fallback to a local backup.

For fail-fast: since the injection is not implemented, enabling the feature while injectService or injectSAN is true returns an error saying to provide the service and the SAN yourself and set both to false. The default path fails loudly, and the manual path stays usable for testing.

Nothing undecided on my side. What is left is implementation: the mutate-cluster hook that adds the managed service and the SAN, and the pgBackRest server lifecycle in the sidecar. I would do both as follow-ups on top of this.

[Agalin]

Before final review round, let's wait for release of 0.8.0 and the changes in #137 to ensure important new features/bugfixes aren't affected by the introduction of an experimental feature. I'll ping you for a rebase when that happens (note: there are merge conflicts already and that PR is likely to introduce other potentially conflicting changes). In the meantime I'll try to review included tests (for now I was only looking at code) and changes for the recent batch of comments (I don't see any push events since then so I believe no changes have yet been made since your acknowledgement).