json: Add backward compatibility with VfkitMagic #382
Conversation
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cfergeau
force-pushed
the
vfkitMagic-json
branch
from
cdb9a89 to
746de49
Compare
WalkthroughUnmarshal for VirtioNet now pre-sets Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant VirtioNet as VirtioNet.unmarshalVirtioNet
participant JSON as encoding/json
Caller->>VirtioNet: unmarshalVirtioNet(jsonData)
Note over VirtioNet: set VfkitMagic = true (pre-unmarshal)
VirtioNet->>JSON: json.Unmarshal into struct
JSON-->>VirtioNet: populated fields
alt UnixSocketPath == ""
Note over VirtioNet: set VfkitMagic = false (post-unmarshal)
else UnixSocketPath != ""
Note over VirtioNet: keep VfkitMagic as set/provided
end
VirtioNet-->>Caller: return VirtioNet config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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 (2)
pkg/config/json_test.go (2)
7-7: LGTM! Good refactor to use standard library.Replacing the custom
containsfunction withslices.Containsfrom the standard library improves maintainability and reduces code duplication.Also applies to: 28-28
333-367: Consider adding test case for overriddenvfkitMagic.The test effectively validates backward compatibility by checking:
- JSON without
vfkitMagicdefaults totrue(lines 334-349)- Explicit
vfkitMagic: falseis respected (lines 351-366)However, consider adding a third test case to verify the post-unmarshal override logic in
json.golines 127-130: JSON withvfkitMagic: truebut emptyunixSocketPathshould result inVfkitMagic: falseafter unmarshalling.Example test case:
/* Check that vfkitMagic is forced to false when unixSocketPath is empty */ jsonStr = `{"vcpus":3,"memoryBytes":4194304000,"bootloader":{"kind":"linuxBootloader","vmlinuzPath":"/vmlinuz","initrdPath":"/initrd","kernelCmdLine":"console=hvc0"},"devices":[{"kind":"virtionet","nat":true,"vfkitMagic":true,"macAddress":"00:11:22:33:44:55"}]}` err = json.Unmarshal([]byte(jsonStr), &unmarshalledVM) require.NoError(t, err) // vfkitMagic should be false since unixSocketPath is empty virtioNet := unmarshalledVM.Devices[0].(*VirtioNet) require.False(t, virtioNet.VfkitMagic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/config/json.go(2 hunks)pkg/config/json_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/config/json_test.go (3)
pkg/vf/vm.go (1)
VirtualMachine(17-20)pkg/config/config.go (1)
VirtualMachine(28-36)pkg/config/virtio.go (1)
VirtioNetNew(406-419)
🔇 Additional comments (2)
pkg/config/json.go (2)
113-114: LGTM! Correct approach for backward compatibility.Setting the default value before unmarshalling ensures that older JSON configurations (which lack the
vfkitMagicfield) will default totrue, preserving backward compatibility with older gvproxy versions. If the field is present in the JSON, it will be overwritten during unmarshalling.
127-130: Verify that overriding explicitvfkitMagicvalues is intentional.This code forces
VfkitMagictofalsewhenUnixSocketPathis empty, even if the JSON explicitly setsvfkitMagic: true. While the comment explains the rationale ("vfkitMagic is only useful in combination with unixSocketPath"), this behavior overrides explicit user intent from the JSON configuration.Please confirm:
- This override is intentional and aligns with the domain requirements
- This behavior is documented or will be made clear to users
- There are no valid use cases for
vfkitMagic=truewithout a socket pathIf this override is intentional, consider adding a test case that verifies this behavior (JSON with
vfkitMagic: truebut nounixSocketPathshould result inVfkitMagic: falseafter unmarshalling).
| // vfkitMagic is only useful in combination with unixSocketPath | ||
| if dev.UnixSocketPath == "" { | ||
| dev.VfkitMagic = false | ||
| } |
There was a problem hiding this comment.
Does it break anything if we keep the default true value?
There was a problem hiding this comment.
I don't think it should because this field is only used when UnixSocketPath is set.
There was a problem hiding this comment.
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.
| skipFields []string | ||
| expectedJSON string | ||
| } | ||
|
|
There was a problem hiding this comment.
It will help to add a comment that this old json does not include vfkitMagic and parsing it should use the default value.
| require.NoError(t, err) | ||
| require.True(t, dev.VfkitMagic) | ||
|
|
||
| require.Equal(t, *vm, unmarshalledVM) |
There was a problem hiding this comment.
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.
Signed-off-by: Christophe Fergeau <[email protected]>
This allows VirtioInputDevices/VirtioGPUs/… to share the same code. This commit also adds a VirtioNetDevices method which will be used in the next commit. Signed-off-by: Christophe Fergeau <[email protected]>
cfergeau
force-pushed
the
vfkitMagic-json
branch
from
746de49 to
4e49fa2
Compare
cfergeau
left a comment
cfergeau
left a comment
There was a problem hiding this comment.
I think I’ve addressed most of the comments. Some of the suggested comments might still be missing, let me know which ones I forgot :)
| // vfkitMagic is only useful in combination with unixSocketPath | ||
| if dev.UnixSocketPath == "" { | ||
| dev.VfkitMagic = false | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/config/config.go(1 hunks)pkg/config/json.go(2 hunks)pkg/config/json_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-12T15:51:07.560Z
Learnt from: nirs
PR: crc-org/vfkit#0
File: :0-0
Timestamp: 2025-10-12T15:51:07.560Z
Learning: In the vfkit virtio-net code (pkg/vf/virtionet.go), the connectUnixPath function change was about closing the connection after duplication (since conn.File() already duplicates the fd), not about introducing the duplication itself.
Applied to files:
pkg/config/json.go
🧬 Code graph analysis (2)
pkg/config/json_test.go (3)
pkg/config/config.go (1)
VirtualMachine(28-36)pkg/vf/vm.go (1)
VirtualMachine(17-20)pkg/config/virtio.go (1)
VirtioNetNew(406-419)
pkg/config/config.go (3)
pkg/vf/vm.go (1)
VirtualMachine(17-20)pkg/config/virtio.go (4)
VirtioGPU(45-48)VirtioVsock(52-61)VirtioInput(35-37)VirtioNet(99-108)pkg/vf/virtio.go (3)
VirtioGPU(30-30)VirtioVsock(28-28)VirtioInput(29-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-15)
- GitHub Check: build (macos-13)
🔇 Additional comments (4)
pkg/config/config.go (1)
164-188: LGTM! Clean refactoring with generics.The introduction of
FilterDevices[V VMComponent]eliminates repetitive type-checking logic and provides a consistent pattern for device filtering. The newVirtioNetDevicesaccessor aligns well with the existing device getters and supports the VFKit magic handling tested in json_test.go.pkg/config/json.go (2)
113-114: LGTM! Backward-compatible default.Setting
VfkitMagic = truebefore unmarshalling ensures that older vfkit-generated JSON configurations (which lack this field) will default to the compatible behavior.
127-130: Correct: VfkitMagic tied to UnixSocketPath presence.Forcing
VfkitMagic = falsewhenUnixSocketPathis empty correctly enforces that this field is only meaningful in combination with a unix socket connection.Based on learnings: this field is only used when
UnixSocketPathis set.pkg/config/json_test.go (1)
7-7: LGTM! Modern stdlib usage.Replacing the custom
containshelper withslices.Containsfrom the standard library is a good modernization.Also applies to: 28-28
pkg/config/json_test.go
Outdated
| func testVirtioNetVfkitMagicJson(t *testing.T, vfkitMagic bool) { | ||
| jsonTemplate := `{"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"%s}]}` | ||
| var jsonStr string | ||
| var unmarshalledVM VirtualMachine | ||
|
|
||
| if vfkitMagic { | ||
| jsonStr = fmt.Sprintf(jsonTemplate, ``) | ||
| } else { | ||
| jsonStr = fmt.Sprintf(jsonTemplate, `,"vfkitMagic":false`) | ||
| } | ||
| err := json.Unmarshal([]byte(jsonStr), &unmarshalledVM) | ||
| require.NoError(t, err) | ||
| netDevs := unmarshalledVM.VirtioNetDevices() | ||
| require.Len(t, netDevs, 1) | ||
| require.Equal(t, netDevs[0].VfkitMagic, vfkitMagic) | ||
|
|
||
| vm := newLinuxVM(t) | ||
| dev, err := VirtioNetNew("00:11:22:33:44:55") | ||
| require.NoError(t, err) | ||
| dev.SetUnixSocketPath("/some/path/to/socket") | ||
| if !vfkitMagic { | ||
| dev.VfkitMagic = false | ||
| } | ||
| err = vm.AddDevice(dev) | ||
| require.NoError(t, err) | ||
| require.Equal(t, dev.VfkitMagic, vfkitMagic) | ||
|
|
||
| require.Equal(t, *vm, unmarshalledVM) | ||
| } | ||
|
|
||
| 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) }) | ||
|
|
||
| /* Check that the vfkitMagic default can be overridden */ | ||
| t.Run("VfkitMagicJsonDefaultOverride", func(t *testing.T) { testVirtioNetVfkitMagicJson(t, false) }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test coverage for empty UnixSocketPath.
The tests verify backward compatibility when UnixSocketPath is set, but don't cover the case where UnixSocketPath is empty. The logic in json.go (lines 127-130) clears VfkitMagic when UnixSocketPath is empty, which should be tested to ensure that even if JSON explicitly sets "vfkitMagic": true, it gets overridden to false when there's no socket path.
Add a third test case:
/* 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")
})🤖 Prompt for AI Agents
In pkg/config/json_test.go around lines 333-369, add a third subtest that
verifies VfkitMagic is cleared when UnixSocketPath is empty: unmarshal a JSON
payload for a virtio-net device that explicitly sets "vfkitMagic": true but
omits unixSocketPath, assert Unmarshal succeeds, assert there is one VirtioNet
device, and assert that device.VfkitMagic is false; place the new
t.Run("VfkitMagicClearedWhenNoSocket", ...) alongside the two existing runs.
cfergeau
force-pushed
the
vfkitMagic-json
branch
from
4e49fa2 to
5b9220a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/config/json_test.go (1)
355-355: Swap argument order inrequire.Equalfor better error messages.The
require.Equalfunction expects(expected, actual)order. Lines 355 and 366 have the arguments reversed, which will produce confusing error messages when tests fail.Apply this diff:
- require.Equal(t, netDevs[0].VfkitMagic, vfkitMagic) + require.Equal(t, vfkitMagic, netDevs[0].VfkitMagic)And:
- require.Equal(t, dev.VfkitMagic, vfkitMagic) + require.Equal(t, vfkitMagic, dev.VfkitMagic)Also applies to: 366-366
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/config/json.go(2 hunks)pkg/config/json_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/json.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/config/json_test.go (2)
pkg/config/config.go (1)
VirtualMachine(28-36)pkg/config/virtio.go (1)
VirtioNetNew(406-419)
🔇 Additional comments (1)
pkg/config/json_test.go (1)
7-7: LGTM! Good refactor to use standard library.Using
slices.Containsfrom the standard library is cleaner and more maintainable than a custom contains function.Also applies to: 28-28
| 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) }) | ||
| } |
There was a problem hiding this comment.
🛠️ 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".
The previous commits added a VfkitMagic property which for now defaults to true for backwards compatibility with older gvproxy versions. However, this default was not used when unmarshalling a json vm configuration made with an older vfkit version. This could cause unexpected issues with networking. This commit sets VfkitMagic to true by default when unmarshalling json. Signed-off-by: Christophe Fergeau <[email protected]> Assisted-by: CodeRabbit.ai Signed-off-by: Christophe Fergeau <[email protected]>
cfergeau
force-pushed
the
vfkitMagic-json
branch
from
5b9220a to
32f1dac
Compare
3 participants
The previous commits added a VfkitMagic property which for now defaults to
true for backwards compatibility with older gvproxy versions. However, this
default was not used when unmarshalling a json vm configuration made with
an older vfkit version. This could cause unexpected issues with networking.
This commit sets VfkitMagic to true by default when unmarshalling json.
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Tests