Clean up Uninstall output #1899

rcritten · 2018-05-02T20:22:39Z

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:

When deleting the last server the server-del command threw an LDAP error trying to write an empty attribute
Two ACI parsing issues related to subtypes

tiran

pylint errors:

************* Module ipapython.admintool
ipapython/admintool.py:177: [E1101(no-member), AdminTool.execute] Instance of 'BaseException' has no 'rval' member)
ipapython/admintool.py:177: [E1101(no-member), AdminTool.execute] Instance of 'BaseException' has no 'rval' member)
ipapython/admintool.py:178: [E1101(no-member), AdminTool.execute] Instance of 'BaseException' has no 'rval' member)
ipapython/admintool.py:181: [E1101(no-member), AdminTool.execute] Instance of 'AdminTool' has no 'ignore_return_codes' member)
************* Module ipaserver.install.dogtaginstance
ipaserver/install/dogtaginstance.py:322: [W1201(logging-not-lazy), DogtagInstance.stop_tracking_certificates] Specify string format arguments as logging function parameters)

rcritten · 2018-05-03T21:23:34Z

I'm baffled about the PEP8 error. I tried to wrap it in another set of parens and same error. I'm tempted to ignore it but would appreciate some input.

stlaz · 2018-05-09T13:50:26Z

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:


if not srvlist: del ret['defaultServerList'] else: ret['defaultServerList'] = ' '.join(srvlist)

^-- removes the possibly unnecessary join() if the list is empty and the condition is a bit more Pythonic I believe :)

issues have been addressed

tiran · 2018-05-24T07:21:40Z

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=[]):


Please don't use mutable data types in argument lists. If you need an iterable, use a tuple as default argument ignore_return_codes=().

tiran · 2018-05-24T07:21:58Z

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*;'


This change looks unrelated to the rest.

This and the change to test_aci.py are related because by enabling logging two ACI parsing errors were exposed by the LDAP updater.

ACI parsing is forgiving and errors are not treated as fatal so this didn't cause any operational issues but two rather nasty looking errors would appear while applying updates as a direct result of my output changes so I fixed this as well.

tiran · 2018-05-24T07:23:56Z

ipapython/admintool.py

@@ -93,6 +93,7 @@ class AdminTool(object):
    log_file_name = None
    usage = None
    description = None
+    ignore_return_codes = []


Same here, use a tuple ().

tiran · 2018-05-24T07:24:17Z

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():


This also looks unrelated.

tiran · 2018-05-24T07:27:16Z

ipaserver/install/cainstance.py

@@ -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",


The second like with profile_id needs to be reindented, too.

tiran · 2018-05-24T07:28:34Z

Thanks for cleanup up the logger! 👍

issues addressed in updated patch

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 <rcritten@redhat.com>

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 <rcritten@redhat.com>

tiran · 2018-06-19T13:36:39Z

************* Module ipaserver.install.custodiainstance
ipaserver/install/custodiainstance.py:87: [W0612(unused-variable), get_custodia_instance] Unused variable 'ldap_uri')

tiran

pylint complains

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 <rcritten@redhat.com>

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 <rcritten@redhat.com>

Tiboris · 2018-06-20T06:38:55Z

master:

b969061 Improve console logging for ipa-server-install
8ea2274 Drop attr defaultServerList if removing the last server
00ddb5d server install: drop some print statements, change log level
036d51d Handle subyptes in ACIs

tiran previously requested changes May 3, 2018

View reviewed changes

rcritten force-pushed the uninstall_output branch from 108c5c9 to ef28174 Compare May 3, 2018 16:59

rcritten added the re-run Trigger a new run of PR-CI label May 3, 2018

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label May 3, 2018

rcritten force-pushed the uninstall_output branch from ef28174 to 41ceeca Compare May 3, 2018 19:54

rcritten force-pushed the uninstall_output branch from 41ceeca to eb4ca3d Compare May 4, 2018 14:20

rcritten added the re-run Trigger a new run of PR-CI label May 4, 2018

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label May 4, 2018

rcritten added the re-run Trigger a new run of PR-CI label May 8, 2018

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label May 8, 2018

stlaz reviewed May 9, 2018

View reviewed changes

rcritten force-pushed the uninstall_output branch from eb4ca3d to 2e987b1 Compare May 9, 2018 20:56

rcritten added the re-run Trigger a new run of PR-CI label May 10, 2018

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label May 10, 2018

tiran previously requested changes May 24, 2018

View reviewed changes

rcritten force-pushed the uninstall_output branch from 2e987b1 to 89cc34f Compare May 25, 2018 13:38

freeipa-pr-ci2 added the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Jun 10, 2018

rcritten added 2 commits June 19, 2018 08:34

Drop attr defaultServerList if removing the last server

bc4f17b

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 <rcritten@redhat.com>

rcritten force-pushed the uninstall_output branch from 89cc34f to ef2e74f Compare June 19, 2018 12:36

rcritten removed the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Jun 19, 2018

tiran self-assigned this Jun 19, 2018

tiran requested changes Jun 19, 2018

View reviewed changes

rcritten added 2 commits June 19, 2018 10:01

rcritten force-pushed the uninstall_output branch from ef2e74f to 332c140 Compare June 19, 2018 14:02

tiran added the ipa-4.7 label Jun 19, 2018

tiran approved these changes Jun 19, 2018

View reviewed changes

tiran added the ack Pull Request approved, can be merged label Jun 19, 2018

Tiboris added the pushed Pull Request has already been pushed label Jun 20, 2018

Tiboris closed this Jun 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up Uninstall output #1899

Clean up Uninstall output #1899

rcritten commented May 2, 2018 •

edited by Tiboris

tiran left a comment

rcritten commented May 3, 2018

stlaz May 9, 2018

rcritten May 9, 2018

tiran May 24, 2018

tiran May 24, 2018

rcritten May 25, 2018

tiran May 24, 2018

tiran May 24, 2018

tiran May 24, 2018

tiran commented May 24, 2018

tiran commented Jun 19, 2018

tiran left a comment

Tiboris commented Jun 20, 2018

Clean up Uninstall output #1899

Clean up Uninstall output #1899

Conversation

rcritten commented May 2, 2018 • edited by Tiboris

tiran left a comment

Choose a reason for hiding this comment

rcritten commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran commented May 24, 2018

tiran commented Jun 19, 2018

tiran left a comment

Choose a reason for hiding this comment

Tiboris commented Jun 20, 2018

rcritten commented May 2, 2018 •

edited by Tiboris