[Freeipa-devel] [PATCH 0024] ipa-replica-manage: added --suffix option for certain commands
Stanislav Laznicka
slaznick at redhat.com
Mon Apr 25 08:39:29 UTC 2016
On 04/22/2016 05:15 PM, Stanislav Laznicka wrote:
> On 04/22/2016 01:13 PM, Martin Basti wrote:
>>
>>
>> On 15.04.2016 14:30, Stanislav Laznicka wrote:
>>> On 04/13/2016 01:40 PM, Martin Basti wrote:
>>>>
>>>>
>>>> On 06.04.2016 14:04, Stanislav Laznicka wrote:
>>>>> 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
>>>>>
>>>>
>>>> 1)
>>>> +class NonFatalError(Exception):
>>>> + pass
>>>>
>>>> IMO we should be more specific and call it NoRUVsFound[Exception]
>>>>
>>>> 2)
>>>> Not sure if this i sstill refactoring, it should be separate patch
>>>> - if dirman_passwd is None:
>>>> + if dirman_passwd is None or (
>>>> + not dirman_passwd and args[0] in
>>>> dirman_passwd_req_commands):
>>>>
>>>> 3)
>>>> +def get_ruv_both_suffixes
>>>>
>>>> I think in case where both suffixes returns RuntimeError we should
>>>> raise RuntimeError too which results into exit with error code.
>>>>
>>> Please see the latest patches.
>> Well, if CA is not installed on replica, it fails, not sure if this
>> is expected behavior of ipa-replica-manage, or if this is related to
>> your current patches.
>>
>> # ipa-replica-manage -p hesloheslo clean-dangling-ruv
>> Failed to obtain information from 'vm-058-051.example.com': no such
>> entry
>>
> It's actually a bug in clean_dangling_ruvs that was not created in
> these patches. I opened a separate ticket for it:
> https://fedorahosted.org/freeipa/ticket/5840.
>
Please see the updated patches. I added verbose flag to
get_ruv_both_suffixes and made list-ruv print that RUVs were not found
in either of the trees instead of empty output for that tree.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0026-4-replica-manage-fail-nicely-when-DM-psswd-required.patch
Type: text/x-patch
Size: 1592 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160425/ecf6c3be/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0027-4-ipa-replica-manage-refactoring.patch
Type: text/x-patch
Size: 5164 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160425/ecf6c3be/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0028-4-abort-clean-list-clean-ruv-now-work-for-both-suffixe.patch
Type: text/x-patch
Size: 9913 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160425/ecf6c3be/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0029-4-Moved-password-check-from-clean_dangling_ruv.patch
Type: text/x-patch
Size: 1827 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160425/ecf6c3be/attachment-0003.bin>
More information about the Freeipa-devel
mailing list