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.") -- 1.7.11.4 Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel