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

Jakub Hrozek jhrozek at redhat.com
Wed Aug 28 15:50:36 UTC 2013


On Wed, Aug 28, 2013 at 05:35:22PM +0200, Sumit Bose wrote:
> On Wed, Aug 28, 2013 at 11:59:59AM +0200, Jakub Hrozek wrote:
> > 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.
> 
> ACK. I do not like that sd_enumerate is put into the sss_domain_info
> struct but since I currently do not have a better idea I think it can be
> fixed later.
> 
> bye,
> Sumit

I don't like it either. Actually my first implementation always read the
attribute from confdb and split the list, avoiding having to store the
list completely. But then I didn't like that I had to pass the confdb
context around.



More information about the sssd-devel mailing list