Skip to content

CNV-94727: fix bad conversion on upgrade #4493

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kubevirt-prow merged 1 commit into from
Aug 11, 2026
+133 −33
Merged

CNV-94727: fix bad conversion on upgrade #4493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 61 additions & 1 deletion pkg/webhooks/mutator/hyperConvergedMutator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package mutator

import (
"context"
"encoding/json"
"errors"
"fmt"
"net/http"
"slices"
Expand All @@ -10,6 +12,8 @@ import (
"github.com/go-logr/logr"
"gomodules.xyz/jsonpatch/v2"
admissionv1 "k8s.io/api/admission/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"

Expand Down Expand Up @@ -100,9 +104,22 @@ func (hcm *HyperConvergedMutator) mutateHyperConverged(req admission.Request, lo
}

oldHC = &hcov1.HyperConverged{}

if err = hcm.decoder.DecodeRaw(req.OldObject, oldHC); err != nil {
logger.Error(err, "failed to read the old HyperConverged custom resource")
return admission.Errored(http.StatusBadRequest, fmt.Errorf("failed to parse the old HyperConverged"))
if jsonErr, ok := errors.AsType[*json.UnmarshalTypeError](err); ok && jsonErr.Field == "spec.featureGates" {
// For some unknown reasons, sometimes during an upgrade from v1.18, the featureGates becomes an empty
// object instead of an empty array.
// Trying to recover from this edge case by removing the featureGates field.
logger.Info("trying to recover from featureGates with wrong format")
var recoverErr error
if patches, recoverErr = hcm.recoverBadFeatureGates(req, hc, oldHC, logger, patches); recoverErr != nil {
logger.Error(recoverErr, "failed to recover old object's featureGates with wrong format")
return admission.Errored(http.StatusBadRequest, fmt.Errorf("failed to parse the old HyperConverged"))
}
} else {
return admission.Errored(http.StatusBadRequest, fmt.Errorf("failed to parse the old HyperConverged"))
}
}

for _, fieldAndFG := range fieldFGDetails {
Expand Down Expand Up @@ -359,3 +376,46 @@ func compareFGPatches(a, b jsonpatch.JsonPatchOperation) int {

return bIdx - aIdx
}

func (hcm *HyperConvergedMutator) recoverBadFeatureGates(req admission.Request, hc, oldHC *hcov1.HyperConverged, logger logr.Logger, patches []jsonpatch.JsonPatchOperation) ([]jsonpatch.JsonPatchOperation, error) {
unstructuredObj := &unstructured.Unstructured{}
if err := hcm.decoder.DecodeRaw(req.OldObject, unstructuredObj); err != nil {
return nil, err
}

spec, ok := unstructuredObj.Object["spec"]
if !ok {
// should never get here
return nil, errors.New("spec field is missing")
}

specMap, ok := spec.(map[string]any)
if !ok {
return nil, errors.New("spec field is not an object")
}

fgs := specMap["featureGates"]
if fgs == nil {
return nil, errors.New("featureGates field is missing")
}

if fgsObj, ok := fgs.(map[string]any); ok && len(fgsObj) == 0 {
logger.Info("featureGates field is in wrong format; fixing it")
specMap["featureGates"] = nil
if len(hc.Spec.FeatureGates) == 0 {
patches = append(patches, jsonpatch.JsonPatchOperation{
Operation: "remove",
Path: featureGatesPath,
})
}
} else {
fgsBytes, err := json.Marshal(specMap["featureGates"])
if err != nil {
logger.Info("can't find the known featureGates issue")
} else {
logger.Info("can't find the known featureGates issue; the featureGates field is " + string(fgsBytes))
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

return patches, runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredObj.Object, oldHC)
}
104 changes: 72 additions & 32 deletions pkg/webhooks/mutator/hyperConvergedMutator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package mutator

import (
"context"
"encoding/json"
"fmt"
"os"

Expand All @@ -11,7 +12,6 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/utils/ptr"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"

Expand Down Expand Up @@ -47,7 +47,7 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
},
Spec: hcov1.HyperConvergedSpec{
Virtualization: hcov1.VirtualizationConfig{
EvictionStrategy: ptr.To(kubevirtcorev1.EvictionStrategyLiveMigrate),
EvictionStrategy: new(kubevirtcorev1.EvictionStrategyLiveMigrate),
},
},
}
Expand Down Expand Up @@ -142,7 +142,7 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
Value: 1,
},
}),
Entry("retentionPolicy is missing", &cdiv1beta1.DataImportCronSpec{ImportsToKeep: ptr.To[int32](1)}, []jsonpatch.JsonPatchOperation{
Entry("retentionPolicy is missing", &cdiv1beta1.DataImportCronSpec{ImportsToKeep: new(int32(1))}, []jsonpatch.JsonPatchOperation{
{
Operation: "add",
Path: fmt.Sprintf(dictsPathTemplate+retentionPolicyPath, 0),
Expand All @@ -151,7 +151,7 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
}),
Entry("importsToKeep is missing",
&cdiv1beta1.DataImportCronSpec{
RetentionPolicy: ptr.To(cdiv1beta1.DataImportCronRetainNone),
RetentionPolicy: new(cdiv1beta1.DataImportCronRetainNone),
},
[]jsonpatch.JsonPatchOperation{
{
Expand Down Expand Up @@ -193,7 +193,7 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
Annotations: map[string]string{goldenimages.CDIImmediateBindAnnotation: "false"},
},
Spec: &cdiv1beta1.DataImportCronSpec{
ImportsToKeep: ptr.To[int32](1),
ImportsToKeep: new(int32(1)),
},
},
{
Expand All @@ -202,7 +202,7 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
Annotations: map[string]string{goldenimages.CDIImmediateBindAnnotation: "false"},
},
Spec: &cdiv1beta1.DataImportCronSpec{
RetentionPolicy: ptr.To(cdiv1beta1.DataImportCronRetainNone),
RetentionPolicy: new(cdiv1beta1.DataImportCronRetainNone),
},
},
{
Expand Down Expand Up @@ -258,7 +258,7 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
Context("Check defaults for cluster level EvictionStrategy", func() {

DescribeTable("check EvictionStrategy default", func(ctx context.Context, SNO bool, strategy *kubevirtcorev1.EvictionStrategy, patches []jsonpatch.JsonPatchOperation) {
cr.Status.InfrastructureHighlyAvailable = ptr.To(!SNO)
cr.Status.InfrastructureHighlyAvailable = new(!SNO)

cr.Spec.Virtualization.EvictionStrategy = strategy

Expand All @@ -281,17 +281,17 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
),
Entry("should not override EvictionStrategy if set and on SNO - 1",
true,
ptr.To(kubevirtcorev1.EvictionStrategyNone),
new(kubevirtcorev1.EvictionStrategyNone),
nil,
),
Entry("should not override EvictionStrategy if set and on SNO - 2",
true,
ptr.To(kubevirtcorev1.EvictionStrategyLiveMigrate),
new(kubevirtcorev1.EvictionStrategyLiveMigrate),
nil,
),
Entry("should not override EvictionStrategy if set and on SNO - 3",
true,
ptr.To(kubevirtcorev1.EvictionStrategyExternal),
new(kubevirtcorev1.EvictionStrategyExternal),
nil,
),
Entry("should set EvictionStrategyLiveMigrate if not set and not on SNO",
Expand All @@ -305,17 +305,17 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
),
Entry("should not override EvictionStrategy if set and not on SNO - 1",
false,
ptr.To(kubevirtcorev1.EvictionStrategyNone),
new(kubevirtcorev1.EvictionStrategyNone),
nil,
),
Entry("should not override EvictionStrategy if set and not on SNO - 2",
false,
ptr.To(kubevirtcorev1.EvictionStrategyLiveMigrate),
new(kubevirtcorev1.EvictionStrategyLiveMigrate),
nil,
),
Entry("should not override EvictionStrategy if set and not on SNO - 3",
false,
ptr.To(kubevirtcorev1.EvictionStrategyExternal),
new(kubevirtcorev1.EvictionStrategyExternal),
nil,
),
)
Expand Down Expand Up @@ -455,7 +455,7 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
Value: 1,
},
}),
Entry("retentionPolicy is missing", &cdiv1beta1.DataImportCronSpec{ImportsToKeep: ptr.To[int32](1)}, []jsonpatch.JsonPatchOperation{
Entry("retentionPolicy is missing", &cdiv1beta1.DataImportCronSpec{ImportsToKeep: new(int32(1))}, []jsonpatch.JsonPatchOperation{
{
Operation: "add",
Path: fmt.Sprintf(dictsPathTemplate+retentionPolicyPath, 0),
Expand All @@ -464,7 +464,7 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
}),
Entry("importsToKeep is missing",
&cdiv1beta1.DataImportCronSpec{
RetentionPolicy: ptr.To(cdiv1beta1.DataImportCronRetainNone),
RetentionPolicy: new(cdiv1beta1.DataImportCronRetainNone),
},
[]jsonpatch.JsonPatchOperation{
{
Expand All @@ -485,8 +485,8 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
},
Spec: &cdiv1beta1.DataImportCronSpec{
// same as the HCO's default values; should not override existing value
RetentionPolicy: ptr.To(cdiv1beta1.DataImportCronRetainNone),
ImportsToKeep: ptr.To[int32](1),
RetentionPolicy: new(cdiv1beta1.DataImportCronRetainNone),
ImportsToKeep: new(int32(1)),
},
},
{
Expand All @@ -496,8 +496,8 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
},
Spec: &cdiv1beta1.DataImportCronSpec{
// same as the CDI's default values; should not override existing value
RetentionPolicy: ptr.To(cdiv1beta1.DataImportCronRetainAll),
ImportsToKeep: ptr.To[int32](3),
RetentionPolicy: new(cdiv1beta1.DataImportCronRetainAll),
ImportsToKeep: new(int32(3)),
},
},
{
Expand All @@ -507,8 +507,8 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
},
Spec: &cdiv1beta1.DataImportCronSpec{
// same as the HCO's default values; should not override existing value
RetentionPolicy: ptr.To(cdiv1beta1.DataImportCronRetainNone),
ImportsToKeep: ptr.To[int32](1),
RetentionPolicy: new(cdiv1beta1.DataImportCronRetainNone),
ImportsToKeep: new(int32(1)),
},
},
{
Expand All @@ -518,8 +518,8 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
},
Spec: &cdiv1beta1.DataImportCronSpec{
// same as the HCO's default values; should not override existing value
RetentionPolicy: ptr.To(cdiv1beta1.DataImportCronRetainNone),
ImportsToKeep: ptr.To[int32](1),
RetentionPolicy: new(cdiv1beta1.DataImportCronRetainNone),
ImportsToKeep: new(int32(1)),
},
},
{
Expand All @@ -528,7 +528,7 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
Annotations: map[string]string{goldenimages.CDIImmediateBindAnnotation: "false"},
},
Spec: &cdiv1beta1.DataImportCronSpec{
ImportsToKeep: ptr.To[int32](1),
ImportsToKeep: new(int32(1)),
},
},
{
Expand All @@ -537,7 +537,7 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
Annotations: map[string]string{goldenimages.CDIImmediateBindAnnotation: "false"},
},
Spec: &cdiv1beta1.DataImportCronSpec{
RetentionPolicy: ptr.To(cdiv1beta1.DataImportCronRetainNone),
RetentionPolicy: new(cdiv1beta1.DataImportCronRetainNone),
},
},
{
Expand Down Expand Up @@ -589,11 +589,51 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
}))
})

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}))
})
Comment on lines +592 to +630

@coderabbitai coderabbitai Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.go

Repository: 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/webhooks

Repository: 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()
PY

Repository: 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()
PY

Repository: 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


Context("Check defaults for cluster level EvictionStrategy", func() {

DescribeTable("check EvictionStrategy default", func(ctx context.Context, SNO bool, strategy *kubevirtcorev1.EvictionStrategy, patches []jsonpatch.JsonPatchOperation) {
origCR := cr.DeepCopy()
cr.Status.InfrastructureHighlyAvailable = ptr.To(!SNO)
cr.Status.InfrastructureHighlyAvailable = new(!SNO)

cr.Spec.Virtualization.EvictionStrategy = strategy

Expand All @@ -615,17 +655,17 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
),
Entry("should not override EvictionStrategy if set and on SNO - 1",
true,
ptr.To(kubevirtcorev1.EvictionStrategyNone),
new(kubevirtcorev1.EvictionStrategyNone),
nil,
),
Entry("should not override EvictionStrategy if set and on SNO - 2",
true,
ptr.To(kubevirtcorev1.EvictionStrategyLiveMigrate),
new(kubevirtcorev1.EvictionStrategyLiveMigrate),
nil,
),
Entry("should not override EvictionStrategy if set and on SNO - 3",
true,
ptr.To(kubevirtcorev1.EvictionStrategyExternal),
new(kubevirtcorev1.EvictionStrategyExternal),
nil,
),
Entry("should set EvictionStrategyLiveMigrate if not set and not on SNO",
Expand All @@ -639,17 +679,17 @@ var _ = Describe("test HyperConverged v1 mutator", func() {
),
Entry("should not override EvictionStrategy if set and not on SNO - 1",
false,
ptr.To(kubevirtcorev1.EvictionStrategyNone),
new(kubevirtcorev1.EvictionStrategyNone),
nil,
),
Entry("should not override EvictionStrategy if set and not on SNO - 2",
false,
ptr.To(kubevirtcorev1.EvictionStrategyLiveMigrate),
new(kubevirtcorev1.EvictionStrategyLiveMigrate),
nil,
),
Entry("should not override EvictionStrategy if set and not on SNO - 3",
false,
ptr.To(kubevirtcorev1.EvictionStrategyExternal),
new(kubevirtcorev1.EvictionStrategyExternal),
nil,
),
)
Expand Down