[PATCHES] Enable enumeration and cleanup tasks for subdomains

[Jakub Hrozek]

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.

[Jakub Hrozek]

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.

[Jakub Hrozek]

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

[Sumit Bose]

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

[Jakub Hrozek]

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

[Sumit Bose]

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

[Jakub Hrozek]

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

[Jakub Hrozek]

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

Pushed to master.