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
>From ff77689e9581eab5b8a2529fc1e2c66867152bad Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Fri, 17 Aug 2012 08:56:45 -0400
Subject: [PATCH] Improves sssd.conf handling during ipa-client uninstall
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
---
ipa-client/ipa-install/ipa-client-install | 110 ++++++++++++++++++++++++++----
ipapython/sysrestore.py | 15 +++-
2 files changed, 108 insertions(+), 17 deletions(-)
diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index a9408eed7cca44700e6b444987a0d93d51b2251e..368cbe5f5d33158c11d84ceea850a62ba88f1304 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -185,6 +185,37 @@ def nssldap_exists():
return (retval, files_found)
+# helper function for uninstall
+# deletes IPA domain from sssd.conf
+def delete_ipa_domain():
+ sssd = ipaservices.service('sssd')
+ try:
+ sssdconfig = SSSDConfig.SSSDConfig()
+ sssdconfig.import_config()
+ domains = sssdconfig.list_active_domains()
+
+ ipa_domain_name = None
+
+ 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 is not None:
+ sssdconfig.delete_domain(ipa_domain_name)
+ sssdconfig.write()
+ else:
+ root_logger.warning("IPA domain could not be found in "
+ "/etc/sssd/sssd.conf and therefore not deleted")
+ except IOError:
+ root_logger.warning("IPA domain could not be deleted. "
+ "No access to the /etc/sssd/sssd.conf file.")
+
def uninstall(options, env):
if not fstore.has_files():
@@ -214,7 +245,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:
@@ -351,6 +387,64 @@ 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()
+
+
+ restored = False
+ try:
+ restored = fstore.restore_file("/etc/sssd/sssd.conf","/etc/sssd/sssd.conf.bkp")
+ except OSError:
+ root_logger.debug("Error while restoring pre-IPA sssd.conf.")
+
+ if restored:
+ 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:
+ root_logger.debug("Error while moving sssd.conf to "
+ "sssd.conf.deleted")
+
+ 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()
@@ -430,20 +524,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.")
diff --git a/ipapython/sysrestore.py b/ipapython/sysrestore.py
index 7720fd6e3da80cbf19bbadb62e4431d45d4fa482..2c4741f3d03445c1bda725a6f0fd28c2c3ab88c4 100644
--- a/ipapython/sysrestore.py
+++ b/ipapython/sysrestore.py
@@ -143,18 +143,26 @@ class FileStore:
break
return result
- def restore_file(self, path):
+ def restore_file(self, path, new_path = None):
"""Restore the copy of a file at @path to its original
location and delete the copy.
+ Takes optional parameter @new_path which specifies the
+ location where the file is to be restored.
+
Returns #True if the file was restored, #False if there
was no backup file to restore
"""
- root_logger.debug("Restoring system configuration file '%s'", path)
+ if new_path is None:
+ root_logger.debug("Restoring system configuration file '%s'", path)
+ else:
+ root_logger.debug("Restoring system configuration file '%s' to '%s'", path, new_path)
if not os.path.isabs(path):
raise ValueError("Absolute path required")
+ if new_path is not None and not os.path.isabs(new_path):
+ raise ValueError("Absolute new path required")
mode = None
uid = None
@@ -175,6 +183,9 @@ class FileStore:
root_logger.debug(" -> Not restoring - '%s' doesn't exist", backup_path)
return False
+ if new_path is not None:
+ path = new_path
+
shutil.move(backup_path, path)
os.chown(path, int(uid), int(gid))
os.chmod(path, int(mode))
--
1.7.11.4
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
