On 11/10/2015 03:35 PM, Martin Babinsky wrote:
> On 10/27/2015 04:24 PM, Tomas Babej wrote:
>> Hi,
>>
>> this couple of patches harden the adtrust installer.
>>
>> Details in the commit messages.
>>
>> Fixes: https://fedorahosted.org/freeipa/ticket/5134
>>
>> Tomas
>>
>>
>>
> NACK,
> 
> in the first patch you forgot to instantiate the caught exception in the
> following snippet:
> 
> +        except Exception:
> +            root_logger.debug("Exception occured during SID generation:
> {0}"
> +                              .format(str(e)))
> 
> You should use 'except Exception as e:'.
> 
> I'm also not quite sure that it is enough to log the error at debug level.
> 
> If the sidgen task somehow fails, isn't it something which should
> interest the user and deserve at least warning-level message?
> 

Thanks for catching this. Inappropriate message level indeed, I probably
wasn't using my brain much when writing that snippet :)

Updated patchset attached.

Tomas
From 38bdd4be27eff57effeb5ff47585378aa92db409 Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Tue, 27 Oct 2015 16:05:03 +0100
Subject: [PATCH] adtrustinstance: Wait for sidgen task completion

As part of hardening of adtrust installer, we should wait until
the sidgen task is completed before continuing, as it can take
considerable amount of time for a larger deployment.

https://fedorahosted.org/freeipa/ticket/5134
---
 ipaserver/install/adtrustinstance.py | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/ipaserver/install/adtrustinstance.py b/ipaserver/install/adtrustinstance.py
index f7a7899906ca47b4901881fb6f4099ace1780741..721cc23e7128751ca71eb68951a200d362ab7a74 100644
--- a/ipaserver/install/adtrustinstance.py
+++ b/ipaserver/install/adtrustinstance.py
@@ -34,6 +34,7 @@ from ipaserver.install import service
 from ipaserver.install import installutils
 from ipaserver.install.bindinstance import get_rr, add_rr, del_rr, \
                                            dns_zone_exists
+from ipaserver.install.replication import wait_for_task
 from ipalib import errors, api
 from ipalib.util import normalize_zone
 from ipapython.dn import DN
@@ -469,13 +470,24 @@ class ADTRUSTInstance(service.Service):
 
     def __add_sids(self):
         """
-        Add SIDs for existing users and groups
+        Add SIDs for existing users and groups. Make sure the task is finished
+        before continuing.
         """
 
         try:
+            # Start the sidgen task
             self._ldap_mod("ipa-sidgen-task-run.ldif", self.sub_dict)
-        except:
-            pass
+
+            # Notify the user about the possible delay
+            self.print_msg("This step may take considerable amount of time, please wait..")
+
+            # Wait for the task to complete
+            task_dn = DN('cn=sidgen,cn=ipa-sidgen-task,cn=tasks,cn=config')
+            wait_for_task(self.admin_conn, task_dn)
+
+        except Exception as e:
+            root_logger.warning("Exception occured during SID generation: {0}"
+                                .format(str(e)))
 
     def __add_s4u2proxy_target(self):
         """
-- 
2.4.3

From efd99d99020e56dabde25f7b6af19e470fbb01e1 Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Tue, 27 Oct 2015 16:05:35 +0100
Subject: [PATCH] adtrustinstance: Restart samba service at the end of
 adtrust-install

Errors related to establishing trust can occur if samba service is not
restarted after ipa-adtrust-install has been run. Restart the service at
the end of the installer to avoid such issues.

https://fedorahosted.org/freeipa/ticket/5134
---
 ipaserver/install/adtrustinstance.py | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/ipaserver/install/adtrustinstance.py b/ipaserver/install/adtrustinstance.py
index 721cc23e7128751ca71eb68951a200d362ab7a74..ab8783b4939d580c6d6b14d385ed4e847ed960e3 100644
--- a/ipaserver/install/adtrustinstance.py
+++ b/ipaserver/install/adtrustinstance.py
@@ -749,6 +749,12 @@ class ADTRUSTInstance(service.Service):
         except:
             pass
 
+    def __restart_smb(self):
+        try:
+            services.knownservices.smb.restart()
+        except Exception:
+            pass
+
     def __enable(self):
         self.backup_state("enabled", self.is_enabled())
         # We do not let the system start IPA components on its own,
@@ -880,6 +886,7 @@ class ADTRUSTInstance(service.Service):
         if self.add_sids:
             self.step("adding SIDs to existing users and groups",
                       self.__add_sids)
+        self.step("restarting smbd", self.__restart_smb)
 
         self.start_creation(show_service_name=False)
 
-- 
2.4.3

From 66ed28240c2b7a808bd752e810770a8d68dfe842 Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Tue, 27 Oct 2015 16:08:10 +0100
Subject: [PATCH] adtrustinstance: Do not use bare except clauses

https://fedorahosted.org/freeipa/ticket/5134
---
 ipaserver/install/adtrustinstance.py | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/ipaserver/install/adtrustinstance.py b/ipaserver/install/adtrustinstance.py
index ab8783b4939d580c6d6b14d385ed4e847ed960e3..b8f1b770a574fdf80b5b40439d6cd9d83b094b68 100644
--- a/ipaserver/install/adtrustinstance.py
+++ b/ipaserver/install/adtrustinstance.py
@@ -215,13 +215,13 @@ class ADTRUSTInstance(service.Service):
 
         try:
             admin_entry = self.admin_conn.get_entry(admin_dn)
-        except:
+        except errors.NotFound:
             self.print_msg("IPA admin object not found")
             return
 
         try:
             admin_group_entry = self.admin_conn.get_entry(admin_group_dn)
-        except:
+        except errors.NotFound:
             self.print_msg("IPA admin group object not found")
             return
 
@@ -232,7 +232,7 @@ class ADTRUSTInstance(service.Service):
                 self.admin_conn.modify_s(admin_dn, \
                             [(ldap.MOD_ADD, "objectclass", self.OBJC_USER), \
                              (ldap.MOD_ADD, self.ATTR_SID, dom_sid + "-500")])
-            except:
+            except Exception:
                 self.print_msg("Failed to modify IPA admin object")
 
         if admin_group_entry.single_value.get(self.ATTR_SID):
@@ -242,7 +242,7 @@ class ADTRUSTInstance(service.Service):
                 self.admin_conn.modify_s(admin_group_dn, \
                             [(ldap.MOD_ADD, "objectclass", self.OBJC_GROUP), \
                              (ldap.MOD_ADD, self.ATTR_SID, dom_sid + "-512")])
-            except:
+            except Exception:
                 self.print_msg("Failed to modify IPA admin group object")
 
     def __add_default_trust_view(self):
@@ -313,7 +313,7 @@ class ADTRUSTInstance(service.Service):
         try:
             mod = [(ldap.MOD_ADD, self.ATTR_FALLBACK_GROUP, fb_group_dn)]
             self.admin_conn.modify_s(self.smb_dom_dn, mod)
-        except:
+        except Exception:
             self.print_msg("Failed to add fallback group to domain object")
 
     def __add_rid_bases(self):
@@ -732,7 +732,7 @@ class ADTRUSTInstance(service.Service):
         try:
             self.start()
             services.service('winbind').start()
-        except:
+        except Exception:
             root_logger.critical("CIFS services failed to start")
 
     def __stop(self):
@@ -740,13 +740,13 @@ class ADTRUSTInstance(service.Service):
         try:
             services.service('winbind').stop()
             self.stop()
-        except:
+        except Exception:
             pass
 
     def __restart_dirsrv(self):
         try:
             services.knownservices.dirsrv.restart()
-        except:
+        except Exception:
             pass
 
     def __restart_smb(self):
-- 
2.4.3

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to