[Freeipa-devel] [PATCH 0024] ipa-replica-manage: added --suffix option for certain commands

Wed Apr 6 12:04:12 UTC 2016

On 03/30/2016 04:52 PM, Martin Basti wrote:
> On 24.03.2016 19:10, Stanislav Laznicka wrote:
>> On 03/23/2016 08:13 PM, Martin Basti wrote:
>>> [...]
>>> Can you please update design
>>> http://www.freeipa.org/page/V4/Manage_replication_topology_4_4 (mainly
>>> the --suffix option)? Also there are missing clean-ruv and list-ruv
>>> commands in design, and fix usage at the bottom.
>>>
>>> 1)
>>> I don't understand this expression
>>> +            if dirman_passwd is None or (
>>> +               not dirman_passwd and args[0] in cs_enabled_commands):
>>>
>>> You already tested if subcommand belongs to cs_enabled_commands few
>>> lines above, IMO the 'dirman_password is None' expression is enough.
>> If I understand it well, when empty password is entered, the program 
>> continues and uses Kerberos credentials for some operations. E.g. for 
>> list-ruv, if empty password is entered, the command would only 
>> display RUVs for domain tree but not for the CA tree no matter if CA 
>> is set up or not (in the current state of the patch, after get_ruv 
>> refactoring). This here is one possible way around this, although the 
>> check for non-empty password might probably just as well be in 
>> get_ruv_both_suffixes.
>
> ok
>>> 2)
>>> +# tuple of commands that work with ca tree and need Directory Manager
>>> password
>>> +cs_enabled_commands = ("list-ruv", "clean-ruv", "abort-clean-ruv")
>>>
>>> this variable is used only toi detect if dirman passwd is needed, I
>>> suggest to rename it to commands_req_dirman_passwd, or something 
>>> better.
>>>
>>> 3)
>>> Q: Do we need is_cs_set() function?
>>> A: Yes!
>>>
>>> I wanted to give you ultimate NACK, but then I checked how get_ruv code
>>> works and I changed my mind.
>>>
>>> Please write a comment where is_cs_set function is used, why we need
>>> extra function instead of catching an exception, possibly you can 
>>> open a
>>> refactoring ticket.
>> After the refactoring changes, is_cs_set should not be needed 
>> anymore, removed it.
>>>
>>> Shame:
>>> 1)
>>> +        if not test_connection(realm, host, options.nolookup) or\
>>> Please use parentheses instead of backslash
>>>
>>> 2)
>>> +           args[0] in cs_enabled_commands:
>>>
>>> +               not dirman_passwd and args[0] in cs_enabled_commands):
>>>
>>> Indentation is not multiplication of 4
>> Shame on me indeed, fixed it.
>>>
>>> Nitpicks (I don't insist on fixing these):
>>> 1)
>>> +    if servers.get('ca', None):
>>>
>>> None is default
>>>
>>> 2)
>>> +        for (netloc, rid) in servers['ca']:
>>> parentheses are not needed
>>>
>>> 3)
>>> + print("\t%s: %s" % (netloc, rid))
>>> Would be nice to use .format() instead of %
>>>
>>> Martin^2
>>>
>>>
>>>
>>> I changed my mind, ultimate NACK.
>>> Please fix get_ruv function, is_cs_set will not help. In case there 
>>> are no RUVs but CA is installed, sys.exit there prevents us from 
>>> removing RUVs (or any else operation) on domain suffix, and vice versa.
>>> I propose to move ticket to 4.4 milestone because I would like to 
>>> avoid breaking something in 4.3, as this will hit many places in 
>>> ipa-replica-manage.
>>>
>>> Please provide the refactoring of get_ruv as separate patch a put 
>>> these patches on top of it.
>>>
>>> Martin2
>> Did the refactoring. There also seemed to be duplicit code in 
>> abort_clean_ruv for some reason, removed it (I hope it does not break 
>> anything, but it seemed rather useless). Also had to change the 
>> numbers of the patches so that they would apply.
>
> NACK
>
> * ipa-replica-manage refactoring *
>
> 1)
> Instead of:
> -        print("Failed to connect to server %s: %s" % (host, e))
> -        sys.exit(1)
> +        root_logger.debug("Failed to connect to server {host}: {err}"
> +                          .format(host=host, err=e))
> +        raise RuntimeError(e)
>
> I expected
>
> -        print("Failed to connect to server %s: %s" % (host, e))
> -        sys.exit(1)
> +        root_logger.debug(traceback.format_exc())
> +        raise RuntimeError("Failed to connect to server {host}: {err}"
> +                                      .format(host=host, err=e)))
Should be fixed now.
>
> 2)
> -        print("No RUV records found.")
> -        sys.exit(0)
>
> Here is exit state 0, so we should not raise error.
>
> I think that we should create new nonfatal exception.
Made a new nonfatal error exception, then. This step was a bit 
controversial when it comes to get_ruv_both_suffixes because it needs to 
catch both this new exception and a RuntimeError exception for both 
trees. As the main idea here was not to stop if either tree is missing 
(resulting in RuntimeError in get_ruv) or contains no records 
(NonFatalError), it is only printed that something bad may have happened 
(or it's debug-logged in case of nonfatal errors). In the end, only 
NonFatalError exception is raised if no records were found for whatever 
reason resulting in the script always returning 0.
>
> 3)
> -                print("unable to decode: %s" % ruv)
> +                root_logger.debug("unable to decode: %s" % ruv)
> This may break tests, because the logger logs to stderr, leave it as 
> print for now
>
> 4)
> -    servers = get_ruv(realm, host, dirman_passwd, nolookup)
> +    try:
> +        servers = get_ruv(realm, host, dirman_passwd, nolookup)
> +    except RuntimeError as e:
> +        print(e)
> +        sys.exit(1)
>
> again we have to print it to stdout for now.
3), 4) done, although it might be better to log to stderr from patch 
number 27 and on since the default behavior is changed anyway.
>
> * abort-clean/list/clean-ruv now work for both suffixes *
>
> -            if dirman_passwd is None:
> +            if dirman_passwd is None or (
> +               not dirman_passwd and args[0] in 
> dirman_passwd_req_commands):
>                  sys.exit("Directory Manager password required")
Done.
>
> Please fix other patch accordingly.
> Martin^2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0026-2-ipa-replica-manage-refactoring.patch
Type: text/x-patch
Size: 5748 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160406/62cfe9d5/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0027-2-abort-clean-list-clean-ruv-now-work-for-both-suffixe.patch
Type: text/x-patch
Size: 10018 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160406/62cfe9d5/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0028-2-Moved-password-check-from-clean_dangling_ruv.patch
Type: text/x-patch
Size: 1829 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160406/62cfe9d5/attachment-0002.bin>