[Freeipa-devel] [PATCH] 0207, 0218-0219 Solving trust conflicts and external trust topology fixes

[Martin Babinsky]

On 08/18/2016 05:13 PM, Martin Babinsky wrote:
> 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
>>
>>
>>
>
>
>
>

Linked patch 207 to ticket 6076:

207 and 218:

Pushed to master: 6332cb3125a42c1bf2680309b5480155e40d3d87

Pushed to ipa-4-3: 324d5aa1d09431c64d817f628b18b01a12253ccf

patch 219:

Pushed to master: 9b3819ea94d3fd8e866d38ccba2051446d057ecd

-- 
Martin^3 Babinsky