[v190] fix: volumeClaimTemplates annotation bypass on re-add

[martindekov]

Fixing volumeClaimTemplates bypassing the following scenarios:

• oldAnn being nil is now a separate case and once we assign storage class we make sure the new annotation is valid json format for further unmarshalling and comparison and storage class exists
• now we reject bad formats on the new annotation from the get go
• oldVM being nil was concern which we discussed but nil guard is done in upper level so nil poiner panic is not possible
• added storage class fakes which return error and extended test cases to cover all the discussed problematic scenarios; all existing tests pass meaning no changes in already existing validation logic only fixing the uncovered bugs

Problem:

Old check allowed if oldAnnotation is empty to bypass the check and allow injecting malformed value to the new annotation. Malformed being - non existing storage class/invalid json etc.

Solution:

Make sure we don't skip validation if oldAnnotation is empty and extract that case in separate check to validate the flow of new annotation being added with proper format.

Related Issue(s):

#10165

Test plan:

Unit tests to cover all discussed scenarios were added

E2E Test Plan:

1. SC name change - rejected
2. Storage size reduction - rejected
3. Normal disk add to existing annotation - allowed
4. Normal annotation removal - allowed
5. Re-add with tampered SC after deletion - is now rejection
6. Re-add with invalid JSON after deletion - rejected
7. Re-add with valid SC after deletion - allowed

E2E Test Results:

Built environment with the change and created vms and storage classes

node-1:/home/rancher # kubectl get storageclass
NAME                                      PROVISIONER          RECLAIMPOLICY   VOLUMEBINDINGMODE   ALLOWVOLUMEEXPANSION   AGE
default-sc (default)                      driver.longhorn.io   Delete          Immediate           true                   119s
harvester-longhorn                        driver.longhorn.io   Delete          Immediate           true                   10m
lh-869693c8-2368-4f27-8a4c-95e655a1c99b   driver.longhorn.io   Delete          Immediate           true                   88s
longhorn                                  driver.longhorn.io   Delete          Immediate           true                   9m46s
longhorn-static                           driver.longhorn.io   Delete          Immediate           true                   9m44s
sc-1                                      driver.longhorn.io   Delete          Immediate           true                   12s
sc-2                                      driver.longhorn.io   Delete          Immediate           true                   5s
vmstate-persistence                       driver.longhorn.io   Delete          Immediate           true                   10m
node-1:/home/rancher # kubectl get vm
NAME   AGE   STATUS    READY
vm-1   28s   Running   True

Store test values in variables

node-1:/home/rancher # VM=vm-1
node-1:/home/rancher # NS=default
node-1:/home/rancher # ORIG='[{"metadata":{"name":"vm-1-disk-0-bzdyy","annotations":{"harvesterhci.io/imageId":"default/image-rf8gs"}},"spec":{"accessModes":["ReadWriteMany"],"resources":{"requests":{"storage":"10Gi"}},"volumeMode":"Block","storageClassName":"lh-869693c8-2368-4f27-8a4c-95e655a1c99b"}}]'

🟢 1. SC name change - rejected

node-1:/home/rancher # kubectl annotate vm $VM -n $NS --overwrite \
>   harvesterhci.io/volumeClaimTemplates='[{"metadata":{"name":"vm-1-disk-0-bzdyy","annotations":{"harvesterhci.io/imageId":"default/image-rf8gs"}},"spec":{"accessModes":["ReadWriteMany"],"resources":{"requests":{"storage":"10Gi"}},"volumeMode":"Block","storageClassName":"sc-1"}}]'
Error from server (InternalError): admission webhook "validator.harvesterhci.io" denied the request: storage class names for the volumes in the harvesterhci.io/volumeClaimTemplates annotation cannot be changed; new name sc-1 in volume vm-1-disk-0-bzdyy;

🟢 2. Storage size reduction - rejected

node-1:/home/rancher # kubectl annotate vm $VM -n $NS --overwrite \
>   harvesterhci.io/volumeClaimTemplates='[{"metadata":{"name":"vm-1-disk-0-bzdyy","annotations":{"harvesterhci.io/imageId":"default/image-rf8gs"}},"spec":{"accessModes":["ReadWriteMany"],"resources":{"requests":{"storage":"5Gi"}},"volumeMode":"Block","storageClassName":"lh-869693c8-2368-4f27-8a4c-95e655a1c99b"}}]'
The request is invalid: metadata.annotations.harvesterhci.io/volumeClaimTemplates: vm-1-disk-0-bzdyy PVC requests storage can't be less than previous value

🟢 3. Normal disk add to existing annotation - allowed

node-1:/home/rancher # kubectl annotate vm $VM -n $NS --overwrite \
>   harvesterhci.io/volumeClaimTemplates='[{"metadata":{"name":"vm-1-disk-0-bzdyy","annotations":{"harvesterhci.io/imageId":"default/image-rf8gs"}},"spec":{"accessModes":["ReadWriteMany"],"resources":{"requests":{"storage":"10Gi"}},"volumeMode":"Block","storageClassName":"lh-869693c8-2368-4f27-8a4c-95e655a1c99b"}},{"metadata":{"name":"vm-1-disk-1-new","annotations":{"harvesterhci.io/imageId":"default/image-rf8gs"}},"spec":{"accessModes":["ReadWriteMany"],"resources":{"requests":{"storage":"10Gi"}},"volumeMode":"Block","storageClassName":"lh-869693c8-2368-4f27-8a4c-95e655a1c99b"}}]'
virtualmachine.kubevirt.io/vm-1 annotated

🟢 4. Normal annotation removal - allowed

node-1:/home/rancher # kubectl annotate vm $VM -n $NS harvesterhci.io/volumeClaimTemplates-
virtualmachine.kubevirt.io/vm-1 annotated

🟢 5. Re-add with tampered SC after deletion - is now rejection

node-1:/home/rancher # kubectl annotate vm $VM -n $NS \
>   harvesterhci.io/volumeClaimTemplates='[{"metadata":{"name":"vm-1-disk-0-bzdyy","annotations":{"harvesterhci.io/imageId":"default/image-rf8gs"}},"spec":{"accessModes":["ReadWriteMany"],"resources":{"requests":{"storage":"10Gi"}},"volumeMode":"Block","storageClassName":"non-existent-sc"}}]'
The request is invalid: metadata.annotations.harvesterhci.io/volumeClaimTemplates: storage class non-existent-sc does not exist

🟢 6. Re-add with invalid JSON after deletion - rejected

node-1:/home/rancher # kubectl annotate vm $VM -n $NS \
>   harvesterhci.io/volumeClaimTemplates='[{"broken json'
The request is invalid: metadata.annotations: the volumeClaimTemplates annotaion is invalid: unexpected end of JSON input

🟢 7. Re-add with valid SC after deletion - allowed

node-1:/home/rancher # kubectl annotate vm $VM -n $NS harvesterhci.io/volumeClaimTemplates="$ORIG"
virtualmachine.kubevirt.io/vm-1 annotated

Additional documentation or context

I wanted to fix the bug, but not change the validation behaviour so traced historical evolution of the function body:

#2810 - added to address the original issue - core logic being added
#8772 - storage class validation logic building on top of the previous assumptions
#8669 - mostly refactoring but no logical changes
#10165 - the actual bug and current change

[Copilot]

Pull request overview

This PR fixes a VM webhook validation gap where harvesterhci.io/volumeClaimTemplates could be deleted and then re-added with malformed content (invalid JSON or non-existent StorageClass) because the prior logic skipped validation when the old annotation was empty.

Changes:

• Adjust checkVolumeAnnotations to always unmarshal/validate the new annotation when it changes, including the “old annotation empty” re-add case.
• Add explicit StorageClass existence validation for first-time (re-)adds of the annotation.
• Extend unit tests and add a fake StorageClass cache that can simulate internal cache errors.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
pkg/webhook/resources/virtualmachine/validator.go	Fixes the bypass by validating new annotations even when the old annotation is empty; adds StorageClass existence checks for first-time adds.
pkg/webhook/resources/virtualmachine/validator_test.go	Adds/extends update-validation test cases, including malformed JSON, missing StorageClass, and internal cache error scenarios.
pkg/util/fakeclients/storageclass.go	Adds an ErroringStorageClassCache test helper to simulate cache failures.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[WebberHuang1118]

LGTM. Just one refactoring suggestion—thanks for the enhancement.

[w13915984028]

v180 added the supporting of datavolume (sc is nested under datavolume), need to double check the datavolume case, if it is also encoded into the annotation, thanks.

e.g.

harvester/pkg/api/volume/handler.go

Line 485 in fdca388

dv := &cdiv1.DataVolume{