[SSSD] [PATCHES] Enable enumeration and cleanup tasks for subdomains

Sumit Bose sbose at redhat.com
Mon Aug 26 14:16:54 UTC 2013


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

> 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.

>          /* 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

> Subject: [PATCH 03/11] LDAP: Move the ldap enum request to its own reusable
>  module
ACK

> Subject: [PATCH 04/11] LDAP: Convert enumeration to the ptask API
ACK

> Subject: [PATCH 05/11] LDAP: Make cleanup synchronous
ACK

> Subject: [PATCH 06/11] LDAP: Make the cleanup task reusable for subdomains
ACK

> Subject: [PATCH 07/11] LDAP: Make sdap_id_setup_tasks reusable for subdomains
ACK

> Subject: [PATCH 08/11] SYSDB: Store enumerate flag for subdomain
ACK

> Subject: [PATCH 09/11] Read enumerate state for subdomains from cache
ACK

> 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.

>      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



More information about the sssd-devel mailing list