cfgupgrade: Fix critical bug overwriting RAPI users file

The cfgupgrade tool was designed to be idempotent, that means it could
be run several times and still give produce the correct result. Ganeti
2.4 moved the file containing the RAPI users to a separate directory
(…/lib/ganeti/rapi/users). If it exists, cfgupgrade would automatically
move an existing file from …/lib/ganeti/rapi_users and replace it with a
symlink.

Unfortunately one of the checks for this was incorrect and, when run
multiple times, replaces the users file at the new location with a
symlink created during a previous run.

In addition the “--dry-run” parameter to cfgupgrade was not respected.
Unittests are updated for all these cases.

Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

diff --git a/test/cfgupgrade_unittest.py b/test/cfgupgrade_unittest.py
index 0819dfa..359a298 100755 (executable)
--- a/test/cfgupgrade_unittest.py
+++ b/test/cfgupgrade_unittest.py
@@ -161,12 +161,16 @@ class TestCfgupgrade(unittest.TestCase):
   def testRapiUsers(self):
     self.assertFalse(os.path.exists(self.rapi_users_path))
     self.assertFalse(os.path.exists(self.rapi_users_path_pre24))
+    self.assertFalse(os.path.exists(os.path.dirname(self.rapi_users_path)))
 
     utils.WriteFile(self.rapi_users_path_pre24, data="some user\n")
     self._TestSimpleUpgrade(constants.BuildVersion(2, 3, 0), False)
 
+    self.assertTrue(os.path.isdir(os.path.dirname(self.rapi_users_path)))
     self.assert_(os.path.islink(self.rapi_users_path_pre24))
     self.assert_(os.path.isfile(self.rapi_users_path))
+    self.assertEqual(os.readlink(self.rapi_users_path_pre24),
+                     self.rapi_users_path)
     for path in [self.rapi_users_path, self.rapi_users_path_pre24]:
       self.assertEqual(utils.ReadFile(path), "some user\n")
 
@@ -180,6 +184,8 @@ class TestCfgupgrade(unittest.TestCase):
 
     self.assert_(os.path.islink(self.rapi_users_path_pre24))
     self.assert_(os.path.isfile(self.rapi_users_path))
+    self.assertEqual(os.readlink(self.rapi_users_path_pre24),
+                     self.rapi_users_path)
     for path in [self.rapi_users_path, self.rapi_users_path_pre24]:
       self.assertEqual(utils.ReadFile(path), "other user\n")
 
@@ -187,13 +193,77 @@ class TestCfgupgrade(unittest.TestCase):
     self.assertFalse(os.path.exists(self.rapi_users_path))
     self.assertFalse(os.path.exists(self.rapi_users_path_pre24))
 
+    os.mkdir(os.path.dirname(self.rapi_users_path))
     os.symlink(self.rapi_users_path, self.rapi_users_path_pre24)
-    utils.WriteFile(self.rapi_users_path_pre24, data="hello world\n")
+    utils.WriteFile(self.rapi_users_path, data="hello world\n")
 
     self._TestSimpleUpgrade(constants.BuildVersion(2, 2, 0), False)
 
-    self.assert_(os.path.isfile(self.rapi_users_path))
+    self.assert_(os.path.isfile(self.rapi_users_path) and
+                 not os.path.islink(self.rapi_users_path))
     self.assert_(os.path.islink(self.rapi_users_path_pre24))
+    self.assertEqual(os.readlink(self.rapi_users_path_pre24),
+                     self.rapi_users_path)
+    for path in [self.rapi_users_path, self.rapi_users_path_pre24]:
+      self.assertEqual(utils.ReadFile(path), "hello world\n")
+
+  def testRapiUsersExistingTarget(self):
+    self.assertFalse(os.path.exists(self.rapi_users_path))
+    self.assertFalse(os.path.exists(self.rapi_users_path_pre24))
+
+    os.mkdir(os.path.dirname(self.rapi_users_path))
+    utils.WriteFile(self.rapi_users_path, data="other user\n")
+    utils.WriteFile(self.rapi_users_path_pre24, data="hello world\n")
+
+    self.assertRaises(Exception, self._TestSimpleUpgrade,
+                      constants.BuildVersion(2, 2, 0), False)
+
+    for path in [self.rapi_users_path, self.rapi_users_path_pre24]:
+      self.assert_(os.path.isfile(path) and not os.path.islink(path))
+    self.assertEqual(utils.ReadFile(self.rapi_users_path), "other user\n")
+    self.assertEqual(utils.ReadFile(self.rapi_users_path_pre24),
+                     "hello world\n")
+
+  def testRapiUsersDryRun(self):
+    self.assertFalse(os.path.exists(self.rapi_users_path))
+    self.assertFalse(os.path.exists(self.rapi_users_path_pre24))
+
+    utils.WriteFile(self.rapi_users_path_pre24, data="some user\n")
+    self._TestSimpleUpgrade(constants.BuildVersion(2, 3, 0), True)
+
+    self.assertFalse(os.path.isdir(os.path.dirname(self.rapi_users_path)))
+    self.assertTrue(os.path.isfile(self.rapi_users_path_pre24) and
+                    not os.path.islink(self.rapi_users_path_pre24))
+    self.assertFalse(os.path.exists(self.rapi_users_path))
+
+  def testRapiUsers24AndAboveDryRun(self):
+    self.assertFalse(os.path.exists(self.rapi_users_path))
+    self.assertFalse(os.path.exists(self.rapi_users_path_pre24))
+
+    os.mkdir(os.path.dirname(self.rapi_users_path))
+    utils.WriteFile(self.rapi_users_path, data="other user\n")
+    self._TestSimpleUpgrade(constants.BuildVersion(2, 3, 0), True)
+
+    self.assertTrue(os.path.isfile(self.rapi_users_path) and
+                    not os.path.islink(self.rapi_users_path))
+    self.assertFalse(os.path.exists(self.rapi_users_path_pre24))
+    self.assertEqual(utils.ReadFile(self.rapi_users_path), "other user\n")
+
+  def testRapiUsersExistingSymlinkDryRun(self):
+    self.assertFalse(os.path.exists(self.rapi_users_path))
+    self.assertFalse(os.path.exists(self.rapi_users_path_pre24))
+
+    os.mkdir(os.path.dirname(self.rapi_users_path))
+    os.symlink(self.rapi_users_path, self.rapi_users_path_pre24)
+    utils.WriteFile(self.rapi_users_path, data="hello world\n")
+
+    self._TestSimpleUpgrade(constants.BuildVersion(2, 2, 0), True)
+
+    self.assertTrue(os.path.islink(self.rapi_users_path_pre24))
+    self.assertTrue(os.path.isfile(self.rapi_users_path) and
+                    not os.path.islink(self.rapi_users_path))
+    self.assertEqual(os.readlink(self.rapi_users_path_pre24),
+                     self.rapi_users_path)
     for path in [self.rapi_users_path, self.rapi_users_path_pre24]:
       self.assertEqual(utils.ReadFile(path), "hello world\n")
 

diff --git a/tools/cfgupgrade b/tools/cfgupgrade
index 2222920..3e89349 100755 (executable)
--- a/tools/cfgupgrade
+++ b/tools/cfgupgrade
@@ -185,17 +185,26 @@ def main():
     raise Error("Configuration version %d.%d.%d not supported by this tool" %
                 (config_major, config_minor, config_revision))
 
-  if os.path.isfile(options.RAPI_USERS_FILE_PRE24):
+  if (os.path.isfile(options.RAPI_USERS_FILE_PRE24) and
+      not os.path.islink(options.RAPI_USERS_FILE_PRE24)):
+    if os.path.exists(options.RAPI_USERS_FILE):
+      raise Error("Found pre-2.4 RAPI users file at %s, but another file"
+                  " already exists at %s" %
+                  (options.RAPI_USERS_FILE_PRE24, options.RAPI_USERS_FILE))
     logging.info("Found pre-2.4 RAPI users file at %s, renaming to %s",
                  options.RAPI_USERS_FILE_PRE24, options.RAPI_USERS_FILE)
-    utils.RenameFile(options.RAPI_USERS_FILE_PRE24, options.RAPI_USERS_FILE,
-                     mkdir=True, mkdir_mode=0750)
+    if not options.dry_run:
+      utils.RenameFile(options.RAPI_USERS_FILE_PRE24, options.RAPI_USERS_FILE,
+                       mkdir=True, mkdir_mode=0750)
 
   # Create a symlink for RAPI users file
-  if not os.path.islink(options.RAPI_USERS_FILE_PRE24):
+  if (not (os.path.islink(options.RAPI_USERS_FILE_PRE24) or
+           os.path.isfile(options.RAPI_USERS_FILE_PRE24)) and
+      os.path.isfile(options.RAPI_USERS_FILE)):
     logging.info("Creating symlink from %s to %s",
                  options.RAPI_USERS_FILE_PRE24, options.RAPI_USERS_FILE)
-    os.symlink(options.RAPI_USERS_FILE, options.RAPI_USERS_FILE_PRE24)
+    if not options.dry_run:
+      os.symlink(options.RAPI_USERS_FILE, options.RAPI_USERS_FILE_PRE24)
 
   try:
     logging.info("Writing configuration file to %s", options.CONFIG_DATA_PATH)