[Freeipa-devel] [PATCH] 0118 add support for subdomains

[Martin Kosek]

On 10/04/2013 09:40 AM, Alexander Bokovoy wrote:
> On Fri, 04 Oct 2013, Alexander Bokovoy wrote:
>> On Fri, 04 Oct 2013, Alexander Bokovoy wrote:
>>> On Thu, 03 Oct 2013, Martin Kosek wrote:
>>>> On 10/03/2013 03:10 PM, Alexander Bokovoy wrote:
>>>>> On Wed, 02 Oct 2013, Sumit Bose wrote:
>>>>>>> Please note that I did not test with more than 1 subdomain, since I
>>>>>>> do not have more ADs available.
>>>>>>>
>>>>>>
>>>>>> I have done some testing as well and the patches are working as expected
>>>>>> except the trustdomain-disable issue Tomas mentioned. But I think it
>>>>>> would be sufficient to add a comment to the release notes and fix this
>>>>>> with the next release to not delay this release anymore.
>>>>>>
>>>>>> The patches are also working for trusts which were added with older
>>>>>> releases. So ACK from my side for the functional part.
>>>>> New patchset is attached. I've fixed all outstanding issues and
>>>>> implemented proper SID filtering for subdomains. In addition, I've
>>>>> added MS-PAC cache eviction when we change blacklists from IPA side
>>>>> and forced removal of the domain from SID blacklist if the domain is
>>>>> being removed by trustdomain-del.
>>>>>
>>>>
>>>> 1) Minor issue in 0118:
>>>>
>>>> +        if keys[0].lower() == keys[1].lower():
>>>> +            raise errors.ValidationError(name='trustdomain_enable',
>>>> +                error=_("Root domain of the trust is always enabled for the
>>>> existing trust"))
>>>>
>>>> The error message looks weird (double trustdomain_enable)
>>>>
>>>> # ipa trustdomain-enable realm domain
>>>> ipa: ERROR: invalid 'trustdomain_enable': Root domain of the trust is always
>>>> enabled for the existing trust
>>>>
>>>> I would rather do something like
>>>>
>>>> +            raise errors.ValidationError(name='domain',
>>>>
>>>>
>>>> 2) trustconfig-enable and trustconfig-disable should use standard output like
>>>> other enable/disable methods. See user-enable/user-disable for example.
>>>> Current
>>>> situation puts all the authoritative information to summary which:
>>>> a) Does not look nice in terminal
>>>> # ipa trustdomain-disable very.long.long.long.realm very.long.long.long.domain
>>>> ----------------------------------------------------------------------------------------------------------------------------
>>>>
>>>> Domain very.long.long.long.domain of trust very.long.long.long.realm is not
>>>> allowed to access IPA resources
>>>> ----------------------------------------------------------------------------------------------------------------------------
>>>>
>>>> b) How am I supposed to parse an information about the result if all I get
>>>> is a
>>>> text in summary? Using standard errors and output values will allow easier
>>>> consumption of the API later (like in Web UI).
>>>>
>>>> I am attaching a patch (0001) how to make it consistent with other
>>>> enable/disable commands. Example:
>>>>
>>>> # ipa trustdomain-disable realm domain
>>>> ipa: ERROR: This entry is already disabled
>>>>
>>>> # ipa trustdomain-enable realm domain
>>>> -----------------------------
>>>> Enabled trust domain "domain"
>>>> -----------------------------
>>>>
>>>> 3) Let's use standard primary key for the trustdomain object. This will let us
>>>> overcome some hacks and also let us use handle_not_found method - patch
>>>> attached (0002).
>>>>
>>>> 0002 also changes some ValidationError errors to standard errors, just for
>>>> being consistent with the rest of the API.
>>>>
>>>> Note that in order to make primary_key=True, I had to enhance trustdomain_del
>>>> command to manage multiple domains.
>>>>
>>>>
>>>> I think these API fixes are a must, it would be very hard to amend the API
>>>> later. If these patches are squashed to your 0118, it would be ACK from me to
>>>> the Python side. I will let C parts and a functional test to Sumit's mighty
>>>> hands.
>>> Thanks. I've merged these changes, along with a API.txt correction. In
>>> my tests these worked fine.
>>>
>>> I'll resend 0118 shortly.
>> New edition of 0118 attached.
> ... and updated 0124 to match the 0118.
> 

Thanks Alexander and Sumit! The patches seem to work now - ACK.

Pushed all to master, ipa-3-3.

I just also updated our spec to require new SSSD.

Martin