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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.

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

> +    method => 'GET',
> +    proxyto => 'node',
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Audit' ]],

why not `/nodes/{node}` ?

> +    },
> +    description => "Get the content of /etc/hosts.",

above is wrong? probably left-over from copying?

> +    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)
}

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