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

[Martin Babinsky]

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.

PATCH 219: ACK

-- 
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-4-3-abbra-0207-1-ipaserver-dcerpc-reformat-to-make-the-code-closer-to.patch
Type: text/x-patch
Size: 51662 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160818/45f48ea9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-4-3-abbra-0218-1-trust-automatically-resolve-DNS-trust-conflicts-for-.patch
Type: text/x-patch
Size: 17184 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160818/45f48ea9/attachment-0001.bin>