[Freeipa-devel] [PATCH 0006] Improves sssd.conf handling during ipa-client uninstall
Martin Kosek
mkosek at redhat.com
Thu Sep 20 16:34:18 UTC 2012
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
More information about the Freeipa-devel
mailing list