ignition: Don’t use a unix socket for the HTTP server #375
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 |
WalkthroughReplaces Unix-socket ignition provisioning with an HTTP server served over a VM-bound virtio-vsock listener; adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as vfkit CLI
participant VM as VirtualMachine
participant VsockDev as VirtioVsock (host-side)
participant Server as Ignition HTTP Server
participant Guest as Guest OS
CLI->>VM: NewVirtualMachine(config)
activate VM
VM->>VM: toVz(config) — eager init, attach vsock devices
VM->>VsockDev: create/listen on vsock port (e.g., 1024)
deactivate VM
CLI->>Server: startIgnitionProvisionerServer(vm, configPath, vsockPort)
activate Server
Server->>VsockDev: obtain net.Listener bound to VM vsock
Server->>Server: startIgnitionProvisionerServerInternal(ignReader, listener)
Server->>Server: Serve HTTP /ignition on listener
deactivate Server
Guest->>Server: GET /ignition (over VSOCK transport)
Server-->>Guest: 200 + ignition payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
d1db6a2 to
ae94d19
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/config/json_test.go (1)
97-97: LGTM! Expected JSON updated correctly.The expected JSON no longer includes
vsockPort, correctly reflecting thatIgnition.VsockPorthas thejson:"-"tag and should not be serialized. Note: The JSON appears to have an extra space after the comma before"ignition", which may cause formatting inconsistencies but won't affect test correctness.cmd/vfkit/main_test.go (1)
32-32: Consider checking the server startup error.The error returned by
startIgnitionProvisionerServerInternalis silently discarded. While the test primarily verifies the HTTP behavior, consider whether server startup errors should fail the test to catch unexpected issues early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/vfkit/main.go(3 hunks)cmd/vfkit/main_test.go(1 hunks)pkg/config/config.go(6 hunks)pkg/config/config_test.go(1 hunks)pkg/config/json_test.go(1 hunks)pkg/config/virtio.go(2 hunks)pkg/vf/vm.go(2 hunks)pkg/vf/vsock.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/config/config_test.go (1)
pkg/config/config.go (2)
VirtualMachine(27-35)Ignition(43-47)
cmd/vfkit/main.go (2)
pkg/config/config.go (2)
Ignition(43-47)VirtualMachine(27-35)pkg/vf/vm.go (1)
VirtualMachine(17-20)
pkg/vf/vm.go (3)
pkg/config/config.go (2)
VirtualMachine(27-35)Ignition(43-47)pkg/config/virtio.go (1)
VirtioVsock(51-60)pkg/vf/virtio.go (2)
VirtioVsock(28-28)AddToVirtualMachineConfig(455-487)
🔇 Additional comments (16)
pkg/config/virtio.go (1)
624-624: LGTM! Safe removal of security linter suppressions.The
#nosec G115comments were appropriately removed. The integer conversions touint32are protected by proper bounds checks: Line 620-622 validatesport > math.MaxUint32before the conversion at Line 624, and Line 651 usesParseUintwithbitSize 32ensuring safe conversion at Line 655.Also applies to: 655-655
pkg/vf/vsock.go (1)
95-95: LGTM! Safe removal of security linter suppression.The
#nosec G115comment removal is appropriate. The conversion touint32at Line 95 is safe because Line 87 usesParseUintwithbitSize 32, ensuring the value fits within theuint32range.pkg/config/config_test.go (2)
16-16: LGTM! Grammar correction in error message.The error message now correctly uses "accepts" for grammatical agreement with the singular subject "ignition".
24-24: LGTM! Test updated to validate VsockPort configuration.The assertion now correctly validates that
VsockPortis set toignitionVsockPortwhen an ignition file is added, aligning with the refactoring from unix socket to vsock-based provisioning.pkg/config/config.go (5)
45-46: LGTM! Ignition config refactored to use vsock.The addition of
VsockPort(uint32) withjson:"-"and markingSocketPathasomitemptyaligns with the PR objective to replace unix socket with vsock for ignition provisioning. TheVsockPortfield is internal and not exposed in JSON serialization.
59-59: LGTM! Type consistency improvement.Changing
ignitionVsockPortfromuinttouint32improves type consistency withIgnition.VsockPortandTimeSync.VsockPort, both of which areuint32.
242-242: LGTM! Grammar correction.Error message now uses "accepts" for proper subject-verb agreement, consistent with the test file changes.
245-245: LGTM! IgnitionNew call updated correctly.The call passes an empty string for the now-ignored socketPath parameter, which is appropriate given the refactoring to use vsock instead of unix sockets.
259-259: LGTM! TimeSync vsock port assignments.The explicit
uint32conversions are correct and consistent with the type changes. The bounds check at Line 255 ensures no overflow occurs.Also applies to: 279-279
pkg/config/json_test.go (1)
94-94: LGTM! Test adjustment for non-serialized VsockPort.Setting
VsockPort = 0after creation ensures the test validates the correct serialization behavior whereVsockPort(tagged withjson:"-") is not included in the JSON output. The comment accurately describes this as a test-specific adjustment.cmd/vfkit/main.go (4)
200-200: LGTM!The ignition server startup call correctly passes the VM reference, config path, and vsock port to align with the new vsock-based provisioning flow.
221-222: LGTM!The comment accurately reflects that both timesync and ignition now use vsock devices without an associated URL.
281-284: LGTM!The validation correctly ensures exactly one virtio-vsock device exists before attempting to listen, preventing runtime errors from ambiguous device selection.
299-317: LGTM!The internal helper correctly sets up an HTTP server with appropriate timeouts and delegates to the provided listener, enabling testability with different listener types.
pkg/vf/vm.go (2)
54-60: LGTM!The refactored constructor consolidates VM initialization by calling
toVz()during construction, ensuring the VM is fully prepared before returning. This eliminates the need for separate initialization steps.
151-160: LGTM!The automatic vsock device addition for Ignition mirrors the Timesync pattern (lines 140-149) and correctly:
- Checks for non-zero VsockPort
- Sets
Listen: falseso the VM can connect to the host- Propagates errors from device addition
This ensures the ignition provisioning infrastructure is properly configured during VM initialization.
ad7f46a to
39760e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/vm_helpers.go (1)
217-221: Consider cleaning up the unused parameter inIgnitionNew.The method correctly uses
config.IgnitionNew, but the second parameter is explicitly ignored (named_in the signature at pkg/config/config.go:225). This suggests incomplete refactoring from the Unix socket path removal.Consider updating the
IgnitionNewsignature in pkg/config/config.go to remove the unused parameter:-func IgnitionNew(configPath string, _ string) (*Ignition, error) { +func IgnitionNew(configPath string) (*Ignition, error) {And update this call accordingly:
- ign, err := config.IgnitionNew(ignConfigPath, "") + ign, err := config.IgnitionNew(ignConfigPath)cmd/vfkit/main.go (1)
221-221: Update comment to reflect vsock instead of unix socket.The comment references "ignition add a vsock device" but should clarify that ignition uses vsock for serving the config, not just for device presence.
Consider clarifying the comment:
- // timesync and ignition add a vsock device without an associated URL. + // timesync and ignition use vsock devices without exposing additional socket URLs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/vfkit/main.go(3 hunks)cmd/vfkit/main_test.go(1 hunks)pkg/config/config.go(6 hunks)pkg/config/config_test.go(1 hunks)pkg/config/json_test.go(1 hunks)pkg/config/virtio.go(2 hunks)pkg/vf/vm.go(2 hunks)pkg/vf/vsock.go(1 hunks)test/vm_helpers.go(1 hunks)test/vm_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/config/virtio.go
- pkg/vf/vm.go
- pkg/config/json_test.go
- pkg/config/config_test.go
- pkg/config/config.go
- test/vm_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
test/vm_helpers.go (1)
pkg/config/config.go (2)
IgnitionNew(226-234)Ignition(43-47)
cmd/vfkit/main.go (2)
pkg/config/config.go (2)
Ignition(43-47)VirtualMachine(27-35)pkg/vf/vm.go (1)
VirtualMachine(17-20)
⏰ 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). (1)
- GitHub Check: build (macos-13)
🔇 Additional comments (5)
pkg/vf/vsock.go (1)
95-95: LGTM! Safe removal of security suppression.The
#nosec G115comment removal is appropriate. The type conversionuint32(port)is safe becauseportis parsed withParseUint(parsed.Port(), 10, 32)at line 87, which already constrains the value to the uint32 range.cmd/vfkit/main_test.go (2)
24-26: LGTM! Error handling fixed.The error handling has been correctly fixed from the previous review. The
require.NoError(t, err)is now outside the conditional block, ensuring the test fails immediately if listener creation fails.
30-30: LGTM! Appropriate error handling in test goroutine.Ignoring the error from
startIgnitionProvisionerServerInternalin the goroutine is acceptable here since the test validates server functionality through the subsequent HTTP request. If the server fails to start, the HTTP client request will fail, causing the test to fail.cmd/vfkit/main.go (2)
274-297: LGTM! Properly addresses multiple-read concern.The implementation correctly handles multiple ignition config requests. Using
os.Openat line 275 returns an*os.Filewhich implementsio.ReadSeeker, and the internal function signature at line 299 explicitly requiresio.ReadSeeker. This ensures the handler can seek back to the beginning for retries or multiple VM requests.The vsock device validation (lines 281-284) properly ensures exactly one vsock device is available before attempting to create the listener.
299-314: LGTM!http.ServeContentcorrectly handles concurrent requests.Using
http.ServeContentat line 302 is the correct choice for serving the ignition config. It automatically:
- Seeks the
io.ReadSeekerto the beginning for each request- Supports HTTP Range requests
- Handles concurrent access properly
- Sets appropriate headers (Content-Length, Last-Modified, ETag)
This fully addresses the previous review concern about EOF after the first read.
This starts a vm with a ignition config file, and makes sure this config can be fetched over http on vsock port 1024 Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
They were added to fix `make lint` failures gosec is now be able to detect that the value ranges are checked before casting, so we no longer need to add these #nosec tags. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
VirtualMachine.toVz() was called at `Start()` time if vm.VirtualMachine was nil. It makes more sense to set vm.VirtualMachine as early as possible, that is at creation time. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
There is now an internal helper shared with the unit tests which accepts an io.Reader and a net.Listener. This is in preparation for the next commits which will change the listener used by the main ignition code. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
While testing ignition changes, `make test-unit` fails more often than not because of an error returned by `startIgnitionProvisionerServerInternal` I don’t know if this is triggered by my changes, or by the fact that before df03cab, this test was seldomly run. However, it’s incorrect to assert with `require.NoError(t, err)` after calling `startIgnitionProvisionerServerInternal` as this ends with `return srv.Serve(listener)` which is documented as always failing: https://pkg.go.dev/net/http#Server.Serve > Serve always returns a non-nil error Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Currently, the ignition http server listens on a unix socket. The network traffic is then tunneled over vsock. However, as the http server runs in the vfkit process, we can directly create a vsock listener to communicate with the VM. This should be more efficient as this removes the tcpproxy layer, and more importantly, this removes the creation of the on-disk unix socket, which had a hardcoded name. This fixes crc-org#369 Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
This does not matter much in the context of ignition, but `http.ServeContent` is more featureful than using `io.Copy`: https://pkg.go.dev/net/http#ServeContent > The main benefit of ServeContent over io.Copy is that it handles Range > requests properly, sets the MIME type, and handles If-Match, > If-Unmodified-Since, If-None-Match, If-Modified-Since, and If-Range requests. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
39760e9 to
ba9c376
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/vf/vm.go (1)
140-149: Prevent multiple vsock devices when both Timesync and Ignition are set.Current code can append two vsock devices, violating the “0 or 1” constraint and causing config validation to fail when both features are enabled.
Apply this diff to add at most one vsock device if either feature needs it and none exists yet:
- if cfg.config.Timesync != nil && cfg.config.Timesync.VsockPort != 0 { - // automatically add the vsock device we'll need for communication over VsockPort - vsockDev := VirtioVsock{ - Port: cfg.config.Timesync.VsockPort, - Listen: false, - } - if err := vsockDev.AddToVirtualMachineConfig(cfg); err != nil { - return nil, err - } - } - - if cfg.config.Ignition != nil && cfg.config.Ignition.VsockPort != 0 { - // automatically add the vsock device we'll need for communication over VsockPort - vsockDev := VirtioVsock{ - Port: cfg.config.Ignition.VsockPort, - Listen: false, - } - if err := vsockDev.AddToVirtualMachineConfig(cfg); err != nil { - return nil, err - } - } + needVsock := (cfg.config.Timesync != nil && cfg.config.Timesync.VsockPort != 0) || + (cfg.config.Ignition != nil && cfg.config.Ignition.VsockPort != 0) + if needVsock && len(cfg.socketDevicesConfiguration) == 0 { + // Only one virtio-vsock device is allowed; ports are bound at runtime. + vsockDev := VirtioVsock{Listen: false} + if err := vsockDev.AddToVirtualMachineConfig(cfg); err != nil { + return nil, err + } + }Also applies to: 151-160
🧹 Nitpick comments (6)
test/vm_helpers.go (1)
217-221: Mark helper for better error reporting in tests.Call t.Helper() so require.NoError points to the caller.
Apply this diff:
func (vm *testVM) AddIgnition(t *testing.T, ignConfigPath string) { + t.Helper() ign, err := config.IgnitionNew(ignConfigPath, "") require.NoError(t, err) vm.config.Ignition = ign }test/vm_test.go (1)
448-476: Harden TestIgnition to avoid flakiness and tool availability issues.Race: curl can hit before socat listens; also curl may be missing. Add retry and curl/wget fallback.
Apply this diff:
- output, err := vm.SSHCombinedOutput(t, "socat -T 2 TCP-LISTEN:8080 VSOCK-CONNECT:2:1024 >/dev/null & curl -q http://localhost:8080") + output, err := vm.SSHCombinedOutput(t, "sh -lc 'socat -T 2 TCP-LISTEN:8080,fork VSOCK-CONNECT:2:1024 >/dev/null & for i in $(seq 1 20); do if command -v curl >/dev/null; then curl -fsS http://localhost:8080 && exit 0; elif command -v wget >/dev/null; then wget -qO- http://localhost:8080 && exit 0; fi; sleep 0.2; done; exit 1'")pkg/config/config.go (4)
245-251: Construction via vsock path removal LGTMPassing an ignored second arg makes the intent clear. Optionally trim spaces on opts[0] to be forgiving of user input.
- ignition, err := IgnitionNew(opts[0], "") + ignition, err := IgnitionNew(strings.TrimSpace(opts[0]), "")
253-261: Prefer uint32 for vsockPort parameter for consistency and simpler validationCurrent API takes uint, then checks > MaxUint32. On 32-bit platforms that check is moot. Consider using uint32 to align with the field and avoid overflow checks.
-func TimeSyncNew(vsockPort uint) (VMComponent, error) { +func TimeSyncNew(vsockPort uint32) (VMComponent, error) { - - if vsockPort > math.MaxUint32 { - return nil, fmt.Errorf("invalid vsock port: %d", vsockPort) - } return &TimeSync{ - VsockPort: uint32(vsockPort), + VsockPort: vsockPort, }, nil }If changing the signature is not feasible now, at least drop the >MaxUint32 guard on 32-bit with a build tag or keep as-is and defer to a major bump.
43-47: Mark SocketPath deprecated and document VsockPort non-serializationtype Ignition struct { ConfigPath string `json:"configPath"` - SocketPath string `json:"socketPath,omitempty"` - VsockPort uint32 `json:"-"` + // Deprecated: SocketPath is ignored since ignition now uses vsock; kept only for backward JSON compatibility. + SocketPath string `json:"socketPath,omitempty"` + // Not serialized; ignition’s applehv provider uses a fixed vsock port. + VsockPort uint32 `json:"-"` }
226-233: Add deprecation comment to IgnitionNew- func IgnitionNew(configPath string, _ string) (*Ignition, error) { + // Deprecated: the second parameter is ignored and will be removed in a future release. + func IgnitionNew(configPath string, _ string) (*Ignition, error) {Vsock devices are already auto-attached in pkg/vf/vm.go when Ignition.VsockPort > 0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/vfkit/main.go(3 hunks)cmd/vfkit/main_test.go(1 hunks)pkg/config/config.go(6 hunks)pkg/config/config_test.go(1 hunks)pkg/config/json_test.go(1 hunks)pkg/config/virtio.go(2 hunks)pkg/vf/vm.go(2 hunks)pkg/vf/vsock.go(1 hunks)test/osprovider.go(2 hunks)test/vm_helpers.go(1 hunks)test/vm_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/config/json_test.go
- pkg/config/virtio.go
- pkg/vf/vsock.go
🧰 Additional context used
🧬 Code graph analysis (5)
pkg/vf/vm.go (3)
pkg/config/config.go (2)
VirtualMachine(27-35)Ignition(43-47)pkg/config/virtio.go (1)
VirtioVsock(51-60)pkg/vf/virtio.go (2)
VirtioVsock(28-28)AddToVirtualMachineConfig(455-487)
test/vm_test.go (2)
test/osprovider.go (1)
NewPuipuiProvider(112-114)test/vm_helpers.go (1)
NewTestVM(155-164)
cmd/vfkit/main.go (2)
pkg/config/config.go (2)
Ignition(43-47)VirtualMachine(27-35)pkg/vf/vm.go (1)
VirtualMachine(17-20)
pkg/config/config_test.go (2)
pkg/config/config.go (2)
VirtualMachine(27-35)Ignition(43-47)pkg/vf/vm.go (1)
VirtualMachine(17-20)
test/vm_helpers.go (1)
pkg/config/config.go (2)
IgnitionNew(226-234)Ignition(43-47)
⏰ 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-15)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
🔇 Additional comments (7)
test/osprovider.go (1)
34-35: LGTM: centralized version and clearer download logging.The const and versioned log improve maintainability and traceability.
Also applies to: 183-183
pkg/config/config_test.go (1)
16-16: LGTM: tests aligned with Vsock ignition and message fixed.Also applies to: 24-24
cmd/vfkit/main_test.go (1)
24-31: LGTM: net.Listener-based server startup test is correct.Using bytes.NewReader (ReadSeeker) with the internal API is appropriate; listener handling looks good.
cmd/vfkit/main.go (1)
197-205: LGTM: ignition served over vsock with rewindable content.Using VM socket device to Listen + http.ServeContent(io.ReadSeeker) is correct and retry-safe. Logging and timeouts reasonable for this scope.
Also applies to: 274-297, 299-314
pkg/config/config.go (3)
241-244: Error message tweak LGTMClearer phrasing. No action needed.
271-284: FromOptions assignment LGTMParsing and assignment to uint32 is correct.
56-60: Hardcoded ignition vsock port is stable
Upstream applehv provider still uses port 1024; keeping the unexported constant is fine.
|
coderabbit has a good point:
Need to double check this |
|
The ignition code starts an HTTP server used by the VM to fetch its
config. This HTTP server currently listens on a unix socket.
This is a bit wasteful since this runs in the vfkit process, we could
directly use a vsock listener. This also sidesteps another issue which
is that the path to the unix socket is hardcoded and can’t be changed.
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation