URL: https://github.com/freeipa/freeipa/pull/1899 Author: rcritten Title: #1899: Clean up Uninstall output Action: opened
PR body: """ ipa-server-install did not configure a console logger. This caused odd output because ipa-client-install does and a normal server uninstall would end with "ipa-client-install command was successful" Because there was no console logger there were print statements in a lot of places. Those were dropped because the equivalent logging statement was already there. A number of info loggers were converted to debug to suppress their output. They were never shown in the past for the most part with the exception of the client installer and Forwarding and trying statements. Adding the logger exposed two additional issues: 1. When deleting the last server the server-del command threw an LDAP error trying to write an empty attribute 2. Two ACI parsing issues related to subtypes """ To pull the PR as Git branch: git remote add ghfreeipa https://github.com/freeipa/freeipa git fetch ghfreeipa pull/1899/head:pr1899 git checkout pr1899
From 9ced8166f45afe2c435ed9470c3f23448d9ef30a Mon Sep 17 00:00:00 2001 From: Rob Crittenden <rcrit...@redhat.com> Date: Wed, 2 May 2018 11:35:34 -0400 Subject: [PATCH 1/4] Improve console logging for ipa-server-install The server installation and uninstallation overlaps both the server and client installers. The output could be confusing with a server uninstall finishing with the message: The ipa-client-install command was successful This was in part due to the fact that the server was not configured with a console format and verbose was False which meant that no logger messages were displayed at all. In order to suppress client installation errors and avoid confusion add a list of errors to ignore. If a server install was not successful and hadn't gotten far enough to do the client install then we shouldn't complain loudly about it. https://pagure.io/freeipa/issue/6760 Signed-off-by: Rob Crittenden <rcrit...@redhat.com> --- ipaclient/install/ipa_client_install.py | 1 + ipapython/admintool.py | 5 ++++- ipapython/install/cli.py | 25 +++++++++++++++++++++++-- ipaserver/install/ipa_server_install.py | 2 ++ 4 files changed, 30 insertions(+), 3 deletions(-) diff --git a/ipaclient/install/ipa_client_install.py b/ipaclient/install/ipa_client_install.py index 76375f8d27..66e7df1651 100644 --- a/ipaclient/install/ipa_client_install.py +++ b/ipaclient/install/ipa_client_install.py @@ -62,6 +62,7 @@ def host_password(self): verbose=True, console_format='%(message)s', uninstall_log_file_name=paths.IPACLIENT_UNINSTALL_LOG, + ignore_return_codes=[client.CLIENT_NOT_CONFIGURED], ) diff --git a/ipapython/admintool.py b/ipapython/admintool.py index 329e20f374..5b31de3c68 100644 --- a/ipapython/admintool.py +++ b/ipapython/admintool.py @@ -173,9 +173,12 @@ def execute(self): self.setup_logging() return_value = self.run() except BaseException as exception: + if isinstance(exception, ScriptError): + if exception.rval and exception.rval > return_value: + return_value = exception.rval traceback = sys.exc_info()[2] error_message, return_value = self.handle_error(exception) - if return_value: + if return_value and return_value not in self.ignore_return_codes: self.log_failure(error_message, return_value, exception, traceback) return return_value diff --git a/ipapython/install/cli.py b/ipapython/install/cli.py index 86503a66bf..c1ddbff657 100644 --- a/ipapython/install/cli.py +++ b/ipapython/install/cli.py @@ -58,7 +58,24 @@ def _get_usage(configurable_class): def install_tool(configurable_class, command_name, log_file_name, debug_option=False, verbose=False, console_format=None, - use_private_ccache=True, uninstall_log_file_name=None): + use_private_ccache=True, uninstall_log_file_name=None, + ignore_return_codes=[]): + """ + Some commands represent multiple related tools, e.g. + ``ipa-server-install`` and ``ipa-server-install --uninstall`` would be + represented by separate classes. Only their options are the same. + + :param configurable_class: the command class for options + :param command_name: the command name shown in logs/output + :param log_file_name: if None, logging is to stderr only + :param debug_option: log level is DEBUG + :param verbose: log level is INFO + :param console_format: logging format for stderr + :param use_private_ccache: a temporary ccache is created and used + :param uninstall_log_file_name: if not None the log for uninstall + :param ignore_return_codes: list of error codes to not log errors + for. Let the caller do it if it wants. + """ if uninstall_log_file_name is not None: uninstall_kwargs = dict( configurable_class=configurable_class, @@ -67,6 +84,7 @@ def install_tool(configurable_class, command_name, log_file_name, debug_option=debug_option, verbose=verbose, console_format=console_format, + ignore_return_codes=ignore_return_codes, ) else: uninstall_kwargs = None @@ -84,12 +102,14 @@ def install_tool(configurable_class, command_name, log_file_name, console_format=console_format, uninstall_kwargs=uninstall_kwargs, use_private_ccache=use_private_ccache, + ignore_return_codes=ignore_return_codes, ) ) def uninstall_tool(configurable_class, command_name, log_file_name, - debug_option=False, verbose=False, console_format=None): + debug_option=False, verbose=False, console_format=None, + ignore_return_codes=[]): return type( 'uninstall_tool({0})'.format(configurable_class.__name__), (UninstallTool,), @@ -101,6 +121,7 @@ def uninstall_tool(configurable_class, command_name, log_file_name, debug_option=debug_option, verbose=verbose, console_format=console_format, + ignore_return_codes=ignore_return_codes, ) ) diff --git a/ipaserver/install/ipa_server_install.py b/ipaserver/install/ipa_server_install.py index 3166844b43..99d230d419 100644 --- a/ipaserver/install/ipa_server_install.py +++ b/ipaserver/install/ipa_server_install.py @@ -40,7 +40,9 @@ class CompatServerMasterInstall(ServerMasterInstall): CompatServerMasterInstall, command_name='ipa-server-install', log_file_name=paths.IPASERVER_INSTALL_LOG, + console_format='%(message)s', debug_option=True, + verbose=True, uninstall_log_file_name=paths.IPASERVER_UNINSTALL_LOG, ) From e6df3f5de46a0f7b60f113f8cd62bcb2da969ade Mon Sep 17 00:00:00 2001 From: Rob Crittenden <rcrit...@redhat.com> Date: Wed, 2 May 2018 11:42:02 -0400 Subject: [PATCH 2/4] Drop attr defaultServerList if removing the last server This otherwise returns a syntax error if trying to set an empty value. Related: https://pagure.io/freeipa/issue/6760 Signed-off-by: Rob Crittenden <rcrit...@redhat.com> --- ipaserver/plugins/server.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ipaserver/plugins/server.py b/ipaserver/plugins/server.py index 59e611fc4c..edef30ff50 100644 --- a/ipaserver/plugins/server.py +++ b/ipaserver/plugins/server.py @@ -619,7 +619,10 @@ def _remove_server_principal_references(self, master): if master in srvlist: srvlist.remove(master) attr = ' '.join(srvlist) - ret['defaultServerList'] = attr + if len(srvlist) == 0: + del ret['defaultServerList'] + else: + ret['defaultServerList'] = attr conn.update_entry(ret) except (errors.NotFound, errors.MidairCollision, errors.EmptyModlist): From 66d1c8418be85e6ca41a248046788a833e8a09e1 Mon Sep 17 00:00:00 2001 From: Rob Crittenden <rcrit...@redhat.com> Date: Wed, 2 May 2018 11:43:09 -0400 Subject: [PATCH 3/4] server install: drop some print statements, change log level The server installer had no console logger set so print statements were used for communication. Now that a logger is enabled the extra prints need to be dropped. A number of logger.info statements have been upgraded to debug since they do not need to appear on the console by default. https://pagure.io/freeipa/issue/6760 Signed-off-by: Rob Crittenden <rcrit...@redhat.com> --- ipaclient/install/client.py | 6 +++--- ipalib/rpc.py | 6 +++--- ipaserver/install/cainstance.py | 18 +++++++++--------- ipaserver/install/custodiainstance.py | 4 ++-- ipaserver/install/dns.py | 1 - ipaserver/install/dogtaginstance.py | 2 +- ipaserver/install/server/install.py | 5 +---- 7 files changed, 19 insertions(+), 23 deletions(-) diff --git a/ipaclient/install/client.py b/ipaclient/install/client.py index cdfde085a8..0f95c0be10 100644 --- a/ipaclient/install/client.py +++ b/ipaclient/install/client.py @@ -861,7 +861,7 @@ def configure_sssd_conf( logger.info( "The old /etc/sssd/sssd.conf is backed up and " "will be restored during uninstall.") - logger.info("New SSSD config will be created") + logger.debug("New SSSD config will be created") sssdconfig = SSSDConfig.SSSDConfig() sssdconfig.new_config() @@ -2488,8 +2488,8 @@ def _install(options): elif options.on_master: # If we're on master skipping the time sync here because it was done # in ipa-server-install - logger.info("Skipping attempt to configure and synchronize time with" - " chrony server as it has been already done on master.") + logger.debug("Skipping attempt to configure and synchronize time with" + " chrony server as it has been already done on master.") else: logger.info("Skipping chrony configuration") diff --git a/ipalib/rpc.py b/ipalib/rpc.py index c6a8989f5d..3957a4b9cc 100644 --- a/ipalib/rpc.py +++ b/ipalib/rpc.py @@ -1053,7 +1053,7 @@ def create_connection(self, ccache=None, verbose=None, fallback=None, transport_class = LanguageAwareTransport proxy_kw['transport'] = transport_class( protocol=self.protocol, service='HTTP', ccache=ccache) - logger.info('trying %s', url) + logger.debug('trying %s', url) setattr(context, 'request_url', url) serverproxy = self.server_proxy_class(url, **proxy_kw) if len(urls) == 1: @@ -1143,8 +1143,8 @@ def forward(self, name, *args, **kw): # each time should we be getting UNAUTHORIZED error from the server max_tries = 5 for try_num in range(0, max_tries): - logger.info("[try %d]: Forwarding '%s' to %s server '%s'", - try_num+1, name, self.protocol, server) + logger.debug("[try %d]: Forwarding '%s' to %s server '%s'", + try_num+1, name, self.protocol, server) try: return self._call_command(command, params) except Fault as e: diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py index 5b469e13fc..fc2438bd68 100644 --- a/ipaserver/install/cainstance.py +++ b/ipaserver/install/cainstance.py @@ -1050,7 +1050,7 @@ def uninstall(self): cmonger.stop() # remove CRL files - logger.info("Remove old CRL files") + logger.debug("Remove old CRL files") try: for f in get_crl_files(): logger.debug("Remove %s", f) @@ -1059,7 +1059,7 @@ def uninstall(self): logger.warning("Error while removing old CRL files: %s", e) # remove CRL directory - logger.info("Remove CRL directory") + logger.debug("Remove CRL directory") if os.path.exists(paths.PKI_CA_PUBLISH_DIR): try: shutil.rmtree(paths.PKI_CA_PUBLISH_DIR) @@ -1271,12 +1271,12 @@ def setup_lightweight_ca_key_retrieval(self): if sysupgrade.get_upgrade_state('dogtag', 'setup_lwca_key_retrieval'): return - logger.info('[Set up lightweight CA key retrieval]') + logger.debug('Set up lightweight CA key retrieval') self.__setup_lightweight_ca_key_retrieval_kerberos() self.__setup_lightweight_ca_key_retrieval_custodia() - logger.info('Configuring key retriever') + logger.debug('Configuring key retriever') directives = [ ('features.authority.keyRetrieverClass', 'com.netscape.ca.ExternalProcessKeyRetriever'), @@ -1292,12 +1292,12 @@ def setup_lightweight_ca_key_retrieval(self): def __setup_lightweight_ca_key_retrieval_kerberos(self): pent = pwd.getpwnam(self.service_user) - logger.info('Creating principal') + logger.debug('Creating principal') installutils.kadmin_addprinc(self.principal) self.suffix = ipautil.realm_to_suffix(self.realm) self.move_service(self.principal) - logger.info('Retrieving keytab') + logger.debug('Retrieving keytab') installutils.create_keytab(self.keytab, self.principal) os.chmod(self.keytab, 0o600) os.chown(self.keytab, pent.pw_uid, pent.pw_gid) @@ -1305,7 +1305,7 @@ def __setup_lightweight_ca_key_retrieval_kerberos(self): def __setup_lightweight_ca_key_retrieval_custodia(self): pent = pwd.getpwnam(self.service_user) - logger.info('Creating Custodia keys') + logger.debug('Creating Custodia keys') custodia_basedn = DN( ('cn', 'custodia'), ('cn', 'ipa'), ('cn', 'etc'), api.env.basedn) ensure_entry( @@ -1686,7 +1686,7 @@ def import_included_profiles(): # Create the profile, replacing any existing profile of same name profile_data = __get_profile_config(profile_id) _create_dogtag_profile(profile_id, profile_data, overwrite=True) - logger.info("Imported profile '%s'", profile_id) + logger.debug("Imported profile '%s'", profile_id) api.Backend.ra_certprofile.override_port = None conn.disconnect() @@ -1794,7 +1794,7 @@ def _create_dogtag_profile(profile_id, profile_data, overwrite): # import the profile try: profile_api.create_profile(profile_data) - logger.info("Profile '%s' successfully migrated to LDAP", + logger.debug("Profile '%s' successfully migrated to LDAP", profile_id) except errors.RemoteRetrieveError as e: logger.debug("Error migrating '%s': %s", profile_id, e) diff --git a/ipaserver/install/custodiainstance.py b/ipaserver/install/custodiainstance.py index 3820eec766..06f75d85b6 100644 --- a/ipaserver/install/custodiainstance.py +++ b/ipaserver/install/custodiainstance.py @@ -65,7 +65,7 @@ def get_custodia_instance(config, mode): *master_host_name* to create a replica with CA. """ assert isinstance(mode, CustodiaModes) - logger.info( + logger.debug( "Custodia client for '%r' with promotion %s.", mode, 'yes' if config.promote else 'no' ) @@ -85,7 +85,7 @@ def get_custodia_instance(config, mode): if custodia_master is None: # use ldapi with local dirsrv instance - logger.info("Custodia uses LDAPI.") + logger.debug("Custodia uses LDAPI.") ldap_uri = None else: logger.info("Custodia uses '%s' as master peer.", custodia_master) diff --git a/ipaserver/install/dns.py b/ipaserver/install/dns.py index e14b353e9c..40a3c0b49b 100644 --- a/ipaserver/install/dns.py +++ b/ipaserver/install/dns.py @@ -127,7 +127,6 @@ def install_check(standalone, api, replica, options, hostname): if not already_enabled: domain = dnsutil.DNSName(util.normalize_zone(api.env.domain)) - print("Checking DNS domain %s, please wait ..." % domain) try: dnsutil.check_zone_overlap(domain, raise_on_error=False) except ValueError as e: diff --git a/ipaserver/install/dogtaginstance.py b/ipaserver/install/dogtaginstance.py index d0c6690faf..32cac509cd 100644 --- a/ipaserver/install/dogtaginstance.py +++ b/ipaserver/install/dogtaginstance.py @@ -319,7 +319,7 @@ def track_servercert(self): def stop_tracking_certificates(self, stop_certmonger=True): """Stop tracking our certificates. Called on uninstall. """ - self.print_msg( + logger.debug( "Configuring certmonger to stop tracking system certificates " "for %s" % self.subsystem) diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py index b17c666a29..e623d68d2d 100644 --- a/ipaserver/install/server/install.py +++ b/ipaserver/install/server/install.py @@ -767,10 +767,7 @@ def install(installer): # chrony will be handled here in uninstall() method as well by invoking # the ipa-server-install --uninstall if not options.no_ntp: - print("Synchronizing time") - if ipaclient.install.client.sync_time(options, fstore, sstore): - print("Time synchronization was successful.") - else: + if not ipaclient.install.client.sync_time(options, fstore, sstore): print("Warning: IPA was unable to sync time with chrony!") print(" Time synchronization is required for IPA " "to work correctly") From 108c5c943cee64f9255f15d0fee06a15207b2b21 Mon Sep 17 00:00:00 2001 From: Rob Crittenden <rcrit...@redhat.com> Date: Wed, 2 May 2018 16:14:56 -0400 Subject: [PATCH 4/4] Handle subyptes in ACIs While enabling console output in the server installation the "Allow trust agents to retrieve keytab keys for cross realm principals" ACI was throwing an unparseable error because it has a subkey which broke parsing (the extra semi-colon): userattr="ipaAllowedToPerform;read_keys#GROUPDN"; The regular expression pattern needed to be updated to handle this case. Related: https://pagure.io/freeipa/issue/6760 Signed-off-by: Rob Crittenden <rcrit...@redhat.com> --- ipalib/aci.py | 3 ++- ipatests/test_ipalib/test_aci.py | 12 ++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/ipalib/aci.py b/ipalib/aci.py index 38cc12642c..09a56a0d1b 100755 --- a/ipalib/aci.py +++ b/ipalib/aci.py @@ -25,7 +25,8 @@ # The Python re module doesn't do nested parenthesis # Break the ACI into 3 pieces: target, name, permissions/bind_rules -ACIPat = re.compile(r'\(version\s+3.0\s*;\s*ac[li]\s+\"([^\"]*)\"\s*;\s*([^;]*);\s*\)', re.UNICODE) +ACIPat = re.compile(r'\(version\s+3.0\s*;\s*ac[li]\s+\"([^\"]*)\"\s*;' + '\s*(.*);\s*\)', re.UNICODE) # Break the permissions/bind_rules out PermPat = re.compile(r'(\w+)\s*\(([^()]*)\)\s*(.*)', re.UNICODE) diff --git a/ipatests/test_ipalib/test_aci.py b/ipatests/test_ipalib/test_aci.py index 5ce23dbcc3..9ddcc54b7b 100644 --- a/ipatests/test_ipalib/test_aci.py +++ b/ipatests/test_ipalib/test_aci.py @@ -162,3 +162,15 @@ def test_aci_parsing_8(): def test_aci_parsing_9(): check_aci_parsing('(targetfilter = "(|(objectClass=person)(objectClass=krbPrincipalAux)(objectClass=posixAccount)(objectClass=groupOfNames)(objectClass=posixGroup))")(targetattr != "aci || userPassword || krbPrincipalKey || sambaLMPassword || sambaNTPassword || passwordHistory")(version 3.0; acl "Account Admins can manage Users and Groups"; allow (add, delete, read, write) groupdn = "ldap:///cn=admins,cn=groups,cn=accounts,dc=greyoak,dc=com";;)', '(targetattr != "aci || userPassword || krbPrincipalKey || sambaLMPassword || sambaNTPassword || passwordHistory")(targetfilter = "(|(objectClass=person)(objectClass=krbPrincipalAux)(objectClass=posixAccount)(objectClass=groupOfNames)(objectClass=posixGroup))")(version 3.0;acl "Account Admins can manage Users and Groups";allow (add,delete,read,write) groupdn = "ldap:///cn=admins,cn=groups,cn=accounts,dc=greyoak,dc=com";;)') + + +def test_aci_parsing_10(): + """test subtypes""" + check_aci_parsing('(targetattr="ipaProtectedOperation;read_keys")' + '(version 3.0; acl "Allow trust agents to retrieve ' + 'keytab keys for cross realm principals"; allow(read) ' + 'userattr="ipaAllowedToPerform;read_keys#GROUPDN";)', + '(targetattr = "ipaProtectedOperation;read || keys")' + '(version 3.0;acl "Allow trust agents to retrieve ' + 'keytab keys for cross realm principals";allow (read) ' + 'userattr = "ipaAllowedToPerform;read_keys#GROUPDN";)')
_______________________________________________ FreeIPA-devel mailing list -- freeipa-devel@lists.fedorahosted.org To unsubscribe send an email to freeipa-devel-le...@lists.fedorahosted.org
