9p: add preflight check for 9p hvsock #5067
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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe change extends Windows preflight validation by adding a check for 9P hvsock registry entries alongside the existing gvisor vsock check. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
/hold |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/crc/preflight/preflight_windows.go (1)
188-206: Implementation correctly validates both gvisor and 9P vsock registry entries.The dual-check approach properly validates both vsock configurations required for Windows networking. The error messages clearly distinguish between gvisor and 9P failures, which will help with troubleshooting.
Optional: Consider extracting a helper function to reduce duplication:
func checkVsockRegistryKey(key, value, component string) error { stdout, _, err := powershell.Execute(fmt.Sprintf(`Get-Item -Path "%s\%s"`, vsockRegistryDirectory, key)) if err != nil { return err } if !strings.Contains(stdout, value) { return fmt.Errorf("VSock registry key for %s not correctly configured", component) } return nil } func checkVsock() error { if err := checkVsockRegistryKey(gvisorVsockRegistryKey, gvisorVsockRegistryValue, "gvisor"); err != nil { return err } if err := checkVsockRegistryKey(fs9pVsockRegistryKey, fs9pVsockRegistryValue, "9P"); err != nil { return err } return nil }However, the current inline approach is perfectly acceptable given the simplicity of the checks.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/crc/preflight/preflight_windows.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: redbeam
Repo: crc-org/crc PR: 4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
Learnt from: redbeam
Repo: crc-org/crc PR: 4879
File: pkg/crc/machine/start.go:254-254
Timestamp: 2025-12-17T10:14:34.618Z
Learning: In the 9P file sharing implementation for Windows (pkg/crc/machine/start.go), the hardcoded "2" parameter in the 9pfs command represents the vsock CID (Context ID) for the host. In vsock addressing: CID 0 = hypervisor, CID 1 = local communication, CID 2 = host, CID 3+ = guest VMs.
📚 Learning: 2025-08-06T09:48:10.441Z
Learnt from: redbeam
Repo: crc-org/crc PR: 4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
Applied to files:
pkg/crc/preflight/preflight_windows.go
📚 Learning: 2025-12-17T10:14:34.618Z
Learnt from: redbeam
Repo: crc-org/crc PR: 4879
File: pkg/crc/machine/start.go:254-254
Timestamp: 2025-12-17T10:14:34.618Z
Learning: In the 9P file sharing implementation for Windows (pkg/crc/machine/start.go), the hardcoded "2" parameter in the 9pfs command represents the vsock CID (Context ID) for the host. In vsock addressing: CID 0 = hypervisor, CID 1 = local communication, CID 2 = host, CID 3+ = guest VMs.
Applied to files:
pkg/crc/preflight/preflight_windows.go
🪛 Gitleaks (8.30.0)
pkg/crc/preflight/preflight_windows.go
[high] 182-182: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 184-184: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (17)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (1)
pkg/crc/preflight/preflight_windows.go (1)
178-186: LGTM! Constants are well-defined.The refactoring to separate gvisor and 9P vsock registry constants is clear and makes the code more maintainable. The GUID-format registry keys correctly encode the vsock ports in hexadecimal (0x0400 for gvisor, 0x9000 for 9P).
Note: The static analysis warning about "Generic API Key" is a false positive—these are Windows registry key GUIDs, not secrets.
|
@redbeam: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/unhold |
Fixes #5066
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.