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

[Martin Kosek]

On Fri, 2012-04-13 at 14:42 -0400, John Dennis wrote:
> On 04/13/2012 07:53 AM, Martin Kosek wrote:
> > 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.
> 
> Fixed the above two issues, revised patch attached.
> 
> 

ACK. I just needed to re-generate API.txt before I pushed the patch.

Pushed to master, ipa-2-2.

Martin