fix(migrations): derive the etcd-adoption snapshot target from the projected bucket creds - #3335
Merged
Merged
Conversation
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical failure in the platform upgrade process where migration 50 would block due to an unreachable or misconfigured etcd-adoption snapshot target. By shifting the source of truth for backup coordinates from the cluster-rendered Strategy CR to the dynamically projected bucket credentials, the migration becomes resilient to stale or missing CR states during upgrades. The changes ensure that the platform can reliably locate the correct S3 target while preserving support for custom external backup configurations. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughPlatform adoption now derives S3 snapshot arguments from projected credentials when available, falls back to Strategy CR values for missing fields, and adds configurable skip-backup behavior with expanded fixture, template, and end-to-end coverage. ChangesPlatform backup destination resolution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ETCDMIGRATE
participant ResolvePlatformBackupArgs
participant CredentialsSecret
participant BucketClaim
participant StrategyCR
ETCDMIGRATE->>ResolvePlatformBackupArgs: resolve backup arguments
ResolvePlatformBackupArgs->>CredentialsSecret: read projected credentials
CredentialsSecret-->>ResolvePlatformBackupArgs: return S3 coordinates
ResolvePlatformBackupArgs->>BucketClaim: classify bucket ownership
BucketClaim-->>ResolvePlatformBackupArgs: return claim state
ResolvePlatformBackupArgs->>StrategyCR: read missing fields
StrategyCR-->>ResolvePlatformBackupArgs: return fallback values
ResolvePlatformBackupArgs-->>ETCDMIGRATE: provide backup S3 flags
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the migration 50 script to resolve platform backup S3 coordinates directly from the projected credentials secret instead of relying on the live Strategy CR, which may be absent or incorrect during a pre-upgrade. It also adds corresponding BATS tests and updates mock data. The review feedback points out a potential issue in the new secret_val helper where decoding empty or invalid base64 values could trigger a script failure under set -e, and suggests a robust alternative implementation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+217
to
+220
| secret_val() { | ||
| printf '%s' "$1" | jq -r --arg k "$2" '.data[$k] // ""' \ | ||
| | base64 -d 2>/dev/null | tr -d '[:space:]' | ||
| } |
Contributor
There was a problem hiding this comment.
Under set -euo pipefail (enabled at line 20), if any command in a pipeline fails, the entire pipeline returns a non-zero exit status. If a key is missing from the Secret, jq will output an empty string (with a newline), which is passed to base64 -d. Depending on the environment and the base64 implementation, decoding empty/invalid input can exit with a non-zero status. This will cause the command substitution $(secret_val ...) to fail and immediately abort the migration script due to set -e.
To make this robust, we can first extract the raw base64 value, check if it is non-empty, and only then decode it. We can also append || true to the decoding pipeline to guarantee it never returns a non-zero exit status and crashes the script.
Suggested change
| secret_val() { | |
| printf '%s' "$1" | jq -r --arg k "$2" '.data[$k] // ""' \ | |
| | base64 -d 2>/dev/null | tr -d '[:space:]' | |
| } | |
| secret_val() { | |
| local val | |
| val=$(printf '%s' "$1" | jq -r --arg k "$2" '.data[$k] // ""') | |
| [ -n "$val" ] || return 0 | |
| printf '%s' "$val" | base64 -d 2>/dev/null | tr -d '[:space:]' || true | |
| } |
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Aleksei Sviridkin (lexfrei)
approved these changes
Jul 17, 2026
Aleksei Sviridkin (lexfrei)
left a comment
Contributor
There was a problem hiding this comment.
LGTM — fixes a real, well-evidenced upgrade-blocking bug in migration 50, with comprehensive tests that all pass and no regressions found.
Business context: migration 50 (etcd v1alpha1→v1alpha2 adoption) is a pre-upgrade hook that gates the whole platform upgrade behind a mandatory S3 safety snapshot; on v1.5.x clusters it resolved the snapshot target from the live cozy-default-etcd Strategy CR, which is either absent (bucketName-lookup race) or carries the plaintext :8333 endpoint that fails TLS — so the upgrade wedges with no reachable escape.
The fix is architecturally correct for a pre-upgrade hook: it takes coordinates from the projected cozy-backups-creds Secret (written at bucket-provisioning time, not chart-render time), classifies COSI vs external-S3 by matching the projected bucketName against BucketClaim .status.bucketName, forces https:// only on the COSI path, and keeps the CR endpoint verbatim for external S3. The two new refuse-states (Terminating claim / unreadable BucketClaim API) fail closed with actionable messages, which is right given live-etcd adoption is irreversible. The escape-hatch plumbing (migrations.etcdAdoptSkipBackup) closes a genuine latent bug — the env var was never set on the hook Job, so every failure message advised an action the platform made impossible.
Verification:
- 28/28 bats and 88/88 helm-unittest pass locally; the template's bool/
"true"/nil/garbage normalization is pinned by tests. - No docs drift: the platform chart has no generated values schema/README; the knob is an emergency break-glass documented in-place.
- The red E2E run is unrelated to this diff (install failed at
flux-shard-operatorliveness, and the migration hook does not even render on a fresh install — it needs a pre-existingcozystack-versionConfigMap below target).
Non-blocking follow-ups
- The deferred
if $bucketNamelookup-race instrategy-etcd-default.yaml(called out in the PR) likely means default etcd backups are silently broken on 1.5.x clusters regardless of upgrade — worth its own tracked issue. tests/migration_hook_skip_backup_test.yamlasserts on positionalenv[3]; a content/name match would be less brittle if the env order ever changes.
1 task
Collaborator
VerdictLGTM with non-blocking notes The fix is well-diagnosed and thoroughly tested; all 28 bats + 88 helm-unittest cases pass, both required regression tests are non-vacuous (verified by mutation), and no regression is introduced on fresh-install or upgrade. The only issues are process gaps in the PR body (empty Findings[MINOR] The PR body contains no Caveats
Verification performed this run:
|
…ojected bucket creds Migration 50 is a pre-upgrade hook, so every chart-rendered resource it reads still belongs to the version being upgraded FROM. It read the snapshot target from the live cozy-default-etcd Strategy CR, which on a v1.5.x cluster is broken in one of two ways — and the platform upgrade is gated behind this snapshot, so neither can self-heal first: 1. Absent. strategy-etcd-default.yaml is guarded by `if $bucketName`, whose helper looks up the BucketClaim status. That BucketClaim is created by the same chart, so on first render the lookup is empty and the Strategy is silently skipped. Helm lookup only runs at install/upgrade time, so a cluster that reached 1.5.x in one hop never grows the CR on reconcile. 2. Present, carrying the static v1.5.x default http://seaweedfs-s3.tenant-root.svc.cozy.local:8333. Cozystack ships SeaweedFS with enableSecurity=true, so :8333 is a TLS listener and plaintext against it fails the handshake. Fixing the scheme alone is not enough: the Etcd Strategy's S3 schema has no caCert/insecureSkipVerify, so the self-signed in-cluster endpoint is unusable at any scheme and a trusted-cert endpoint is required. Either way migration 50 hard-fails and blocks the upgrade, and the documented ETCD_ADOPT_SKIP_BACKUP escape hatch cannot be reached — migration-hook.yaml passes only NAMESPACE/CURRENT_VERSION/TARGET_VERSION. Take the coordinates from cozy-backups-creds instead. The projector writes it from the COSI-provisioned bucket's system credentials — bucket provisioning, not chart values — so it describes the live bucket regardless of which version rendered the charts, and it carries endpoint, bucketName, region and forcePathStyle alongside the AWS keys. resolve_platform_backup_args already read this Secret to validate those keys, so preferring it adds no new dependency. The endpoint is a bare host fronted by the always-TLS S3 ingress, so force https://, exactly as the 1.6 chart helper backupstrategy-controller.endpoint does. The Strategy CR remains the fallback for the admin-managed external-S3 case (provisionBucket: false), where .Values.backupStorage.endpoint is authoritative and may legitimately be plaintext against a private store; that path is detected by the absence of a projected endpoint and its scheme is preserved verbatim. The bats fixture modelled cozy-backups-creds as carrying only the AWS keys, and test 1 asserted the broken plaintext endpoint as expected output — the test was written against the implementation rather than the requirement, which is why the defect shipped green. The fixture now carries the coordinates the real projected Secret has, test 1 asserts the derived https endpoint, and two cases are added: an absent Strategy CR still resolving, and external S3 keeping the CR endpoint verbatim. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…d bucket Deriving the safety-snapshot target from the projected cozy-backups-creds Secret is right, but detecting the admin-managed external-S3 case (provisionBucket: false) by "the projected endpoint is absent" is not: that state does not exist. The projector substitutes BACKUP_STORAGE_ENDPOINT and fails ReasonSourceMalformed when both sources are empty, so a projected Secret always carries a non-empty, scheme-stripped endpoint. So for provisionBucket: false against a plaintext external S3 the projected endpoint is a bare host, the script saw it non-empty, forced https:// and never consulted the Strategy CR — a regression against main, which read the CR verbatim and passed http:// through. Being a pre-upgrade hook with no ETCD_ADOPT_SKIP_BACKUP plumbed through migration-hook.yaml, that is a total upgrade block with no operator escape. Classify on data instead: match the projected bucketName against the .status.bucketName of any BucketClaim. The COSI driver assigns that name and the projector republishes exactly it (packages/system/bucket user-credentials.yaml reads BucketInfo spec.bucketName), so a match means the creds describe a COSI bucket => the host is the always-TLS S3 ingress => force https, as the 1.6 chart helper does. No match means external S3. Keying on data rather than on the claim's name and namespace matters twice. backupStorage.namespace/.bucketName are supported Package-CR overrides that this hook cannot see, so a name lookup would miss a renamed bucket and fall back to the v1.5.x plaintext CR — the original P0. And a BucketClaim wedged Terminating (its cosi bucketclaim-protection finalizer outlives an uninstalled COSI controller) keeps answering a name lookup while its status names some other bucket. Split the two paths by which source is authoritative. On COSI the projected coordinates win: the bucket is the COSI-assigned name the CR can only reproduce through a live lookup that may never have run. On external S3 the Strategy CR wins WHOLESALE, not just for the endpoint — the CR renders backupStorage.* and points at this same cozy-backups-creds, so CR coordinates plus these credentials is exactly what the cluster's own BackupJobs already use, and the bucket an operator will look in. Keeping the Secret's bucket while taking the CR's endpoint assembled a pairing nothing else produces: the projector copies bucketName straight from the admin-managed source Secret and never from backupStorage.bucketName, so the two can disagree outright and the snapshot could land in a bucket the platform never writes. Relying on the CR is safe on this path alone, because bucketName short-circuits to values before the BucketClaim lookup that races on the COSI path, so the CR is guaranteed present. Match with a single jq `any`, never `jq -r | grep -q`: grep exits on the first match and SIGPIPEs jq, and pipefail reports jq's 141 rather than grep's 0, so a MATCH returns as a failure and the cluster is misclassified external. It is a race on the list outgrowing one stdio write (~8KB) with only a last-line match safe, so it strikes exactly the big multi-tenant clusters that can least afford it, and backoffLimit 3 can decide differently on each retry. Measured with the match first, 20 runs each: 300 claims went 0/20 correct (rc=141 every time) and 2000 likewise, while `any` is 20/20 at 3, 300 and 2000. Refuse rather than guess when the answer is not knowable. Three states qualify: the BucketClaim API is unreadable; the list is not interpretable (no producer emits that, but without a shape gate it reads as "no claim" = external = the plaintext P0, silently); or the bucket is claimed ONLY by a Terminating claim. That last one is ambiguous — an admin who moved COSI -> external S3 reusing the bucket name, versus a still-COSI cluster whose claim is mid-recreate — and both readings fail as the same confusing handshake error when wrong, so a guess buys no reliability while reading it as "external" specifically resurrects the plaintext-:8333 P0. A live claim still wins over an unrelated Terminating one, so healthy clusters are unaffected. Each refusal names the condition and the operator action; the migration is idempotent, so re-running after resolving it is the remedy. Behaviour change worth noting: a cluster on external S3 that still has COSI installed now refuses on a sustained BucketClaim-API failure where main would have proceeded off the Strategy CR. That is the intended trade — main proceeded only because it never had to tell the two apart — but it does mean an apiserver outage can now block this hook where it previously did not. ETCD_ADOPT_SKIP_BACKUP is still not plumbed through migration-hook.yaml, so the messages deliberately point at resolving the condition rather than at an escape hatch the operator cannot reach. Force the scheme only onto a non-empty host: an absent endpoint must degrade to the CR as main did, not to a literal "https://" that would pass the final non-empty guard and reach etcd-migrate as a destination. The old test modelled external S3 by dropping the projected endpoint entirely — a state the projector cannot produce — so it green-lit the regression. It now models what the projector really writes, and the suite pins the stale-claim, overridden-coordinates, empty-endpoint, 300-claim-multi-tenant, Terminating-match, hybrid-coordinate and unreadable/uninterpretable API cases. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Migration 50 has honoured ETCD_ADOPT_SKIP_BACKUP since it was written, and its failure paths tell operators to "re-run with ETCD_ADOPT_SKIP_BACKUP=1". But migration-hook.yaml passes only NAMESPACE/CURRENT_VERSION/ TARGET_VERSION, and the Job is a Helm hook the chart re-renders, so there has never been a way to set it. The script advertised an escape the platform made impossible. The endpoint-classification work landing alongside this makes that worse: it adds refuse-rather-than-guess paths, so more clusters can now legitimately stop here. Plumb it through migrations.etcdAdoptSkipBackup, default false. Skipping the pre-adoption snapshot rewires ownership of LIVE etcd storage with no way back, so it stays opt-in and deliberate — the flag is the last resort for a cluster that has nothing to fix, not the first response to a refusal. Render "1"/"0" rather than the bare bool. The script matches the literal string "1", so a bool would render "true", sail through the Job spec looking correct, and be ignored — a safety valve that is set but silently disregarded is worse than one that was never offered, because the operator believes they opted in. The value is normalised through a string, so a bool, a quoted "true" out of the Package CR's Values JSON, or a numeric 1 all land on the same answer, while nil or garbage resolves to "0": taking this hatch must require an affirmative spelling, never a typo. Resolved with `dig` so a Platform Package predating the key renders "0" instead of failing the render of the entire platform chart and bricking every upgrade. Widen the script's own check to accept 1/true/yes as well. The template guarantees the GitOps path, but an operator running this image by hand will reasonably type "true", and that is exactly the moment they are already stuck. The two layers cover independent failure modes. Emit the var unconditionally, including the "0" default, so the rendered Job states the cluster's safety posture outright rather than leaving an operator to diff values to find out whether a snapshot was skipped. Point the failure messages at the values path that actually exists, and describe it honestly: editing the Platform Package and letting Flux re-render is a real operation, not a one-liner, and the flag has to be reverted afterwards or the next etcd migration skips its snapshot too. Fixing the condition the migration names remains the recommended path — the migration is idempotent, so re-running after a fix is safe. templates/migration-hook.yaml had no coverage at all, because the Job only renders when a lookup of the cozystack-version ConfigMap reports a version below targetVersion, and lookup is nil under helm template. The new suite mocks that ConfigMap and asserts the RENDERED VALUE — enabled, disabled, key absent, stringified true, garbage, and the pre-existing three env vars — rather than merely that the variable exists, which is what let the trap above go unnoticed in the first place. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
myasnikovdaniil
force-pushed
the
fix/etcd-adopt-backup-endpoint
branch
from
July 20, 2026 06:31
2a7ec86 to
efa85a4
Compare
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-3335-to-release-1.5
git worktree add --checkout .worktree/backport-3335-to-release-1.5 backport-3335-to-release-1.5
cd .worktree/backport-3335-to-release-1.5
git reset --hard HEAD^
git cherry-pick -x 9ceccd2b07d1e9b7a34c8927e2d47489c35728ea 0ba53375264e55e1f50e7a58442f595cd62eb5b9 efa85a4f615c982258f2fe39294a6c218a860f98
git push --force-with-lease |
myasnikovdaniil
added a commit
that referenced
this pull request
Jul 20, 2026
…e path (#3339) ## What this PR does Cozystack v1.5.0 bumped the vendored SeaweedFS chart 4.0.405 → 4.31.0, which renamed every workload after the Helm release (`<name>-system-*`). StatefulSet names are immutable, so upgrades through 1.5.x stood up a second, empty set beside the running one instead of renaming. #3282 pinned `fullnameOverride: seaweedfs` so 1.6 adopts running workloads in place — this PR closes the remaining holes on that adoption path, found by driving a disposable 3-node cluster through a real v1.4.5 → v1.5.3 → main upgrade with SeaweedFS tenants planted in every reachable state. - **The naming guard moves into `packages/system/seaweedfs`** — the render a platform upgrade actually re-renders: the `<name>-system` HelmRelease pulls this chart from a platform-managed ExternalArtifact, so the previous guard in `extra/seaweedfs` was never in the path. Upgrading a fresh-1.5.x tenant therefore created an empty chart-named set beside its live data and flipped the guard's own legacy-data signal. `extra/` keeps a sibling copy for operator visibility; a bats parity suite pins the two detection blocks byte-identical. - **The guard refuses instead of guessing when both naming generations exist.** Nothing durable distinguishes a duplicate that never served from one that served and crashed: claim timestamps invert during the recovery runbook's own re-bind, `readyReplicas: 0` is a snapshot, and Helm birth order answers "which generation is original", not "is the other one empty". Exactly one generation present is decidable, and the render proceeds (or refuses as class S) on its own. - **Cluster-scoped RBAC is named per namespace again.** 4.31 named four cluster-scoped objects after the release — identical for every tenant — so all tenants collided on one ClusterRole/ClusterRoleBinding and only the last-reconciled tenant's COSI provisioner kept its RBAC. Names return to `global.seaweedfs.serviceAccountName`; three of four are byte-identical to pre-4.31 and adopt in place. - **The `seaweedfs-db` hand-over runs for every instance name.** Migration 43 compared the owning release against the literal `seaweedfs-system`, so an instance named `foo` was skipped and its CNPG Cluster — the filer metadata for every object in that tenant's S3 — was pruned on the next reconcile, PVC included. The comparison now matches the `-system` suffix (shared `lib/seaweedfs-db-adopt.sh`), and new migration 53 re-runs the hand-over for clusters already past 43, before anything re-renders. - **`hack/seaweedfs-naming-audit.sh` + `docs/operations/seaweedfs-431-rename-recovery.md`** — what the guard's refusal points operators at: read-only classification (`L` / `S` / `MIXED`, naming the candidate duplicate from relative PV vintage, never a clock window) and the recovery procedures. **Scope.** The supported SeaweedFS deployment is the tenant module, which hardcodes the instance name (`packages/apps/tenant/templates/seaweedfs.yaml`); a tenant only enables or disables it. The runbook and its selectors are scoped to that name; instances created directly against the API under other names are classified by the audit but routed to escalation. Zone/pool keys of ~40 or more characters fall outside the guard's reconstruction — an accepted limit, recorded in `_naming.tpl` and the runbook. **Upgrade impact.** 1.4.x → 1.6: no manual steps — one generation, adopted in place. 1.5.x → 1.6 with SeaweedFS: migration 53 protects every `seaweedfs-db` first; then the upgrade **refuses** for any tenant holding both naming generations until the operator resolves the duplicate. This is deliberate — which generation holds the data is not decidable from inside a render, and guessing wrong destroys it. Release notes should present the refusal as expected behavior. **Testing.** 55 chart unit tests across both packages (including new MultiZone zone-component guard cases — the suite previously had no zone shapes), 11 audit bats, 8 guard-parity bats, migration bats. Validated end-to-end on a disposable 3-node cluster driven v1.4.5 → v1.5.3 → main with five tenants covering: never-saw-4.31 (renders untouched), fresh-1.5.x (refused as class S), wedged duplicate, split duplicate, and a non-default-named instance (its database survives only with this fix). The audit classifies all five correctly, and its two independent signals — revision-1 birth scheme and relative PV vintage — agree on every MIXED tenant. Related: #3282 (fullnameOverride pin), #3335 (etcd adoption backup gate — separate, also required for the 1.5.x→1.6 path). ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change Walked the trigger map against the diff: no package added/renamed under `packages/{apps,extra}/`, the `packages/core/platform/values.yaml` change is only the migrations `targetVersion` bump (no `spec.components.platform.values.*` key changes), no variant/bundle/component changes, no asset renames, no ApplicationDefinition semantic changes. ### Release note ```release-note fix(seaweedfs): the 1.6 upgrade no longer renames a SeaweedFS instance away from its data. The naming guard now runs in the chart a platform upgrade actually re-renders and refuses when both pre- and post-4.31 naming generations exist; hack/seaweedfs-naming-audit.sh and docs/operations/seaweedfs-431-rename-recovery.md guide recovery, and the refusal is expected for tenants that passed through 1.5.x. Cluster-scoped COSI RBAC is named per namespace again (the 4.31 release-based names collided across tenants), and the seaweedfs-db hand-over runs for every instance name — previously an instance not named `seaweedfs` had its filer metadata database pruned on upgrade; new migration 53 repairs clusters that already ran the old hand-over. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added fail-closed upgrade protections for SeaweedFS naming migrations (mixed/damaged classification), including safer behavior when cluster visibility is limited. * Added cluster-scoped RBAC uniqueness safeguards to prevent cross-tenant name collisions during rendering. * Improved SeaweedFS database adoption/repair migrations and strengthened post-delete cleanup ownership checks. * **Bug Fixes** * Hardened SeaweedFS 4.31 rename recovery and PV rebind flow, including reclaim policy preservation and long/non-default instance-name edge cases. * **Documentation** * Expanded the SeaweedFS 4.31 rename-recovery runbook with clarified auditing, verification, and escalation. * **Tests** * Added/expanded integration and Helm rendering tests for the above scenarios and refusal/fail-closed behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Migration 50 takes a mandatory S3 safety snapshot before adopting live etcd onto the v1alpha2 operator. It is a
pre-upgradehook, so every chart-rendered resource it reads still belongs to the version being upgraded from. It resolved the snapshot target from the livecozy-default-etcdStrategy CR — and on a v1.5.x cluster that CR cannot be trusted to describe a reachable target. The platform upgrade is gated behind the snapshot, so nothing can self-heal first: the correctedbackupstrategy-controller.endpointhelper only reconciles after the upgrade this hook blocks.Two failure modes, both a hard block
1. The Strategy CR does not exist.
strategy-etcd-default.yamlis guarded by{{- if $bucketName -}}, whose helperlookups the BucketClaim's.status.bucketName. That BucketClaim is created by the same chart, so on first render the lookup is empty and the Strategy is silently skipped. Helmlookuponly runs at install/upgrade time, so a cluster that reached 1.5.x in one hop never grows the CR on reconcile — a forced reconcile does not produce it.2. The Strategy CR exists carrying
http://seaweedfs-s3.tenant-root.svc.cozy.local:8333(the static v1.5.xbackupStorage.endpointdefault). Cozystack ships SeaweedFS withenableSecurity=true, so:8333is a TLS listener — plaintext against it fails the handshake and every snapshot dies. Fixing the scheme is not sufficient: asbackupstrategy-controller.endpointdocuments, the Etcd Strategy's S3 schema has nocaCert/insecureSkipVerify, so the self-signed in-cluster endpoint is unusable at any scheme. A trusted-cert endpoint is required.The fix
Take the coordinates from
cozy-backups-credsinstead. The projector writes it from the COSI-provisioned bucket's system credentials — bucket provisioning, not chart values — so it describes the live bucket regardless of which version rendered the charts. It carriesendpoint,bucketName,regionandforcePathStylealongside the AWS keys, andresolve_platform_backup_argsalready read this Secret to validate those keys, so preferring it adds no new dependency.Both modes disappear: mode 1 because the CR is no longer needed, mode 2 because its endpoint is ignored.
Classifying COSI vs. external S3
The projected endpoint is a bare host fronted by the always-TLS S3 ingress, so on the platform-managed path we force
https://— exactly whatbackupstrategy-controller.endpointdoes. But the admin-managed external-S3 case (provisionBucket: false) must keep.Values.backupStorage.endpointverbatim, since it may legitimately be plaintext against a private store. The two therefore have to be told apart.Endpoint presence cannot do that.
ProjectBackupCredentialssubstitutesBACKUP_STORAGE_ENDPOINTwhen the source Secret is silent and then fails loud rather than projecting an endpoint-less Secret, so a projectedendpointkey is always present and always scheme-stripped — for external S3 too. Classifying on its absence would forcehttps://onto a plaintext external store and never consult the CR, which is a regression against a supported, documented configuration.Instead the hook asks whether the bucket the credentials name is one COSI actually provisioned: it matches the projected
bucketNameagainst.status.bucketNameof any BucketClaim. That is keyed on data rather than names, so it is independent ofbackupStorage.namespace/.bucketName/bucketNameOverride— all supported Package-CR overrides the hook cannot see — and a claim-name lookup would misclassify a renamed bucket as external.Resolution is then:
https://On the external path every coordinate comes from the CR, never a Secret/CR hybrid:
strategy-etcd-default.yamlpairsbucketwithcredentialsSecretRef: cozy-backups-creds, so CR coordinates plus the projected credentials is exactly the pairing the cluster's real etcd BackupJobs already use.Deliberate behaviour changes
Two states now refuse where
mainproceeded. Both are cases where the classifier cannot read its input, and adopting live etcd is irreversible, so it fails closed with an actionable message rather than guessing:bucketclaim-protectionfinalizer wedges the corpse permanently. Both readings are reachable and guessing wrong in either direction also ends in a block — just with a confusing TLS/handshake error instead of a precise one. Remedy: let COSI reap the claim, or clear its finalizer, then re-run (migration 50 is idempotent).The escape hatch is now reachable
The script has always supported
ETCD_ADOPT_SKIP_BACKUP=1for clusters that intentionally have no backup storage, butmigration-hook.yamlpassed no such variable, so the failure paths advised an action the platform made impossible. It is now plumbed asmigrations.etcdAdoptSkipBackup, an explicit opt-in defaulting to off, and the operator-facing messages name that knob rather than a variable nobody can set. It also unblocks the two refuse states above.The template normalises to the exact string the script matches: a bare bool renders
true, which the script's= "1"test would accept into the Job spec and then silently ignore — the worst outcome for a valve reached for under duress. Anything unrecognised resolves to"0", so taking the hatch requires an affirmative spelling and never a typo. A Platform Package predating the key renders cleanly as off.The script's own check was widened alongside it to accept
1|true|yescase-insensitively, so a hand-run image honours what an operator actually types. That is a behaviour change beyond plumbing, and it has a cost worth naming: if the template's normalisation were ever dropped, a raw bool would previously have failed safe (snapshot taken) and now fails unsafe (snapshot skipped). The rendered value is pinned by a unit test to prevent that.migration-hook.yamlhad no test coverage at all before this: the Job renders only when alookupofcozystack-versionreports a version below target, andlookupreturns nil underhelm template, so lint and CI never rendered the Job. That is structurally why an unreachable escape hatch shipped unnoticed. The new suite mocks the ConfigMap so the Job is rendered and its env asserted by value.Tests
The bats fixture modelled
cozy-backups-credsas carrying only the AWS keys, and test 1 asserted the broken plaintext endpoint as expected output — written against the implementation rather than the requirement, against a mock that did not match a real cluster, which is why this shipped green. The fixture now models what the projector and COSI really produce.Added, each verified to fail before the corresponding fix and pass after:
https://from the projected coordinates, and the in-cluster host never appearsSuite: 15 → 28, plus a new helm-unittest suite covering the hook's rendered env (chart: 81 → 88 tests).
Validation
Reproduced end-to-end on a throwaway 3-node stand (fresh v1.4.5 → v1.5.3 → this branch's build). Before: mode 1, version stamp stuck,
cozy-backupsBucket Ready,backupstrategy-controllerReady, creds projected, zero Strategy CRs. After: the stamp advanced and the hook completed, with endpoint, bucket, region and path-style all derived from the projected Secret and no Strategy CR in existence. Failure was safe in both cases — it refuses before any mutation: etcd stayed 3/3 Running, the legacy CR un-adopted, no scale-down. The derivedhttps://endpoint reproduces the 1.6 chart helper's own output byte-for-byte, and the projectedbucketNameequals the BucketClaim's.status.bucketNameon a live stand.Follow-ups this PR deliberately does not do
if $bucketNamelookup race instrategy-etcd-default.yaml, so the Strategy exists at all on affected clusters. This PR makes migration 50 independent of it, but the missing CR likely means default etcd backups are silently broken on 1.5.x clusters, upgrade or not — the backup e2e is.disabled, so CI cannot see it. Possibly related to cozy-default BackupClass S3 endpoint (internal seaweedfs-s3.tenant-root.svc) unreachable from network-isolated child tenants #3256. Worth its own issue.Summary by CodeRabbit
migrations.etcdAdoptSkipBackupto control whether the migration job skips pre-adoption snapshot safety checks.ETCD_ADOPT_SKIP_BACKUPnow honors true/false and rejects unrecognized values (does not skip by accident).ETCD_ADOPT_SKIP_BACKUPenv-var mapping and defaulting.