Re: [pve-devel] [PATCH manager] API: add node address(es) API endpoint

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On April 16, 2021 12:17 pm, Thomas Lamprecht wrote:
> On 13.04.21 14:16, Fabian Grünbichler wrote:
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>>  PVE/API2/Nodes.pm | 70 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>> 
>> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
>> index ba6621c6..b30d0739 100644
>> --- a/PVE/API2/Nodes.pm
>> +++ b/PVE/API2/Nodes.pm
>> @@ -222,6 +222,7 @@ __PACKAGE__->register_method ({
>>  	my ($param) = @_;
>>  
>>  	my $result = [
>> +	    { name => 'addr' },
>>  	    { name => 'aplinfo' },
>>  	    { name => 'apt' },
>>  	    { name => 'capabilities' },
>> @@ -2183,6 +2184,75 @@ __PACKAGE__->register_method ({
>>  	return undef;
>>      }});
>>  
>> +__PACKAGE__->register_method ({
>> +    name => 'get_node_addr',
>> +    path => 'addr',
> 
> hmm, not so sure if this the best path choice, if network wouldn't be templated by
> the {iface} param that would be a better choice to avoid crowding here.
> 
> I pondered over moving that one a level deeper to network/if/{name} for a 
> major releas,
> and mode some things like netstat, possible even dns, under there - but it's work and
> the gain was rather small - with this call which could fit there ROI would be slightly
> bigger, but not too sure if worth it, just throwing out the idea there.

yeah, I would have liked to put it under network/, but didn't want to 
bother with the breaking change. with 7.0 it might be a good opportunity 
though.. maybe there is SDN stuff that could fit in as well?

> Besides that, the name could be slightly more descriptive `ip-addr` or even 
> ip-addresses`?

sure, could be done.

>> +    method => 'GET',
>> +    proxyto => 'node',
>> +    permissions => {
>> +	check => ['perm', '/', [ 'Sys.Audit' ]],
> 
> why not `/nodes/{node}` ?

not sure, but probably a good idea (also required to read the interface 
config which is about the same level of "secrecy" I'd say.

> 
>> +    },
>> +    description => "Get the content of /etc/hosts.",
> 
> above is wrong? probably left-over from copying?

yes.

> 
>> +    parameters => {
>> +	additionalProperties => 0,
>> +	properties => {
>> +	    node => get_standard_option('pve-node'),
>> +	    cidr => {
>> +		type => 'string',
>> +		format => 'CIDR',
>> +		format_description => 'CIDR',
>> +		description => 'Extra network for which to retrieve local address(es).',
>> +		optional => 1,
>> +	    },
>> +	},
>> +    },
>> +    returns => {
>> +	type => 'object',
>> +	properties => {
>> +	    default => {
>> +		type => 'string',
>> +		description => 'Default node IP.',
>> +		format => 'ip',
>> +	    },
>> +	    migration => {
>> +		type => 'array',
>> +		items => {
>> +		    type => 'string',
>> +		    description => 'Migration network IP(s).',
>> +		    format => 'ip',
>> +		},
>> +		optional => 1,
>> +	    },
>> +	    extra => {
>> +		type => 'array',
>> +		items => {
>> +		    type => 'string',
>> +		    description => 'Extra network IP(s).',
>> +		    format => 'ip',
>> +		},
>> +		optional => 1,
>> +	    },
>> +	},
>> +    },
>> +    code => sub {
>> +	my ($param) = @_;
>> +
>> +	my $data = {};
>> +
>> +	my $default = PVE::Cluster::remote_node_ip($param->{node});
>> +
>> +	my $dc_conf = cfs_read_file('datacenter.cfg');
>> +	my $migration = $dc_conf->{migration}->{network};
>> +
>> +	$data->{default} = $default if defined($default);
>> +	$data->{migration} = PVE::Network::get_local_ip_from_cidr($migration, 1)
>> +	    if $migration;
> 
> style nit, probably opinionated and thus really no hard feelings, but maybe the
> following is lightly more legible due to declaration being nearer to usage:
> 
> if (my $default = PVE::Cluster::remote_node_ip($param->{node}) {
>     $data->{default} = $default;
> }
> 
> my $dc_conf = cfs_read_file('datacenter.cfg');
> if (my $migration = $dc_conf->{migration}->{network}) {
>     $data->{migration} = PVE::Network::get_local_ip_from_cidr($migration, 1);
> }
> 
> if (my $cidr = $param->{cidr}) {
>     $data->{extra} = PVE::Network::get_local_ip_from_cidr($param->{cidr}, 1)
> }

LGTM

> 
>> +	$data->{extra} = PVE::Network::get_local_ip_from_cidr($param->{cidr}, 1)
>> +	    if $param->{cidr};
>> +
>> +	return $data;
>> +    }});
>> +
>>  # bash completion helper
>>  
>>  sub complete_templet_repo {
>> 
> 
>