From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 0BC7E73A83 for ; Fri, 16 Apr 2021 13:38:00 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E99B523DC3 for ; Fri, 16 Apr 2021 13:37:29 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 8F8AE23DB7 for ; Fri, 16 Apr 2021 13:37:28 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 5B08345AE8 for ; Fri, 16 Apr 2021 13:37:28 +0200 (CEST) Date: Fri, 16 Apr 2021 13:37:21 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion , Thomas Lamprecht References: <20210413121640.3602975-1-f.gruenbichler@proxmox.com> <20210413121640.3602975-15-f.gruenbichler@proxmox.com> <6d9ddd05-4a58-dcdd-3a3a-988708fd97dd@proxmox.com> In-Reply-To: <6d9ddd05-4a58-dcdd-3a3a-988708fd97dd@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1618572897.rqgkiac2ss.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.026 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [nodes.pm] Subject: Re: [pve-devel] [PATCH manager] API: add node address(es) API endpoint X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Apr 2021 11:38:00 -0000 On April 16, 2021 12:17 pm, Thomas Lamprecht wrote: > On 13.04.21 14:16, Fabian Gr=C3=BCnbichler wrote: >> Signed-off-by: Fabian Gr=C3=BCnbichler >> --- >> PVE/API2/Nodes.pm | 70 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 70 insertions(+) >>=20 >> 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) =3D @_; >> =20 >> my $result =3D [ >> + { name =3D> 'addr' }, >> { name =3D> 'aplinfo' }, >> { name =3D> 'apt' }, >> { name =3D> 'capabilities' }, >> @@ -2183,6 +2184,75 @@ __PACKAGE__->register_method ({ >> return undef; >> }}); >> =20 >> +__PACKAGE__->register_method ({ >> + name =3D> 'get_node_addr', >> + path =3D> 'addr', >=20 > hmm, not so sure if this the best path choice, if network wouldn't be tem= plated by > the {iface} param that would be a better choice to avoid crowding here. >=20 > I pondered over moving that one a level deeper to network/if/{name} for a= =20 > major releas, > and mode some things like netstat, possible even dns, under there - but i= t's work and > the gain was rather small - with this call which could fit there ROI woul= d 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=20 bother with the breaking change. with 7.0 it might be a good opportunity=20 though.. maybe there is SDN stuff that could fit in as well? > Besides that, the name could be slightly more descriptive `ip-addr` or ev= en=20 > ip-addresses`? sure, could be done. >> + method =3D> 'GET', >> + proxyto =3D> 'node', >> + permissions =3D> { >> + check =3D> ['perm', '/', [ 'Sys.Audit' ]], >=20 > why not `/nodes/{node}` ? not sure, but probably a good idea (also required to read the interface=20 config which is about the same level of "secrecy" I'd say. >=20 >> + }, >> + description =3D> "Get the content of /etc/hosts.", >=20 > above is wrong? probably left-over from copying? yes. >=20 >> + parameters =3D> { >> + additionalProperties =3D> 0, >> + properties =3D> { >> + node =3D> get_standard_option('pve-node'), >> + cidr =3D> { >> + type =3D> 'string', >> + format =3D> 'CIDR', >> + format_description =3D> 'CIDR', >> + description =3D> 'Extra network for which to retrieve local address(e= s).', >> + optional =3D> 1, >> + }, >> + }, >> + }, >> + returns =3D> { >> + type =3D> 'object', >> + properties =3D> { >> + default =3D> { >> + type =3D> 'string', >> + description =3D> 'Default node IP.', >> + format =3D> 'ip', >> + }, >> + migration =3D> { >> + type =3D> 'array', >> + items =3D> { >> + type =3D> 'string', >> + description =3D> 'Migration network IP(s).', >> + format =3D> 'ip', >> + }, >> + optional =3D> 1, >> + }, >> + extra =3D> { >> + type =3D> 'array', >> + items =3D> { >> + type =3D> 'string', >> + description =3D> 'Extra network IP(s).', >> + format =3D> 'ip', >> + }, >> + optional =3D> 1, >> + }, >> + }, >> + }, >> + code =3D> sub { >> + my ($param) =3D @_; >> + >> + my $data =3D {}; >> + >> + my $default =3D PVE::Cluster::remote_node_ip($param->{node}); >> + >> + my $dc_conf =3D cfs_read_file('datacenter.cfg'); >> + my $migration =3D $dc_conf->{migration}->{network}; >> + >> + $data->{default} =3D $default if defined($default); >> + $data->{migration} =3D PVE::Network::get_local_ip_from_cidr($migration= , 1) >> + if $migration; >=20 > style nit, probably opinionated and thus really no hard feelings, but may= be the > following is lightly more legible due to declaration being nearer to usag= e: >=20 > if (my $default =3D PVE::Cluster::remote_node_ip($param->{node}) { > $data->{default} =3D $default; > } >=20 > my $dc_conf =3D cfs_read_file('datacenter.cfg'); > if (my $migration =3D $dc_conf->{migration}->{network}) { > $data->{migration} =3D PVE::Network::get_local_ip_from_cidr($migratio= n, 1); > } >=20 > if (my $cidr =3D $param->{cidr}) { > $data->{extra} =3D PVE::Network::get_local_ip_from_cidr($param->{cidr= }, 1) > } LGTM >=20 >> + $data->{extra} =3D PVE::Network::get_local_ip_from_cidr($param->{cidr}= , 1) >> + if $param->{cidr}; >> + >> + return $data; >> + }}); >> + >> # bash completion helper >> =20 >> sub complete_templet_repo { >>=20 >=20 >=20 =