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

[Martin Kosek]

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