Conversation
Some objects persisted before v1.19 have spec.featureGates stored as an
empty JSON object ("{}") instead of an empty array. The mutating webhook
already has a narrow recovery for this shape (recoverBadFeatureGates),
but only when it hits an old-object decode failure during an Update
admission request.
The CRD conversion webhook has no such safety net: controller-runtime
decodes the raw stored (hub) bytes into hcov1.HyperConverged before our
ConvertFrom/ConvertTo ever run, so any HyperConverged with this legacy
shape becomes permanently unreadable via the v1beta1 API.
Give HyperConvergedFeatureGates a custom UnmarshalJSON that treats an
empty JSON object as an empty list, fixing every decode path uniformly
instead of just the admission-webhook update path.
Fixes kubevirt#4549
> [!Note]
> Responses generated with Claude
Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubevirt-prow
Bot
added
do-not-merge/work-in-progress
|
Invalid commit message issues detected Invalid commit messagesKeywords which can automatically close issues and hashtag(#) mentions are not allowed.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
kubevirt-prow
Bot
added
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @kaovilai. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @kaovilai. Thanks for your PR. I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
kubevirt-prow
Bot
removed
the
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change adds custom JSON decoding for Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to The change safely restores decoding of legacy empty feature-gate objects while continuing to reject non-empty malformed objects, reducing conversion failures for affected resources without broadening feature-gate behavior. The PR is not currently merge-ready because required release-note, commit-message, and test-approval gates remain outstanding. Assessment against linked issues
Warning Linked repositories: Your configuration references 13 linked repositories, but your current plan allows 10. Analyzed Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. Analyzed 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 |
|
/ok-to-test Thanks you very much for this PR @kaovilai ! I'm hunting this issue for a long time now. I couldn't find the root cause. I mean - yes, somehow the stored v1 object is with In the meantime, please fix the commit message and add the missing release-notes to the PR description, because currently the PR is blocked. |
openshift-ci
Bot
added
ok-to-test
|
@kaovilai: The following test failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
OK, the logic added to pkg/webhooks/mutator/hyperConvergedMutator.go in #4493 is now redundant and should be removed. However, let's keep the |
|
@kaovilai: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
hco-e2e-consecutive-operator-sdk-upgrades-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure, ci/prow/hco-e2e-operator-sdk-gcp, ci/prow/hco-e2e-upgrade-operator-sdk-aws DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
hco-e2e-upgrade-prev-operator-sdk-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
hco-e2e-kv-smoke-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-gcp DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
I found the actual root cause. Details are here: #4549 (comment) Giving this new information, the fix here is not enough, and will hide other issues. |
|
/hold |
kubevirt-prow
Bot
added
the
do-not-merge/hold
|
Opened #4557 to try to solve this issue. |
|
Ok. thanks for review. |
|
Superceded by 4557 |
3 participants
Summary
Root cause of #4549: some objects persisted before v1.19 have
spec.featureGatesstored as an empty JSON object ({}) instead of an empty array — a legacy artifact of an older, struct-based representation.The mutating webhook already has a narrow recovery for this shape (
recoverBadFeatureGatesinpkg/webhooks/mutator/hyperConvergedMutator.go), but only when it hits an old-object decode failure during anUpdateadmission request.The CRD conversion webhook has no equivalent safety net:
controller-runtimedecodes the raw stored (hub,v1) bytes intohcov1.HyperConvergedbeforeConvertFrom/ConvertToever run (seevendor/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/conversion.go'shandleConvertRequest), so anyHyperConvergedobject with this legacy shape becomes permanently unreadable via thev1beta1API — reproducing the exact error from the issue on every read.Fix
Give
HyperConvergedFeatureGates(api/v1/featuregates/feature_gates.go) a customUnmarshalJSONthat treats an empty JSON object as an empty list, and returns a real error for a non-empty object (so genuine corruption isn't silently swallowed). Since this is a method on the type itself, it fixes every decode path uniformly (conversion webhook, mutating webhook, reconciler, apiserver) instead of only the one admission-webhook code path that already had a workaround.Testing
api/v1/featuregates/feature_gates_test.go: unmarshal of{}(bare and nested in a struct field, mirroring the webhook decode) now succeeds as an empty list; a non-empty object is still rejected.api/v1beta1/conversion_test.go: reproduces the exact issue scenario —json.Unmarshalof raw hub-version bytes with"featureGates":{}"now succeeds, and the object converts viaConvertFromcleanly.main, and pass with this fix.Fixes #4549
Note
Responses generated with Claude