Hi,

the attached patch fixes <https://fedorahosted.org/freeipa/ticket/4797>.

Note that --data with data-only backup and --logs-only with data-only restore are deliberately ignored and considered no-op.

Honza

--
Jan Cholasta
>From 6b14a609d726f5b6dc8e94b1d3d21123637599c1 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Mon, 12 Jan 2015 12:44:21 +0000
Subject: [PATCH] Fix validation of ipa-restore options

Fix restore mode checks. Do some of the existing checks earlier to make them
effective. Check if --instance and --backend exist both in the filesystem and
in the backup.

Log backup type and restore mode before performing restore.

Update ipa-restore man page.

https://fedorahosted.org/freeipa/ticket/4797
---
 install/tools/man/ipa-restore.1  |   6 +-
 ipaserver/install/ipa_restore.py | 159 +++++++++++++++++++++++----------------
 2 files changed, 97 insertions(+), 68 deletions(-)

diff --git a/install/tools/man/ipa-restore.1 b/install/tools/man/ipa-restore.1
index d758490..e94d92f 100644
--- a/install/tools/man/ipa-restore.1
+++ b/install/tools/man/ipa-restore.1
@@ -65,16 +65,16 @@ Restore the data only. The default is to restore everything in the backup.
 The full path to a GPG keyring. The keyring consists of two files, a public and a private key (.sec and .pub respectively). Specify the path without an extension.
 .TP
 \fB\-\-no\-logs\fR
-Exclude the IPA service log files in the backup (if they were backed up). Applicable only with a full backup.
+Exclude the IPA service log files in the backup (if they were backed up).
 .TP
 \fB\-\-online\fR
 Perform the restore on\-line. Requires the \-\-data option.
 .TP
 \fB\-\-instance\fR=\fIINSTANCE\fR
-Restore only the databases in this 389\-ds instance. The default is to restore all found (at most this is the IPA REALM instance and the PKI\-IPA instance).
+Restore only the databases in this 389\-ds instance. The default is to restore all found (at most this is the IPA REALM instance and the PKI\-IPA instance). Requires the \-\-data option.
 .TP
 \fB\-\-backend\fR=\fIBACKEND\fR
-The backend to restore within an instance or instances.
+The backend to restore within an instance or instances. Requires the \-\-data option.
 .TP
 \fB\-\-v\fR, \fB\-\-verbose\fR
 Print debugging information
diff --git a/ipaserver/install/ipa_restore.py b/ipaserver/install/ipa_restore.py
index 0977039..ce7aa62 100644
--- a/ipaserver/install/ipa_restore.py
+++ b/ipaserver/install/ipa_restore.py
@@ -134,32 +134,21 @@ class Restore(admintool.AdminTool):
 
 
     def validate_options(self):
+        parser = self.option_parser
         options = self.options
         super(Restore, self).validate_options(needs_root=True)
-        if options.data_only:
-            installutils.check_server_configuration()
 
         if len(self.args) < 1:
-            self.option_parser.error(
-                "must provide the backup to restore")
+            parser.error("must provide the backup to restore")
         elif len(self.args) > 1:
-            self.option_parser.error(
-                "must provide exactly one name for the backup")
-
-        dirname = self.args[0]
-        if not os.path.isabs(dirname):
-            self.backup_dir = os.path.join(BACKUP_DIR, dirname)
-        else:
-            self.backup_dir = dirname
-
-        if not os.path.isdir(dirname):
-            raise self.option_parser.error("must provide path to backup directory")
+            parser.error("must provide exactly one name for the backup")
+        if not os.path.isdir(self.args[0]):
+            parser.error("must provide path to backup directory")
 
         if options.gpg_keyring:
             if (not os.path.exists(options.gpg_keyring + '.pub') or
-               not os.path.exists(options.gpg_keyring + '.sec')):
-                raise admintool.ScriptError('No such key %s' %
-                    options.gpg_keyring)
+                not os.path.exists(options.gpg_keyring + '.sec')):
+                parser.error("no such key %s" % options.gpg_keyring)
 
 
     def ask_for_options(self):
@@ -185,38 +174,14 @@ class Restore(admintool.AdminTool):
         api.bootstrap(in_server=False, context='restore')
         api.finalize()
 
-        self.log.info("Preparing restore from %s on %s",
-            self.backup_dir, api.env.host)
-
-        if options.instance and options.backend:
-            database = (options.instance, options.backend)
-            if not os.path.exists(paths.SLAPD_INSTANCE_DB_DIR_TEMPLATE %
-                                  database):
-                raise admintool.ScriptError(
-                    "Instance %s with backend %s does not exist" % database)
-
-            databases = [database]
+        dirname = self.args[0]
+        if not os.path.isabs(dirname):
+            self.backup_dir = os.path.join(BACKUP_DIR, dirname)
         else:
-            if options.instance:
-                instances = [options.instance]
-            else:
-                instances = [realm_to_serverid(api.env.realm), 'PKI-IPA']
-
-            if options.backend:
-                backends = [options.backend]
-            else:
-                backends = ['userRoot', 'ipaca']
-
-            databases = []
-            for instance in instances:
-                for backend in backends:
-                    database = (instance, backend)
-                    if os.path.exists(paths.SLAPD_INSTANCE_DB_DIR_TEMPLATE %
-                                      database):
-                        databases.append(database)
+            self.backup_dir = dirname
 
-            if options.data_only and not databases:
-                raise admintool.ScriptError('No instances to restore to')
+        self.log.info("Preparing restore from %s on %s",
+            self.backup_dir, api.env.host)
 
         create_ds_user()
         pent = pwd.getpwnam(DS_USER)
@@ -240,20 +205,69 @@ class Restore(admintool.AdminTool):
                 self.read_header()
             except IOError as e:
                 raise admintool.ScriptError('Cannot read backup metadata: %s' % e)
-            # These two checks would normally be in the validate method but
+
+            if options.data_only:
+                restore_type = 'DATA'
+            else:
+                restore_type = self.backup_type
+
+            instances = [realm_to_serverid(api.env.realm), 'PKI-IPA']
+            backends = ['userRoot', 'ipaca']
+
+            # These checks would normally be in the validate method but
             # we need to know the type of backup we're dealing with.
-            if (self.backup_type != 'FULL' and not options.data_only and
-                not databases):
-                raise admintool.ScriptError('Cannot restore a data backup into an empty system')
-            if (self.backup_type == 'FULL' and not options.data_only and
-                (options.instance or options.backend)):
-                raise admintool.ScriptError('Restore must be in data-only mode when restoring a specific instance or backend.')
+            if restore_type == 'FULL':
+                if options.online:
+                    raise admintool.ScriptError(
+                        "File restoration cannot be done online")
+                if options.instance or options.backend:
+                    raise admintool.ScriptError(
+                        "Restore must be in data-only mode when restoring a "
+                        "specific instance or backend")
+            else:
+                installutils.check_server_configuration()
+
+                if options.instance:
+                    instance_dir = (paths.VAR_LIB_SLAPD_INSTANCE_DIR_TEMPLATE %
+                                    options.instance)
+                    if not os.path.exists(instance_dir):
+                        raise admintool.ScriptError(
+                            "Instance %s does not exist" % options.instance)
+
+                    instances = [options.instance]
+
+                if options.backend:
+                    for instance in instances:
+                        db_dir = (paths.SLAPD_INSTANCE_DB_DIR_TEMPLATE %
+                                  (instance, options.backend))
+                        if os.path.exists(db_dir):
+                            break
+                    else:
+                        raise admintool.ScriptError(
+                            "Backend %s does not exist" % options.backend)
+
+                    backends = [options.backend]
+
+                for instance in instances:
+                    for backend in backends:
+                        db_dir = (paths.SLAPD_INSTANCE_DB_DIR_TEMPLATE %
+                                  (instance, backend))
+                        if os.path.exists(db_dir):
+                            break
+                else:
+                    raise admintool.ScriptError(
+                        "Cannot restore a data backup into an empty system")
+
+            self.log.info("Performing %s restore from %s backup" %
+                          (restore_type, self.backup_type))
+
             if self.backup_host != api.env.host:
                 self.log.warning('Host name %s does not match backup name %s' %
                     (api.env.host, self.backup_host))
                 if (not options.unattended and
                     not user_input("Continue to restore?", False)):
                     raise admintool.ScriptError("Aborted")
+
             if self.backup_ipa_version != str(version.VERSION):
                 self.log.warning(
                     "Restoring data from a different release of IPA.\n"
@@ -266,12 +280,29 @@ class Restore(admintool.AdminTool):
 
             self.extract_backup(options.gpg_keyring)
 
-            for database in databases:
-                ldifname = '%s-%s.ldif' % database
-                ldiffile = os.path.join(self.dir, ldifname)
-                if not os.path.exists(ldiffile):
+            databases = []
+            for instance in instances:
+                for backend in backends:
+                    database = (instance, backend)
+                    ldiffile = os.path.join(self.dir, '%s-%s.ldif' % database)
+                    if os.path.exists(ldiffile):
+                        databases.append(database)
+
+            if options.instance:
+                for instance, backend in databases:
+                    if instance == options.instance:
+                        break
+                else:
                     raise admintool.ScriptError(
-                        "Instance %s with backend %s not in backup" % database)
+                        "Instance %s not found in backup" % options.instance)
+
+            if options.backend:
+                for instance, backend in databases:
+                    if backend == options.backend:
+                        break
+                else:
+                    raise admintool.ScriptError(
+                        "Backend %s not found in backup" % options.backend)
 
             # Big fat warning
             if  (not options.unattended and
@@ -290,7 +321,7 @@ class Restore(admintool.AdminTool):
             self.log.info("Disabling all replication.")
             self.disable_agreements()
 
-            if options.data_only:
+            if restore_type != 'FULL':
                 if not options.online:
                     self.log.info('Stopping Directory Server')
                     dirsrv.stop(capture_output=False)
@@ -307,11 +338,9 @@ class Restore(admintool.AdminTool):
 
 
             # We do either a full file restore or we restore data.
-            if self.backup_type == 'FULL' and not options.data_only:
+            if restore_type == 'FULL':
                 if 'CA' in self.backup_services:
                     create_ca_user()
-                if options.online:
-                    raise admintool.ScriptError('File restoration cannot be done online.')
                 self.cert_restore_prepare()
                 self.file_restore(options.no_logs)
                 self.cert_restore()
@@ -326,7 +355,7 @@ class Restore(admintool.AdminTool):
             for instance, backend in databases:
                 self.ldif2db(instance, backend, online=options.online)
 
-            if options.data_only:
+            if restore_type != 'FULL':
                 if not options.online:
                     self.log.info('Starting Directory Server')
                     dirsrv.start(capture_output=False)
-- 
2.1.0

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to