]> git.proxmox.com Git - pve-cluster.git/commitdiff
corosync: add verify_conf
authorStefan Reiter <s.reiter@proxmox.com>
Thu, 9 Jan 2020 15:31:33 +0000 (16:31 +0100)
committerThomas Lamprecht <t.lamprecht@proxmox.com>
Thu, 30 Jan 2020 19:26:09 +0000 (20:26 +0100)
It does some basic sanity checking, warns the user about encryption
settings and unresolved hostnames, and finally makes sure that all nodes
have the same links configured (as well as comparing the configured
links to specified interfaces, if there are any).

A corosync.conf that has been created and modified strictly through our
API should *always* be valid.

verify_conf is called in 'addnode', warnings and errors are returned via
the API to be displayed in the task log of the node asking to join. If a
verification error occurs, it is handled specially via a "raise" outside
of any lock code that strips extra information from an Exception
instance. This ensures that multi-line formatted errors can be returned.
Warnings are always returned as array, to be printed on the caller.

Includes testing.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
data/PVE/API2/ClusterConfig.pm
data/PVE/Cluster/Setup.pm
data/PVE/Corosync.pm
data/test/corosync_parser_test.pl

index c426a307759f66dcd441ff484d6146026d048715..e05fd556e93789e7539855c6f1543bd3b3dc256b 100644 (file)
@@ -3,6 +3,7 @@ package PVE::API2::ClusterConfig;
 use strict;
 use warnings;
 
+use PVE::Exception;
 use PVE::Tools;
 use PVE::SafeSyslog;
 use PVE::RESTHandler;
@@ -219,7 +220,13 @@ __PACKAGE__->register_method ({
            },
            corosync_conf => {
                type => 'string',
-           }
+           },
+           warnings => {
+               type => 'array',
+               items => {
+                   type => 'string',
+               },
+           },
        },
     },
     code => sub {
@@ -227,11 +234,17 @@ __PACKAGE__->register_method ({
 
        PVE::Cluster::check_cfs_quorum();
 
+       my $vc_errors;
+       my $vc_warnings;
+
        my $code = sub {
            my $conf = PVE::Cluster::cfs_read_file("corosync.conf");
            my $nodelist = PVE::Corosync::nodelist($conf);
            my $totem_cfg = PVE::Corosync::totem_config($conf);
 
+           ($vc_errors, $vc_warnings) = PVE::Corosync::verify_conf($conf);
+           die if scalar(@$vc_errors);
+
            my $name = $param->{node};
 
            # ensure we do not reuse an address, that can crash the whole cluster!
@@ -317,11 +330,36 @@ __PACKAGE__->register_method ({
        };
 
        $config_change_lock->($code);
+
+       # If vc_errors is set, we died because of verify_conf.
+       # Raise this error, since it contains more information than just a
+       # single-line string.
+       if (defined($vc_errors) && scalar(@$vc_errors)) {
+           my $err_hash = {};
+           my $add_errs = sub {
+               my $type = shift;
+               my @arr = @_;
+               return if !scalar(@arr);
+
+               my %newhash = map { $type . $_ => $arr[$_] } 0..$#arr;
+               $err_hash = {
+                   %$err_hash,
+                   %newhash,
+               };
+           };
+
+           $add_errs->("warning", @$vc_warnings);
+           $add_errs->("error", @$vc_errors);
+
+           PVE::Exception::raise("invalid corosync.conf\n", errors => $err_hash);
+       }
+
        die $@ if $@;
 
        my $res = {
            corosync_authkey => PVE::Tools::file_get_contents($authfile),
            corosync_conf => PVE::Tools::file_get_contents($clusterconf),
+           warnings => $vc_warnings,
        };
 
        return $res;
index f8638851dfd99f6df37b2d0c4663eefa4e3c1737..b115d45d00245323f92aaa73a87c05bc678fa37c 100644 (file)
@@ -685,7 +685,28 @@ sub join {
     $args->{link1} = $param->{link1} if defined($param->{link1});
 
     print "Request addition of this node\n";
-    my $res = $conn->post("/cluster/config/nodes/$nodename", $args);
+    my $res = eval { $conn->post("/cluster/config/nodes/$nodename", $args); };
+    if (my $err = $@) {
+       if (ref($err) && $err->isa('PVE::APIClient::Exception')) {
+           # we received additional info about the error, show the user
+           chomp $err->{msg};
+           warn "An error occured on the cluster node: $err->{msg}\n";
+           foreach my $key (sort keys %{$err->{errors}}) {
+               my $symbol = ($key =~ m/^warning/) ? '*' : '!';
+               warn "$symbol $err->{errors}->{$key}\n";
+           }
+
+           die "Cluster join aborted!\n";
+       }
+
+       die $@;
+    }
+
+    if (defined($res->{warnings})) {
+       foreach my $warn (@{$res->{warnings}}) {
+           warn "cluster: $warn\n";
+       }
+    }
 
     print "Join request OK, finishing setup locally\n";
 
index 3208a6bf1925ac6166d90d57d991047d5b1b14e9..c69c8d8ca690b900f3af30a3db89cb4fec9e7c5a 100644 (file)
@@ -15,6 +15,7 @@ use PVE::Tools;
 use PVE::Tools qw($IPV4RE $IPV6RE);
 
 my $basedir = "/etc/pve";
+our $link_addr_re = qw/^(ring|link)(\d+)_addr$/;
 
 my $conf_array_sections = {
     node => 1,
@@ -292,6 +293,119 @@ sub create_conf {
     return { main => $conf };
 }
 
+# returns (\@errors, \@warnings) to the caller, does *not* 'die' or 'warn'
+# verification was successful if \@errors is empty
+sub verify_conf {
+    my ($conf) = @_;
+
+    my @errors = ();
+    my @warnings = ();
+
+    my $nodelist = nodelist($conf);
+    if (!$nodelist) {
+       push @errors, "no nodes found";
+       return (\@errors, \@warnings);
+    }
+
+    my $totem = $conf->{main}->{totem};
+    if (!$totem) {
+       push @errors, "no totem found";
+       return (\@errors, \@warnings);
+    }
+
+    if ((!defined($totem->{secauth}) || $totem->{secauth} ne 'on') &&
+       (!defined($totem->{crypto_cipher}) || $totem->{crypto_cipher} eq 'none')) {
+       push @warnings, "warning: authentication/encryption is not explicitly enabled"
+           . " (secauth / crypto_cipher / crypto_hash)";
+    }
+
+    my $interfaces = $totem->{interface};
+
+    my $verify_link_ip = sub {
+       my ($key, $link, $node) = @_;
+       my ($resolved_ip, undef) = resolve_hostname_like_corosync($link, $conf);
+       if (!defined($resolved_ip)) {
+           push @warnings, "warning: unable to resolve $key '$link' for node '$node'"
+               . " to an IP address according to Corosync's resolve strategy -"
+               . " cluster could fail on restart!";
+       } elsif ($resolved_ip ne $link) {
+           push @warnings, "warning: $key '$link' for node '$node' resolves to"
+               . " '$resolved_ip' - consider replacing it with the currently"
+               . " resolved IP address for stability";
+       }
+    };
+
+    # sort for output order stability
+    my @node_names = sort keys %$nodelist;
+
+    my $node_links = {};
+    foreach my $node (@node_names) {
+       my $options = $nodelist->{$node};
+       foreach my $opt (keys %$options) {
+           my ($linktype, $linkid) = parse_link_entry($opt);
+           next if !defined($linktype);
+           $node_links->{$node}->{$linkid} = {
+               name => "${linktype}${linkid}_addr",
+               addr => $options->{$opt},
+           };
+       }
+    }
+
+    if (%$interfaces) {
+       # if interfaces are defined, *all* links must have a matching interface
+       # definition, and vice versa
+       for my $link (0..1) {
+           my $have_interface = defined($interfaces->{$link});
+           foreach my $node (@node_names) {
+               my $linkdef = $node_links->{$node}->{$link};
+               if (defined($linkdef)) {
+                   $verify_link_ip->($linkdef->{name}, $linkdef->{addr}, $node);
+                   if (!$have_interface) {
+                       push @errors, "node '$node' has '$linkdef->{name}', but"
+                           . " there is no interface number $link configured";
+                   }
+               } else {
+                   if ($have_interface) {
+                       push @errors, "node '$node' is missing address for"
+                           . "interface number $link";
+                   }
+               }
+           }
+       }
+    } else {
+       # without interfaces, only check that links are consistent among nodes
+       for my $link (0..1) {
+           my $nodes_with_link = {};
+           foreach my $node (@node_names) {
+               my $linkdef = $node_links->{$node}->{$link};
+               if (defined($linkdef)) {
+                   $verify_link_ip->($linkdef->{name}, $linkdef->{addr}, $node);
+                   $nodes_with_link->{$node} = 1;
+               }
+           }
+
+           if (%$nodes_with_link) {
+               foreach my $node (@node_names) {
+                   if (!defined($nodes_with_link->{$node})) {
+                       push @errors, "node '$node' is missing link $link,"
+                           . " which is configured on other nodes";
+                   }
+               }
+           }
+       }
+    }
+
+    return (\@errors, \@warnings);
+}
+
+# returns ($linktype, $linkid) with $linktype being 'ring' for now, and possibly
+# 'link' with upcoming corosync versions
+sub parse_link_entry {
+    my ($opt) = @_;
+    return (undef, undef) if $opt !~ $link_addr_re;
+    return ($1, $2);
+}
+
 sub for_all_corosync_addresses {
     my ($corosync_conf, $ip_version, $func) = @_;
 
@@ -302,7 +416,7 @@ sub for_all_corosync_addresses {
     foreach my $node_name (sort keys %$nodelist) {
        my $node_config = $nodelist->{$node_name};
        foreach my $node_key (sort keys %$node_config) {
-           if ($node_key =~ /^(ring|link)\d+_addr$/) {
+           if ($node_key =~ $link_addr_re) {
                my $node_address = $node_config->{$node_key};
 
                my($ip, $version) = resolve_hostname_like_corosync($node_address, $corosync_conf);
index 18ee4f7b2630d8e7631878d0f02802449b6f982f..6999d3abb64965a406d27c22b82ab62f5f4bd945 100755 (executable)
@@ -5,10 +5,35 @@ use lib '..';
 use strict;
 use warnings;
 
+use Test::MockModule;
 use Test::More;
 
 use PVE::Corosync;
 
+my $known_hosts = {
+    "prox1" => "127.0.1.1",
+    "prox1-ring1" => "127.0.2.1",
+    "prox2" => "127.0.1.2",
+    "prox2-ring1" => "127.0.2.2",
+    "prox3" => "127.0.1.3",
+    "prox3-ring1" => "127.0.2.3",
+    "prox4" => "127.0.1.4",
+    "prox4-ring1" => "127.0.2.4",
+};
+
+sub mocked_resolve {
+    my ($hostname) = @_;
+
+    foreach my $host (keys %$known_hosts) {
+       return $known_hosts->{$host} if $hostname eq $host;
+    }
+
+    die "got unknown hostname '$hostname' during mocked resolve_hostname_like_corosync";
+}
+
+my $mocked_corosync_module = new Test::MockModule('PVE::Corosync');
+$mocked_corosync_module->mock('resolve_hostname_like_corosync', \&mocked_resolve);
+
 sub parser_self_check {
     my $cfg_fn = shift;
 
@@ -30,6 +55,10 @@ sub parser_self_check {
        $config2 = PVE::Corosync::parse_conf(undef, $raw2);
     }; warn $@ if $@;
 
+    # test verify_config
+    my ($err, $warn) = PVE::Corosync::verify_conf($config1);
+    die "verify_conf failed: " . join(" ++ ", @$err) if scalar(@$err);
+
     # do not care for whitespace differences
     delete $config1->{digest};
     delete $config2->{digest};