On Mon, Aug 26, 2013 at 04:16:54PM +0200, Sumit Bose wrote:
On Sat, Aug 24, 2013 at 06:42:21PM +0200, Jakub Hrozek wrote:
> On Thu, Aug 22, 2013 at 12:25:28PM +0200, Jakub Hrozek wrote:
> > On Thu, Aug 22, 2013 at 12:06:33PM +0200, Jakub Hrozek wrote:
> > > Hi,
> > >
> > > the attached patch implements enumeration and cleanup for the IPA server
> > > mode and also makes it possible to support enumeration and cleanup for
> > > other subdomains in general (we already have a request from one of our
> > > users to enumerate trusted AD domains).
> > >
> > > Some of the changes can also be leveraged to special-case enumeration
> > > requests in AD or IPA providers to e.g. download the master domain data
> > > before enumerating the domain for the first time.
> > >
> > > I hope the patches are split well to make it possible to review them
> > > easily. The bigger patches usually just move code around.
> >
> > I forgot to note two important things:
> >
> > 1) the subdomain enumeration setting is inherited from the master domain
> > enumeration. Is this OK or do we need to enumerate the AD trusted domain
> > automatically? I think that only a minority of the legacy clients
> > actually need enumeration, so as long as we document how enumeration
> > works in the server mode, we should be fine.
> >
> > 2) These patches currently do not optimize the enumeration which is what
> > the ticket initially talked about. The reason is that just enabling the
> > enumeration properly took a long time and also performance is only a
> > problem for the initial enumeration. The subsequent ones can leverage
> > lastUSN to only download deltas. Because the IPA server would mostly
> > stay online and running, I think the initial enumeration can be further
> > optimized in 1.11.1. Sumit came up with some idea when he visited Brno,
> > so I'll work on that next week.
>
> I found a couple of bugs (tevent_req_error not terminated with return,
> uninitialized variable etc). New patches are attached.
>
> Please note that these patches depend on the patch with subject "DB:
> Update sss_domain_info with new updated data".
Please find my comments below. The patches work as expected. So far I've
only tested against a small amount of users and groups but I'll try to
test with a larger domain as well.
bye,
Sumit
Thanks for the review.
> Subject: [PATCH 01/11] LDAP: Add enum_{users,groups} to follow
the tevent_req
> style
>
...
> @@ -296,22 +298,12 @@ static void ldap_id_enum_users_done(struct tevent_req
*subreq)
> struct tevent_req);
> struct global_enum_state *state = tevent_req_data(req,
> struct global_enum_state);
> - enum tevent_req_state tstate;
> uint64_t err = 0;
> int ret, dp_error = DP_ERR_FATAL;
>
> - if (tevent_req_is_error(subreq, &tstate, &err)) {
> - if (tstate != TEVENT_REQ_USER_ERROR) {
> - err = EIO;
> - }
> -
> - if (err == ENOENT) {
> - err = EOK;
> - }
> - }
> + err = enum_users_recv(subreq);
> talloc_zfree(subreq);
> -
> - if (err != EOK) {
> + if (err != EOK && err != ENOENT) {
> /* We call sdap_id_op_done only on error
> * as the connection is reused by groups enumeration */
> ret = sdap_id_op_done(state->op, (int)err, &dp_error);
> @@ -351,21 +343,11 @@ static void ldap_id_enum_groups_done(struct tevent_req
*subreq)
> struct tevent_req);
> struct global_enum_state *state = tevent_req_data(req,
> struct global_enum_state);
> - enum tevent_req_state tstate;
> uint64_t err = 0;
> int ret, dp_error = DP_ERR_FATAL;
>
> - if (tevent_req_is_error(subreq, &tstate, &err)) {
> - if (tstate != TEVENT_REQ_USER_ERROR) {
> - err = EIO;
> - }
> -
> - if (err == ENOENT) {
> - err = EOK;
> - }
> - }
> + err = enum_groups_recv(subreq);
> talloc_zfree(subreq);
> -
> if (err != EOK) {
I wonder how ENOENT is handled or of it needs to be handled at all? In
the old code for users and groups err was set to EOK is it was ENOENT.
Now you check for (err != EOK && err != ENOENT) for users but only (err
!= EOK) for groups. In a later patch you drop the ENOENT check for users
as well.
Can enum_users_recv() or enum_groups_recv() return ENOENT? If yes and if
it means the either no users or groups were found I think we should not
treat it as an error.
You're right, the attached patches ignore ENOENT from enum_users_recv,
enum_groups_recv and enum_services_recv. The enumeration request itself
doesn't report ENOENT back to the caller.
> /* We call sdap_id_op_done only on error
> * as the connection is reused by services enumeration */
> @@ -632,6 +614,13 @@ static void enum_users_op_done(struct tevent_req *subreq)
> tevent_req_done(req);
> }
>
> Subject: [PATCH 02/11] LDAP: Remove unused constant
ACK
No changes to this patch.
> Subject: [PATCH 03/11] LDAP: Move the ldap enum request to its own reusable
> module
ACK
ENOENT handled same as after patch #1.
> Subject: [PATCH 04/11] LDAP: Convert enumeration to the ptask API
ACK
No changes to this patch.
> Subject: [PATCH 05/11] LDAP: Make cleanup synchronous
ACK
No changes to this patch.
> Subject: [PATCH 06/11] LDAP: Make the cleanup task reusable for
subdomains
ACK
No changes to this patch.
> Subject: [PATCH 07/11] LDAP: Make sdap_id_setup_tasks reusable
for subdomains
ACK
No changes to this patch.
> Subject: [PATCH 08/11] SYSDB: Store enumerate flag for subdomain
ACK
No changes to this patch.
> Subject: [PATCH 09/11] Read enumerate state for subdomains from cache
ACK
No changes to this patch.
> Subject: [PATCH 10/11] IPA: enable enumeration if parent domain enumerates in
> server mode
...
>
> static errno_t ipa_subdom_store(struct sss_domain_info *domain,
> struct sdap_idmap_ctx *sdap_idmap_ctx,
> - struct sysdb_attrs *attrs)
> + struct sysdb_attrs *attrs,
> + bool server_mode)
I think it would be better to pass just the enumerate flag here instead
of the server_mode flag and make the decision if enumeration should be
used or not in the caller.
> {
> TALLOC_CTX *tmp_ctx;
> const char *name;
> @@ -413,6 +432,7 @@ static errno_t ipa_subdom_store(struct sss_domain_info *domain,
> const char *id;
> int ret;
> bool mpg;
> + bool enumerate;
>
> tmp_ctx = talloc_new(domain);
> if (tmp_ctx == NULL) {
> @@ -445,8 +465,14 @@ static errno_t ipa_subdom_store(struct sss_domain_info
*domain,
>
> mpg = sdap_idmap_domain_has_algorithmic_mapping(sdap_idmap_ctx, id);
>
> + if (server_mode == true) {
> + enumerate = domain->enumerate;
> + } else {
> + enumerate = false;
> + }
> +
As said above I would prefer that the decision is made in the caller.
Additionally I do not like the logic, because it forces to use
enumerating for the IPA domain as well which in my opinion is only of
limited use because since we are in server mode sssd is running on the
IPA server.
A separate option like ipa_server_mode_enum with values like 'all',
'none' or a list of subdomains which should be enumerated would help.
Or, and I think I like this better even if it requires some work on the
IPA side as well, read a flag from the IPA trust object like
ipaEnumerate which indicates if the domain should be enumerated or not.
OK, there is a new patch (#10) that adds a subdomain_enumerate option to
the domain section. Currently it defaults to "none" as without slapi-nis
taking advantage of the enumeration, there can't even be a consumer. Can
you file a freeipa ticket so that they implement the ipaEnumerate flag?
I agree it would be the best way. Then we can change the default of the
SSSD option to be "autodetect".
> ret = sysdb_subdomain_store(domain->sysdb, name, realm, flat,
> - id, mpg, false);
> + id, mpg, enumerate);
> if (ret) {
> DEBUG(SSSDBG_OP_FAILURE, ("sysdb_subdomain_store failed.\n"));
> goto done;
> Subject: [PATCH 11/11] NSS: Descend into subdomains if enumerate=true
ACK
Since enumeration of domain and subdomain is configured separately now,
I also amended the check if any domain enumerates to be able to recurse
to subdomains.