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.
From 540b7c68aac03cc2b7bde00c8e36abdac5f14582 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
Date: Mon, 29 Sep 2014 13:54:21 +0200
Subject: [PATCH 1/2] fix debug message - squash into "IPA: add view support
and get view name"
---
src/providers/ipa/ipa_subdomains.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c
index 50f4921d3e3390bf67527f8a3b96d1aad8b83b97..35cbf4b51fea60e2915236b72cc2677a53a259d9 100644
--- a/src/providers/ipa/ipa_subdomains.c
+++ b/src/providers/ipa/ipa_subdomains.c
@@ -1012,7 +1012,7 @@ static void ipa_get_view_name_done(struct tevent_req *req)
if (ret == EAGAIN) {
return;
} else if (ret == EOK) {
- DEBUG(SSSDBG_TRACE_ALL, "No view found, using default");
+ DEBUG(SSSDBG_TRACE_ALL, "No view found, using default.\n");
view_name = SYSDB_DEFAULT_VIEW_NAME;
} else {
goto done;
--
1.7.11.7
From 4de696ec9172e12cfd3c6e1bd19eed09f62aee39 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
Date: Mon, 29 Sep 2014 13:55:05 +0200
Subject: [PATCH 2/2] decrease debug level - squash into "IPA: add view
support and get view name"
---
src/providers/ipa/ipa_subdomains.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c
index 35cbf4b51fea60e2915236b72cc2677a53a259d9..d9a7b31c6ce91eea496ed9138bdf776b4651ba32 100644
--- a/src/providers/ipa/ipa_subdomains.c
+++ b/src/providers/ipa/ipa_subdomains.c
@@ -1012,7 +1012,7 @@ static void ipa_get_view_name_done(struct tevent_req *req)
if (ret == EAGAIN) {
return;
} else if (ret == EOK) {
- DEBUG(SSSDBG_TRACE_ALL, "No view found, using default.\n");
+ DEBUG(SSSDBG_TRACE_FUNC, "No view found, using default.\n");
view_name = SYSDB_DEFAULT_VIEW_NAME;
} else {
goto done;
@@ -1031,7 +1031,7 @@ static void ipa_get_view_name_done(struct tevent_req *req)
}
}
- DEBUG(SSSDBG_TRACE_ALL, "Found view name [%s].\n", view_name);
+ DEBUG(SSSDBG_TRACE_FUNC, "Found view name [%s].\n", view_name);
if (ctx->sd_ctx->id_ctx->view_name != NULL
&& strcmp(ctx->sd_ctx->id_ctx->view_name, view_name) != 0) {
--
1.7.11.7
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
