On Thu, 2012-09-20 at 16:20 +0200, Tomas Babej wrote: > On 09/20/2012 02:42 PM, Martin Kosek wrote: > > On 09/18/2012 11:21 AM, Tomas Babej wrote: > >> On 09/12/2012 05:29 PM, Martin Kosek wrote: > >>> On 08/29/2012 02:54 PM, Tomas Babej wrote: > >>>> On 08/27/2012 04:55 PM, Martin Kosek wrote: > >>>>> On 08/27/2012 03:37 PM, Jakub Hrozek wrote: > >>>>>> On Mon, Aug 27, 2012 at 02:57:44PM +0200, Martin Kosek wrote: > >>>>>>> I think that the right behavior of SSSD conf uninstall should be the > >>>>>>> following: > >>>>>>> > >>>>>>> * sssd.conf existed before IPA install + non-IPA domains in sssd.conf > >>>>>>> found: > >>>>>>> - move backed conf up sssd.conf.bkp (and inform the user) > >>>>>>> - use SSSDConfig delete_domain function to remove ipa domain from > >>>>>>> sssd.conf > >>>>>>> - restart sssd afterwards > >>>>>> I'm confused here, which of the files is the original > >>>>>> pre-ipa-client-install file? > >>>>> This is the "backed up sssd.conf". I thought that it may be useful for > >>>>> user to > >>>>> still have an access to it after uninstall. > >>>>> > >>>>>> How does the non-ipa domain end up in the sssd.conf file? Does it have > >>>>>> to be configured manually or does ipa-client-install merge the list of > >>>>>> domains on installation? > >>>>> ipa-client-install merge the list of the domains. It overrides the old > >>>>> sssd.conf only when we cannot parse the sssd.conf and --preserve-sssd > >>>>> option > >>>>> was not set. > >>>>> > >>>>> Martin > >>>> Hi, > >>>> > >>>> The sssd.conf file is no longer left behind in case sssd was not > >>>> configured before the installation. However, the patch goes behind > >>>> the scope of this ticked and improves the handling of sssd.conf > >>>> during the ipa-client-install --uninstall in general. > >>>> > >>>> The current behaviour (well documented in source code) is as follows: > >>>> - In general, the IPA domain is simply removed from the sssd.conf > >>>> file, instead of sssd.conf being rewritten from the backup. This > >>>> preserves any domains added after installation. > >>>> > >>>> - If sssd.conf existed before the installation, it is restored to > >>>> sssd.conf.bkp. However, any IPA domains from pre-installation > >>>> sssd.conf should have been merged during the installation. > >>>> > >>>> - If sssd.conf did not exist before the installation, and no other > >>>> domains than IPA domain exist in it, the patch makes sure that > >>>> sssd.conf is moved to sssd.conf.deleted so user experiences no > >>>> crash during any next installation due to its existence. > >>>> > >>>> https://fedorahosted.org/freeipa/ticket/2740 > >>>> > >>>> Tomas > >>>> > >>> Good job, SSSD uninstall process now looks more consistent and better > >>> documented. I just found the following (mainly minor) issues. Comments in > >>> the > >>> patch: > >>> > >>> diff --git a/ipa-client/ipa-install/ipa-client-install > >>> b/ipa-client/ipa-install/ipa-client-install > >>> index > >>> 2e65921e8de2dfe68443f5b5875954d71dd48ed2..c5cef15e1fb3a3e1d7cfd070f4288d3839accfc8 > >>> > >>> 100755 > >>> --- a/ipa-client/ipa-install/ipa-client-install > >>> +++ b/ipa-client/ipa-install/ipa-client-install > >>> @@ -183,6 +183,36 @@ def nssldap_exists(): > >>> > >>> return (retval, files_found) > >>> > >>> +# helper function for uninstall > >>> +# deletes IPA domain from sssd.conf > >>> +def delete_IPA_domain(): > >>> > >>> Function names should be lowercase -> delete_ipa_domain > >>> > >>> + sssd = ipaservices.service('sssd') > >>> + try: > >>> + sssdconfig = SSSDConfig.SSSDConfig() > >>> + sssdconfig.import_config() > >>> + domains = sssdconfig.list_active_domains() > >>> + > >>> + IPA_domain_name = None > >>> > >>> Variables should be lowercase -> ipa_domain_name > >>> > >>> + > >>> + for name in domains: > >>> + domain = sssdconfig.get_domain(name) > >>> + try: > >>> + provider = domain.get_option('id_provider') > >>> + if provider == "ipa": > >>> + IPA_domain_name = name > >>> + break > >>> + except SSSDConfig.NoOptionError: > >>> + continue > >>> + > >>> + if IPA_domain_name != None: > >>> > >>> Do not use != with None, True, False - use "is not None". > >>> > >>> + sssdconfig.delete_domain(IPA_domain_name) > >>> + sssdconfig.write() > >>> + else: > >>> + root_logger.warning("IPA domain could not be found in " + > >>> + "sssd.conf and therefore not deleted") > >>> + except IOError: > >>> + root_logger.warning("IPA domain could not be deleted. No access > >>> to the > >>> sssd.conf file.") > >>> > >>> There should be full path to sssd.conf in this error message. It is very > >>> useful > >>> sometimes. > >>> > >>> + > >>> def uninstall(options, env): > >>> > >>> if not fstore.has_files(): > >>> @@ -212,7 +242,12 @@ def uninstall(options, env): > >>> sssdconfig = SSSDConfig.SSSDConfig() > >>> sssdconfig.import_config() > >>> domains = sssdconfig.list_active_domains() > >>> - if len(domains) > 1: > >>> + all_domains = sssdconfig.list_domains() > >>> + > >>> + # we consider all the domains, because handling sssd.conf > >>> + # during uninstall is dependant on was_sssd_configured flag > >>> + # so the user does not lose info about inactive domains > >>> + if len(all_domains) > 1: > >>> # There was more than IPA domain configured > >>> was_sssd_configured = True > >>> for name in domains: > >>> @@ -349,6 +384,62 @@ def uninstall(options, env): > >>> "Failed to remove krb5/LDAP configuration: %s", str(e)) > >>> return CLIENT_INSTALL_ERROR > >>> > >>> + # Next if-elif-elif construction deals with sssd.conf file. > >>> + # Old pre-IPA domains are preserved due merging the old sssd.conf > >>> + # during the installation of ipa-client but any new domains are > >>> + # only present in sssd.conf now, so we don't want to delete them > >>> + # by rewriting sssd.conf file. IPA domain is removed gracefully. > >>> + > >>> + # SSSD was installed before our installation and other non-IPA > >>> domains > >>> + # found, restore backed up sssd.conf to sssd.conf.bkp and remove IPA > >>> + # domain from the current sssd.conf > >>> + if was_sssd_installed and was_sssd_configured: > >>> + root_logger.info( > >>> + "The original configuration of SSSD included other domains > >>> than " + > >>> + "the IPA-based one.") > >>> + > >>> + delete_IPA_domain() > >>> + > >>> > >>> + try: > >>> + > >>> os.rename("/etc/sssd/sssd.conf","/etc/sssd/sssd.conf.current") > >>> + if fstore.restore_file("/etc/sssd/sssd.conf"): > >>> + > >>> os.rename("/etc/sssd/sssd.conf","/etc/sssd/sssd.conf.bkp") > >>> + > >>> os.rename("/etc/sssd/sssd.conf.current","/etc/sssd/sssd.conf") > >>> > >>> I don't like this very much. I would rather not mess with > >>> /etc/sssd/sssd.conf, > >>> it may surprise other tools/user and we could also end with no > >>> /etc/sssd/sssd.conf at all if OSError is raised in second or third step. > >>> > >>> I would rather enhance fstore and implement function like > >>> "restore_file_to" > >>> which would accept a second parameter with a custom path, like > >>> "/etc/sssd/sssd.conf.bkp". > >>> > >>> + except OSError: > >>> + pass > >>> > >>> The exception should be at least logged to debug log. > >>> > >>> + > >>> + root_logger.info("Original pre-IPA SSSD configuration file was " > >>> + > >>> + "restored to /etc/sssd/sssd.conf.bkp.") > >>> + root_logger.info("IPA domain removed from current one, " + > >>> + "restarting SSSD service") > >>> + sssd = ipaservices.service('sssd') > >>> + try: > >>> + sssd.restart() > >>> + except CalledProcessError: > >>> + root_logger.warning("SSSD service restart was unsuccessful.") > >>> + > >>> + # SSSD was not installed before our installation, but other domains > >>> found, > >>> + # delete IPA domain, but leave other domains intact > >>> + elif not was_sssd_installed and was_sssd_configured: > >>> + delete_IPA_domain() > >>> + root_logger.info("Other domains than IPA domain found, " + > >>> + "IPA domain was removed from sssd.conf.") > >>> + try: > >>> + sssd.restart() > >>> + except CalledProcessError: > >>> + root_logger.warning("SSSD service restart was unsuccessful.") > >>> + > >>> + # SSSD was not installed before our installation, and no other > >>> domains > >>> + # than IPA are configured in sssd.conf - make sure config file is > >>> removed > >>> + elif not was_sssd_installed and not was_sssd_configured: > >>> + try: > >>> + > >>> os.rename("/etc/sssd/sssd.conf","/etc/sssd/sssd.conf.deleted") > >>> + except OSError: > >>> + pass > >>> > >>> > >>> Error should be logged to debug log. > >>> > >>> + > >>> + root_logger.info("Redundant SSSD configuration file " + > >>> + "/etc/sssd/sssd.conf was moved to /etc/sssd.conf.deleted") > >>> + > >>> if fstore.has_files(): > >>> root_logger.info("Restoring client configuration files") > >>> fstore.restore_all_files() > >>> @@ -428,20 +519,6 @@ def uninstall(options, env): > >>> if was_sshd_configured and > >>> ipaservices.knownservices.sshd.is_running(): > >>> ipaservices.knownservices.sshd.restart() > >>> > >>> - if was_sssd_installed and was_sssd_configured: > >>> - # SSSD was installed before our installation, config now is > >>> restored, > >>> restart it > >>> - root_logger.info( > >>> - "The original configuration of SSSD included other domains > >>> than " + > >>> - "the IPA-based one.") > >>> - root_logger.info( > >>> - "Original configuration file was restored, restarting SSSD " > >>> + > >>> - "service.") > >>> - sssd = ipaservices.service('sssd') > >>> - try: > >>> - sssd.restart() > >>> - except CalledProcessError: > >>> - root_logger.warning("SSSD service restart was unsuccessful.") > >>> - > >>> if not options.unattended: > >>> root_logger.info( > >>> "The original nsswitch.conf configuration has been > >>> restored.") > >> Final (hopefully) version attached. > >> > >> Tomas > > Well... almost the final version :-) > > > > 1) Wrong path to sssd.conf.deleted: > > > > #Unenrolling client from IPA server > > Removing Kerberos service principals from /etc/krb5.keytab > > Disabling client Kerberos and LDAP configurations > > Redundant SSSD configuration file /etc/sssd/sssd.conf was moved to > > /etc/sssd.conf.deleted <<<<<<<< > > Restoring client configuration files > > Client uninstall complete. > > ipa-client-install --uninstall --unattended > > > > > > 2) Uninstall crashes in the second case: > > # ipa-client-install --uninstall --unattended > > Unenrolling client from IPA server > > Removing Kerberos service principals from /etc/krb5.keytab > > Disabling client Kerberos and LDAP configurations > > Other domains than IPA domain found, IPA domain was removed from sssd.conf. > > Traceback (most recent call last): > > File "/sbin/ipa-client-install", line 1913, in <module> > > sys.exit(main()) > > File "/sbin/ipa-client-install", line 1891, in main > > return uninstall(options, env) > > File "/sbin/ipa-client-install", line 432, in uninstall > > sssd.restart() > > UnboundLocalError: local variable 'sssd' referenced before assignment > > > > > > Also, full path to sssd.conf in the info message would be fine. > > > > > > Otherwise, the patch seems to work fine. > > > > Martin > All issues corrected. > > Tomas
Pushed to master, ipa-3-0. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel