[pve-devel] [PATCH-SERIES v2 common/manager] fix #2422: allow multiple Ceph public networks

[pve-devel] [PATCH-SERIES v2 common/manager] fix #2422: allow multiple Ceph public networks

[Fabian Ebner]

picked up from an old patch by Alwin[0].

Changes from v1:
    * avoid passing user-provided duplicate IP to monmaptool
    * match IPv6 addresses semantically
    * added Dominik's R-b and T-b tags for the unchanged patches
    * add two minor fixes for is_ip_in_cidr


New dependency on libnetaddr-ip-perl for pve-common needed!

Dependency bump pve-manager -> pve-common needed.


[0]: https://lists.proxmox.com/pipermail/pve-devel/2020-March/042304.html


common:

Fabian Ebner (4):
  network: is_ip_in_cidr: correctly handle the CIDR being a singleton
    range
  network: is_ip_in_cidr: avoid warning when versions don't match
  network: add canonical_ip function
  network: add unique_ips function

 src/PVE/Network.pm | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)


manager:

Fabian Ebner (10):
  api: ceph: mon: split up arguments for run_command
  api: ceph: create mon: handle ms_bind_ipv* options more generally
  api: ceph: create mon: factor out monmaptool command
  api: ceph: create mon: explicitly add subsequent monitors to the
    monmap
  api: ceph: mon: fix handling of IPv6 addresses in find_mon_ip
  api: ceph: mon: add ips_from_mon_host helper
  api: ceph: mon: fix handling of IPv6 addresses in
    assert_mon_prerequisites
  api: ceph: mon: factor out mon_host regex address removal
  api: ceph: mon: fix handling of IPv6 addresses in destroymon
  fix #2422: allow multiple Ceph public networks

 PVE/API2/Ceph/MON.pm | 297 +++++++++++++++++++++++++++++++++----------
 1 file changed, 229 insertions(+), 68 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH v2 common 01/14] network: is_ip_in_cidr: correctly handle the CIDR being a singleton range

[Fabian Ebner]

i.e.  is_ip_in_cidr('127.0.0.1', '127.0.0.1/32', 4) should return 1;

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 src/PVE/Network.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Network.pm b/src/PVE/Network.pm
index 38019f7..366d242 100644
--- a/src/PVE/Network.pm
+++ b/src/PVE/Network.pm
@@ -592,7 +592,9 @@ sub is_ip_in_cidr {
     my $ip_obj = Net::IP->new($ip, $version);
     return undef if !$ip_obj;
 
-    return $cidr_obj->overlaps($ip_obj) == $Net::IP::IP_B_IN_A_OVERLAP;
+    my $overlap = $cidr_obj->overlaps($ip_obj);
+
+    return $overlap == $Net::IP::IP_B_IN_A_OVERLAP || $overlap == $Net::IP::IP_IDENTICAL;
 }
 
 
-- 
2.20.1

[pve-devel] [PATCH v2 common 02/14] network: is_ip_in_cidr: avoid warning when versions don't match

[Fabian Ebner]

is_ip_in_cidr('fd80:1::10', '127.0.0.1/24') would result in
    Use of uninitialized value in numeric eq (==)
as overlaps() returns undef in such a case.

Note that there are (albeit few) existing callers that don't specify $version.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 src/PVE/Network.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/PVE/Network.pm b/src/PVE/Network.pm
index 366d242..2d63a45 100644
--- a/src/PVE/Network.pm
+++ b/src/PVE/Network.pm
@@ -594,6 +594,8 @@ sub is_ip_in_cidr {
 
     my $overlap = $cidr_obj->overlaps($ip_obj);
 
+    return if !defined($overlap);
+
     return $overlap == $Net::IP::IP_B_IN_A_OVERLAP || $overlap == $Net::IP::IP_IDENTICAL;
 }
 
-- 
2.20.1

[pve-devel] [PATCH v2 common 03/14] network: add canonical_ip function

[Fabian Ebner]

Net::IP doesn't seem to have a function for it and normalizing to the full
quad-form is less then ideal if we inted to output IPv6 addresses returned by
that function at some point.

Instead, use NetAddr::IP, which is already used in pve-network.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

Introduces a new dependency on the libnetaddr-ip-perl package!

 src/PVE/Network.pm | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/PVE/Network.pm b/src/PVE/Network.pm
index 2d63a45..48268b1 100644
--- a/src/PVE/Network.pm
+++ b/src/PVE/Network.pm
@@ -10,6 +10,7 @@ use PVE::Tools qw(run_command lock_file);
 use File::Basename;
 use IO::Socket::IP;
 use Net::IP;
+use NetAddr::IP qw(:lower);
 use POSIX qw(ECONNREFUSED);
 use Socket qw(NI_NUMERICHOST NI_NUMERICSERV);
 
@@ -655,4 +656,13 @@ sub lock_network {
     return $res;
 }
 
+# the canonical form of the given IP, i.e. dotted quad for IPv4 and RFC 5952 for IPv6
+sub canonical_ip {
+    my ($ip) = @_;
+
+    my $ip_obj = NetAddr::IP->new($ip) or die "invalid IP string '$ip'\n";
+
+    return $ip_obj->canon();
+}
+
 1;
-- 
2.20.1

[pve-devel] [PATCH v2 common 04/14] network: add unique_ips function

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 src/PVE/Network.pm | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/src/PVE/Network.pm b/src/PVE/Network.pm
index 48268b1..bb574e0 100644
--- a/src/PVE/Network.pm
+++ b/src/PVE/Network.pm
@@ -665,4 +665,24 @@ sub canonical_ip {
     return $ip_obj->canon();
 }
 
+# List of unique, canonical IPs in the provided list.
+# Keeps the original order, filtering later duplicates.
+sub unique_ips {
+    my ($ips) = @_;
+
+    my $res = [];
+    my $seen = {};
+
+    for my $ip (@{$ips}) {
+	$ip = canonical_ip($ip);
+
+	next if $seen->{$ip};
+
+	$seen->{$ip} = 1;
+	push @{$res}, $ip;
+    }
+
+    return $res;
+}
+
 1;
-- 
2.20.1

[pve-devel] [PATCH v2 manager 05/14] api: ceph: mon: split up arguments for run_command

[Fabian Ebner]

no functional change is intended.

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

No changes from v1.

 PVE/API2/Ceph/MON.pm | 46 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 7 deletions(-)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index f10c1064..c759fdc8 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -229,9 +229,23 @@ __PACKAGE__->register_method ({
 
 		if (! -f $mon_keyring) {
 		    print "creating new monitor keyring\n";
-		    run_command("ceph-authtool --create-keyring $mon_keyring ".
-			" --gen-key -n mon. --cap mon 'allow *'");
-		    run_command("ceph-authtool $mon_keyring --import-keyring $client_keyring");
+		    run_command([
+			'ceph-authtool',
+			'--create-keyring',
+			$mon_keyring,
+			'--gen-key',
+			'-n',
+			'mon.',
+			'--cap',
+			'mon',
+			'allow *',
+		    ]);
+		    run_command([
+			'ceph-authtool',
+			$mon_keyring,
+			'--import-keyring',
+			$client_keyring,
+		    ]);
 		}
 
 		my $ccname = PVE::Ceph::Tools::get_config('ccname');
@@ -243,7 +257,7 @@ __PACKAGE__->register_method ({
 		eval {
 		    mkdir $mondir;
 
-		    run_command("chown ceph:ceph $mondir");
+		    run_command(['chown', 'ceph:ceph', $mondir]);
 
 		    if (defined($rados)) { # we can only have a RADOS object if we have a monitor
 			my $mapdata = $rados->mon_command({ prefix => 'mon getmap', format => 'plain' });
@@ -255,11 +269,29 @@ __PACKAGE__->register_method ({
 			    $cfg->{global}->{ms_bind_ipv6} = 'true';
 			    $cfg->{global}->{ms_bind_ipv4} = 'false';
 			}
-			run_command("monmaptool --create --clobber --addv $monid '[v2:$monaddr:3300,v1:$monaddr:6789]' --print $monmap");
+			run_command([
+			    'monmaptool',
+			    '--create',
+			    '--clobber',
+			    '--addv',
+			    $monid,
+			    "[v2:$monaddr:3300,v1:$monaddr:6789]",
+			    '--print',
+			    $monmap,
+			]);
 		    }
 
-		    run_command("ceph-mon --mkfs -i $monid --monmap $monmap --keyring $mon_keyring");
-		    run_command("chown ceph:ceph -R $mondir");
+		    run_command([
+			'ceph-mon',
+			'--mkfs',
+			'-i',
+			$monid,
+			'--monmap',
+			$monmap,
+			'--keyring',
+			$mon_keyring,
+		    ]);
+		    run_command(['chown', 'ceph:ceph', '-R', $mondir]);
 		};
 		my $err = $@;
 		unlink $monmap;
-- 
2.20.1

[pve-devel] [PATCH v2 manager 06/14] api: ceph: create mon: handle ms_bind_ipv* options more generally

[Fabian Ebner]

mostly relevant to prepare support for IPv4/IPv6 dual stack mode as a special
case of the planned support for mutliple public networks.

As before, only set the false value when we are dealing with the first address,
but also be explicit about the IPv4 case as the defaults might change in the
future.

Then, when an address of a different type comes along later, set the relevant
bind option to true.

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

No changes from v1.

 PVE/API2/Ceph/MON.pm | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index c759fdc8..b59b88f0 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -259,6 +259,15 @@ __PACKAGE__->register_method ({
 
 		    run_command(['chown', 'ceph:ceph', $mondir]);
 
+		    my $is_first_address = !defined($rados);
+		    if (Net::IP::ip_is_ipv6($ip)) {
+			$cfg->{global}->{ms_bind_ipv6} = 'true';
+			$cfg->{global}->{ms_bind_ipv4} = 'false' if $is_first_address;
+		    } else {
+			$cfg->{global}->{ms_bind_ipv4} = 'true';
+			$cfg->{global}->{ms_bind_ipv6} = 'false' if $is_first_address;
+		    }
+
 		    if (defined($rados)) { # we can only have a RADOS object if we have a monitor
 			my $mapdata = $rados->mon_command({ prefix => 'mon getmap', format => 'plain' });
 			file_set_contents($monmap, $mapdata);
@@ -266,8 +275,6 @@ __PACKAGE__->register_method ({
 			my $monaddr = $ip;
 			if (Net::IP::ip_is_ipv6($ip)) {
 			    $monaddr = "[$ip]";
-			    $cfg->{global}->{ms_bind_ipv6} = 'true';
-			    $cfg->{global}->{ms_bind_ipv4} = 'false';
 			}
 			run_command([
 			    'monmaptool',
-- 
2.20.1

[pve-devel] [PATCH v2 manager 07/14] api: ceph: create mon: factor out monmaptool command

[Fabian Ebner]

so it's easier to re-use for a future variant.

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

No changes from v1.

 PVE/API2/Ceph/MON.pm | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index b59b88f0..7a72cd37 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -268,24 +268,24 @@ __PACKAGE__->register_method ({
 			$cfg->{global}->{ms_bind_ipv6} = 'false' if $is_first_address;
 		    }
 
+		    my $monaddr = Net::IP::ip_is_ipv6($ip) ? "[$ip]" : $ip;
+
+		    my $monmaptool_cmd = [
+			'monmaptool',
+			'--create',
+			'--clobber',
+			'--addv',
+			$monid,
+			"[v2:$monaddr:3300,v1:$monaddr:6789]",
+			'--print',
+			$monmap,
+		    ];
+
 		    if (defined($rados)) { # we can only have a RADOS object if we have a monitor
 			my $mapdata = $rados->mon_command({ prefix => 'mon getmap', format => 'plain' });
 			file_set_contents($monmap, $mapdata);
 		    } else { # we need to create a monmap for the first monitor
-			my $monaddr = $ip;
-			if (Net::IP::ip_is_ipv6($ip)) {
-			    $monaddr = "[$ip]";
-			}
-			run_command([
-			    'monmaptool',
-			    '--create',
-			    '--clobber',
-			    '--addv',
-			    $monid,
-			    "[v2:$monaddr:3300,v1:$monaddr:6789]",
-			    '--print',
-			    $monmap,
-			]);
+			run_command($monmaptool_cmd);
 		    }
 
 		    run_command([
-- 
2.20.1

[pve-devel] [PATCH v2 manager 08/14] api: ceph: create mon: explicitly add subsequent monitors to the monmap

[Fabian Ebner]

in preparation for supporting multiple addresses. The config section does not
allow more than one public_addr.

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

No changes from v1.

 PVE/API2/Ceph/MON.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 7a72cd37..24318a36 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -272,7 +272,6 @@ __PACKAGE__->register_method ({
 
 		    my $monmaptool_cmd = [
 			'monmaptool',
-			'--create',
 			'--clobber',
 			'--addv',
 			$monid,
@@ -284,7 +283,9 @@ __PACKAGE__->register_method ({
 		    if (defined($rados)) { # we can only have a RADOS object if we have a monitor
 			my $mapdata = $rados->mon_command({ prefix => 'mon getmap', format => 'plain' });
 			file_set_contents($monmap, $mapdata);
+			run_command($monmaptool_cmd);
 		    } else { # we need to create a monmap for the first monitor
+			push @{$monmaptool_cmd}, '--create';
 			run_command($monmaptool_cmd);
 		    }
 
-- 
2.20.1

[pve-devel] [PATCH v2 manager 09/14] api: ceph: mon: fix handling of IPv6 addresses in find_mon_ip

[Fabian Ebner]

by comparing their canonical forms.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

Dependency bump for pve-common needed.

 PVE/API2/Ceph/MON.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 24318a36..f7af3c37 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -38,16 +38,20 @@ my $find_mon_ip = sub {
     }
 
     my $allowed_ips = PVE::Network::get_local_ip_from_cidr($pubnet);
+    $allowed_ips = PVE::Network::unique_ips($allowed_ips);
+
     die "No active IP found for the requested ceph public network '$pubnet' on node '$node'\n"
 	if scalar(@$allowed_ips) < 1;
 
     if (!$overwrite_ip) {
-	if (scalar(@$allowed_ips) == 1 || !grep { $_ ne $allowed_ips->[0] } @$allowed_ips) {
+	if (scalar(@$allowed_ips) == 1) {
 	    return $allowed_ips->[0];
 	}
 	die "Multiple IPs for ceph public network '$pubnet' detected on $node:\n".
 	    join("\n", @$allowed_ips) ."\nuse 'mon-address' to specify one of them.\n";
     } else {
+	$overwrite_ip = PVE::Network::canonical_ip($overwrite_ip);
+
 	if (grep { $_ eq $overwrite_ip } @$allowed_ips) {
 	    return $overwrite_ip;
 	}
-- 
2.20.1

[pve-devel] [PATCH v2 manager 10/14] api: ceph: mon: add ips_from_mon_host helper

[Fabian Ebner]

Partially based on pve-storage's CephConfig.pm get_monaddr_list, but the
interface is not the best for the use case here.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 PVE/API2/Ceph/MON.pm | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index f7af3c37..e4e8ecfb 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -62,6 +62,30 @@ my $find_mon_ip = sub {
     }
 };
 
+my $ips_from_mon_host = sub {
+    my ($mon_host) = @_;
+
+    my $ips = [];
+
+    my @hosts = PVE::Tools::split_list($mon_host);
+
+    for my $host (@hosts) {
+	$host =~ s|^\[?v\d+\:||; # remove beginning of vector
+	$host =~ s|/\d+\]?||; # remove end of vector
+
+	($host) = PVE::Tools::parse_host_and_port($host);
+	next if !defined($host);
+
+	# filter out hostnames
+	my $ip = PVE::JSONSchema::pve_verify_ip($host, 1);
+	next if !defined($ip);
+
+	push @{$ips}, $ip;
+    }
+
+    return $ips;
+};
+
 my $assert_mon_prerequisites = sub {
     my ($cfg, $monhash, $monid, $monip) = @_;
 
-- 
2.20.1

[pve-devel] [PATCH v2 manager 11/14] api: ceph: mon: fix handling of IPv6 addresses in assert_mon_prerequisites

[Fabian Ebner]

by comparing their canonical forms.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 PVE/API2/Ceph/MON.pm | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index e4e8ecfb..27278bc1 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -89,15 +89,26 @@ my $ips_from_mon_host = sub {
 my $assert_mon_prerequisites = sub {
     my ($cfg, $monhash, $monid, $monip) = @_;
 
-    my $ip_regex = '(?:^|[^\d])' . # start or nondigit
-	       "\Q$monip\E" .  # the ip not interpreted as regex
-	       '(?:[^\d]|$)';  # end or nondigit
+    $monip = PVE::Network::canonical_ip($monip);
 
-    if (($cfg->{global}->{mon_host} && $cfg->{global}->{mon_host} =~ m/$ip_regex/) ||
-	grep { $_->{addr} && $_->{addr}  =~ m/$ip_regex/ } values %$monhash) {
-	die "monitor address '$monip' already in use\n";
+    my $used_ips = {};
+
+    my $mon_host_ips = $ips_from_mon_host->($cfg->{global}->{mon_host});
+
+    for my $mon_host_ip (@{$mon_host_ips}) {
+	my $ip = PVE::Network::canonical_ip($mon_host_ip);
+	$used_ips->{$ip} = 1;
+    }
+
+    for my $mon (values %{$monhash}) {
+	next if !defined($mon->{addr});
+
+	my $ip = PVE::Network::canonical_ip($mon->{addr});
+	$used_ips->{$ip} = 1;
     }
 
+    die "monitor address '$monip' already in use\n" if $used_ips->{$monip};
+
     if (defined($monhash->{$monid})) {
 	die "monitor '$monid' already exists\n";
     }
-- 
2.20.1

[pve-devel] [PATCH v2 manager 12/14] api: ceph: mon: factor out mon_host regex address removal

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 PVE/API2/Ceph/MON.pm | 52 ++++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 27278bc1..e5f11c8a 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -127,6 +127,34 @@ my $assert_mon_can_remove = sub {
     die "can't remove last monitor\n" if scalar(@$monlist) <= 1;
 };
 
+my $remove_addr_from_mon_host = sub {
+    my ($monhost, $addr) = @_;
+
+    # various replaces to remove the ip
+    # we always match the beginning or a separator (also at the end)
+    # so we do not accidentally remove a wrong ip
+    # e.g. removing 10.0.0.1 should not remove 10.0.0.101 or 110.0.0.1
+
+    # remove vector containing this ip
+    # format is [vX:ip:port/nonce,vY:ip:port/nonce]
+    my $vectorpart_re = "v\\d+:\Q$addr\E:\\d+\\/\\d+";
+    $monhost =~ s/(^|[ ,;]*)\[$vectorpart_re(?:,$vectorpart_re)*\](?:[ ,;]+|$)/$1/;
+
+    # ip (+ port)
+    $monhost =~ s/(^|[ ,;]+)\Q$addr\E(?::\d+)?(?:[ ,;]+|$)/$1/;
+
+    # ipv6 only without brackets
+    if ($addr =~ m/^\[?(.*?:.*?)\]?$/) {
+	$addr = $1;
+	$monhost =~ s/(^|[ ,;]+)\Q$addr\E(?:[ ,;]+|$)/$1/;
+    }
+
+    # remove trailing separators
+    $monhost =~ s/[ ,;]+$//;
+
+    return $monhost;
+};
+
 __PACKAGE__->register_method ({
     name => 'listmon',
     path => '',
@@ -471,29 +499,7 @@ __PACKAGE__->register_method ({
 
 		# delete from mon_host
 		if (my $monhost = $cfg->{global}->{mon_host}) {
-		    # various replaces to remove the ip
-		    # we always match the beginning or a separator (also at the end)
-		    # so we do not accidentally remove a wrong ip
-		    # e.g. removing 10.0.0.1 should not remove 10.0.0.101 or 110.0.0.1
-
-		    # remove vector containing this ip
-		    # format is [vX:ip:port/nonce,vY:ip:port/nonce]
-		    my $vectorpart_re = "v\\d+:\Q$addr\E:\\d+\\/\\d+";
-		    $monhost =~ s/(^|[ ,;]*)\[$vectorpart_re(?:,$vectorpart_re)*\](?:[ ,;]+|$)/$1/;
-
-		    # ip (+ port)
-		    $monhost =~ s/(^|[ ,;]+)\Q$addr\E(?::\d+)?(?:[ ,;]+|$)/$1/;
-
-		    # ipv6 only without brackets
-		    if ($addr =~ m/^\[?(.*?:.*?)\]?$/) {
-			$addr = $1;
-			$monhost =~ s/(^|[ ,;]+)\Q$addr\E(?:[ ,;]+|$)/$1/;
-		    }
-
-		    # remove trailing separators
-		    $monhost =~ s/[ ,;]+$//;
-
-		    $cfg->{global}->{mon_host} = $monhost;
+		    $cfg->{global}->{mon_host} = $remove_addr_from_mon_host->($monhost, $addr);
 		}
 
 		cfs_write_file('ceph.conf', $cfg);
-- 
2.20.1

[pve-devel] [PATCH v2 manager 13/14] api: ceph: mon: fix handling of IPv6 addresses in destroymon

[Fabian Ebner]

by also comparing the canonical form to decide when to remove an address. When
getting the IP from the rados information, also drop eventual brackets, so our
existing function can handle it. Add the brackets back within the
remove_addr_from_mon_host function.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 PVE/API2/Ceph/MON.pm | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index e5f11c8a..6244655d 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -130,6 +130,8 @@ my $assert_mon_can_remove = sub {
 my $remove_addr_from_mon_host = sub {
     my ($monhost, $addr) = @_;
 
+    $addr = "[$addr]" if PVE::JSONSchema::pve_verify_ipv6($addr, 1);
+
     # various replaces to remove the ip
     # we always match the beginning or a separator (also at the end)
     # so we do not accidentally remove a wrong ip
@@ -485,6 +487,7 @@ __PACKAGE__->register_method ({
 		    if ($mon->{name} eq $monid) {
 			$addr = $mon->{public_addr} // $mon->{addr};
 			($addr) = $addr =~ m|^(.*):\d+/\d+$|; # extract the ip without port/nonce
+			($addr) = $addr =~ m|^\[?(.*?)\]?$|; # remove brackets
 			last;
 		    }
 		}
@@ -499,7 +502,22 @@ __PACKAGE__->register_method ({
 
 		# delete from mon_host
 		if (my $monhost = $cfg->{global}->{mon_host}) {
-		    $cfg->{global}->{mon_host} = $remove_addr_from_mon_host->($monhost, $addr);
+		    $monhost = $remove_addr_from_mon_host->($monhost, $addr);
+
+		    # also remove matching IPs that differ syntactically
+		    if (PVE::JSONSchema::pve_verify_ip($addr, 1)) {
+			$addr = PVE::Network::canonical_ip($addr);
+
+			my $mon_host_ips = $ips_from_mon_host->($cfg->{global}->{mon_host});
+
+			for my $mon_host_ip (@{$mon_host_ips}) {
+			    # match canonical addresses, but remove as present in mon_host
+			    if (PVE::Network::canonical_ip($mon_host_ip) eq $addr) {
+				$monhost = $remove_addr_from_mon_host->($monhost, $mon_host_ip);
+			    }
+			}
+		    }
+		    $cfg->{global}->{mon_host} = $monhost;
 		}
 
 		cfs_write_file('ceph.conf', $cfg);
-- 
2.20.1

[pve-devel] [PATCH v2 manager 14/14] fix #2422: allow multiple Ceph public networks

[Fabian Ebner]

From: Alwin Antreich <a.antreich@proxmox.com>

Multiple public networks can be defined in the ceph.conf. The networks need to
be routed to each other.

Support handling multiple IPs for a single monitor. By default, one address from
each public network is selected for monitor creation, but, as before, it can be
overwritten with the mon-address parameter, now taking a list of addresses.

On removal, make sure the all addresses are removed from the mon_host entry in
the ceph configuration.

Originally-by: Alwin Antreich <a.antreich@proxmox.com>
[handling of multiple addresses]
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v1:
    * adapt to new handling of IPv6 addresses
    * make overwrite_ips unique

Sorry if the patch is a bit messy to review, what it essentially is, is creating
loops over nets/addresses around the old existing logic.

Viewing with a word-based diff might help in some places.


Older changes:

Changes from original patch as suggested by Fabian G.:
    * split list after truth check for $pubnet
    * avoid long post-if statement
    * style/indentation fixes
    * verify cidr

Other changes from original patch:
    * test/select IPs from each public network
    * return and handle a list of IPs, so that it's not necessary to create a
      monitor for each public network
    * also handle multiple addresses upon removal

 PVE/API2/Ceph/MON.pm | 164 +++++++++++++++++++++++++++++--------------
 1 file changed, 111 insertions(+), 53 deletions(-)

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 6244655d..12c9caf0 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -20,8 +20,11 @@ use PVE::API2::Ceph::MGR;
 
 use base qw(PVE::RESTHandler);
 
-my $find_mon_ip = sub {
-    my ($cfg, $rados, $node, $overwrite_ip) = @_;
+my $find_mon_ips = sub {
+    my ($cfg, $rados, $node, $mon_address) = @_;
+
+    my $overwrite_ips = [ PVE::Tools::split_list($mon_address) ];
+    $overwrite_ips = PVE::Network::unique_ips($overwrite_ips);
 
     my $pubnet;
     if ($rados) {
@@ -34,32 +37,60 @@ my $find_mon_ip = sub {
     $pubnet //= $cfg->{global}->{public_network};
 
     if (!$pubnet) {
-	return $overwrite_ip // PVE::Cluster::remote_node_ip($node);
+	if (scalar(@{$overwrite_ips})) {
+	    return $overwrite_ips;
+	} else {
+	   # don't refactor into '[ PVE::Cluster::remote... ]' as it uses wantarray
+	   my $ip = PVE::Cluster::remote_node_ip($node);
+	   return [ $ip ];
+	}
     }
 
-    my $allowed_ips = PVE::Network::get_local_ip_from_cidr($pubnet);
-    $allowed_ips = PVE::Network::unique_ips($allowed_ips);
+    my $public_nets = [ PVE::Tools::split_list($pubnet) ];
+    if (scalar(@{$public_nets}) > 1) {
+	warn "Multiple Ceph public networks detected on $node: $pubnet\n";
+	warn "Networks must be capable of routing to each other.\n";
+    }
+
+    my $res = [];
+
+    if (!scalar(@{$overwrite_ips})) { # auto-select one address for each public network
+	for my $net (@{$public_nets}) {
+	    $net = PVE::JSONSchema::pve_verify_cidr($net);
+
+	    my $allowed_ips = PVE::Network::get_local_ip_from_cidr($net);
+	    $allowed_ips = PVE::Network::unique_ips($allowed_ips);
 
-    die "No active IP found for the requested ceph public network '$pubnet' on node '$node'\n"
-	if scalar(@$allowed_ips) < 1;
+	    die "No active IP found for the requested ceph public network '$net' on node '$node'\n"
+		if scalar(@$allowed_ips) < 1;
 
-    if (!$overwrite_ip) {
-	if (scalar(@$allowed_ips) == 1) {
-	    return $allowed_ips->[0];
+	    if (scalar(@$allowed_ips) == 1) {
+		push @{$res}, $allowed_ips->[0];
+	    } else {
+		die "Multiple IPs for ceph public network '$net' detected on $node:\n".
+		    join("\n", @$allowed_ips) ."\nuse 'mon-address' to specify one of them.\n";
+	    }
 	}
-	die "Multiple IPs for ceph public network '$pubnet' detected on $node:\n".
-	    join("\n", @$allowed_ips) ."\nuse 'mon-address' to specify one of them.\n";
-    } else {
-	$overwrite_ip = PVE::Network::canonical_ip($overwrite_ip);
+    } else { # check if overwrite IPs are active and in any of the public networks
+	my $allowed_list = [];
+
+	for my $net (@{$public_nets}) {
+	    $net = PVE::JSONSchema::pve_verify_cidr($net);
 
-	if (grep { $_ eq $overwrite_ip } @$allowed_ips) {
-	    return $overwrite_ip;
+	    push @{$allowed_list}, @{PVE::Network::get_local_ip_from_cidr($net)};
 	}
-	die "Monitor IP '$overwrite_ip' not in ceph public network '$pubnet'\n"
-	    if !PVE::Network::is_ip_in_cidr($overwrite_ip, $pubnet);
 
-	die "Specified monitor IP '$overwrite_ip' not configured or up on $node!\n";
+	my $allowed_ips = PVE::Network::unique_ips($allowed_list);
+
+	for my $overwrite_ip (@{$overwrite_ips}) {
+	    die "Specified monitor IP '$overwrite_ip' not configured or up on $node!\n"
+		if !grep { $_ eq $overwrite_ip } @{$allowed_ips};
+
+	    push @{$res}, $overwrite_ip;
+	}
     }
+
+    return $res;
 };
 
 my $ips_from_mon_host = sub {
@@ -87,9 +118,7 @@ my $ips_from_mon_host = sub {
 };
 
 my $assert_mon_prerequisites = sub {
-    my ($cfg, $monhash, $monid, $monip) = @_;
-
-    $monip = PVE::Network::canonical_ip($monip);
+    my ($cfg, $monhash, $monid, $monips) = @_;
 
     my $used_ips = {};
 
@@ -107,7 +136,10 @@ my $assert_mon_prerequisites = sub {
 	$used_ips->{$ip} = 1;
     }
 
-    die "monitor address '$monip' already in use\n" if $used_ips->{$monip};
+    for my $monip (@{$monips}) {
+	$monip = PVE::Network::canonical_ip($monip);
+	die "monitor address '$monip' already in use\n" if $used_ips->{$monip};
+    }
 
     if (defined($monhash->{$monid})) {
 	die "monitor '$monid' already exists\n";
@@ -246,9 +278,9 @@ __PACKAGE__->register_method ({
 		description => "The ID for the monitor, when omitted the same as the nodename",
 	    },
 	    'mon-address' => {
-		description => 'Overwrites autodetected monitor IP address. ' .
-		               'Must be in the public network of ceph.',
-		type => 'string', format => 'ip',
+		description => 'Overwrites autodetected monitor IP address(es). ' .
+		               'Must be in the public network(s) of Ceph.',
+		type => 'string', format => 'ip-list',
 		optional => 1,
 	    },
 	},
@@ -276,9 +308,9 @@ __PACKAGE__->register_method ({
 
 	my $monid = $param->{monid} // $param->{node};
 	my $monsection = "mon.$monid";
-	my $ip = $find_mon_ip->($cfg, $rados, $param->{node}, $param->{'mon-address'});
+	my $ips = $find_mon_ips->($cfg, $rados, $param->{node}, $param->{'mon-address'});
 
-	$assert_mon_prerequisites->($cfg, $monhash, $monid, $ip);
+	$assert_mon_prerequisites->($cfg, $monhash, $monid, $ips);
 
 	my $worker = sub  {
 	    my $upid = shift;
@@ -291,7 +323,7 @@ __PACKAGE__->register_method ({
 		    $rados = PVE::RADOS->new(timeout => PVE::Ceph::Tools::get_config('long_rados_timeout'));
 		}
 		$monhash = PVE::Ceph::Services::get_services_info('mon', $cfg, $rados);
-		$assert_mon_prerequisites->($cfg, $monhash, $monid, $ip);
+		$assert_mon_prerequisites->($cfg, $monhash, $monid, $ips);
 
 		my $client_keyring = PVE::Ceph::Tools::get_or_create_admin_keyring();
 		my $mon_keyring = PVE::Ceph::Tools::get_config('pve_mon_key_path');
@@ -329,22 +361,31 @@ __PACKAGE__->register_method ({
 		    run_command(['chown', 'ceph:ceph', $mondir]);
 
 		    my $is_first_address = !defined($rados);
-		    if (Net::IP::ip_is_ipv6($ip)) {
-			$cfg->{global}->{ms_bind_ipv6} = 'true';
-			$cfg->{global}->{ms_bind_ipv4} = 'false' if $is_first_address;
-		    } else {
-			$cfg->{global}->{ms_bind_ipv4} = 'true';
-			$cfg->{global}->{ms_bind_ipv6} = 'false' if $is_first_address;
-		    }
 
-		    my $monaddr = Net::IP::ip_is_ipv6($ip) ? "[$ip]" : $ip;
+		    my $monaddrs = [];
+
+		    for my $ip (@{$ips}) {
+			if (Net::IP::ip_is_ipv6($ip)) {
+			    $cfg->{global}->{ms_bind_ipv6} = 'true';
+			    $cfg->{global}->{ms_bind_ipv4} = 'false' if $is_first_address;
+			} else {
+			    $cfg->{global}->{ms_bind_ipv4} = 'true';
+			    $cfg->{global}->{ms_bind_ipv6} = 'false' if $is_first_address;
+			}
+
+			my $monaddr = Net::IP::ip_is_ipv6($ip) ? "[$ip]" : $ip;
+			push @{$monaddrs}, "v2:$monaddr:3300";
+			push @{$monaddrs}, "v1:$monaddr:6789";
+
+			$is_first_address = 0;
+		    }
 
 		    my $monmaptool_cmd = [
 			'monmaptool',
 			'--clobber',
 			'--addv',
 			$monid,
-			"[v2:$monaddr:3300,v1:$monaddr:6789]",
+			"[" . join(',', @{$monaddrs}) . "]",
 			'--print',
 			$monmap,
 		    ];
@@ -385,10 +426,10 @@ __PACKAGE__->register_method ({
 			$monhost .= " " . $monhash->{$mon}->{addr};
 		    }
 		}
-		$monhost .= " $ip";
+		$monhost .= " " . join(' ', @{$ips});
 		$cfg->{global}->{mon_host} = $monhost;
 		# The IP is needed in the ceph.conf for the first boot
-		$cfg->{$monsection}->{public_addr} = $ip;
+		$cfg->{$monsection}->{public_addr} = $ips->[0];
 
 		cfs_write_file('ceph.conf', $cfg);
 
@@ -482,12 +523,27 @@ __PACKAGE__->register_method ({
 		$monstat = $rados->mon_command({ prefix => 'quorum_status' });
 		$monlist = $monstat->{monmap}->{mons};
 
-		my $addr;
+		my $addrs = [];
+
+		my $add_addr = sub {
+		    my ($addr) = @_;
+
+		    # extract the ip without port and nonce (if present)
+		    ($addr) = $addr =~ m|^(.*):\d+(/\d+)?$|;
+		    ($addr) = $addr =~ m|^\[?(.*?)\]?$|; # remove brackets
+		    push @{$addrs}, $addr;
+		};
+
 		for my $mon (@$monlist) {
 		    if ($mon->{name} eq $monid) {
-			$addr = $mon->{public_addr} // $mon->{addr};
-			($addr) = $addr =~ m|^(.*):\d+/\d+$|; # extract the ip without port/nonce
-			($addr) = $addr =~ m|^\[?(.*?)\]?$|; # remove brackets
+			if ($mon->{public_addrs} && $mon->{public_addrs}->{addrvec}) {
+			    my $addrvec = $mon->{public_addrs}->{addrvec};
+			    for my $addr (@{$addrvec}) {
+				$add_addr->($addr->{addr});
+			    }
+			} else {
+			    $add_addr->($mon->{public_addr} // $mon->{addr});
+			}
 			last;
 		    }
 		}
@@ -502,18 +558,20 @@ __PACKAGE__->register_method ({
 
 		# delete from mon_host
 		if (my $monhost = $cfg->{global}->{mon_host}) {
-		    $monhost = $remove_addr_from_mon_host->($monhost, $addr);
+		    my $mon_host_ips = $ips_from_mon_host->($cfg->{global}->{mon_host});
 
-		    # also remove matching IPs that differ syntactically
-		    if (PVE::JSONSchema::pve_verify_ip($addr, 1)) {
-			$addr = PVE::Network::canonical_ip($addr);
+		    for my $addr (@{$addrs}) {
+			$monhost = $remove_addr_from_mon_host->($monhost, $addr);
 
-			my $mon_host_ips = $ips_from_mon_host->($cfg->{global}->{mon_host});
+			# also remove matching IPs that differ syntactically
+			if (PVE::JSONSchema::pve_verify_ip($addr, 1)) {
+			    $addr = PVE::Network::canonical_ip($addr);
 
-			for my $mon_host_ip (@{$mon_host_ips}) {
-			    # match canonical addresses, but remove as present in mon_host
-			    if (PVE::Network::canonical_ip($mon_host_ip) eq $addr) {
-				$monhost = $remove_addr_from_mon_host->($monhost, $mon_host_ip);
+			    for my $mon_host_ip (@{$mon_host_ips}) {
+				# match canonical addresses, but remove as present in mon_host
+				if (PVE::Network::canonical_ip($mon_host_ip) eq $addr) {
+				    $monhost = $remove_addr_from_mon_host->($monhost, $mon_host_ip);
+				}
 			    }
 			}
 		    }
-- 
2.20.1

[pve-devel] applied-partially: [PATCH-SERIES v2 common/manager] fix #2422: allow multiple Ceph public networks

[Thomas Lamprecht]

On 10.05.21 14:18, Fabian Ebner wrote:
> common:
> 
> Fabian Ebner (4):
>   network: is_ip_in_cidr: correctly handle the CIDR being a singleton
>     range
>   network: is_ip_in_cidr: avoid warning when versions don't match
>   network: add canonical_ip function
>   network: add unique_ips function
> 
>  src/PVE/Network.pm | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 

thanks, applied the pve-common patches for now and added the libnetaddr-ip-perl dependency
to d/control.

Re: [pve-devel] [PATCH-SERIES v2 common/manager] fix #2422: allow multiple Ceph public networks

[Thomas Lamprecht]

On 10.05.21 14:18, Fabian Ebner wrote:
> manager:
> 
> Fabian Ebner (10):
>   api: ceph: mon: split up arguments for run_command
>   api: ceph: create mon: handle ms_bind_ipv* options more generally
>   api: ceph: create mon: factor out monmaptool command
>   api: ceph: create mon: explicitly add subsequent monitors to the
>     monmap
>   api: ceph: mon: fix handling of IPv6 addresses in find_mon_ip
>   api: ceph: mon: add ips_from_mon_host helper
>   api: ceph: mon: fix handling of IPv6 addresses in
>     assert_mon_prerequisites
>   api: ceph: mon: factor out mon_host regex address removal
>   api: ceph: mon: fix handling of IPv6 addresses in destroymon
>   fix #2422: allow multiple Ceph public networks
> 
>  PVE/API2/Ceph/MON.pm | 297 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 229 insertions(+), 68 deletions(-)
> 

Now applied the remaining manager part, much thanks to you and Alwin!