On 08/30/2013 04:37 PM, Petr Viktorin wrote:
> On 08/29/2013 02:16 PM, Ana Krivokapic wrote:
>> Hello,
>>
>> This patch addresses tickethttps://fedorahosted.org/freeipa/ticket/3856.
>>
>
> Thanks for the patch!
> I haven't tested it yet, but I've read it and I have some comments below.
>
>
>> diff --git a/install/share/copy-schema-to-ca.py
>> b/install/share/copy-schema-to-ca.py
>> index
>> 1888f12513aa3edf22149e9330afea99f62bf41d..fe99a9256f1298bae1c746ea0c4d41339a4fbebb
>> 100755
>> --- a/install/share/copy-schema-to-ca.py
>> +++ b/install/share/copy-schema-to-ca.py
>> @@ -15,10 +15,11 @@
>>   import pwd
>>   import shutil
>>
>> -from ipapython import services, ipautil, dogtag
>> +from ipapython import services, ipautil
>>   from ipapython.ipa_log_manager import root_logger, standard_logging_setup
>> -from ipaserver.install.dsinstance import DS_USER, schema_dirname
>> +from ipaserver.install.dsinstance import schema_dirname
>>   from ipaserver.install.cainstance import PKI_USER
>> +from ipaserver.install.installutils import DS_USER
>>   from ipalib import api
>
> copy-schema-to-ca.py is intended to be copied to older systems, to prepare the
> CA DSs of old IPA masters for replication with new masters with unified DSs.
> This means that we can't remove anything we import here; the constants need to
> be available at the old locations.

Thanks for clearing that up, I didn't know about this requirement.

>
> That's the hard requirement, but anyway I think the constants, and the
> create_ds_user & create_ds_group functions, are specific to the DS and so
> belong in dsinstance.
> Did I overlook a reason for moving them to installutils?

You are right. I refactored these functions so that they can be more easily used
from other modules, but there is no special reason to put them in installutils.
I moved them back to dsinstance.

>
> [...]
>> diff --git a/ipaserver/install/installutils.py
>> b/ipaserver/install/installutils.py
>> index
>> 268279dc9d22b9f983406303cbfc80c00a2b8fa0..84846221d2800443ba6e291ec9c28b37a482d735
>> 100644
> [...]
>> +def create_ds_user():
>> +    """
>> +    Create DS user if it doesn't exist yet
>> +    """
>> +    try:
>> +        pwd.getpwnam(DS_USER)
>> +        root_logger.debug("DS user %s exists" % DS_USER)
>
> Use comma for debug() arguments, i.e. debug("DS group %s exists", DS_GROUP).
> Same in other places.

Fixed.

>
>> +    except KeyError:
>> +        root_logger.debug("Adding DS user %s" % DS_USER)
>> +        args = ["/usr/sbin/useradd", "-g", DS_GROUP,
>> +                                     "-c", "DS System User",
>> +                                     "-d", "/var/lib/dirsrv",
>> +                                     "-s", "/sbin/nologin",
>> +                                     "-M", "-r", DS_USER]
>> +        try:
>> +            ipautil.run(args)
>> +            root_logger.debug("Done adding DS user")
>> +        except ipautil.CalledProcessError, e:
>> +            root_logger.critical("Failed to add DS user %s" % e)
>> +
>> +
>> +def create_ds_group():
>> +    """
>> +    Create DS group if it doesn't exist yet
>> +    """
>
> Please document the return value in the docstring, it's not obvious that this
> returns True iff it didn't do anything.
>

Done.

Updated patch attached.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

From 217bc68bbee6ef658e6248fa069252184ca1f171 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic <akriv...@redhat.com>
Date: Mon, 2 Sep 2013 10:56:19 +0200
Subject: [PATCH] Create DS user and group during ipa-restore

ipa-restore would fail if DS user did not exist. Check for presence of DS
user and group and create them if needed.

https://fedorahosted.org/freeipa/ticket/3856
---
 install/tools/ipa-replica-install | 22 +++----------
 install/tools/ipa-server-install  | 11 +------
 ipaserver/install/dsinstance.py   | 66 ++++++++++++++++++++++++++++-----------
 ipaserver/install/ipa_restore.py  | 12 +++----
 4 files changed, 59 insertions(+), 52 deletions(-)

diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install
index 947c51f6f287ffce52994408352601388faf56a6..2a88c102175f7c9bf7c173f9f9f3437ccc963f29 100755
--- a/install/tools/ipa-replica-install
+++ b/install/tools/ipa-replica-install
@@ -22,7 +22,6 @@ import sys
 import socket
 
 import os, pwd, shutil
-import grp
 from optparse import OptionGroup
 from contextlib import contextmanager
 
@@ -33,13 +32,13 @@ import dns.exception
 from ipapython import ipautil
 
 from ipaserver.install import dsinstance, installutils, krbinstance, service
-from ipaserver.install import bindinstance, httpinstance, ntpinstance, certs
+from ipaserver.install import bindinstance, httpinstance, ntpinstance
 from ipaserver.install import memcacheinstance
 from ipaserver.install import otpdinstance
 from ipaserver.install.replication import replica_conn_check, ReplicationManager
-from ipaserver.install.installutils import (HostnameLocalhost, resolve_host,
-        ReplicaConfig, expand_replica_info, read_replica_info ,get_host_name,
-        BadHostError, private_ccache)
+from ipaserver.install.installutils import (ReplicaConfig, expand_replica_info,
+                                            read_replica_info ,get_host_name,
+                                            BadHostError, private_ccache)
 from ipaserver.plugins.ldap2 import ldap2
 from ipaserver.install import cainstance
 from ipalib import api, errors, util
@@ -574,18 +573,7 @@ def main():
     api.finalize()
 
     # Create DS group if it doesn't exist yet
-    try:
-        grp.getgrnam(dsinstance.DS_GROUP)
-        root_logger.debug("ds group %s exists" % dsinstance.DS_GROUP)
-        group_exists = True
-    except KeyError:
-        group_exists = False
-        args = ["/usr/sbin/groupadd", "-r", dsinstance.DS_GROUP]
-        try:
-            ipautil.run(args)
-            root_logger.debug("done adding DS group")
-        except ipautil.CalledProcessError, e:
-            root_logger.critical("failed to add DS group: %s" % e)
+    group_exists = dsinstance.create_ds_group()
     sstore.backup_state("install", "group_exists", group_exists)
 
     #Automatically disable pkinit w/ dogtag until that is supported
diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install
index 86ca3447bfaab1763324ce57c67c073a8fa93963..bfdef82ab7be1dd76efcab2514cda73445b483a9 100755
--- a/install/tools/ipa-server-install
+++ b/install/tools/ipa-server-install
@@ -971,16 +971,7 @@ def main():
         ipaservices.backup_and_replace_hostname(fstore, sstore, host_name)
 
     # Create DS group if it doesn't exist yet
-    try:
-        grp.getgrnam(dsinstance.DS_GROUP)
-        root_logger.debug("ds group %s exists" % dsinstance.DS_GROUP)
-    except KeyError:
-        args = ["/usr/sbin/groupadd", "-r", dsinstance.DS_GROUP]
-        try:
-            ipautil.run(args)
-            root_logger.debug("done adding DS group")
-        except ipautil.CalledProcessError, e:
-            root_logger.critical("failed to add DS group: %s" % e)
+    dsinstance.create_ds_group()
 
     # Create a directory server instance
     if external != 2:
diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index f543efadc6568a022fbb0a2ee07833612f9466f7..ab3e9375bf495150fa098eb9d9472838d48c7506 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -27,6 +27,7 @@
 import tempfile
 import base64
 import stat
+import grp
 
 from ipapython.ipa_log_manager import *
 from ipapython import ipautil, sysrestore, ipaldap
@@ -130,6 +131,52 @@ def check_ports():
 def is_ds_running(server_id=''):
     return ipaservices.knownservices.dirsrv.is_running(instance_name=server_id)
 
+
+def create_ds_user():
+    """
+    Create DS user if it doesn't exist yet.
+    """
+    try:
+        pwd.getpwnam(DS_USER)
+        root_logger.debug('DS user %s exists', DS_USER)
+    except KeyError:
+        root_logger.debug('Adding DS user %s', DS_USER)
+        args = [
+            '/usr/sbin/useradd',
+            '-g', DS_GROUP,
+            '-c', 'DS System User',
+            '-d', '/var/lib/dirsrv',
+            '-s', '/sbin/nologin',
+            '-M', '-r', DS_USER
+        ]
+        try:
+            ipautil.run(args)
+            root_logger.debug('Done adding DS user')
+        except ipautil.CalledProcessError, e:
+            root_logger.critical('Failed to add DS user: %s', e)
+
+
+def create_ds_group():
+    """
+    Create DS group if it doesn't exist yet.
+    Returns True if DS already exists.
+    """
+    try:
+        grp.getgrnam(DS_GROUP)
+        root_logger.debug('DS group %s exists', DS_GROUP)
+        group_exists = True
+    except KeyError:
+        group_exists = False
+        root_logger.debug('Adding DS group %s', DS_GROUP)
+        args = ['/usr/sbin/groupadd', '-r', DS_GROUP]
+        try:
+            ipautil.run(args)
+            root_logger.debug('Done adding DS group')
+        except ipautil.CalledProcessError, e:
+            root_logger.critical('Failed to add DS group: %s', e)
+
+    return group_exists
+
 INF_TEMPLATE = """
 [General]
 FullMachineName=   $FQDN
@@ -194,7 +241,7 @@ def __init__(self, realm_name=None, domain_name=None, dm_password=None,
 
     def __common_setup(self, enable_ssl=False):
 
-        self.step("creating directory server user", self.__create_ds_user)
+        self.step("creating directory server user", create_ds_user)
         self.step("creating directory server instance", self.__create_instance)
         self.step("adding default schema", self.__add_default_schemas)
         self.step("enabling memberof plugin", self.__add_memberof_module)
@@ -346,23 +393,6 @@ def __setup_sub_dict(self):
                              IDRANGE_SIZE=idrange_size
                          )
 
-    def __create_ds_user(self):
-        try:
-            pwd.getpwnam(DS_USER)
-            root_logger.debug("ds user %s exists" % DS_USER)
-        except KeyError:
-            root_logger.debug("adding ds user %s" % DS_USER)
-            args = ["/usr/sbin/useradd", "-g", DS_GROUP,
-                                         "-c", "DS System User",
-                                         "-d", "/var/lib/dirsrv",
-                                         "-s", "/sbin/nologin",
-                                         "-M", "-r", DS_USER]
-            try:
-                ipautil.run(args)
-                root_logger.debug("done adding user")
-            except ipautil.CalledProcessError, e:
-                root_logger.critical("failed to add user %s" % e)
-
     def __create_instance(self):
         pent = pwd.getpwnam(DS_USER)
 
diff --git a/ipaserver/install/ipa_restore.py b/ipaserver/install/ipa_restore.py
index 2d4be57f7c9643edcee58f35b00baebbb18257f8..82113716068220ed3a1dc733a2cea8f0eefd735a 100644
--- a/ipaserver/install/ipa_restore.py
+++ b/ipaserver/install/ipa_restore.py
@@ -20,28 +20,24 @@
 import os
 import sys
 import shutil
-import glob
 import tempfile
 import time
 import pwd
-from optparse import OptionGroup
 from ConfigParser import SafeConfigParser
 
 from ipalib import api, errors
 from ipapython import version
 from ipapython.ipautil import run, user_input
 from ipapython import admintool
-from ipapython.config import IPAOptionParser
 from ipapython.dn import DN
-from ipaserver.install.dsinstance import realm_to_serverid, DS_USER
+from ipaserver.install.dsinstance import (realm_to_serverid, create_ds_group,
+                                          create_ds_user, DS_USER)
 from ipaserver.install.cainstance import PKI_USER
 from ipaserver.install.replication import (wait_for_task, ReplicationManager,
-    CSReplicationManager, get_cs_replication_manager)
+                                           get_cs_replication_manager)
 from ipaserver.install import installutils
 from ipapython import services as ipaservices
 from ipapython import ipaldap
-from ipapython import version
-from ipalib.session import ISO8601_DATETIME_FMT
 from ipaserver.install.ipa_backup import BACKUP_DIR
 
 
@@ -190,6 +186,8 @@ def run(self):
         if options.data_only and not instances:
             raise admintool.ScriptError('No instances to restore to')
 
+        create_ds_group()
+        create_ds_user()
         pent = pwd.getpwnam(DS_USER)
 
         # Temporary directory for decrypting files before restoring
-- 
1.8.3.1

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

Reply via email to