Skip to content

json: Add backward compatibility with VfkitMagic #382

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

json: Add backward compatibility with VfkitMagic #382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0555c1c
test: Use slices.Contains
cfergeau Oct 8, 2025
f802d4e
config: Add generic FilterDevice function
cfergeau Oct 13, 2025
32f1dac
json: Add backward compatibility with VfkitMagic
cfergeau Oct 8, 2025
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
34 changes: 14 additions & 20 deletions pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,36 +161,30 @@ func (vm *VirtualMachine) AddDevicesFromCmdLine(cmdlineOpts []string) error {
return nil
}

func (vm *VirtualMachine) VirtioGPUDevices() []*VirtioGPU {
gpuDevs := []*VirtioGPU{}
func FilterDevices[V VMComponent](vm *VirtualMachine) []V {
devs := []V{}
for _, dev := range vm.Devices {
if gpuDev, isVirtioGPU := dev.(*VirtioGPU); isVirtioGPU {
gpuDevs = append(gpuDevs, gpuDev)
if dev, isV := dev.(V); isV {
devs = append(devs, dev)
}
}
return devs
}

return gpuDevs
func (vm *VirtualMachine) VirtioGPUDevices() []*VirtioGPU {
return FilterDevices[*VirtioGPU](vm)
}

func (vm *VirtualMachine) VirtioVsockDevices() []*VirtioVsock {
vsockDevs := []*VirtioVsock{}
for _, dev := range vm.Devices {
if vsockDev, isVirtioVsock := dev.(*VirtioVsock); isVirtioVsock {
vsockDevs = append(vsockDevs, vsockDev)
}
}

return vsockDevs
return FilterDevices[*VirtioVsock](vm)
}

func (vm *VirtualMachine) VirtioInputDevices() []*VirtioInput {
inputDevs := []*VirtioInput{}
for _, dev := range vm.Devices {
if inputDev, isVirtioInput := dev.(*VirtioInput); isVirtioInput {
inputDevs = append(inputDevs, inputDev)
}
}
return inputDevs
return FilterDevices[*VirtioInput](vm)
}

func (vm *VirtualMachine) VirtioNetDevices() []*VirtioNet {
return FilterDevices[*VirtioNet](vm)
}

func (vm *VirtualMachine) NetworkBlockDevice(deviceID string) *NetworkBlockDevice {
Expand Down
7 changes: 7 additions & 0 deletions pkg/config/json.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ func unmarshalIgnition(rawMsg json.RawMessage) (Ignition, error) {
func unmarshalVirtioNet(rawMsg json.RawMessage) (*VirtioNet, error) {
var dev virtioNetForMarshalling

// defaults to true for backwards compatibility with vfkit versions which did not have this field
dev.VfkitMagic = true

err := json.Unmarshal(rawMsg, &dev)
if err != nil {
return nil, err
Expand All @@ -121,6 +124,10 @@ func unmarshalVirtioNet(rawMsg json.RawMessage) (*VirtioNet, error) {
}
dev.VirtioNet.MacAddress = macAddr
}
// vfkitMagic is only useful in combination with unixSocketPath
if dev.UnixSocketPath == "" {
dev.VfkitMagic = false
}
Copy link
Collaborator

@nirs nirs Oct 12, 2025

Choose a reason for hiding this comment

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

Does it break anything if we keep the default true value?

Copy link
Contributor

@vyasgun vyasgun Oct 13, 2025

Choose a reason for hiding this comment

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

I don't think it should because this field is only used when UnixSocketPath is set.

Copy link
Collaborator Author

@cfergeau cfergeau Oct 13, 2025

Choose a reason for hiding this comment

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

This breaks one of the test cases:

--- FAIL: TestJSON (0.00s)
    --- FAIL: TestJSON/json (0.00s)
        --- FAIL: TestJSON/json/TestAllVirtioDevices (0.00s)
            json_test.go:402: 
                	Error Trace:	/var/home/teuf/dev/vfkit/pkg/config/json_test.go:402
                	            				/var/home/teuf/dev/vfkit/pkg/config/json_test.go:374
                	Error:      	Not equal: 
                	            	expected: config.VirtualMachine{Vcpus:0x3, Memory:0xfa000000, Bootloader:(*config.LinuxBootloader)(0xc00019e540), Devices:[]config.VirtioDevice{(*config.VirtioSerial)(0xc00019e570), (*config.VirtioInput)(0xc000029970), (*config.VirtioGPU)(0xc0000186c0), (*config.VirtioNet)(0xc000015400), (*config.VirtioRng)(0x8ea820), (*config.VirtioBlk)(0xc000015440), (*config.VirtioVsock)(0xc00007c9a0), (*config.VirtioFs)(0xc00007c9c0), (*config.USBMassStorage)(0xc00019e5d0), (*config.RosettaShare)(0xc000010b40), (*config.NetworkBlockDevice)(0xc0000d0ff0)}, Timesync:(*config.TimeSync)(nil), Ignition:(*config.Ignition)(nil), Nested:false}
                	            	actual  : config.VirtualMachine{Vcpus:0x3, Memory:0xfa000000, Bootloader:(*config.LinuxBootloader)(0xc0001b61b0), Devices:[]config.VirtioDevice{(*config.VirtioSerial)(0xc0001b6240), (*config.VirtioInput)(0xc0001ba6d0), (*config.VirtioGPU)(0xc0000189d8), (*config.VirtioNet)(0xc0000d10e0), (*config.VirtioRng)(0x8ea820), (*config.VirtioBlk)(0xc000015640), (*config.VirtioVsock)(0xc00007cb60), (*config.VirtioFs)(0xc00007cb80), (*config.USBMassStorage)(0xc0001b6a50), (*config.RosettaShare)(0xc000010ff0), (*config.NetworkBlockDevice)(0xc0000d1130)}, Timesync:(*config.TimeSync)(nil), Ignition:(*config.Ignition)(nil), Nested:false}
                	            	
                	            	Diff:
                	            	--- Expected
                	            	+++ Actual
                	            	@@ -32,3 +32,3 @@
                	            	    UnixSocketPath: (string) "",
                	            	-   VfkitMagic: (bool) false
                	            	+   VfkitMagic: (bool) true
                	            	   }),
                	Test:       	TestJSON/json/TestAllVirtioDevices
FAIL
FAIL	github.com/crc-org/vfkit/pkg/config	0.009s
FAIL

We could change the default even in the !unixsocketpath case, but then there are more tests to modify, json serialization of a virtionet device will always have vfkitMagic set even when it’s not used, …
In my opinion it’s preferrable to keep the default to false when unixSocketPath is not used.

return &dev.VirtioNet, nil
}

Expand Down
62 changes: 51 additions & 11 deletions pkg/config/json_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,12 @@ import (
"encoding/json"
"fmt"
"reflect"
"slices"
"testing"

"github.com/stretchr/testify/require"
)

func contains(strings []string, val string) bool {
for _, str := range strings {
if val == str {
return true
}
}

return false
}

// This sets all the fields of the `obj` struct to non-empty values.
// This will be used to test JSON serialization as extensively as possible to
// avoid breaking backwards compatibility.
Expand All @@ -34,7 +25,7 @@ func fillStruct(t *testing.T, obj interface{}, skipFields []string) {
fieldVal := val.FieldByIndex(e.Index)
typeName := val.Type().Name()

if contains(skipFields, field.Name) {
if slices.Contains(skipFields, field.Name) {
continue
}
switch fieldVal.Kind() {
Expand Down Expand Up @@ -339,6 +330,55 @@ type jsonStabilityTest struct {
expectedJSON string
}

Copy link
Collaborator

@nirs nirs Oct 12, 2025

Choose a reason for hiding this comment

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

It will help to add a comment that this old json does not include vfkitMagic and parsing it should use the default value.

func testVirtioNetVfkitMagicJson(t *testing.T, vfkitMagic bool, useVfkitMagicDefault bool) {
const (
jsonVfkitMagicDefault = `{"vcpus":3,"memoryBytes":4194304000,"bootloader":{"kind":"linuxBootloader","vmlinuzPath":"/vmlinuz","initrdPath":"/initrd","kernelCmdLine":"console=hvc0"},"devices":[{"kind":"virtionet","nat":false,"unixSocketPath":"/some/path/to/socket","macAddress":"00:11:22:33:44:55"}]}`
jsonVfkitMagicFalse = `{"vcpus":3,"memoryBytes":4194304000,"bootloader":{"kind":"linuxBootloader","vmlinuzPath":"/vmlinuz","initrdPath":"/initrd","kernelCmdLine":"console=hvc0"},"devices":[{"kind":"virtionet","nat":false,"unixSocketPath":"/some/path/to/socket","macAddress":"00:11:22:33:44:55","vfkitMagic":false}]}`
jsonVfkitMagicTrue = `{"vcpus":3,"memoryBytes":4194304000,"bootloader":{"kind":"linuxBootloader","vmlinuzPath":"/vmlinuz","initrdPath":"/initrd","kernelCmdLine":"console=hvc0"},"devices":[{"kind":"virtionet","nat":false,"unixSocketPath":"/some/path/to/socket","macAddress":"00:11:22:33:44:55","vfkitMagic":true}]}`
)
var jsonStr string
var unmarshalledVM VirtualMachine

if useVfkitMagicDefault {
jsonStr = jsonVfkitMagicDefault
} else {
if vfkitMagic {
jsonStr = jsonVfkitMagicTrue
} else {
jsonStr = jsonVfkitMagicFalse
}
}
err := json.Unmarshal([]byte(jsonStr), &unmarshalledVM)
require.NoError(t, err)
netDevs := unmarshalledVM.VirtioNetDevices()
require.Len(t, netDevs, 1)
require.Equal(t, vfkitMagic, netDevs[0].VfkitMagic)

vm := newLinuxVM(t)
dev, err := VirtioNetNew("00:11:22:33:44:55")
require.NoError(t, err)
dev.SetUnixSocketPath("/some/path/to/socket")
if !useVfkitMagicDefault {
dev.VfkitMagic = vfkitMagic
}
err = vm.AddDevice(dev)
require.NoError(t, err)
require.Equal(t, vfkitMagic, dev.VfkitMagic)

require.Equal(t, *vm, unmarshalledVM)
Copy link
Collaborator

@nirs nirs Oct 12, 2025

Choose a reason for hiding this comment

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

It will will help to comment about the check - I think what you try to do here is verify that json from older version is manipulated to match json created by current version.

}

func TestVirtioNetBackwardsCompat(t *testing.T) {
/* Check that the vfkitMagic default is true when deserializing json */
t.Run("VfkitMagicJsonDefault", func(t *testing.T) { testVirtioNetVfkitMagicJson(t, true, true) })

/* Check that the vfkitMagic default can be overridden */
t.Run("VfkitMagicJsonDefaultOverride", func(t *testing.T) { testVirtioNetVfkitMagicJson(t, false, false) })

/* Check that explicitly setting vfkitMagic to true works as expected */
t.Run("VfkitMagicJsonExplicitDefault", func(t *testing.T) { testVirtioNetVfkitMagicJson(t, true, false) })
}
Comment on lines +371 to +380
Copy link

@coderabbitai coderabbitai bot Oct 21, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Add test coverage for empty UnixSocketPath case.

The tests verify backward compatibility when UnixSocketPath is set, but don't cover the case where UnixSocketPath is empty. According to the PR description and the logic in json.go, VfkitMagic should be cleared when UnixSocketPath is empty. This scenario should be tested to ensure the backward compatibility logic works correctly.

Add a fourth subtest:

/* Check that vfkitMagic is cleared when UnixSocketPath is empty */
t.Run("VfkitMagicClearedWhenNoSocket", func(t *testing.T) {
	jsonStr := `{"vcpus":3,"memoryBytes":4194304000,"bootloader":{"kind":"linuxBootloader","vmlinuzPath":"/vmlinuz","initrdPath":"/initrd","kernelCmdLine":"console=hvc0"},"devices":[{"kind":"virtionet","nat":false,"macAddress":"00:11:22:33:44:55","vfkitMagic":true}]}`
	var unmarshalledVM VirtualMachine
	err := json.Unmarshal([]byte(jsonStr), &unmarshalledVM)
	require.NoError(t, err)
	netDevs := unmarshalledVM.VirtioNetDevices()
	require.Len(t, netDevs, 1)
	require.False(t, netDevs[0].VfkitMagic, "VfkitMagic should be false when UnixSocketPath is empty")
})

Based on learnings.

🤖 Prompt for AI Agents 
In pkg/config/json_test.go around lines 371 to 380, add a fourth subtest to
TestVirtioNetBackwardsCompat that verifies VfkitMagic is cleared when
UnixSocketPath is empty: create a JSON string representing a VM with a
virtio-net device that has vfkitMagic true but no unixSocketPath, unmarshal into
VirtualMachine, assert no error, get VirtioNetDevices(), assert length 1 and
assert the device's VfkitMagic is false; place this subtest alongside the
existing three using t.Run with the name "VfkitMagicClearedWhenNoSocket".


func TestJSON(t *testing.T) {
t.Run("json", func(t *testing.T) {
for name := range jsonTests {
Expand Down
Loading