all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Aaron Lauterer <a.lauterer@proxmox.com>
To: Fiona Ebner <f.ebner@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH manager v2] proxy: ui: vm console: autodetect novnc or xtermjs
Date: Tue, 25 Feb 2025 16:48:07 +0100	[thread overview]
Message-ID: <143e5d03-7d95-4dae-a733-b8119bd18dbc@proxmox.com> (raw)
In-Reply-To: <57fddd54-432c-46b7-8012-701be6265304@proxmox.com>

I sent a v3 that implements the functionality in the UI alone.

https://lore.proxmox.com/pve-devel/20250225154706.1108094-1-a.lauterer@proxmox.com/T/#u

On  2025-02-03  11:01, Fiona Ebner wrote:
> Am 25.11.24 um 11:04 schrieb Aaron Lauterer:
>> Some users configure their VMs to use serial as their display. The big
>> benefit is that in combination with the xtermjs remote console, copy &
>> paste works a lot better than via novnc.
>>
>> Unfortunately, the main console panel defaulted to novnc, not matter
>> what the guest had configured. One always had to open the console of the
>> VM in a separate window to make use of xtermjs.
>>
>> This patch changes the behavior and lets it autodetect the guest
>> configuration to either use xtermjs or novnc.
>>
>> If we previously selected the console submenu in one VM and then
>> switched to another VM, the console submenu is the preselect item for
>> the VM we switched to. But at this point, the default would be used
>> (novnc), making it an unpleasant experience. If we would use the same
>> mechanism as for the top right console button - `me.mon()` - it would
>> work, but only if we (re)select the console submenu after the first API
>> call to `nodes/{nola}/qemu/{vmid}/status` finished. On the initial
>> load, if the console is the active submenu, it would still default to
>> novnc.
>>
>> While we probably could have implemented in just in the UI, for example
>> by waiting until the first call to `status` is done, this would
>> potentially introduce "laggyness" when opening the console.
> 
> Wondering why you would need vmstatus() for this? Isn't all the
> information already present in the VM configuration?
> 
>>
>> Another option is to handle it in the backend. The backend can then
>> check the VM config and override the novnc/xtermjs setting. The result
>> is, that even when switching VMs in the web UI with the console submenu
>> selected, it will load xtermjs for the VMs that use it.
>>
>> We only do it if the HTTP call has the new 'autodetect' parameter
>> enabled. Additionally we introduce a permission check for 'VM.Console'
>> at this level and only adjust the used console if the user does have the
>> correct permissions. Otherwise we leak the existence of the VM to
>> unauthorized users if it has 'serial' configured due to the change in
>> the returned console (noVNC/xtermjs).
>>
>> The actual check if the user is allowed to access the console is
>> happening once the loaded console implementation establishes a
>> connection to the console API endpoint of the guest. But at this point,
>> either the noVNC or xtermjs console is already sent to the user's
>> browser.
>>
>> Potential further improvements could be to also check the console
>> settings in datacenter.cfg and consider it. As that isn't checked in the
>> inline console panel for CTs and VMs at all, with out without this
>> patch.
>> The setting does have an impact on the buttons that open the console in
>> a new window.
>>
>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>> ---
>> Another thing that I noticed is that the property we use to decide if we
>> enable xtermjs for VMs in the top right console button, and for now also
>> in this patch, only checks if the VM has a serial device configured.
>> PVE::QemuServer::vmstatus() calls `conf_has_serial()`.
>>
>> A better approach would be to have a vmstatus property that actually
>> tells us if the VM has serial set as display option. As the display
>> could be set to something else, even if a serial device exists. There
>> are other use-cases for a serial device in the VM, like passing through
>> an actual serial port.
>>
>> But I didn't want to open that can of worms just yet ;)
>>
> 
> Again, can't this be done just via the configuration?
> 
>> diff --git a/PVE/Service/pveproxy.pm b/PVE/Service/pveproxy.pm
>> index ac108545..4cd281c7 100755
>> --- a/PVE/Service/pveproxy.pm
>> +++ b/PVE/Service/pveproxy.pm
>> @@ -228,6 +228,22 @@ sub get_index {
>>       my $novnc = defined($args->{console}) && $args->{novnc};
>>       my $xtermjs = defined($args->{console}) && $args->{xtermjs};
>>   
>> +    my $rpcenv = PVE::RPCEnvironment::get();
>> +    if (
>> +	defined($args->{console})
>> +	&& $args->{console} eq 'kvm'
>> +	&& $args->{autodetect}
> 
> The name 'autodetect' is too generic, maybe 'console-autodetect'?
> 
> To me, it really feels like the caller should just directly pass in
> either novnc or xtermjs as the argument depending on what it actually
> wants. pveproxy calling into vmstatus() feels weird.
> 
>> +	&& $username
>> +	&& $rpcenv->check_full($username, "/vms/$args->{vmid}", ["VM.Console"], 1, 1)
>> +    ) {
>> +	my $vmid = $args->{vmid};
>> +	my $vmstatus = PVE::QemuServer::vmstatus($vmid);
> 
> Missing "use" statement for PVE::QemuServer.
> 
>> +	if (defined($vmstatus->{$vmid}) && $vmstatus->{$vmid}->{serial}) {
>> +		$novnc = 0;
>> +		$xtermjs = 1;
>> +	}
>> +    }
>> +
>>       my $langfile = -f "$basedirs->{i18n}/pve-lang-$lang.js" ? 1 : 0;
>>   
>>       my $version = PVE::pvecfg::version();



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


      reply	other threads:[~2025-02-25 15:48 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-11-25 10:04 Aaron Lauterer
2025-02-03 10:01 ` Fiona Ebner
2025-02-25 15:48   ` Aaron Lauterer [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=143e5d03-7d95-4dae-a733-b8119bd18dbc@proxmox.com \
    --to=a.lauterer@proxmox.com \
    --cc=f.ebner@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal