[Freeipa-devel] [PATCH 0006] Improves sssd.conf handling during ipa-client uninstall
Tomas Babej
tbabej at redhat.com
Thu Sep 20 14:20:22 UTC 2012
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0006-4-Improves-sssd.conf-handling-during-ipa-client-uninst.patch
Type: text/x-patch
Size: 9110 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120920/9abdda73/attachment.bin>
More information about the Freeipa-devel
mailing list