-
Notifications
You must be signed in to change notification settings - Fork 248
RFD 189: Console Access - vmadmd console proxy #1159
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.
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
base: master
Are you sure you want to change the base?
Conversation
Implement TCP proxy for console access across all VM brands: - KVM: Proxy to unix socket at /tmp/vm.console (serial console) - Bhyve/Joyent/LX: Proxy to zone console device at /dev/zcons/<zonename>/zoneconsole Changes: - Add CONSOLE object to track console proxy state - Add spawnConsoleProxy() to create TCP listeners for console access - Add clearConsoleProxy() and reloadConsoleProxy() for lifecycle management - Add infoConsole() to return console connection details via vm.info() - Add console proxy spawning alongside VNC/SPICE in VM lifecycle - Add vm.info() support for joyent, joyent-minimal, and lx brands (console only) Console proxies are automatically created when VMs start and cleaned up when VMs stop. Connection details (host, port, type) are available via vm.info(['console']). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add 'console' to runtime_info feature list for all brands to enable vmadm info queries for console connection details. Changes: - KVM: Add 'console' to existing runtime_info array - Bhyve: Add 'console' to runtime_info array - Joyent: Add runtime_info: ['console'] (new) - Joyent-minimal: Add runtime_info: ['console'] (new) - LX: Add runtime_info: ['console'] (new) This allows vm.info(['console']) to work for all brands, which is required by CNAPI console endpoint and vmadmd console proxy. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fix issue where non-KVM/Bhyve VMs (Joyent, LX) were not getting console proxies spawned at vmadmd startup. Previously only KVM/Bhyve called loadVM() which spawned the console proxy. Now all running VMs get console proxies regardless of brand. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Zone console devices (/dev/zcons/*/zoneconsole) are character devices, not unix sockets. net.connect() fails with ENOTSOCK when trying to connect to them. Fix by using different connection methods based on brand: - KVM: net.Stream().connect() for unix socket - Others: fs.createReadStream/WriteStream for device file This allows proper bidirectional data flow for zone console devices. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
fs.createReadStream doesn't work properly with character devices like /dev/zcons/*/zoneconsole. Use socat instead to handle the device I/O properly. For non-KVM brands, spawn socat process per connection to proxy between TCP socket and zone console device with proper raw/echo settings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The correct console socket for zlogin -C is managed by zoneadmd at /var/run/zones/<zonename>.console_sock, not the /dev/zcons device. All brands now use unix sockets: - KVM: /zones/<uuid>/root/tmp/vm.console (serial console) - Others: /var/run/zones/<zonename>.console_sock (zoneadmd console) This allows simple net.Stream() connection for all brands. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The zoneadmd console_sock requires a protocol handshake before data flows. Send 'IDENT C 0\n' and wait for 'OK' response before proxying data. This matches the zlogin -C handshake protocol. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Critical fixes for console connection handler: 1. Replace deprecated net.Stream() with new net.Socket() 2. Add cleanup() function with idempotent guard to prevent double-cleanup 3. Add 5-second timeout for zone console handshake 4. Ensure socket.destroy() called on all error paths: - Client close - Console socket error - Client error - Handshake timeout - Handshake failure 5. Rename 'console' variable to 'consoleSocket' to avoid shadowing global 6. Truncate handshake error messages to prevent log spam These fixes prevent file descriptor leaks and hanging connections that could exhaust system resources under error conditions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements console proxy infrastructure in vmadmd to support RFD 189 Console Access Through CloudAPI. The implementation adds TCP-based console proxy functionality for all VM brands (KVM, Bhyve, Joyent, Joyent-minimal, LX) with proper lifecycle management and resource leak prevention.
- Adds console proxy infrastructure with TCP server that proxies to console sockets
- Implements proper handshake protocol for zoneadmd console connections for non-KVM brands
- Integrates console proxy into VM lifecycle management (spawn on start, cleanup on stop)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| server.listen(port, SDC.sysinfo.admin_ip, function () { | ||
| addr = server.address(); | ||
|
|
||
| CONSOLE[vmobj.uuid] = { | ||
| 'host': SDC.sysinfo.admin_ip, | ||
| 'port': addr.port, | ||
| 'server': server, | ||
| 'type': 'socket', | ||
| 'path': consolePath | ||
| }; |
Copilot
AI
Oct 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CONSOLE object is being populated inside the async callback, but the function returns immediately without waiting for the server to start listening. This creates a race condition where other code trying to access CONSOLE[vmobj.uuid] might find it undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A JS person should reality-check this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this moves to non-draft we'll need >= two reviewers for JS reality checking.
src/vm/sbin/vmadmd.js
Outdated
| clearTimeout(handshakeTimer); | ||
| handshakeTimer = null; | ||
|
|
||
| if (data.toString().indexOf('OK') === 0) { |
Copilot
AI
Oct 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using indexOf for protocol validation is fragile. The response should be validated more strictly to ensure it matches the expected protocol format exactly.
| if (data.toString().indexOf('OK') === 0) { | |
| if (data.toString().trim() === 'OK') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A JS person should reality-check this.
src/vm/sbin/vmadmd.js
Outdated
| consoleSocket.pipe(c); | ||
| } else { | ||
| log.error('console handshake failed for VM ' + vmobj.uuid + | ||
| ': ' + data.toString().substring(0, 100)); |
Copilot
AI
Oct 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number 100 for substring length should be defined as a named constant to make the truncation limit explicit and configurable.
| ': ' + data.toString().substring(0, 100)); | |
| ': ' + data.toString().substring(0, CONSOLE_HANDSHAKE_LOG_TRUNCATE_LEN)); |
src/vm/sbin/vmadmd.js
Outdated
| cleanup(); | ||
| c.end(); | ||
| } | ||
| }, 5000); |
Copilot
AI
Oct 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number 5000 for handshake timeout should be defined as a named constant to make the timeout configurable and self-documenting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, copilot is making its case well so far.
|
1.) Thank you for the draft PR. 2.) Copilot does not count as an official reviewer for our purposes. It'll be interesting to see if its feedback is helpful or distracting. |
1 similar comment
|
1.) Thank you for the draft PR. 2.) Copilot does not count as an official reviewer for our purposes. It'll be interesting to see if its feedback is helpful or distracting. |
- Add CONSOLE_HANDSHAKE_TIMEOUT (5000ms) and CONSOLE_LOG_TRUNCATE_LEN (100)
named constants to replace magic numbers
- Use stricter protocol validation: data.toString().trim() === 'OK'
instead of indexOf('OK') === 0 for more robust handshake checking
The stricter validation prevents false positives if the response contains
"OK" as a substring of a different message.
Addresses PR review feedback from #1159
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
danmcd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading this correctly, this will always open a TCP console port in the global zone for any BHYVE or KVM VM IF AND ONLY IF the "console" property in the VM object exists.
We should have a documentation update in the PR if I'm understanding correctly, and it should be off by default. OMG vnc is ENABLED BY DEFAULT. AND HAS BEEN SINCE THE DAYS OF JOYENT?!?
This is not your fault, but I just noticed this (eeesh).
There is an easy way to disable it (vmadm update $UUID vnc_port=-1), and if we go down this path we need to behave like VNC does, or even MORE strict, because there's no password-authentication save the console itself, unlike VNC.
| server.listen(port, SDC.sysinfo.admin_ip, function () { | ||
| addr = server.address(); | ||
|
|
||
| CONSOLE[vmobj.uuid] = { | ||
| 'host': SDC.sysinfo.admin_ip, | ||
| 'port': addr.port, | ||
| 'server': server, | ||
| 'type': 'socket', | ||
| 'path': consolePath | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this moves to non-draft we'll need >= two reviewers for JS reality checking.
| // Start Remote Display | ||
| spawnRemoteDisplay(vmobj); | ||
|
|
||
| // Start Console Proxy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and other always-on calls to spawnRemoteDisplay() have me curious about an overarching question. Will ask it in the overarching comment.
Summary
Implements console proxy infrastructure in vmadmd to support RFD 189 Console Access Through CloudAPI.
consoleto runtime_info for all brands to enable vmadm info queriesChanges
src/vm/sbin/vmadmd.js- Console proxy infrastructuresrc/vm/node_modules/proptable.js- Runtime_info support for all brandsRelated PRs
Commits