BUG#26830224: DBA.CONFIGURELOCALINSTANCE() CLAIMS STATUS IS OK WHEN MY.CNF UPDATE FAILED

When using the configureLocalInstance command to update the
configuration file of a server already belonging to a cluster no error
message would be displayed if the user didn't have enough file system
privileges to update the configuration file.
This patch fixes that issue and now, an appropriate error message is shown
in the output if the user is lacking privileges to update the configuration
file.
Furthermore, some test related code was moved.
A new test was added to verify the new behavior, although it only runs on
Unix systems.

Author: Nelson Goncalves

python/front_end/mysqlprovision.py

@@ -877,7 +877,10 @@
         command_error_msg = "executing operation"
         try:
             if command == CHECK:
-                command_error_msg = "checking instance"
+                if cmd_options.get("update", False):
+                    command_error_msg = "configuring instance"
+                else:
+                    command_error_msg = "checking instance"
                 check(**cmd_options)
 
             elif command == CLONE:

python/mysql_gadgets/command/gr_admin.py

@@ -299,8 +299,7 @@ def check(**kwargs):
                 _LOGGER.info("Updating option file '%s' with Group "
                              "Replication settings from "
                              "%s", option_file, str(server))
-                persist_gr_config(option_file, gr_configs)
-                result = True
+                result = persist_gr_config(option_file, gr_configs)
             else:
                 result = False
         # If the server doesn't belong to any group, check if it meets GR

python/mysql_gadgets/common/config_parser.py

@@ -1,5 +1,5 @@
 #
-# Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+# Copyright (c) 2016, 2017, Oracle and/or its affiliates. All rights reserved.
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -434,7 +434,7 @@ def __init__(self, filename):  # pylint: disable=W0231
 
         self.default_extension = DEFAULT_EXTENSIONS[os.name]
         # get the absolute path for the provided filename
-        self.filename = get_abs_path(filename, os.getcwd())
+        self.filename = os.path.normpath(get_abs_path(filename, os.getcwd()))
         self._parse_option_file(self.filename)
 
         # Convert all section names to lower case.
@@ -495,8 +495,8 @@ def _parse_option_file(self, filename):
                             files.insert(len(files), filename)
             except (IOError, OSError) as err:
                 raise GadgetConfigParserError(
-                    "Option file '{0}' is not readable: "
-                    "'{1}'".format(self.filename, str(err)),
+                    "Option file '{0}' is not readable."
+                    "".format(self.filename),
                     cause=err)
 
         # Read configurations from option files.
@@ -858,23 +858,23 @@ def optval_tostr(opt, val):
                 lines = f.readlines()
         except IOError as err:
             raise GadgetConfigParserError(
-                "Option file '{0}' is not readable: "
-                "'{1}'".format(self.filename, str(err)),
+                "Option file '{0}' is not readable."
+                "".format(self.filename),
                 cause=err)
         # Create a backup file if provided
         if backup_file_path:
             if not os.path.isabs(backup_file_path):
                 raise GadgetConfigParserError(
                     "'{0}' is not an absolute path. Please provide an "
-                    "absolute path to the backup file")
+                    "absolute path to the backup file".format(backup_file_path))
             else:
                 try:
                     with open(backup_file_path, 'w') as bf:
                         bf.writelines(lines)
                 except IOError as err:
                     raise GadgetConfigParserError(
-                        "Backup file '{0}' is not writable: "
-                        "'{1}'".format(backup_file_path, str(err)),
+                        "Backup file '{0}' is not writable."
+                        "".format(backup_file_path),
                         cause=err)
         f = None
         try:
@@ -1002,8 +1002,8 @@ def optval_tostr(opt, val):
                         f.write("{0}\n".format(optval_tostr(opt, val)))
         except IOError as err:
             raise GadgetConfigParserError(
-                "Option file '{0}' is not writable: "
-                "'{1}'".format(self.filename, str(err)),
+                "Option file '{0}' is not writable."
+                "".format(self.filename),
                 cause=err)
         else:
             # we've written changes to file, reset modified flag

python/mysql_gadgets/common/group_replication.py

@@ -835,9 +835,6 @@ def update_option_file(opt_parser, missing_values, update_values,
     dic_msg = str(update_values)
     dic_msg = dic_msg.replace("\\\\", "\\")
     _LOGGER.debug("update_values %s", dic_msg)
-    # Verify option parser can update file
-    if not os.access(opt_parser.filename, os.W_OK):
-        return False
 
     # Create the section if does not exist already
     if not opt_parser.has_section(MYSQLD_SECTION):

unittest/scripts/js_devapi/scripts/dba_configure_local_instance_errors.js

@@ -0,0 +1,3 @@
+//@<OUT> Error no write privileges
+var cnfPath = __sandbox_dir + __mysql_sandbox_port1 + "/my.cnf";
+dba.configureLocalInstance('root@localhost:' + __mysql_sandbox_port1, {mycnfPath:cnfPath, password:'root'});

unittest/scripts/js_devapi/validation/dba_configure_local_instance_errors.js

@@ -0,0 +1,8 @@
+//@<OUT> Error no write privileges
+{
+    "errors": [
+        "Error configuring instance: Option file '<<<__output_sandbox_dir>>><<<__mysql_sandbox_port1>>><<<__path_splitter>>>my.cnf' is not writable."
+    ],
+    "restart_required": false,
+    "status": "error"
+}

unittest/shell_js_dba_t.cc

@@ -17,6 +17,10 @@
 * 02110-1301  USA
 */
 
+#ifdef _WIN32
+#include <windows.h>
+#endif
+
 #include <algorithm>
 #include "modules/adminapi/mod_dba_sql.h"
 #include "modules/base_session.h"
@@ -477,6 +481,46 @@ TEST_F(Shell_js_dba_tests, configure_local_instance) {
   execute("cleanup_sandboxes(true)");
 }
 
+TEST_F(Shell_js_dba_tests, configure_local_instance_errors) {
+  _options->wizards = false;
+  reset_shell();
+
+  // Execute setup script to be able to use smart deployment functions.
+  execute_setup();
+
+  // Clean and delete all sandboxes.
+  execute("cleanup_sandbox(__mysql_sandbox_port1);");
+
+  // Deploy new sandbox instance (required for this test).
+  execute(
+      "var deployed_here = reset_or_deploy_sandbox(__mysql_sandbox_port1);");
+
+  // create Cluster
+  execute(
+      "shell.connect({scheme: 'mysql', host: localhost, port: "
+      "__mysql_sandbox_port1, user: 'root', password: 'root'});");
+  execute("var cluster = dba.createCluster('Cluster')");
+
+#ifndef _WIN32
+  // Set permissions on configuration file to 444 (chmod only works on
+  // unix systems).
+  chmod(_sandbox_cnf_1.c_str(), S_IRUSR | S_IRGRP | S_IROTH);
+#else
+  auto dwAttrs = GetFileAttributes(_sandbox_cnf_1.c_str());
+  // set permissions on configuration file to read only
+  SetFileAttributes(_sandbox_cnf_1.c_str(), dwAttrs | FILE_ATTRIBUTE_READONLY);
+#endif
+  validate_interactive("dba_configure_local_instance_errors.js");
+
+#ifdef _WIN32
+  // Restore original permissions of configuration file
+  SetFileAttributes(_sandbox_cnf_1.c_str(), dwAttrs);
+#endif
+  // Cleans up the used sandbox
+  execute("cleanup_sandbox(__mysql_sandbox_port1)");
+}
+
+
 TEST_F(Shell_js_dba_tests, force_quorum) {
   _options->wizards = false;
   reset_shell();

unittest/test_utils/shell_base_test.cc

@@ -15,6 +15,7 @@
 
 #include <fstream>
 #include <boost/algorithm/string.hpp>
+#include <utils/utils_path.h>
 #include "unittest/test_utils/shell_base_test.h"
 #include "shellcore/types.h"
 #include "utils/utils_general.h"
@@ -100,16 +101,15 @@ void Shell_base_test::SetUp() {
       _sandbox_dir = shcore::get_binary_folder();
     }
   }
-
-  std::vector<std::string> path_components = {_sandbox_dir,
-                                              _mysql_sandbox_port1, "my.cnf"};
-  _sandbox_cnf_1 = shcore::join_strings(path_components, _path_splitter);
+  std::vector<std::string> path_components {_sandbox_dir,
+                                            _mysql_sandbox_port1, "my.cnf"};
+  _sandbox_cnf_1 = shcore::join_path(path_components);
 
   path_components[1] = _mysql_sandbox_port2;
-  _sandbox_cnf_2 = shcore::join_strings(path_components, _path_splitter);
+  _sandbox_cnf_2 = shcore::join_path(path_components);
 
   path_components[1] = _mysql_sandbox_port3;
-  _sandbox_cnf_3 = shcore::join_strings(path_components, _path_splitter);
+  _sandbox_cnf_3 = shcore::join_path(path_components);
 
   _new_line_char = "\n";
 #ifdef WIN32