prole/docs/completed/06-patch-garage-script-fixes.md

# Junie brief — Queue #6: fix `scripts/patch_garage_cross_cluster.sh`

> **Self-contained brief.** Three concrete defects in the script + a related
> manifest split. Land as one or two commits (your call — see §5).

---

## 1. Why

`scripts/patch_garage_cross_cluster.sh` was written one-shot during the
2026-04-29 cluster recovery to migrate CNPG backups off Garage and onto
GCS+Workload-Identity. It worked at the time, but reading it back surfaced
three defects that would bite the next person who runs it. We want it to
be re-runnable cleanly (e.g. for a future second customer or DR rebuild)
without surprises.

Live-cluster context (do not change): GCS migration is complete, ObjectStore
is active, ScheduledBackup is plugin-method, three CNPG pods on
`knoe-dev-cnpg-0` are healthy. **You don't need to run the script to fix
it.** This is a code-only commit.

## 2. The three defects

### Defect A — stale `DB_CLUSTER` default

`scripts/patch_garage_cross_cluster.sh:39`

```bash
DB_CLUSTER="${DB_CLUSTER:-knoe-cnpg-0}"
```

Should be `knoe-dev-cnpg-0` (matches `conf/gke.cfg` line `init_cluster.db_cluster_kubecontext`
and `CLAUDE.md` cluster table). The current default resolves to the wrong
context and `kubectl` operations would silently target the wrong cluster
(or fail with "context not found", depending on the environment).

The script's `APP_CLUSTER="${APP_CLUSTER:-knoe-dev-0}"` on line 40 is
already correct.

Header comment on lines 16-18 also references the wrong name in the
`./etc/init_cnpg_gke.sh ... --cluster knoe-cnpg-0` example — fix that too.

### Defect B — Phase 1 misses `service/garage-s3-ilb`

`scripts/patch_garage_cross_cluster.sh:86-95` — Phase 1's deletion loop:

```bash
for resource in "statefulset/garage" "service/garage" "configmap/garage-config"; do
  ...
done
```

The loop deletes `service/garage` (the ClusterIP) but not
`service/garage-s3-ilb` (the LoadBalancer). The ObjectStore's
`endpointURL` historically pointed at the ILB, so leaving the LB orphaned
costs money and confuses cluster state. The 2026-04-29 cleanup did this
manually after the script ran.

**Fix**: add `"service/garage-s3-ilb"` to the loop's resource list. Order
doesn't matter (all `--ignore-not-found=true`).

### Defect C — Phase 2 applies a manifest containing both SA AND a legacy ScheduledBackup

`scripts/patch_garage_cross_cluster.sh:114-122` — Phase 2 applies
`deploy/gcp/gke/knoe-db-backup-gcs.yaml`. That manifest currently contains:

1. **ServiceAccount `cnpg-backup-sa`** — wanted; this is the WI-bound SA.
2. **ScheduledBackup `knoe-db-daily`** with `method: barmanObjectStore` —
   **unwanted**. Two problems:
   - Live cluster already has a plugin-method ScheduledBackup. Applying
     this would create a duplicate that runs alongside the active one.
   - `method: barmanObjectStore` (the legacy in-tree method) is being
     removed in CNPG v1.30 per upstream release notes. New work should not
     pin to it.

**Fix** (one of two paths — pick the cleanest):

**Path 1 (recommended): split the manifest.** Move the ScheduledBackup
out of `deploy/gcp/gke/knoe-db-backup-gcs.yaml` into its own file (or
delete it outright if there's no consumer — verify by `grep -r "knoe-db-daily"`
across the repo first; if nothing references it, just remove it). Keep
`knoe-db-backup-gcs.yaml` as the canonical SA-only manifest. The script's
Phase 2 stays unchanged (still applies the SA file).

**Path 2 (minimal): adjust the script only.** Use a kubectl filter so
Phase 2 only applies the ServiceAccount portion of the file:

```bash
GCP_PROJECT_ID="${GCP_PROJECT}" envsubst '${GCP_PROJECT_ID}' < "${GCS_MANIFEST}" \
  | yq 'select(.kind == "ServiceAccount")' \
  | kubectl --context="${DB_CTX}" apply -f -
```

(`yq` is already used elsewhere in `etc/`. If it's not in the script's
preflight `command -v` check, add it.)

**Path 1 is recommended** because it makes the manifest file's contents
match its filename's intent (`-gcs.yaml` should be GCS plumbing, not
backup scheduling), and removes the deprecated-method footgun for any
future runner.

If you take Path 1, also verify the live ScheduledBackup in
`knoe-db-0` namespace still exists and uses the plugin method — but **do
not run kubectl against the live cluster from the script change.** Just
inspect via `kubectl --context=$DB get scheduledbackup -n knoe-db-0 -o yaml`
once if you want to confirm; don't modify it.

### Defect D (bonus, mentioned in TODO) — Phase 3 unconditional re-apply

Phase 3 (lines 127-134) unconditionally re-applies
`k8s/knoe/knoe-db-barman-objectstore-gcs.yaml`. Manifest currently matches
live state, so this is harmless. The TODO suggests "worth a guard" — add
a `kubectl get objectstore knoe-db-barman-objectstore -n $DB_NS` check
that diffs against the manifest and skips re-apply if matching. **Skip
this one if it adds significant scope** — it's a nice-to-have, not a
defect.

## 3. What to verify

```bash
# 1) Defaults resolve correctly when no env vars set
bash -n scripts/patch_garage_cross_cluster.sh    # syntax check
( unset DB_CLUSTER APP_CLUSTER GCP_PROJECT GCP_REGION SERVICE_NS DB_NS
  source <(sed -n '36,50p' scripts/patch_garage_cross_cluster.sh)
  echo "DB_CLUSTER=$DB_CLUSTER, DB_CTX=$DB_CTX" )
# Expected: DB_CLUSTER=knoe-dev-cnpg-0, DB_CTX=gke_..._knoe-dev-cnpg-0

# 2) Dry-run the script in CONFIRM=false mode (existing safety: prints the
#    plan and exits 1) — verify the printed plan reads sensibly
./scripts/patch_garage_cross_cluster.sh
# Expected: prints "1. Remove Garage from knoe-dev-cnpg-0/knoe-system" etc.

# 3) Phase 1 deletion list now includes garage-s3-ilb
grep -A8 'for resource in' scripts/patch_garage_cross_cluster.sh
# Expected: list contains "service/garage-s3-ilb"

# 4) (If Path 1 chosen) the SA-only manifest has only ServiceAccount kind
grep '^kind:' deploy/gcp/gke/knoe-db-backup-gcs.yaml
# Expected: only "kind: ServiceAccount"
```

## 4. Out of scope

- Don't run the live script. This is a code-only fix. The script is for
  someone migrating a fresh cluster; we're not migrating.
- Don't change any RBAC or IAM. The Workload Identity binding is correct
  in the live cluster; this brief touches the script and (optionally) the
  manifest split, nothing more.
- The CNPG SA wiring inside `etc/init_cnpg_gke.sh` (`cluster.spec.serviceAccountName`
  + extended RoleBindings) is queue item **#7** with its own brief. Don't
  conflate.

## 5. Commit shape

If you take Path 1 (manifest split): two commits, atomic each:
1. `chore(deploy): split knoe-db-backup-gcs manifest — SA only, drop legacy ScheduledBackup`
2. `fix(scripts): patch_garage_cross_cluster defaults + missing garage-s3-ilb`

If you take Path 2 (script-only): one commit.

```
fix(scripts): patch_garage_cross_cluster — three defects from 2026-04-29 review

Defects A, B, C from docs/plans/junie/06-patch-garage-script-fixes.md:

  A. DB_CLUSTER default was knoe-cnpg-0 (stale rebrand artifact);
     should be knoe-dev-cnpg-0 to match conf/gke.cfg.

  B. Phase 1 deletion loop missed service/garage-s3-ilb (the
     LoadBalancer the ObjectStore endpoint historically pointed at).
     Now included.

  C. Phase 2 was applying knoe-db-backup-gcs.yaml whole, including a
     legacy ScheduledBackup using method:barmanObjectStore (being
     removed in CNPG v1.30). Manifest split: SA stays in
     knoe-db-backup-gcs.yaml; ScheduledBackup removed.

Header comment + usage example also updated for the rename.
Closes queue item #6 in docs/TODO.md (drift R9).
```

## 6. Definition of done

- [ ] `DB_CLUSTER` default + header comment updated.
- [ ] Phase 1 loop includes `service/garage-s3-ilb`.
- [ ] Phase 2 doesn't apply a legacy ScheduledBackup (Path 1 or 2).
- [ ] `bash -n` clean; CONFIRM=false dry-run prints sensible plan.
- [ ] If Path 1: `kind: ServiceAccount` is the only kind in
  `deploy/gcp/gke/knoe-db-backup-gcs.yaml`.
- [ ] `docs/TODO.md` queue item #6 + drift R9 row archived to **Done** with
  date and commit ref.