[pve-devel] [PATCH pve-container 1/2] api: lxc: add 'interfaces' endpoint to the index

[pve-devel] [PATCH pve-container 1/2] api: lxc: add 'interfaces' endpoint to the index

[Johannes Cornelis Draaijer via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 5335 bytes --]

From: Johannes Cornelis Draaijer <jcdra1@gmail.com>
To: pve-devel@lists.proxmox.com
Subject: [PATCH pve-container 1/2] api: lxc: add 'interfaces' endpoint to the index
Date: Thu, 18 Apr 2024 22:49:32 +0200
Message-ID: <20240418204933.58521-2-jcdra1@gmail.com>

Signed-off-by: Johannes Cornelis Draaijer <jcdra1@gmail.com>
---
 src/PVE/API2/LXC.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index fd42ccf..89ba64c 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -590,6 +590,7 @@ __PACKAGE__->register_method({
 	    { subdir => 'firewall' },
 	    { subdir => 'snapshot' },
 	    { subdir => 'resize' },
+	    { subdir => 'interfaces' }
 	    ];
 
 	return $res;
-- 
2.34.1



[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[pve-devel] applied: [PATCH pve-container 1/2] api: lxc: add 'interfaces' endpoint to the index

[Fabian Grünbichler]

sorry it took me so long to get back to this, applied this one!

> Johannes Cornelis Draaijer via pve-devel <pve-
> Signed-off-by: Johannes Cornelis Draaijer <jcdra1@gmail.com>
> ---
>  src/PVE/API2/LXC.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index fd42ccf..89ba64c 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -590,6 +590,7 @@ __PACKAGE__->register_method({
>  	    { subdir => 'firewall' },
>  	    { subdir => 'snapshot' },
>  	    { subdir => 'resize' },
> +	    { subdir => 'interfaces' }
>  	    ];
>  
>  	return $res;
> -- 
> 2.34.1


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

[pve-devel] [PATCH pve-container 2/2] fix #5339: api: lxc: ip: add 'all' option so that all addresses can be returned.

[Johannes Cornelis Draaijer via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 7405 bytes --]

From: Johannes Cornelis Draaijer <jcdra1@gmail.com>
To: pve-devel@lists.proxmox.com
Subject: [PATCH pve-container 2/2] fix #5339: api: lxc: ip: add 'all' option so that all addresses can be returned.
Date: Thu, 18 Apr 2024 22:49:33 +0200
Message-ID: <20240418204933.58521-3-jcdra1@gmail.com>

Signed-off-by: Johannes Cornelis Draaijer <jcdra1@gmail.com>
---
 src/PVE/API2/LXC.pm | 16 +++++++++++++---
 src/PVE/LXC.pm      |  9 +++++++--
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 89ba64c..3561317 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -2533,6 +2533,12 @@ __PACKAGE__->register_method({
 	properties => {
 	    node => get_standard_option('pve-node'),
 	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid }),
+	    all => {
+		type => 'boolean',
+		default => 0,
+		optional => 1,
+		description => 'Return all adresses of each interface instead of only one',
+	    }
 	},
     },
     returns => {
@@ -2552,12 +2558,14 @@ __PACKAGE__->register_method({
 		},
 		inet => {
 		    type => 'string',
-		    description => 'The IPv4 address of the interface',
+		    format => 'CIDRv4-list',
+		    description => 'A list of IPv4 CIDRs. This will only contain a single address if \'all\' is not set to true',
 		    optional => 1,
 		},
 		inet6 => {
 		    type => 'string',
-		    description => 'The IPv6 address of the interface',
+		    format => 'CIDRv6-list',
+		    description => 'A list of IPv6 CIDRs. This will only contain a single address if \'all\' is not set to true',
 		    optional => 1,
 		},
 	    }
@@ -2565,8 +2573,10 @@ __PACKAGE__->register_method({
     },
     code => sub {
 	my ($param) = @_;
+	my $vmid = extract_param($param, 'vmid');
+	my $alladdrs = extract_param($param, 'all') // 0;
 
-	return PVE::LXC::get_interfaces($param->{vmid});
+	return PVE::LXC::get_interfaces($vmid, $alladdrs);
     }});
 
 __PACKAGE__->register_method({
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 7883cfb..6d00141 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1088,7 +1088,7 @@ sub hotplug_net {
 }
 
 sub get_interfaces {
-    my ($vmid) = @_;
+    my ($vmid, $alladdrs) = @_;
 
     my $pid = eval { find_lxc_pid($vmid); };
     return if $@;
@@ -1104,7 +1104,12 @@ sub get_interfaces {
     for my $interface ($config->@*) {
 	my $obj = { name => $interface->{ifname} };
 	for my $ip ($interface->{addr_info}->@*) {
-	    $obj->{$ip->{family}} = $ip->{local} . "/" . $ip->{prefixlen};
+	    my $cidr = $ip->{local} . "/" . $ip->{prefixlen};
+	    if ($alladdrs eq 1) {
+		push(@{$obj->{$ip->{family}}}, $cidr);
+            } else {
+		$obj->{$ip->{family}} = $cidr;
+	    }
 	}
 	$obj->{hwaddr} = $interface->{address};
 	push @$res, $obj
-- 
2.34.1



[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [pve-devel] [PATCH pve-container 2/2] fix #5339: api: lxc: ip: add 'all' option so that all addresses can be returned.

[Fabian Grünbichler]

apologies again for the long delay!

> Johannes Cornelis Draaijer via pve-devel <pve-devel@lists.proxmox.com> hat am 18.04.2024 22:49 CEST geschrieben:

> Signed-off-by: Johannes Cornelis Draaijer <jcdra1@gmail.com>
> ---
>  src/PVE/API2/LXC.pm | 16 +++++++++++++---
>  src/PVE/LXC.pm      |  9 +++++++--
>  2 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 89ba64c..3561317 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -2533,6 +2533,12 @@ __PACKAGE__->register_method({
>  	properties => {
>  	    node => get_standard_option('pve-node'),
>  	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid }),
> +	    all => {
> +		type => 'boolean',
> +		default => 0,
> +		optional => 1,
> +		description => 'Return all adresses of each interface instead of only one',

typo: s/adresses/addresses

> +	    }
>  	},
>      },
>      returns => {
> @@ -2552,12 +2558,14 @@ __PACKAGE__->register_method({
>  		},
>  		inet => {
>  		    type => 'string',
> -		    description => 'The IPv4 address of the interface',
> +		    format => 'CIDRv4-list',

this format here and the code below don't agree. a string type with the -list suffix needs actually be a string with the list elements delimited by either space, ',' or ';'. in this case, comma or semicolon is probably okay.

> +		    description => 'A list of IPv4 CIDRs. This will only contain a single address if \'all\' is not set to true',
>  		    optional => 1,
>  		},
>  		inet6 => {
>  		    type => 'string',
> -		    description => 'The IPv6 address of the interface',
> +		    format => 'CIDRv6-list',

same here

> +		    description => 'A list of IPv6 CIDRs. This will only contain a single address if \'all\' is not set to true',
>  		    optional => 1,
>  		},
>  	    }
> @@ -2565,8 +2573,10 @@ __PACKAGE__->register_method({
>      },
>      code => sub {
>  	my ($param) = @_;
> +	my $vmid = extract_param($param, 'vmid');
> +	my $alladdrs = extract_param($param, 'all') // 0;

existing code is not always consistent, but this should be all_addrs

>  
> -	return PVE::LXC::get_interfaces($param->{vmid});
> +	return PVE::LXC::get_interfaces($vmid, $alladdrs);
>      }});
>  
>  __PACKAGE__->register_method({
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 7883cfb..6d00141 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1088,7 +1088,7 @@ sub hotplug_net {
>  }
>  
>  sub get_interfaces {
> -    my ($vmid) = @_;
> +    my ($vmid, $alladdrs) = @_;
>  
>      my $pid = eval { find_lxc_pid($vmid); };
>      return if $@;
> @@ -1104,7 +1104,12 @@ sub get_interfaces {
>      for my $interface ($config->@*) {
>  	my $obj = { name => $interface->{ifname} };
>  	for my $ip ($interface->{addr_info}->@*) {
> -	    $obj->{$ip->{family}} = $ip->{local} . "/" . $ip->{prefixlen};
> +	    my $cidr = $ip->{local} . "/" . $ip->{prefixlen};
> +	    if ($alladdrs eq 1) {

eq is for string comparison, this can just use

if ($all_addrs) {

but for regular comparison you'd use `==` otherwise :)

> +		push(@{$obj->{$ip->{family}}}, $cidr);

if you do this, then you'd need to `join` this list into a string at the end. or you could just build the string directly here (special casing the first and subsequent addresses), both would work in this case.

or you do this here unconditionally, and then after the loop either join (if $all_addr) or just pick the first element, which would be equivalent to the old code here as well.

> +            } else {
> +		$obj->{$ip->{family}} = $cidr;
> +	    }
>  	}
>  	$obj->{hwaddr} = $interface->{address};
>  	push @$res, $obj
> -- 
> 2.34.1


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

Re: [pve-devel] [PATCH pve-container 2/2] fix #5339: api: lxc: ip: add 'all' option so that all addresses can be returned.

[Wolfgang Bumiller]

On Tue, Jul 02, 2024 at 04:29:25PM GMT, Fabian Grünbichler wrote:
> apologies again for the long delay!
> 
> > Johannes Cornelis Draaijer via pve-devel <pve-devel@lists.proxmox.com> hat am 18.04.2024 22:49 CEST geschrieben:
> 
> > Signed-off-by: Johannes Cornelis Draaijer <jcdra1@gmail.com>
> > ---
> >  src/PVE/API2/LXC.pm | 16 +++++++++++++---
> >  src/PVE/LXC.pm      |  9 +++++++--
> >  2 files changed, 20 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> > index 89ba64c..3561317 100644
> > --- a/src/PVE/API2/LXC.pm
> > +++ b/src/PVE/API2/LXC.pm
> > @@ -2533,6 +2533,12 @@ __PACKAGE__->register_method({
> >  	properties => {
> >  	    node => get_standard_option('pve-node'),
> >  	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid }),
> > +	    all => {
> > +		type => 'boolean',
> > +		default => 0,
> > +		optional => 1,
> > +		description => 'Return all adresses of each interface instead of only one',
> 
> typo: s/adresses/addresses
> 
> > +	    }
> >  	},
> >      },
> >      returns => {
> > @@ -2552,12 +2558,14 @@ __PACKAGE__->register_method({
> >  		},
> >  		inet => {
> >  		    type => 'string',
> > -		    description => 'The IPv4 address of the interface',
> > +		    format => 'CIDRv4-list',
> 
> this format here and the code below don't agree. a string type with the -list suffix needs actually be a string with the list elements delimited by either space, ',' or ';'. in this case, comma or semicolon is probably okay.
> 

Since the caller needs to specify a new parameter, shouldn't we just
return an actual array instead?


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

Re: [pve-devel] [PATCH pve-container 2/2] fix #5339: api: lxc: ip: add 'all' option so that all addresses can be returned.

[Fabian Grünbichler]

> Wolfgang Bumiller <w.bumiller@proxmox.com> hat am 05.07.2024 09:45 CEST geschrieben:
> 
>  
> On Tue, Jul 02, 2024 at 04:29:25PM GMT, Fabian Grünbichler wrote:
> > apologies again for the long delay!
> > 
> > > Johannes Cornelis Draaijer via pve-devel <pve-devel@lists.proxmox.com> hat am 18.04.2024 22:49 CEST geschrieben:
> > 
> > > Signed-off-by: Johannes Cornelis Draaijer <jcdra1@gmail.com>
> > > ---
> > >  src/PVE/API2/LXC.pm | 16 +++++++++++++---
> > >  src/PVE/LXC.pm      |  9 +++++++--
> > >  2 files changed, 20 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> > > index 89ba64c..3561317 100644
> > > --- a/src/PVE/API2/LXC.pm
> > > +++ b/src/PVE/API2/LXC.pm
> > > @@ -2533,6 +2533,12 @@ __PACKAGE__->register_method({
> > >  	properties => {
> > >  	    node => get_standard_option('pve-node'),
> > >  	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid }),
> > > +	    all => {
> > > +		type => 'boolean',
> > > +		default => 0,
> > > +		optional => 1,
> > > +		description => 'Return all adresses of each interface instead of only one',
> > 
> > typo: s/adresses/addresses
> > 
> > > +	    }
> > >  	},
> > >      },
> > >      returns => {
> > > @@ -2552,12 +2558,14 @@ __PACKAGE__->register_method({
> > >  		},
> > >  		inet => {
> > >  		    type => 'string',
> > > -		    description => 'The IPv4 address of the interface',
> > > +		    format => 'CIDRv4-list',
> > 
> > this format here and the code below don't agree. a string type with the -list suffix needs actually be a string with the list elements delimited by either space, ',' or ';'. in this case, comma or semicolon is probably okay.
> > 
> 
> Since the caller needs to specify a new parameter, shouldn't we just
> return an actual array instead?

the retun schema wouldn't match then for either variant of the call..


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

Re: [pve-devel] [PATCH pve-container 2/2] fix #5339: api: lxc: ip: add 'all' option so that all addresses can be returned.

[Gabriel Goller]

Hi Johannes, 
I recently submitted a patch similar to this one
(https://lore.proxmox.com/pve-devel/20241210151130.321984-1-g.goller@proxmox.com/T/#ma90b76fb51c7b24b8a1eb6031fd874f481d8b4a8)
for a different feature and completely overlooked that this patch exists
already. In my opinion, we should continue the work on my more recent
patch, as I prefer to always return an array, rather than only doing so
when a specific property is passed. The reason for this is that when
returning a single IP, there is no clear way to choose the "correct" one
(and, in fact, there may not even be a "correct" one to choose).

Let me know what you think!


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