[Freeipa-devel] [PATCH 72] Validate DN & RDN parameters for migrate command

[John Dennis]

On 04/06/2012 10:11 AM, John Dennis wrote:
> On 04/06/2012 04:40 AM, Martin Kosek wrote:
>> 1) We still crash when the parameter is empty. We may want to make it
>> required (the same fix Rob did for cert rejection reason):
>>
>> # echo "secret123" | ipa migrate-ds ldap://vm-054.idm.lab.bos.redhat.com
>> --with-compat --base-dn="dc=greyoak,dc=com" --user-container=
>> ipa: ERROR: cannot connect to
>> u'http://vm-022.idm.lab.bos.redhat.com/ipa/xml': Internal Server Error
>
> Good point, will fix.
>
>> 2) Do you think it would make sense to create a special Param for DN?
>> Its quite general type and I bet there are other Params that could use
>> DN instead of Str. It could look like that:
>>           DN('usercontainer?',
>>               rdn=True,<<<<   can be RDN, not DN
>
> Yes, I considered introducing a new DN parameter type as well. I think
> this is a good approach and will have payoff down the road. I will make
> that change. However I'm inclined to introduce both a DN parameter and a
> RDN parameter, they really are different entities, if you use a flag to
> indicate you require a RDN then you lose the type of the parameter, it
> could be either, and it really should be one or the other and knowing
> which it is has value.
>
>> 3) We should not restrict users from passing a user/group container with
>> more than one RDN:
>
> Yeah, I wasn't too sure about that. The parameter distinctly called for
> an RDN, but it seemed to me it should support any container below the
> base, which would make it a DN not a RDN.
>
> FWIW, a DN is an ordered sequence of 1 or more RDN's. DN's do not have
> to be "absolute", they can be any subset of a "path sequence".  A DN
> with exactly one RDN is equivalent to an RDN, a special case).
>
> I will change the parameter to be a DN since that is what was the
> original intent.
>
> All good suggestions, a revised patch will follow shortly.

Hmm... I'm rethinking this. The suggestion of adding a new DN parameter 
is the right thing to do and I tried to implement it, but as I was 
working I realized I needed to touch a fair amount of code to support 
the new parameter type and to modify multiple places in the migration 
code to work with the new type. That's more code changes at the very 
last minute for this release than I'm comfortable with, we're too close 
the freeze date for invasive modifications.

We have a work item open to introduce DN types throughout the code 
including as a new Param type. I think it's best to make just a small 
tweak to fix the traceback today and wait till the 3.0 work to "do it 
right". Therefore I'm no longer in favor of the new Param approach. I 
will fix the problem you reported when the parameter is empty, but merge 
that into the original patch.


-- 
John Dennis <jdennis at redhat.com>

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/