BUG#35992885 BUG#35997569

BUG#35992885 .rescan() fails to add unmanaged instance if recovery
account is unsupported

During Cluster.rescan(), a check is made to verify if the recovery
account of a given instance has the correct format. If it hasn't, an
error is thrown ("ERROR: Unsupported recovery account 'rpl_user' has
been found for instance ..." as reported in the issue), meaning that the
account isn't updated. This patch makes sure that rescan properly deals
with accounts with unknown formats, by creating a new user (with the
correct format) and assigning it to the recovery channel.

BUG#35997569 Replica misconfiguration after rescan() +
setPrimaryInstance()

Up until now, the commands ReplicaSet.rescan() and dba.createReplicaSet
(with "adoptFromAR" to 1), didn't change the replication account already
associated with the async replication channel. This means that the
account format would be different than the format of an account created
by the AdminAPI. This could lead to problems as described in
BUG#35997569. This patch makes sure that rescan(), if necessary, creates
new users with the correct format and updates the channel to use that
user instead. Also, when createReplicaSet() is adopting from an existing
AR channel, it now guarantees that said channel is using the correct
account format, by creating a new user if necessary.

Change-Id: Ia8601d72b67e6b31fe1c7cff2432beec11d62397

Author: joaosigma

modules/adminapi/CMakeLists.txt

@@ -78,8 +78,8 @@ file(GLOB adminapi_module_SOURCES
       "replica_set/describe.cc"
       "replica_set/dissolve.cc"
       "replica_set/replica_set_impl.cc"
-      "replica_set/replica_set_status.cc"
       "replica_set/rescan.cc"
+      "replica_set/status.cc"
       "cluster/add_instance.cc"
       "cluster/add_replica_instance.cc"
       "cluster/rejoin_instance.cc"

modules/adminapi/cluster/cluster_impl.cc

@@ -645,7 +645,8 @@ void Cluster_impl::ensure_metadata_has_server_id(
       });
 }
 
-void Cluster_impl::ensure_metadata_has_recovery_accounts() {
+void Cluster_impl::ensure_metadata_has_recovery_accounts(
+    bool allow_non_standard_format) {
   auto endpoints =
       get_metadata_storage()->get_instances_with_recovery_accounts(get_id());
 
@@ -657,16 +658,17 @@ void Cluster_impl::ensure_metadata_has_recovery_accounts() {
 
   execute_in_members(
       {}, get_cluster_server()->get_connection_options(), {},
-      [this, &console, &endpoints](const std::shared_ptr<Instance> &instance,
-                                   const mysqlshdk::gr::Member &gr_member) {
+      [this, &console, &endpoints, &allow_non_standard_format](
+          const std::shared_ptr<Instance> &instance,
+          const mysqlshdk::gr::Member &gr_member) {
         std::string recovery_user = mysqlshdk::mysql::get_replication_user(
             *instance, mysqlshdk::gr::k_gr_recovery_channel);
 
         log_debug("Fixing recovering account '%s' in instance '%s'",
                   recovery_user.c_str(), instance->descr().c_str());
 
-        bool recovery_is_valid = false;
-        if (!recovery_user.empty()) {
+        bool recovery_is_valid = allow_non_standard_format;
+        if (!recovery_is_valid && !recovery_user.empty()) {
           recovery_is_valid =
               shcore::str_beginswith(
                   recovery_user,
@@ -675,31 +677,30 @@ void Cluster_impl::ensure_metadata_has_recovery_accounts() {
                   recovery_user, mysqlshdk::gr::k_group_recovery_user_prefix);
         }
 
-        if (!recovery_user.empty()) {
-          if (!recovery_is_valid) {
-            console->print_error(shcore::str_format(
-                "Unsupported recovery account '%s' has been found for instance "
-                "'%s'. Operations such as "
-                "<Cluster>.<<<resetRecoveryAccountsPassword>>>() and "
-                "<Cluster>.<<<addInstance>>>() may fail. Please remove and add "
-                "the instance back to the Cluster to ensure a supported "
-                "recovery account is used.",
-                recovery_user.c_str(), instance->descr().c_str()));
-            return true;
-          }
-
-          auto it = endpoints.find(instance->get_uuid());
-          if ((it != endpoints.end()) && (it->second != recovery_user)) {
-            get_metadata_storage()->update_instance_repl_account(
-                gr_member.uuid, Cluster_type::GROUP_REPLICATION,
-                Replica_type::GROUP_MEMBER, recovery_user,
-                get_replication_user_host());
-          }
+        if (recovery_user.empty())
+          throw std::logic_error(shcore::str_format(
+              "Recovery user account not found for server address '%s' with "
+              "UUID %s",
+              instance->descr().c_str(), instance->get_uuid().c_str()));
+
+        if (!recovery_is_valid) {
+          console->print_error(shcore::str_format(
+              "Unsupported recovery account '%s' has been found for instance "
+              "'%s'. Operations such as "
+              "<Cluster>.<<<resetRecoveryAccountsPassword>>>() and "
+              "<Cluster>.<<<addInstance>>>() may fail. Please remove and add "
+              "the instance back to the Cluster to ensure a supported "
+              "recovery account is used.",
+              recovery_user.c_str(), instance->descr().c_str()));
+          return true;
+        }
 
-        } else {
-          throw std::logic_error(
-              "Recovery user account not found for server address: " +
-              instance->descr() + " with UUID " + instance->get_uuid());
+        auto it = endpoints.find(instance->get_uuid());
+        if ((it != endpoints.end()) && (it->second != recovery_user)) {
+          get_metadata_storage()->update_instance_repl_account(
+              gr_member.uuid, Cluster_type::GROUP_REPLICATION,
+              Replica_type::GROUP_MEMBER, recovery_user,
+              get_replication_user_host());
         }
 
         return true;
@@ -1432,7 +1433,7 @@ void Cluster_impl::update_replication_allowed_host(const std::string &host) {
         current_console()->print_note(
             "Legacy cluster account management detected, will update it "
             "first.");
-        ensure_metadata_has_recovery_accounts();
+        ensure_metadata_has_recovery_accounts(false);
         goto retry;
       } else {
         throw shcore::Exception::runtime_error(

modules/adminapi/cluster/cluster_impl.h

@@ -492,6 +492,7 @@ class Cluster_impl final : public Base_cluster_impl,
 
   /**
    * Get the list of instances in the states described in the states vector.
+   *
    * @param states Vector of strings with the states of members whose
    * instance_metadata we will retrieve from the cluster. If the states vector
    * is empty, return the list of all instances.
@@ -521,8 +522,12 @@ class Cluster_impl final : public Base_cluster_impl,
    * Checks for missing metadata recovery account entries, and fix them using
    * info from mysql.slave_master_info.
    *
+   * @param allow_non_standard_format If true, account that don't follow the
+   * AdminAPI format are stored, otherwise, a message is generated and the
+   * account ignored.
+   *
    */
-  void ensure_metadata_has_recovery_accounts();
+  void ensure_metadata_has_recovery_accounts(bool allow_non_standard_format);
 
   /**
    * Get an online instance from the cluster.

modules/adminapi/cluster/rescan.cc

@@ -1019,7 +1019,7 @@ shcore::Value Rescan::execute() {
   }
 
   // Print warning about not used instances in addInstances.
-  {
+  if (!m_options.add_instances_list.empty()) {
     std::vector<std::string> not_used_add_instances;
     not_used_add_instances.reserve(m_options.add_instances_list.size());
 
@@ -1039,20 +1039,16 @@ shcore::Value Rescan::execute() {
   }
 
   // Check if there are missing instances
-  std::shared_ptr<shcore::Value::Array_type> missing_instances;
-
   if (result->has_key("unavailableInstances")) {
-    missing_instances = result->get_array("unavailableInstances");
-
-    remove_metadata_for_instances(missing_instances);
+    remove_metadata_for_instances(result->get_array("unavailableInstances"));
   }
 
   if (result->has_key("updatedInstances")) {
     update_metadata_for_instances(result->get_array("updatedInstances"));
   }
 
   // Fix all missing recovery accounts from metadata.
-  m_cluster->ensure_metadata_has_recovery_accounts();
+  m_cluster->ensure_metadata_has_recovery_accounts(true);
 
   // This calls ensures all of the instances have server_id as instance
   // attribute, including the case were rescan is executed and no new/updated
@@ -1108,7 +1104,7 @@ shcore::Value Rescan::execute() {
   ensure_transaction_size_limit_consistency();
 
   // Print warning about not used instances in removeInstances.
-  {
+  if (!m_options.remove_instances_list.empty()) {
     std::vector<std::string> not_used_remove_instances;
     not_used_remove_instances.reserve(m_options.remove_instances_list.size());
 
@@ -1133,7 +1129,7 @@ shcore::Value Rescan::execute() {
   //       the auto-increment settings on all of them.
   if (result->has_key("newTopologyMode") &&
       !result->is_null("newTopologyMode")) {
-    std::string new_topology_mode = result->get_string("newTopologyMode");
+    auto new_topology_mode = result->get_string("newTopologyMode");
 
     if (!new_topology_mode.empty()) {
       console->print_note("The topology mode of the cluster changed to '" +

modules/adminapi/cluster_set/cluster_set_impl.cc

@@ -43,7 +43,6 @@
 #include "modules/adminapi/common/server_features.h"
 #include "modules/adminapi/common/undo.h"
 #include "modules/adminapi/dba/create_cluster.h"
-#include "modules/adminapi/replica_set/replica_set_status.h"
 #include "mysqlshdk/include/shellcore/shell_notifications.h"
 #include "mysqlshdk/libs/mysql/async_replication.h"
 #include "mysqlshdk/libs/mysql/binlog_utils.h"