fix: split migrate-to-v1.sh into pre/post phases to fix username backfill - #164
Open
barunavo wants to merge 6 commits into
Open
fix: split migrate-to-v1.sh into pre/post phases to fix username backfill#164barunavo wants to merge 6 commits into
barunavo wants to merge 6 commits into
Conversation
…fill Signed-off-by: barunavo <barunavopal@outlook.com>
…ration Signed-off-by: barunavo <barunavopal@outlook.com>
Signed-off-by: barunavo <barunavopal@outlook.com>
Signed-off-by: barunavo <barunavopal@outlook.com>
…pecs Two more gaps found via live testing, on top of the CRD-transition fix: 1. Post-phase Step 1 patched username/authDatabaseName but never checked the patch actually stuck. If an owning composite's pause hasn't taken effect, or a patch is otherwise silently dropped, the script reported success while the object still failed v1.x's CEL requirement. Now it re-reads the object after patching and fails loudly (listing every affected CR in the backup dir) instead of continuing silently. 2. Post-phase Step 2 only "touched" AdvancedCluster CRs to trigger re-storage, assuming CRD version conversion would reshape the object. It doesn't: this provider has no conversion webhook, so Kubernetes' default (None) conversion is a pure passthrough. electableSpecs, autoScaling, analyticsAutoScaling, readOnlySpecs and analyticsSpecs are all arrays-of-one-object in v1alpha2 and plain objects in v1alpha3 - a touch alone gets the mismatched array pruned to nothing, losing every field in it (not just the new diskSizeGb location). Same touch also can't backfill v1alpha3's new required spec.forProvider.name, which has no v1alpha2 equivalent at all. Step 2 now reads each CR's pre-migration shape from the pre-phase's own backup, builds the reshaped v1alpha3 structure explicitly (moving diskSizeGb into electableSpecs/readOnlySpecs/analyticsSpecs, converting the five array fields to objects), derives name from the CR's own external-name (matching the "<name>:<cluster_id>" format config/mongodbatlas/config.go already uses for this resource), and verifies the patch persisted the same way Step 1 does. Verified end-to-end against a real apiserver (kind) with the actual v0.4.1 and v1.0.0 CRDs: pre + post now leave both a DatabaseUser and an AdvancedCluster fully valid under v1alpha3, with diskSizeGb correctly relocated and no data loss in electableSpecs/autoScaling. Signed-off-by: barunavo <barunavopal@outlook.com>
…them post's backfill/reshape steps previously attempted every CR unconditionally, relying on the new post-patch verification alone to catch a still-live composite silently overwriting the result. That's backwards for a claim that's genuinely live (GitOps-managed, an owner still actively reconciling): the safer move is to not touch it at all, and say so clearly, rather than attempt the patch and hope the race doesn't land. pre already records exactly this in unpaused-owners.txt when a CR or its owning composite doesn't confirm ReconcilePaused. post now reads that file via a new is_unpaused() check before processing each User/AdvancedCluster, and skips (recording it in backfill-verification-failed.txt, so it's still surfaced in the final failure list) rather than patching. Unit-tested directly: a CR listed by name, a CR whose owner is listed, and a CR that's neither are all handled correctly. Signed-off-by: barunavo <barunavopal@outlook.com>
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
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.
Summary
Fixes #163.
Update 3: the previous update's fixes only made silent overwrites fail
loudly after attempting a patch. That's still the wrong default for a
genuinely live claim - one actively reconciled by GitOps or a still-busy
owning composite - where the right move is to not touch it at all rather
than gamble on the race not landing.
prealready records exactly this case: if a CR or its owning compositedoesn't confirm
ReconcilePausedwithin the timeout, it's logged tounpaused-owners.txt.postnow actually reads that file (is_unpaused())before processing each
User/AdvancedCluster, and skips it entirely -still recording the skip in the same failure list that blocks proceeding to
install v1.x - instead of patching and relying on the post-verification
alone. Unit-tested directly against the three cases (CR listed by name, CR
whose owner is listed, CR that's neither).
Update 2: live testing against a real cluster (not the maintainer's, a
different one) surfaced two more real gaps, fixed in the latest commit:
Post-phase Step 1 never verified the username patch actually stuck.
In one observed run,
kubectl patchreported success andauthDatabaseNamepersisted correctly from the same patch call, butusernamecame back empty on a freshget- a config-error we haven'tfully root-caused (the owning composite was independently confirmed to
be genuinely paused at the time, ruling out the composite-overwrite race
the previous update in this PR described - so it isn't that, at least
not in every case). Rather than guess further, Step 1 now re-reads the
object after patching and fails loudly - listing every affected CR - if
it doesn't match, instead of reporting success either way. This turns
any future occurrence of this (whatever the exact cause) into a clear,
actionable failure instead of a silent one that only surfaces once
v1.x's CEL validation rejects the CR.
If you're already stuck with a CR in this state: the CRD conversion
itself did finish correctly in the case we looked at (it now serves
only
v1alpha3), so a directkubectl patch ... -p '{"spec":{"forProvider":{"username":"<name>"}}}'should work today -the schema has the field now, and if the owning composite is genuinely
paused there's nothing left to race with. Verify with a fresh
getafterward regardless.
Post-phase Step 2 only touched AdvancedCluster CRs, assuming CRD
conversion would reshape the object. It doesn't: this provider has no
conversion webhook, so Kubernetes' default (
None) conversion is a purepassthrough.
electableSpecs,autoScaling,analyticsAutoScaling,readOnlySpecsandanalyticsSpecsare all arrays-of-one-object inv1alpha2and plain objects inv1alpha3- a bare touch gets themismatched array pruned to nothing by structural-schema validation,
losing every field in it (not just failing to add the new
diskSizeGblocation -
instanceSize/nodeCount/etc. are lost too). The same touchalso can't backfill
v1alpha3's new requiredspec.forProvider.name,which has no
v1alpha2equivalent at all and is enforced by a CEL rule.Step 2 now reads each CR's pre-migration shape from the pre-phase's own
backup, builds the reshaped
v1alpha3structure explicitly (movingdiskSizeGbintoelectableSpecs/readOnlySpecs/analyticsSpecs,converting the five array fields to objects), derives
namefrom theCR's own
external-name(matching the"<name>:<cluster_id>"formatconfig/mongodbatlas/config.goalready uses for this resource), andverifies the patch persisted the same way Step 1 does.
Verified end-to-end against a real apiserver (
kind, not mocked) using theactual
v0.4.1andv1.0.0CRDs:pre+postnow leave both aDatabaseUserand anAdvancedClusterfully valid underv1alpha3, withdiskSizeGbcorrectly relocated and no data loss inelectableSpecs/autoScaling.Update 1 (previous): the first version of this PR (commit 0cb64f3) turned out to be insufficient. Testing it end-to-end against a real cluster (not just dry-run) surfaced two further problems, fixed in that revision:
The same chicken-and-egg problem, just relocated.
spec.forProvider.usernamedoesn't exist in the v0.x (v1alpha2) schema — only in v1.x'sv1alpha3, which replacesv1alpha2rather than adding alongside it. Moving the backfill to apostphase (after installing v1.x) doesn't help: Kubernetes flatly refuses to install a CRD that drops a version still listed instatus.storedVersions. There's no point in the naive sequence where the field can be written. The only way through is a transitional CRD — built by merging the live CRD with the target v1.x CRD's version block, applied directly viakubectl apply(bypassing Crossplane's package manager, which only ships the final single-version CRD) — that serves bothv1alpha2andv1alpha3side by side. This lets existing objects be re-stored atv1alpha3(andusernamebackfilled) beforev1alpha2is dropped and the real v1.x provider is installed.Pausing the leaf CR isn't enough. If a Crossplane Composition owns the
User/AdvancedClusterCR (the normal way these are consumed on a real platform), pausing the CR only stops the provider's reconciler — it does not stop the owning Composite Resource (XR) from continuing to render and re-apply its (v1alpha2-shaped,username-less)basetemplate over it, silently wiping the backfilled field on the next reconcile. Confirmed live: backfilledusername, moved on, came back later, and it was gone — the owning XR'sComposeResourceshad overwritten it in between. The script now discovers each CR's controlling owner (ownerReferences[]wherecontroller: true) and pauses that too.What changed
scripts/migrate-to-v1.shnow takes an explicit phase argument and a--v1-crds-dir:pre: backs up CRs, pauses each CR and its owning composite (if any), builds and applies a transitional dual-version CRD from the live CRD + the v1.x CRD YAML you provide.post: backfillsusername/authDatabaseName, touchesAdvancedClusters to trigger re-storage atv1alpha3, removesv1alpha2fromstatus.storedVersions, then dropsv1alpha2fromspec.versionsentirely so the CRD matches what the real v1.x package will install.Also fixed in passing: the original script computed
existing_auth_dbbut never included it in the patch, despite the docs claiming it does.docs/migration-to-v1.mdrewritten to explain the transitional-CRD rationale and the composite-pausing requirement.Test plan
Verified live, end-to-end, against a real cluster running v0.4.1 with pre-existing
User/AdvancedClusterCRs owned by Crossplane Compositions (not synthetic test data):v1alpha2+v1alpha3,v1alpha3as storage) for bothusers.database.mongodbatlas.crossplane.ioandadvancedclusters.mongodbatlas.crossplane.io— confirmed Crossplane's provider-revision controller does not fight or revert a hand-applied CRD change.usernameagainst the livev1alpha3schema — confirmed it's silently dropped by the unpaused owning XR's next reconcile (reproducing problem Set default external name config as IdentifierFromProvider and a common referencer for project_id #2 above), and confirmed it sticks once the owning XR is actually paused.v1alpha2fromstoredVersions, then dropped it fromspec.versions— confirmed the real v1.x provider package then activatesHealthywith no CRD-establishment error (previously failed withcannot establish control of object: ... v1alpha2 was previously a storage version ...).UserCR reconciles cleanly (SYNCED=True, READY=True) under the real, running v1.0.0 provider controller.v1alpha2,spec.versions/storedVersionscleanup) and confirmed theUserCR resyncs cleanly under v0.4.1 too — proving the transitional-CRD technique is safe in both directions.bash -n scripts/migrate-to-v1.sh(syntax check)DRY_RUN=true ./scripts/migrate-to-v1.sh pre --v1-crds-dir ...correctly discovers and would-pause each CR's actual owning composite kind (verified against 4 different real XR kinds on the test cluster)Not verified:
--v1-crds-dirpointed at CRDs for resources with namespaced (*.m.crossplane.io) variants beyondusers/advancedclusters— the script'sAFFECTED_CRDSlist still only names these two resource families explicitly.