WL#12773 BUG#29629121 BUG#29867008 BUG#29559303 BUG#29868204 BUG#29869023 BUG#29862474 Simplification of internal recovery accounts

This simplifies the number of recovery accounts that are created.
After this patch a only a single recovery account is created for each
member of the cluster, even if the ipWhitelist option is used.

Furthermore, the recovery user with the "localhost" host part is no
longer created (only '%'). This can lead to an issue in local
experimental setups using "localhost" for the report_host. It is a
known limitation, but not considered a problem because "localhost" is
not expected to be used (especially not in a real production
settings).

This patch also deprecates the superReadOnly option for the
createCluster and rebootClusterFromCompleteOutage commands. The
super-read-only mode is now automatically disabled.

Existing tests were modified and updated according to the new number
and name format of the recovery accounts. No new tests were added because
existing tests already covered the test plan proposed by QA.

BUG#29629121: track created users, do not use pattern to guess when
cleaning up

This bug was fixed by using the information about the recovery users
stored in the metadata or in the mysql.slave_master_info table when
dropping the recovery user. Users are no longer dropped according to a
regular expression.

BUG#29867008: removeinstance(), rejoininstance() shouldn't drop all accounts

This bug was only partially fixed in the context of the WL.
This wl fixed it by dropping the recovery account that belongs to
the instance removed instance at the primary instance and then waiting
for that information to be replicated to the instance being removed
before actually removing the instance from the cluster.

BUG#29559303: cluster.dissolve() fails

This bug was fixed by handling both recovery account formats used by
the shell: the new name format defined by this worklog
'mysql_innodb_cluster_<server_id>' and the old account format
'mysql_innodb_cluster_r<random_number>' when dropping a recovery user
of an instance. This is done in both cluster.removeInstance and
cluster.dissolve.

BUG#29868204: cluster.addinstance() fails complaining about read-only
mode

This bug was a symptom of a new check that was added during the
context of this worklog but has since been removed. The check needed
to create a new user and that check was being done before the SRO was
disabled by the addInstance operation. Since the check is no longer made,
nothing really needed to be done to fix this issue.

BUG#29869023: cluster.rejoininstance() fails: instances cannot
communicate with each other

This bug was a symptom of a new check that was added during the
context of this worklog but has since been removed. The check executed
a "change master" that was not correctly handling cases where the
servers were using ssl. This issue has since been fixed even tough the
check is not currently used.

BUG#29862474: dba.checkinstanceconfiguration fails when
--super-read-only is on

This bug was a symptom of a new check that was added to
dba.checkInstanceConfiguration() during the context of this worklog
but has since been removed. The new check would not work if the target
instance had SRO enabled. This was a new requrement, as previously
none of the checks made by dba.checkInstanceConfiguration() would
require SRO to be disabled and that was the cause of the issue. Since
the check has since been removed, there was nothing that needed to be
done to fix this issue.

Change-Id: Ib567e3f2c7a3bd23c515b04d91354fb1e6352cf9

Author: Nelson Goncalves

modules/adminapi/cluster/cluster_impl.cc

@@ -45,6 +45,7 @@
 #include "mysqlshdk/include/shellcore/console.h"
 #include "mysqlshdk/libs/mysql/group_replication.h"
 #include "mysqlshdk/libs/mysql/replication.h"
+#include "mysqlshdk/libs/mysql/utils.h"
 #include "shellcore/utils_help.h"
 #include "utils/debug.h"
 #include "utils/utils_general.h"
@@ -378,5 +379,100 @@ mysqlshdk::utils::Version Cluster_impl::get_lowest_instance_version() const {
   return lowest_version;
 }
 
+mysqlshdk::mysql::Auth_options Cluster_impl::create_replication_user(
+    mysqlshdk::mysql::IInstance *target) {
+  assert(target);
+  mysqlshdk::mysql::Instance primary_master(get_group_session());
+  std::string recovery_user =
+      mysqlshdk::gr::k_group_recovery_user_prefix +
+      std::to_string(target->get_sysvar_int("server_id").get_safe());
+
+  log_info("Creating recovery account '%s'@'%%' for instance '%s'",
+           recovery_user.c_str(), target->descr().c_str());
+
+  // Check if the replication user already exists and delete it if it does,
+  // before creating it again.
+  bool rpl_user_exists = primary_master.user_exists(recovery_user, "%");
+  if (rpl_user_exists) {
+    auto console = current_console();
+    console->print_warning(shcore::str_format(
+        "User '%s'@'%%' already existed at instance '%s'. It will be "
+        "deleted and created again with new credentials.",
+        recovery_user.c_str(), primary_master.descr().c_str()));
+    primary_master.drop_user(recovery_user, "%");
+  }
+  return mysqlshdk::gr::create_recovery_user(recovery_user, &primary_master,
+                                             {"%"}, {});
+}
+
+void Cluster_impl::drop_replication_user(mysqlshdk::mysql::IInstance *target) {
+  assert(target);
+  mysqlshdk::mysql::Instance primary_master(get_group_session());
+  auto console = current_console();
+
+  std::string recovery_user, recovery_user_host;
+  std::tie(recovery_user, recovery_user_host) =
+      get_metadata_storage()->get_instance_recovery_account(target->get_uuid());
+  if (recovery_user.empty()) {
+    // TODO(Miguel): WL13208: handle cases where the replication user was stolen
+    // from donor during clone
+
+    // Assuming the account was created by an older version of the shell,
+    // which did not store account name in metadata
+    log_info(
+        "No recovery account details in metadata for instance '%s', assuming "
+        "old account style",
+        target->descr().c_str());
+
+    // Get replication user (recovery) used by the instance to remove
+    // on remaining members.
+    recovery_user = mysqlshdk::gr::get_recovery_user(*target);
+
+    // Remove the replication user used by the removed instance on all
+    // cluster members through the primary (using replication).
+    // NOTE: Make sure to remove the user if it was an user created by
+    // the Shell, i.e. with the format: mysql_innodb_cluster_r[0-9]{10}
+    if (shcore::str_beginswith(
+            recovery_user, mysqlshdk::gr::k_group_recovery_old_user_prefix)) {
+      log_info("Removing replication user '%s'", recovery_user.c_str());
+      try {
+        mysqlshdk::mysql::drop_all_accounts_for_user(get_group_session(),
+                                                     recovery_user);
+      } catch (const std::exception &e) {
+        console->print_warning("Error dropping recovery accounts for user " +
+                               recovery_user + ": " + e.what());
+      }
+    } else {
+      console->print_note("The recovery user name for instance '" +
+                          target->descr() +
+                          "' does not match the expected format for users "
+                          "created automatically "
+                          "by InnoDB Cluster. Skipping its removal.");
+    }
+  } else {
+    log_info("Dropping recovery account '%s'@'%s' for instance '%s'",
+             recovery_user.c_str(), recovery_user_host.c_str(),
+             target->descr().c_str());
+
+    try {
+      primary_master.drop_user(recovery_user, "%", true);
+    } catch (const std::exception &e) {
+      console->print_warning(shcore::str_format(
+          "Error dropping recovery account '%s'@'%s' for instance '%s'",
+          recovery_user.c_str(), recovery_user_host.c_str(),
+          target->descr().c_str()));
+    }
+
+    // Also update metadata
+    try {
+      get_metadata_storage()->update_instance_recovery_account(
+          target->get_uuid(), "", "");
+    } catch (const std::exception &e) {
+      log_warning("Could not update recovery account metadata for '%s': %s",
+                  target->descr().c_str(), e.what());
+    }
+  }
+}
+
 }  // namespace dba
 }  // namespace mysqlsh

modules/adminapi/cluster/cluster_impl.h

@@ -37,9 +37,6 @@
 #include "mysqlshdk/libs/db/connection_options.h"
 #include "mysqlshdk/libs/innodbcluster/cluster_metadata.h"
 
-#define ACC_USER "username"
-#define ACC_PASSWORD "password"
-
 #define ATT_DEFAULT "default"
 
 namespace mysqlsh {
@@ -98,6 +95,11 @@ class Cluster_impl {
     return _group_session;
   }
 
+  mysqlshdk::mysql::Auth_options create_replication_user(
+      mysqlshdk::mysql::IInstance *target);
+
+  void drop_replication_user(mysqlshdk::mysql::IInstance *target);
+
   void disconnect();
 
  public:

modules/adminapi/cluster/dissolve.cc

@@ -329,9 +329,6 @@ void Dissolve::remove_instance(const std::string &instance_address,
     // Stop Group Replication and reset (persist) GR variables.
     mysqlsh::dba::leave_replicaset(*m_available_instances[instance_index]);
 
-    // Remove existing replications users.
-    m_cluster->get_default_replicaset()->remove_replication_users(
-        *m_available_instances[instance_index], false);
   } catch (const std::exception &err) {
     auto console = mysqlsh::current_console();
 
@@ -345,7 +342,7 @@ void Dissolve::remove_instance(const std::string &instance_address,
       log_error("Instance '%s' failed to leave the ReplicaSet: %s",
                 instance_address.c_str(), err.what());
       console->print_warning(
-          "An error occured when trying to remove instance '" +
+          "An error occurred when trying to remove instance '" +
           instance_address +
           "' from the cluster. The instance might have been left active "
           "and in an inconsistent state, requiring manual action to "
@@ -356,13 +353,20 @@ void Dissolve::remove_instance(const std::string &instance_address,
 }
 
 shcore::Value Dissolve::execute() {
-  // Drop the cluster's metadata.
   std::shared_ptr<MetadataStorage> metadata = m_cluster->get_metadata_storage();
+  auto console = mysqlsh::current_console();
+
+  // JOB: Remove replication accounts used for recovery of GR.
+  // Note: This operation MUST be performed before leave-replicaset to ensure
+  // that all changed are propagated to the online instances.
+  for (auto &instance : m_available_instances) {
+    m_cluster->drop_replication_user(instance.get());
+  }
+
+  // Drop the cluster's metadata.
   std::string cluster_name = m_cluster->get_name();
   metadata->drop_cluster(cluster_name);
 
-  auto console = mysqlsh::current_console();
-
   // We must stop GR on the online instances only, otherwise we'll
   // get connection failures to the (MISSING) instances
   // BUG#26001653.
@@ -406,7 +410,7 @@ shcore::Value Dissolve::execute() {
               "%s",
               instance_address.c_str(), err.what());
           console->print_warning(
-              "An error occured when trying to catch up with cluster "
+              "An error occurred when trying to catch up with cluster "
               "transactions and instance '" +
               instance_address +
               "' might have been left in an inconsistent state that will lead "
@@ -480,7 +484,7 @@ shcore::Value Dissolve::execute() {
     warning_msg.append("in order to be able to be reused.");
     console->print_warning(warning_msg);
   } else {
-    // Full cluster sucessfuly removed.
+    // Full cluster successfully removed.
     console->println("The cluster was successfully dissolved.");
     console->println("Replication was disabled but user data was left intact.");
     console->println();

modules/adminapi/common/common.cc

@@ -103,26 +103,6 @@ std::string get_mysqlprovision_error_string(
   return shcore::str_join(str_errors, "\n");
 }
 
-std::vector<std::string> convert_ipwhitelist_to_netmask(
-    const std::vector<std::string> &ip_whitelist) {
-  std::vector<std::string> ret;
-
-  for (const std::string &value : ip_whitelist) {
-    // Strip any blank chars from the ip_whitelist value and
-    // Translate CIDR to netmask notation
-    ret.push_back(
-        mysqlshdk::utils::Net::cidr_to_netmask(shcore::str_strip(value)));
-  }
-
-  return ret;
-}
-
-std::vector<std::string> convert_ipwhitelist_to_netmask(
-    const std::string &ip_whitelist) {
-  return convert_ipwhitelist_to_netmask(
-      shcore::str_split(shcore::str_strip(ip_whitelist), ",", -1));
-}
-
 bool is_group_replication_option_supported(
     const mysqlshdk::utils::Version &version, const std::string &option,
     const std::map<std::string, Option_availability> &options_map) {
@@ -1017,29 +997,10 @@ bool validate_super_read_only(const mysqlshdk::mysql::IInstance &instance,
         "For more information see: https://dev.mysql.com/doc/refman/en/"
         "server-system-variables.html#sysvar_super_read_only.");
 
-    auto show_open_sessions = [&](const std::string &message) {
-      // Get the list of open session to the instance
-      std::vector<std::pair<std::string, int>> open_sessions =
-          get_open_sessions(instance.get_session());
-      if (!open_sessions.empty()) {
-        console->print_note(message);
-        for (const auto &value : open_sessions) {
-          console->print_info(std::to_string(value.second) +
-                              " open session(s) of '" + value.first + "'. \n");
-        }
-      }
-    };
-
     if (clear_read_only.is_null() && interactive) {
       console->print_info(error_message);
       console->print_info();
 
-      show_open_sessions(
-          "There are open sessions to '" + instance.descr() +
-          "'.\n"
-          "You may want to kill these sessions to prevent them from "
-          "performing unexpected updates: \n");
-
       if (console->confirm(
               "Do you want to disable super_read_only and continue?",
               mysqlsh::Prompt_answer::NO) == mysqlsh::Prompt_answer::NO) {
@@ -1060,10 +1021,6 @@ bool validate_super_read_only(const mysqlshdk::mysql::IInstance &instance,
     } else {
       console->print_error(error_message);
 
-      show_open_sessions(
-          "If you unset super_read_only you should consider closing the "
-          "following: ");
-
       throw shcore::Exception::runtime_error("Server in SUPER_READ_ONLY mode");
     }
   }

modules/adminapi/common/common.h

@@ -182,21 +182,6 @@ extern const char *kMemberSSLModeAuto;
 extern const char *kMemberSSLModeRequired;
 extern const char *kMemberSSLModeDisabled;
 
-/**
- * Converts an ipWhitelist to a list of addresses using the netmask notation
- *
- * @param ip_whitelist The vector of strings containing the ipWhitelist to
- * convert
- *
- * @return A vector of strings containing the ipWhitelist converted to netmask
- * notation
- */
-std::vector<std::string> convert_ipwhitelist_to_netmask(
-    const std::vector<std::string> &ip_whitelist);
-
-std::vector<std::string> convert_ipwhitelist_to_netmask(
-    const std::string &ip_whitelist);
-
 /**
  * Check if a group_replication setting is supported on the target
  * instance