Re: [SSSD] [PATCHES] Add backend part of user views

[Sumit Bose]

On Fri, Sep 26, 2014 at 01:41:39PM +0200, Pavel Březina wrote:
> On 09/26/2014 01:38 PM, Pavel Březina wrote:
> > On Wed, Sep 24, 2014 at 10:25:22AM +0200, Sumit Bose wrote:
> > > Hi,
> > >
> > > this patches contain the IPA provider part of
> > > https://fedorahosted.org/sssd/ticket/2375  as described in the FreeIPA
> > > design page
> > > http://www.freeipa.org/page/V4/Migrating_existing_environments_to_Trust
> > >
> > > Patches 0001, 0006 and 0007 contain new sysdb interfaces for views and
> > > overrides. 0002 and 0003 refactor some already existing calls.
> > >
> > > 0004 adds the initial support for views and overrides and reads the view
> > > name for the client from the FreeIPA server or sets it to 'default' when
> > > running in ipa-server-mode. It adds some new options which currently
> > > misses man page entries and are not added to the python config API. I
> > > did this on purpose for the time being because I wanted to see first if
> > > the list is complete or if some options ca be dropped.
> > >
> > > 0005 adds the request to get a specific override from the FreeIPA server
> > > and finally in 0008 the overrides are read during a request and saves or
> > > applied.
> > >
> > > This patches must be applied on to of the extdom patch I send yesterday.
> > > To add and manage views and override on the server side please have a
> > > look at Tomas's patch set on the freeipa-devel list
> > > (https://www.redhat.com/archives/freeipa-devel/2014-September/msg00336.html).
> > >
> > > bye,
> > > Sumit

Pavel, Jakub, thank you for the review.
> > > From dedbb5ae2faa13d396e84ce9911ab1e9c0246836 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose 
> > > Date: Tue, 16 Sep 2014 15:18:53 +0200
> > > Subject: [PATCH 1/8] sysdb: add sysdb_update_view_name()
> >
> > ACK
> >
> > A unit test for the functions would be nice in order to avoid decreasing
> > the code coverage of the sysdb module, but I realize we have a deadline.
> > We should file a ticket, though.

yes, I feel quite bad about the missing test, but my plan is to add them
after the dealine
> > It would also be nice to add a comment that it's not possible to have a
> > view w/o a name but we are trying to be on the safe side b/c name is not
> > MUST as we discussed on IRC with Sumit.

done
> > > From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose 
> > > Date: Tue, 16 Sep 2014 15:22:08 +0200
> > > Subject: [PATCH 2/8] Add sdap_deref_search_with_filter_send()
> >
> > I would prefer to also add a matching _done and _recv functions, even if
> > they were wrappers around existing functions. But tevent coding style is
> > important to follow, we already had bugs about reusing a single tevent
> > callbacks for multiple requests.

done
> > We can do it post-rebase, though. Just please file a ticket.
> > > From 3097c76ba9498b90df69935d24dc9ce55f602937 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose 
> > > Date: Fri, 19 Sep 2014 10:26:01 +0200
> > > Subject: [PATCH 3/8] IPA: make IPA ID context available to extdom
> > > client code
> >
> > ACK
> >
> > ...aaand I stopped here.
> >
> > That was Jakub's part of the review.
> >
> > Patch 4: IPA: add view support and get view name
> > >     if (ret != EOK) {
> > >        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to set SRV lookup
> > >
> > > plugin "
> > >                                    "[%d]: %s\n", ret,
> > >
> > > strerror(ret));
> > >        goto done;
> > >        if (ret == ENOENT) {
> > >            DEBUG(SSSDBG_CRIT_FAILURE,
> > >                  "Cannot find view name in the cache. " \
> > >                  "Will do online llokup later\n");
> >                                      ^ typo

fixed
> > >        } else {
> > >            DEBUG(SSSDBG_OP_FAILURE, "sysdb_get_view_name
> > >
> > > failed.\n");
> > >            goto done;
> > >        }
> > >    }
> > > ctx->ranges_search_bases = id_ctx->ipa_options->ranges_search_bases;
> > > ctx->host_search_bases = id_ctx->ipa_options->host_search_bases;
> > > ctx->configured_explicit = configured_explicit;
> >
> > This change seems to be unrelated to this patch. Shouldn't it be a
> > separate commit?

no, the view name is an attribute of the host object so we must be able
to search it.
> > Patch 5: views: add ipa_get_ad_override_send()
> >
> > Looks good, but can you add some more tracing debug messages there? For
> > example what view are we searching for and that an override was found.

done
> > Patch 6:
> > > const char *obj_attrs[] = { SYSDB_OBJECTCLASS,
> > >                          SYSDB_OVERRIDE_DN,
> > >                               NULL};
> >
> > Indentation.

fixed
> > >    if (ret != EOK) {
> > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > >              "Missing anchor in overide attributes.\n");
> >                                        ^ typo

fixed
> > >        ret = EINVAL;
> > >        goto done;
> > >    }
> > > if (ret != EOK) {
> > >    if (ret == ENOENT) {
> > >        DEBUG(SSSDBG_CRIT_FAILURE, "Object to override does not
> > >
> > > exists.\n");
> > >    } else {
> > >        DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_entry failed.\n");
> > >    }
> > >    goto done;
> > > }
> >
> > Can you use if (ret == ENOENT) else if (ret != EOK) here? I think it is
> > simpler.

I prefer it the other way, becasue I think it makes it more clear that
it is an error in all cases and only adds a special messages for the
ENOENT case.
> > > if (obj_override_dn != NULL) {
> > >    if (strcmp(obj_override_dn, override_dn_str) != 0) {
> > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > >              "Existing [%s] and new [%s] overid DN do not
> > >
> > > match.\n",
> >                                              ^ typo

fixed
> > >               obj_override_dn, override_dn_str);
> > >        ret = EINVAL;
> > >        goto done;
> > >    }
> > >    add_ref = false;
> > > }
> >
> > Patch 7: sysdb: sysdb_apply_default_override
> >
> > Ack.
> >
> > Patch 8: views: get overrides during user and group lookups
> >
> > Ack.
>
> BTW I checked out your repo. The patches couldn't be applied to master even
> with the extdom patch here on the list.

I send a new version of the extdom patch as well all rebase on master, I
hope it will work now.

New version attached.

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel