On Mon, Sep 29, 2014 at 02:52:10PM +0200, Pavel Březina wrote: > On 09/29/2014 01:01 PM, Sumit Bose wrote: > >On Mon, Sep 29, 2014 at 11:52:52AM +0200, Pavel Březina wrote: > >>On 09/26/2014 04:36 PM, Sumit Bose wrote: > >>>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 <sb...@redhat.com> > >>>>>>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 > >> > >>Ack. > >> > >>>>> > >>>>>>From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001 > >>>>>>From: Sumit Bose <sb...@redhat.com> > >>>>>>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. > >> > >>>+ if (sdap_is_control_supported(sh, LDAP_CONTROL_X_DEREF)) { > >>>+ DEBUG(SSSDBG_TRACE_INTERNAL, "Server supports OpenLDAP deref\n"); > >>>+ state->deref_type = SDAP_DEREF_OPENLDAP; > >>>+ > >>>+ subreq = sdap_x_deref_search_send(state, ev, opts, sh, > >>>search_base, > >>>+ filter, deref_attr, attrs, maps, > >>>+ num_maps, timeout); > >>>+ if (!subreq) { > >>>+ DEBUG(SSSDBG_OP_FAILURE, "Cannot start OpenLDAP deref > >>>search\n"); > >>>+ goto fail; > >>>+ } > >>>+ } else { > >>>+ DEBUG(SSSDBG_OP_FAILURE, > >>>+ "Server does not support any known deref method!\n"); > >>>+ goto fail; > >>>+ } > >> > >> > >>Would it be possible to use sdap_has_deref_support() instead of manually > >>testing the support for OpenLDAP deref or you can't use ASQ for some reason? > > > >no, afaik ASQ can only do base searches and follow the references in the > >given DN, so adding a filter won't work. > > > >> > >>> > >>>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 <sb...@redhat.com> > >>>>>>Date: Fri, 19 Sep 2014 10:26:01 +0200 > >>>>>>Subject: [PATCH 3/8] IPA: make IPA ID context available to extdom > >>>>>>client code > >>>>> > >>>>>ACK > >> > >>Still 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. > >> > >>Ok. Typo is still there, I'm sending a trivial patch. Feel free to squash > >>it. > > > >squashed in. > > > >> > >>Ack. > >> > >>> > >>>>> > >>>>>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 > >> > >>Ack. > >> > >>> > >>>>> > >>>>>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. > >> > >>Ok. > >> > >>> > >>>>> > >>>>>>+ 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; > >>>>> > >>>+ } > >> > >>>+ case SYSDB_MEMBER_GROUP: > >>>+ ret = ldb_msg_add_string(msg, SYSDB_OBJECTCLASS, > >>>+ SYSDB_OVERRIDE_USER_CLASS); > >>>+ break; > >> > >>Shouldn't it say SYSDB_OVERRIDE_GROUP_CLASS? > > > >D'oh, fixed. > > > >> > >>>>> > >>>>>Patch 7: sysdb: sysdb_apply_default_override > >>>>> > >>>>>Ack. > >> > >>Ack stays, but can you clarify what is the intended use case of this > >>function in the commit message please? > > > >Comment added. > > > >> > >>>>> > >>>>>Patch 8: views: get overrides during user and group lookups > >>>>> > >>>>>Ack. > >> > >>> if (override_attrs != NULL) { > >>> ret = sysdb_apply_default_override(state->user_dom->sysdb, > >>> override_attrs, > >>> state->obj_msg->dn); > >>> if (ret != EOK) { > >>> DEBUG(SSSDBG_OP_FAILURE, "sysdb_apply_default_override > >>> failed.\n"); > >>> goto fail; > >>> } > >>> } > >> > >>I thought the sysdb_apply_default_override is used to override original > >>object in sysdb with information from the default view. This looks like it > >>is overwritten with data from whatever view we are using. > > > >This code path is in ipa-server-mode where the default view is > >hardcoded/fixed by definition. I added a comment to make this clear. > > > >> > >>>> > >>>>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 > >> > >>I get this warning: > >>/home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c: In function > >>'ipa_get_ad_override_done': > >>/home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c:207:9: error: > >>format '%d' expects argument of type 'int', but argument 6 has type 'size_t' > >>[-Werror=format] > > > >Your fix is squashed in. > > > >Thank you for the review. New version attached. > > > >bye, > >Sumit > > Hi, > I'm sending two more trivial debug patches to squash in. Otherwise its an > code wise ack. I'm running some tests now. >
Thank you. Jakub, in case there are no other issues, can you add them when committing the patches, or shall I send a new round? bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
