[Freeipa-devel] [PATCH] 993 disable UPG for migration

[Rob Crittenden]

Martin Kosek wrote:
> On Thu, 2012-03-22 at 15:21 -0400, Rob Crittenden wrote:
>> We don't want to create private groups automatically for migrated users,
>> there could be namespace overlap (and race conditions prevent us from
>> trying to check in advance).
>>
>> Check the sanity of groups in general, warn if the group for the
>> gidnumber doesn't exist at least on the remote server.
>>
>> Fill in the user's shell as well.
>>
>> This will migrate users that are non-POSIX on the remote server.
>>
>> rob
>
> This patch fixes the problem of creating UPGs for migrated users, but
> there are several parts which confused me.
>
> 1) Confusing defaults
>
> +    if 'def_group_gid' in ctx:
> +        entry_attrs.setdefault('gidnumber', ctx['def_group_gid'])
>
> This statement seems redundant, because the account either is posix and
> has both uidnumber and gidnumber or it is non-posix and does not have
> any.
>
> Now, we don't touch gidNumber for posix user, and non-posix users are
> made posix with this statement:
>
> +    # migrated user is not already POSIX, make it so
> +    if 'uidnumber' not in entry_attrs:
> +        entry_attrs['uidnumber'] = entry_attrs['gidnumber'] = [999]
>
>
> 2) Missing UPG
> When UPG is disabled, the following statement will result in a user with
> a GID pointing to non-existent group.
>
> +    # migrated user is not already POSIX, make it so
> +    if 'uidnumber' not in entry_attrs:
> +        entry_attrs['uidnumber'] = entry_attrs['gidnumber'] = [999]
>
> We may want to run ldap.has_upg() and report a add this user to "failed
> users" with appropriate error.
>
> 3) Check for GID
> The patch implements a check if a group with the gidNumber exists on a
> remote LDAP server and the result is a warning:
>
> -            (g_dn, g_attrs) = ldap.get_entry(ctx['def_group_dn'], ['gidnumber'])
> +            (remote_dn, remote_entry) = ds_ldap.find_entry_by_attr(
> +                'gidnumber', entry_attrs['gidnumber'][0], 'posixgroup', [''],
> +                 search_bases['group']
> +            )
>
> I have few (minor-ish) questions there:
> a) Is the warning in a apache log enough? Maybe it should be included in
> migrate-ds output.
> b) This will be a one more remote LDAP call for every user, we may want
> to optimize it with something like that:
>
> valid_gids = []
> if user.gidnumber not in valid_gids:
>     run the check in remote LDAP
>     valid_gids.append(user.gidnumber)
>
> That would save us 999 LDAP queries for 1000 migrated with the same
> primary group.
>
> 4) Extraneous Warning:
> When non-posix user is migrated and thus we make it a posix user, we
> still produce a warning for non-existent group:
>
> [Fri Mar 23 04:21:36 2012] [error] ipa: WARNING: Migrated user's GID
> number 999 does not point to a known group.
>
> 5) Extraneous LDAP call
>
> Isn't the following call to LDAP to get a description redundant? We
> already have the description in entry_attrs.
>
> +    (dn, desc_attr) = ldap.get_entry(dn, ['description'])
> +    entry_attrs.update(desc_attr)
> +    if 'description' in entry_attrs and NO_UPG_MAGIC in
> entry_attrs['description']:
>
>
> Martin
>

I think this covers your concerns.

I can't do anything but log warnings at this point in order to maintain 
backwards compatibility. I looked into returning a warning entry and it 
blew up in validate_output() on older clients.

rob

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-993-2-migration.patch
Type: text/x-patch
Size: 10263 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120328/cca7a186/attachment.bin>