CNV-94727: fix bad conversion on upgrade - #4493
Conversation
kubevirt-prow
Bot
added
release-note-none
|
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 (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📝 WalkthroughWalkthroughThe update webhook now recovers old HyperConverged objects with an empty-object Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Warning Review ran into problems🔥 ProblemsLinked 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/webhooks/mutator/hyperConvergedMutator.go (1)
397-417: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse descriptive feature-gate variable names.
Rename
fgs,fgsObj, andfgsBytesto names such asfeatureGates,featureGatesObject, andfeatureGatesBytes. This avoids lowercase acronyms in new code.As per path instructions, “use fully upper-cased acronyms in names.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/webhooks/mutator/hyperConvergedMutator.go` around lines 397 - 417, Rename the local variables fgs, fgsObj, and fgsBytes in the feature-gates handling logic to descriptive names such as featureGates, featureGatesObject, and featureGatesBytes, updating all references while preserving the existing behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/webhooks/mutator/hyperConvergedMutator_test.go`:
- Around line 592-630: Add a negative update test alongside the existing bad
feature-gate recovery cases that corrupts a field outside spec.featureGates in
the old object, causing DecodeRaw to fail. Invoke mutator.Handle and assert the
response is rejected with HTTP 400, ensuring unrelated decode errors are not
recovered from.
In `@pkg/webhooks/mutator/hyperConvergedMutator.go`:
- Around line 412-417: Update the error-reporting branch in the featureGates
handling to stop logging req.OldObject.Raw and the serialized fgsBytes values.
Replace both concatenated messages with a safe static message describing the
unexpected featureGates format, while preserving the existing err-dependent
branching.
---
Nitpick comments:
In `@pkg/webhooks/mutator/hyperConvergedMutator.go`:
- Around line 397-417: Rename the local variables fgs, fgsObj, and fgsBytes in
the feature-gates handling logic to descriptive names such as featureGates,
featureGatesObject, and featureGatesBytes, updating all references while
preserving the existing behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a01eea01-aa05-4b00-829e-6141457b2008
📒 Files selected for processing (2)
pkg/webhooks/mutator/hyperConvergedMutator.gopkg/webhooks/mutator/hyperConvergedMutator_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
kubevirt/kubevirt(manual)kubevirt/hyperconverged-cluster-operator(manual)kubevirt/monitoring(manual)
| It("should recover from a bad featureGate format", func(ctx context.Context) { | ||
| origCR := cr.DeepCopy() | ||
|
|
||
| req := admission.Request{AdmissionRequest: newUpdateRequest(origCR, cr, testCodec)} | ||
|
|
||
| unstructuredObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&origCR) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| unstructuredObj["spec"].(map[string]any)["featureGates"] = map[string]any{} | ||
| badOrigHC, err := json.Marshal(unstructuredObj) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| req.OldObject.Raw = badOrigHC | ||
|
|
||
| res := mutator.Handle(ctx, req) | ||
| Expect(res.Allowed).To(BeTrue()) | ||
|
|
||
| Expect(res.Patches).ToNot(BeEmpty()) | ||
| Expect(res.Patches).To(ContainElement(jsonpatch.JsonPatchOperation{Operation: "remove", Path: featureGatesPath})) | ||
| }) | ||
|
|
||
| It("should recover from a bad featureGate format, if new HC contains FGs", func(ctx context.Context) { | ||
| origCR := cr.DeepCopy() | ||
| cr.Spec.FeatureGates.Enable("some-fake-fg") | ||
|
|
||
| req := admission.Request{AdmissionRequest: newUpdateRequest(origCR, cr, testCodec)} | ||
|
|
||
| unstructuredObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&origCR) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| unstructuredObj["spec"].(map[string]any)["featureGates"] = map[string]any{} | ||
| badOrigHC, err := json.Marshal(unstructuredObj) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| req.OldObject.Raw = badOrigHC | ||
|
|
||
| res := mutator.Handle(ctx, req) | ||
| Expect(res.Allowed).To(BeTrue()) | ||
|
|
||
| Expect(res.Patches).ToNot(ContainElement(jsonpatch.JsonPatchOperation{Operation: "remove", Path: featureGatesPath})) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline pkg/webhooks/mutator/hyperConvergedMutator_test.go --items all
rg -n -C 6 \
'OldObject\.Raw|featureGates|failed to parse the old HyperConverged|StatusBadRequest|Allowed\)\.To\(BeFalse' \
pkg/webhooks/mutator/hyperConvergedMutator_test.goRepository: kubevirt/hyperconverged-cluster-operator
Length of output: 3320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- DecodeRaw and rejection paths ---'
rg -n -C 10 'DecodeRaw|featureGates|Allowed: false|StatusBadRequest|failed to parse|bad feature' pkg/webhooks/mutator pkg/webhooks --glob '*.go' || true
printf '%s\n' '--- Test assertions across mutator tests ---'
rg -n -C 5 'Allowed\)\.To\(BeFalse|Allowed.*BeFalse|StatusBadRequest|DecodeRaw|OldObject\.Raw' pkg --glob '*_test.go' || true
printf '%s\n' '--- Mutator file map ---'
fd -i 'mutator' pkg/webhooksRepository: kubevirt/hyperconverged-cluster-operator
Length of output: 50397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
roots = [Path("pkg/webhooks/mutator"), Path("pkg/webhooks")]
for path in sorted({p for root in roots if root.exists() for p in root.rglob("*.go")}):
text = path.read_text()
if "DecodeRaw" in text or "OldObject.Raw" in text or "Allowed: false" in text:
print(f"\n--- {path} ---")
for i, line in enumerate(text.splitlines(), 1):
if any(token in line for token in ("DecodeRaw", "OldObject.Raw", "Allowed: false", "StatusBadRequest")):
lo, hi = max(1, i-8), min(len(text.splitlines()), i+12)
lines = text.splitlines()
for n in range(lo, hi+1):
print(f"{n}: {lines[n-1]}")
print()
PYRepository: kubevirt/hyperconverged-cluster-operator
Length of output: 25602
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in sorted(Path("pkg/webhooks").rglob("*.go")):
lines = path.read_text().splitlines()
hits = [i for i, line in enumerate(lines, 1)
if any(token in line for token in
("DecodeRaw", "OldObject.Raw", "Allowed: false", "StatusBadRequest"))]
if hits:
print(f"\n--- {path} ---")
for i in hits:
for n in range(max(1, i - 8), min(len(lines), i + 12) + 1):
print(f"{n}: {lines[n-1]}")
print()
PYRepository: kubevirt/hyperconverged-cluster-operator
Length of output: 26795
Add a negative test for unrelated decode errors.
Add an update case with a malformed old object that triggers a DecodeRaw error outside spec.featureGates. Assert that the response is rejected with HTTP 400.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/webhooks/mutator/hyperConvergedMutator_test.go` around lines 592 - 630,
Add a negative update test alongside the existing bad feature-gate recovery
cases that corrupts a field outside spec.featureGates in the old object, causing
DecodeRaw to fail. Invoke mutator.Handle and assert the response is rejected
with HTTP 400, ensuring unrelated decode errors are not recovered from.
Source: Path instructions
|
hco-e2e-upgrade-operator-sdk-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-operator-sdk-aws, ci/prow/hco-e2e-upgrade-prev-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. |
513f9ee to
6798f03
Compare
For some unknown reasons, sometimes during an upgrade from v1.18, the featureGates field becomes an empty object instead of an empty array. Trying to recover from this edge case by removing the featureGates field. Signed-off-by: Nahshon Unna Tsameret <nunnatsa@redhat.com>
6798f03 to
6ae39d1
Compare
|
Coverage Report for CI Build 31486248985Coverage decreased (-0.09%) to 85.512%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: orenc1 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubevirt-prow
Bot
added
the
approved
|
hco-e2e-operator-sdk-sno-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-sno-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. |
|
@nunnatsa: 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-upgrade-prev-operator-sdk-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-azure, ci/prow/hco-e2e-upgrade-operator-sdk-aws, ci/prow/hco-e2e-upgrade-prev-operator-sdk-aws, ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-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. |
4 participants
What this PR does / why we need it:
For some unknown reasons, sometimes during an upgrade from v1.18, the featureGates field becomes an empty object instead of an empty array.
Trying to recover from this edge case by removing the featureGates field.
Jira Ticket:
Release note: