[Freeipa-devel] [PATCH 0006] Improves sssd.conf handling during ipa-client uninstall
Tomas Babej
tbabej at redhat.com
Tue Sep 18 09:21:11 UTC 2012
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0006-3-Improves-sssd.conf-handling-during-ipa-client-uninst.patch
Type: text/x-patch
Size: 9019 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120918/49fef58d/attachment.bin>
More information about the Freeipa-devel
mailing list