Skip to content

fix: import dynamic CSI snapshot handles for PSQLBranches - #37

Merged
patrickleet merged 1 commit into
mainfrom
fix/harmony-1394-dynamic-snapshot-handles
Oct 1, 2026
Merged

patrickleet merged 1 commit into
mainfrom
fix/harmony-1394-dynamic-snapshot-handles

Conversation

@patrickleet

@patrickleet patrickleet commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fix cross-namespace PSQLBranch recovery from dynamically created CSI snapshots. Read VolumeSnapshotContent.status.snapshotHandle, with spec.source.snapshotHandle as the fallback for pre-provisioned snapshots.

Dynamic content identifies the original volume in spec.source.volumeHandle and reports the created snapshot ID in status. Previously the branch function found no handle and emitted only the source Cluster observation, source VolumeSnapshot, and source content observation. It never created branch import resources, recovered CNPG clusters, or app credential Secrets.

Preserve the existing static-import test and add a dynamic-source regression that expects the snapshot ID rather than the volume ID. Update staged and ready fixtures to match the actual dynamic CSI shape.

Reproduction and verification

Replayed both stuck production previews with their captured XR and observed Object state. Local replay removes server ownership metadata; snapshot spec/status fields are unchanged. No production resources were modified.

Preview Before After Imported snapshot
API #600 3 source Objects 5 Objects, including branch content and snapshot snap-0150cca495c196e6b
Billing #37 3 source Objects 5 Objects, including branch content and snapshot snap-028d83c31f9c34bdb

Assertions verified that the imported content uses the actual ebs.csi.aws.com driver, the snapshot ID rather than the source volume ID, deletionPolicy: Retain, and a matching branch snapshot binding. Both replays correctly remain unready and defer CNPG creation until the imported snapshot becomes ready.

  • Both production-state replays and import assertions
  • git diff --check
  • All 50 composition tests in PR CI, including dynamic-source regression and retained static-import coverage
  • Five PR CI example/schema validations
  • Existing AWS E2E in PR CI

Composition test job.
Local up test run stalled after its first render and was terminated; the successful suite result above is from CI. Both production-state replays completed locally.

The existing AWS E2E exercises same-namespace branching; the added composition regression and production replays exercise the cross-namespace path involved in this incident. Actual preview recovery and generated Secrets must be verified after the fixed package is released and adopted through GitOps.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: aa187948-c9b0-4d18-857c-f27d5b13f0e6

📥 Commits

Reviewing files that changed from the base of the PR and between 7530f83 and d4df150.

📒 Files selected for processing (4)
  • functions/branch/010-state-status.yaml.gotmpl
  • tests/test-branch/main.k
  • tests/test-branch/observed/cross-namespace-ready.yaml
  • tests/test-branch/observed/cross-namespace-staged.yaml

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The branch state now prefers status.snapshotHandle from source snapshot content, falls back to spec.source.snapshotHandle, and uses an empty string if neither is available. Updated fixtures and a cross-namespace composition test cover dynamic snapshot content.

Changes

Snapshot handle selection

Layer / File(s) Summary
Select and validate the snapshot handle
functions/branch/010-state-status.yaml.gotmpl, tests/test-branch/main.k, tests/test-branch/observed/cross-namespace-ready.yaml, tests/test-branch/observed/cross-namespace-staged.yaml
The branch state selects status.snapshotHandle before the spec.source.snapshotHandle fallback. The updated fixtures place generated handles in status. The cross-namespace test checks the imported handle, branch content settings and reference, generated content name, and composite readiness.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to d4df1

The branch import change is ready to merge after normal checks; the supplied evidence identifies no unresolved issue requiring a code change.

Security Architecture Review

Security architecture risk: 🔵 Low · up to d4df1

Dynamic snapshot recovery now trusts an identifier reported by the source snapshot controller. The existing import retains the physical snapshot and waits for readiness before reporting recovery complete, but it can create import content before the source content is ready. The authorization and provenance guarantees for the reported identifier remain unverified.

Retained concerns

  • Low · security · inferred: The newly reachable dynamic import can create branch snapshot content as soon as an observed status handle and driver are nonempty, before source content readiness is confirmed. The local templates do not establish how an early, stale, or changed handle is contained across retries; unauthorized access is not established.
Security review details

Security Blast Radius

  • inferred — An accepted handle determines which physical CSI snapshot is referenced by the new branch-namespace import. The evidence does not establish whether another tenant could influence that handle or access the resulting snapshot.

Trust Boundaries and Controls

  • observed — The source content is observed by the content name reported on the source snapshot under Observe-only management; the import uses that observation's driver and handle, references the branch snapshot, and retains the physical snapshot on deletion.

Hardening Proposals

  • proposed — Confirm the snapshot controller's status-handle provenance and provider authorization across source and branch namespaces; consider gating import on source-content readiness and defining behavior if its reported handle changes during recovery.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: importing dynamic CSI snapshot handles for PSQLBranches.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

Published Crossplane Package

The following Crossplane package was published as part of this PR:

Package: ghcr.io/hops-ops/psql-stack:pr-37-1f7c3736e48d442a2dfc52403d7008fe80d48f55

View Package

@patrickleet
patrickleet merged commit 174dfd0 into main Oct 1, 2026
11 checks passed
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.

1 participant