[pve-devel] [PATCH-SERIES pve-http-server/pve-manager] fix#4689: rewrite_uri: autofind nodename for qemu/lxc

[pve-devel] [PATCH-SERIES pve-http-server/pve-manager] fix#4689: rewrite_uri: autofind nodename for qemu/lxc

[Alexandre Derumier]

Hi,

Currently, to manage qemu && lxc vms, we always need to specify nodename in uri.

This is a problem with automation tools like terraform, where is node is registered
in the state of terraform.
(That mean, than if we move the vm on another node, terraform don't known it, and try to create the vm
again or can't delete the vm,...)
https://github.com/Telmate/terraform-provider-proxmox/issues/168

This can also be a potential problem with race, if we need to query /cluster/ressources to find the node, then another
query on the vm.

I have some discussion with fabian about it:
https://bugzilla.proxmox.com/show_bug.cgi?id=4689


This patch series, simply find the nodename if not specified and rewrite url.

/nodes/(qemu|lxc)/<vmid> ---> /nodes/<nodename>/<qemu|lxc>/<vmid>



Alexandre Derumier (1):
  anyevent: add rewrite_uri

 src/PVE/APIServer/AnyEvent.pm | 7 +++++++
 1 file changed, 7 insertions(+)

Alexandre Derumier (1):
  httpserver: add rewrite uri for /nodes/(qemu/lxc)/<vmid>

 PVE/HTTPServer.pm | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

-- 
2.30.2

[pve-devel] [PATCH pve-http-server 1/1] anyevent: add rewrite_uri

[Alexandre Derumier]

allow to rewrite some uri, do nothing by default

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 src/PVE/APIServer/AnyEvent.pm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index b2ae99b..0be9b1f 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -1618,6 +1618,7 @@ sub push_request_header {
 
 		if ($line =~ /(\S+)\040(\S+)\040HTTP\/(\d+)\.(\d+)/o) {
 		    my ($method, $url, $maj, $min) = ($1, $2, $3, $4);
+		    $url = $self->rewrite_uri($url);
 
 		    if ($maj != 1) {
 			$self->error($reqstate, 506, "http protocol version $maj.$min not supported");
@@ -2072,6 +2073,12 @@ sub verify_spice_connect_url {
     #return ($vmid, $node, $port);
 }
 
+sub rewrite_uri {
+    my ($self, $uri) = @_;
+
+    return ($uri); #do nothing by default
+}
+
 # formatters can call this when the generate a new page
 sub generate_csrf_prevention_token {
     my ($username) = @_;
-- 
2.30.2

[pve-devel] [PATCH pve-manager 1/1] httpserver: add rewrite uri for /nodes/(qemu/lxc)/<vmid>

[Alexandre Derumier]

If nodename is not defined, we search the nodename
and rewrite uri to /nodes/<nodename>/(qemu|lxc)/<vmid

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/HTTPServer.pm | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/PVE/HTTPServer.pm b/PVE/HTTPServer.pm
index dabdf7f3..b43611e1 100755
--- a/PVE/HTTPServer.pm
+++ b/PVE/HTTPServer.pm
@@ -46,6 +46,21 @@ sub verify_spice_connect_url {
     return ($vmid, $node, $port);
 }
 
+sub rewrite_uri {
+    my ($self, $uri) = @_;
+
+    my $base_uri = $self->{base_uri};
+    if($uri =~ m/^(\Q$base_uri\E\/+([a-z][a-z0-9]+)\/nodes\/)((qemu|lxc)\/(\d+)(.*))/) {
+	PVE::Cluster::cfs_update();
+	my $vmid = $5;
+	my $vmlist = PVE::Cluster::get_vmlist();
+	my $nodename = $vmlist->{ids}->{$vmid}->{node};
+	return $uri if !$nodename;
+	$uri = "$1$nodename/$3";
+    }
+    return $uri;
+}
+
 sub generate_csrf_prevention_token {
     my ($username) = @_;
     return PVE::AccessControl::assemble_csrf_prevention_token($username);
-- 
2.30.2

Re: [pve-devel] [PATCH-SERIES pve-http-server/pve-manager] fix#4689: rewrite_uri: autofind nodename for qemu/lxc

[Wolfgang Bumiller]

On Wed, May 31, 2023 at 01:19:50PM +0200, Alexandre Derumier wrote:
> Hi,
> 
> Currently, to manage qemu && lxc vms, we always need to specify nodename in uri.
> 
> This is a problem with automation tools like terraform, where is node is registered
> in the state of terraform.
> (That mean, than if we move the vm on another node, terraform don't known it, and try to create the vm
> again or can't delete the vm,...)
> https://github.com/Telmate/terraform-provider-proxmox/issues/168
> 
> This can also be a potential problem with race, if we need to query /cluster/ressources to find the node, then another
> query on the vm.
> 
> I have some discussion with fabian about it:
> https://bugzilla.proxmox.com/show_bug.cgi?id=4689
> 
> 
> This patch series, simply find the nodename if not specified and rewrite url.
> 
> /nodes/(qemu|lxc)/<vmid> ---> /nodes/<nodename>/<qemu|lxc>/<vmid>

But then `qemu`/`lxc` look like a node name, which is weird.
If we do this, maybe we should use a different start entry altogether to
not mix too many "aliases" into the "real" api tree, like
`/virtual/qemu/*` or `/guests/qemu/*`?

Re: [pve-devel] [PATCH-SERIES pve-http-server/pve-manager] fix#4689: rewrite_uri: autofind nodename for qemu/lxc

[DERUMIER, Alexandre]

> But then `qemu`/`lxc` look like a node name, which is weird.

oh yes,indeed !

> If we do this, maybe we should use a different start entry altogether
> to
> not mix too many "aliases" into the "real" api tree, like
> `/virtual/qemu/*` or `/guests/qemu/*`?
> 

/guests/(qemu|lxc)/*  sound great for me.


(do you have any opinion avec proxyto_callback suggestion  from Dominik
&& Fabian) ?

Re: [pve-devel] [PATCH-SERIES pve-http-server/pve-manager] fix#4689: rewrite_uri: autofind nodename for qemu/lxc

[Dominik Csapak]

On 5/31/23 13:19, Alexandre Derumier wrote:
> Hi,
> 
> Currently, to manage qemu && lxc vms, we always need to specify nodename in uri.
> 
> This is a problem with automation tools like terraform, where is node is registered
> in the state of terraform.
> (That mean, than if we move the vm on another node, terraform don't known it, and try to create the vm
> again or can't delete the vm,...)
> https://github.com/Telmate/terraform-provider-proxmox/issues/168
> 
> This can also be a potential problem with race, if we need to query /cluster/ressources to find the node, then another
> query on the vm.
> 
> I have some discussion with fabian about it:
> https://bugzilla.proxmox.com/show_bug.cgi?id=4689
> 
> 
> This patch series, simply find the nodename if not specified and rewrite url.
> 
> /nodes/(qemu|lxc)/<vmid> ---> /nodes/<nodename>/<qemu|lxc>/<vmid>
> 
> 
> 
> Alexandre Derumier (1):
>    anyevent: add rewrite_uri
> 
>   src/PVE/APIServer/AnyEvent.pm | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> Alexandre Derumier (1):
>    httpserver: add rewrite uri for /nodes/(qemu/lxc)/<vmid>
> 
>   PVE/HTTPServer.pm | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 

hi, while this can work, it introduces a very specific rewriting into the
http server, which might not be obvious and AFAICT won't show up in the
auto-generated api docs?

wouldn't it be better to have some api calls defined for real
(the ones used most often for example) that make use of the
proxyto_callback method instead?

that way they would at least show up in the api documentation,
even though it's probably a bit of code duplication
(altough we could maybe just insert the api at a different place
and do some perl magic refactoring to have it once
with node parameter and once without?)

or does that seem like a bad idea?

Re: [pve-devel] [PATCH-SERIES pve-http-server/pve-manager] fix#4689: rewrite_uri: autofind nodename for qemu/lxc

[DERUMIER, Alexandre]

> 
> 
> hi, while this can work, it introduces a very specific rewriting into
> the
> http server, which might not be obvious and AFAICT won't show up in
> the
> auto-generated api docs?
> 
yes, indeed, it doesnt show up in the doc.


> wouldn't it be better to have some api calls defined for real
> (the ones used most often for example) that make use of the
> proxyto_callback method instead?
> 
I'm quoting Fabian from bugzilla:
"
Fabian Grünbichler 2023-04-24 13:48:07 CEST
yeah, some parts of the machinery are already in place, e.g., it's
possible to define a "proxyto_callback" that gets called instead of
resolving proxyto straight via the parameter. AFAICT that is not used
anywhere though ;)

probably it would mean caching the vmlist in each http server worker to
avoid expensive queries, plus implementing filtering of the vmid list
for the requesting user/token since we'd not want to leak existence or
location of guests that the user doesn't have access to.
"

So, it seem that Fabian have same opinion than you.

I was doing a simply rewrite to keep it simple, but
I'll have a look at proxyto_callback.

(BTW, vmlist don't seem to be so slow, i'm around 10 extra ms. I don't
think it need to be cached.  (it's still faster than doing 1
cluster/ressource api call to find the node + the final call).


> that way they would at least show up in the api documentation,
> even though it's probably a bit of code duplication
> (altough we could maybe just insert the api at a different place
> and do some perl magic refactoring to have it once
> with node parameter and once without?)
> 
> or does that seem like a bad idea?
>