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 <sbose(a)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 <sbose(a)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 <sbose(a)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