[Freeipa-devel] [PATCH] 0207, 0218-0219 Solving trust conflicts and external trust topology fixes
Martin Babinsky
mbabinsk at redhat.com
Thu Aug 18 15:13:43 UTC 2016
On 08/18/2016 01:25 PM, Martin Babinsky wrote:
> On 08/17/2016 01:20 PM, Alexander Bokovoy wrote:
>> On Wed, 17 Aug 2016, Martin Babinsky wrote:
>>> Hi Alexander,
>>>
>>> patch 207: LGTM, but I have a feeling that the patch should be linked
>>> to both #6021 and #6076 so that it is not lost during backports.
>>>
>>> patch 218:
>>>
>>> ipalib/errors.py:
>>>
>>> 1.)
>>> I'm not sure if TrustTopologyConflictError should inherit from
>>> InvocationError. The semantics of InvocationError implies that
>>> something was wrong when trying to invoke the command (a param failed
>>> to validate/convert, incorrect number of args, etc.), while this is
>>> more of an exception during command execution (no. and type of params
>>> was correct, command started to execute but encountered an error
>>> condition). Thus I think it should inherit from ExecutionError. CC'ing
>>> Jan for more thoughts on this.
>> Using ExecutionError would work to me too, as long as we display the
>> error to a user.
>>> Why is TrustTopologyConflictSolved listed amogn public errors? Since
>>> it is used only in dcerpc.py to restart trust establishment after
>>> resolving conflicts, it should be a private exception in dcerpc.py for
>>> this purpose.
>> I originally wanted to make it a warning -- i.e. if we fixed the
>> conflict, return the result and show the warning that we did solve the
>> conflict. After all, the code is modifying another trusted forest's
>> topology on behalf of the user. I can move the error class to dcerpc.py
>>
>>
>>> 3.)
>>> Also please split the exception format string like this so that the
>>> line is not too long (there is not much we can do about doctest so
>>> leave that line as it is):
>>>
>>> @@ -882,7 +882,8 @@ class TrustTopologyConflictError(InvocationError):
>>> """
>>>
>>> errno = 3017
>>> - format = _("Forest '%(forest)s' has existing trust to forest(s)
>>> %(domains)s which prevents a trust to '%(conflict)s'")
>>> + format = _("Forest '%(forest)s' has existing trust to forest(s) "
>>> + "%(domains)s which prevents a trust to '%(conflict)s'")
>>>
>>> Do not worry about gettext, it can handle it just fine, there are
>>> plenty of examples in server plugins, for example.
>> Done.
>>
>>> ipaserver/dcerpc.py:
>>>
>>> 1.)
>>>
>>> I think that instead of returning result and raising
>>> TrustTopologyConflictError based on that, the 'clear_ftinfo_conflict'
>>> can raise this exception directly. You can have an empty list defined
>>> at the beginning instead of 'result list', append unresolvable
>>> conflicts to it and then at the end of the method check if it is
>>> non-empty and raise the exception.
>> Good suggestion, fixed.
>>
>>>
>>> 2.)
>>>
>>> + # In the code below:
>>> + # self -- the forest we establish trust to
>>> + # another_domain -- a forest that establishes trust to 'self'
>>> + # cinfo -- lsa_ForestTrustCollisionInfo structure that contain
>>> + # set of of lsa_ForestTrustCollisionRecord structures
>>> I would add this directly into the method docstring:
>>>
>>> """
>>> ...
>>>
>>> :param self: the forest we establish trust to
>>> :param another_domain: a forest that establishes trust to 'self'
>>> :param cinfo: lsa_ForestTrustCollisionInfo structure that contain
>>> set of of lsa_ForestTrustCollisionRecord structures
>>> """
>> Added.
>>
>>> Additionally, the behavior specifed in previous comment can be added
>>> using :raises: stanza:
>>>
>>> """
>>> :raises: errors.TrustTopologyConflictError if there are unresolvable
>>> namespace conflicts between trusted domains
>>> """
>> Added.
>>
>>>
>>> 3.)
>>>
>>> rewriting 'clear_ftinfo_conflict' according to point 1.) will allow to
>>> simplify code in 'update_ftinfo' like this:
>>>
>>> """
>>> - res = self.clear_ftinfo_conflict(another_domain, cinfo)
>>> - if len(res[1]) != 0:
>>> - domains = [x.name.string for x in res[1]]
>>> - raise errors.TrustTopologyConflictError(
>>> - target=self.info['dns_domain'],
>>> -
>>> conflict=another_domain.info['dns_domain'],
>>> - domains=domains)
>>> - else:
>>> - raise errors.TrustTopologyConflictSolved(
>>> - target=self.info['dns_domain'],
>>> -
>>> conflict=another_domain.info['dns_domain'])
>>> + self.clear_ftinfo_conflict(another_domain, cinfo)
>>> + raise errors.TrustTopologyConflictSolved(
>>> + target=self.info['dns_domain'],
>>> + conflict=another_domain.info['dns_domain'])
>>> """
>> done.
>>
>>>
>>> Patch 218:
>>>
>>> 1.)
>>> typo in the commit message:
>>>
>>> """
>>> ...
>>> suffixes are forest-wide, there *are could be* user accounts in the
>>> ...
>>> """
>> Fixed.
>>
>> Updated patches attached.
>
> PATCH 207: ACK, I am attaching rebased version for ipa-4-3. Please check
> if the rebase is correct.
>
> PATCH 218: I am attaching rebased version for control. Unfortunately, I
> am unable to properly test conflict resolution due to reasons beyond my
> control but it does not break any ordinary workflows and code looks OK,
> so ACK.
>
I have noticed that raising of TrustTopologyConflictSolved is broken. I
have changed the base class to Exception and it works. Attaching patches
with the change.
> PATCH 219: ACK
>
>
>
--
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-4-3-abbra-218-2-trust-automatically-resolve-DNS-trust-conflicts-for-.patch
Type: text/x-patch
Size: 17172 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160818/f60460ee/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-abbra-218-2-trust-automatically-resolve-DNS-trust-conflicts-for-.patch
Type: text/x-patch
Size: 17393 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160818/f60460ee/attachment-0001.bin>
More information about the Freeipa-devel
mailing list