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

[Martin Kosek]

On Thu, 2012-04-12 at 10:03 -0400, John Dennis wrote:
> On 04/12/2012 04:17 AM, Martin Kosek wrote:
> > On Wed, 2012-04-11 at 22:05 -0400, John Dennis wrote:
> >> Revised patch attached. We'll leave the DN parameter changes till later.
> >> This is essentially the same as the original patch with the addition of
> >> the fixes necessary to support passing an empty container arg, an issue
> >> Martin discovered in his review. FWIW the answer was not to make the
> >> param required (actually it would have been adding the flag 'nonempty')
> >> because you should be able to say you don't want to introduce a
> >> container into the search bases (see commit comment)
> >>
> >
> > I don't agree with the removal of default values for the containers and
> > allowing an empty value for them. Please, see my reasoning:
> >
> > 1) I don't think its unlikely to have ou=People and ou=groups as
> > containers for users/groups as they are default containers in fresh LDAP
> > installs. I think most of the small LDAP deployments will use these
> > values.
> >
> > 2) I am also not sure if somebody would want to pass empty user and
> > group container. Users and groups won't be shared in the same container
> > and since we search with _ldap.SCOPE_ONELEVEL the migration would not
> > find users or groups in containers nested under the search base anyway.
> 
> OK. Patch is revised, restored the defaults, usercontainer and 
> groupcontainer are now required to be non-empty. Also, basedn had been 
> optional without a default which didn't make much sense, now basedn is a 
> required parameter.
> 

I still have few issues:

1) basedn should not be required by default. When it is not supplied, we
proactively check remote LDAP DSE and try to either read
nsslapd-defaultnamingcontext or pick the first naming context to make
the migration process easier for the end user. This is what migration
plugin doc says:

If a base DN is not provided with --basedn then IPA will use either
the value of defaultNamingContext if it is set or the first value
in namingContexts set in the root of the remote LDAP server.


2) I don't understand what's the advantage of using an optional param
for usercontainer/groupcontainer and flag 'nonempty' compared to using a
required param.

IMHO, using a required param for this use case is perfectly appropriate
as we indeed require these containers. Besides, 'nonempty' flag is quite
rare - in fact its not used anywhere but migration plugin.

Martin