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