This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to the 1.5.x branch. It makes one major change: 1.5.x does not have support for deref/ASQ searches, so the deref lookup for hostgroups has been replaced by parallel LDAP "base" searches.
I'm attaching two patches. One is the complete patch to backport, the other is just an interdiff of the changes from Jan's original patch to support the parallel lookups.
I'm still in the process of testing this patch, but I wanted to get it out for review in the meantime.
On Mon, 2012-08-13 at 15:03 -0400, Stephen Gallagher wrote:
This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to the 1.5.x branch. It makes one major change: 1.5.x does not have support for deref/ASQ searches, so the deref lookup for hostgroups has been replaced by parallel LDAP "base" searches.
I'm attaching two patches. One is the complete patch to backport, the other is just an interdiff of the changes from Jan's original patch to support the parallel lookups.
I'm still in the process of testing this patch, but I wanted to get it out for review in the meantime.
Ok, during testing I found one bug where I forgot to initialize a loop-control/array-index variable. Oops.
My basic smoketesting is now complete. Both modes (srchost enabled and disabled) are working as expected for a small set of hosts. I don't have the infrastructure to test large numbers, but I suspect the only difference will be performance, not function.
New patches attached. As before, one is the complete patch ("Add ipa_hbac_support_srchost option to IPA provider") and the other is just the changes from Jan Zeleny's original patch (to simplify the review process).
On Mon, Aug 13, 2012 at 07:27:42PM -0400, Stephen Gallagher wrote:
On Mon, 2012-08-13 at 15:03 -0400, Stephen Gallagher wrote:
This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to the 1.5.x branch. It makes one major change: 1.5.x does not have support for deref/ASQ searches, so the deref lookup for hostgroups has been replaced by parallel LDAP "base" searches.
I'm attaching two patches. One is the complete patch to backport, the other is just an interdiff of the changes from Jan's original patch to support the parallel lookups.
I'm still in the process of testing this patch, but I wanted to get it out for review in the meantime.
Ok, during testing I found one bug where I forgot to initialize a loop-control/array-index variable. Oops.
My basic smoketesting is now complete. Both modes (srchost enabled and disabled) are working as expected for a small set of hosts. I don't have the infrastructure to test large numbers, but I suspect the only difference will be performance, not function.
New patches attached. As before, one is the complete patch ("Add ipa_hbac_support_srchost option to IPA provider") and the other is just the changes from Jan Zeleny's original patch (to simplify the review process).
I don't understand this part:
- if (queue_len == 0) {
/* This host is not in any hostgroups */ret = ENOMEM;goto error;- }
This seems to be preventing me from logging into a host that is not part of any host group:
[ipa_hbac_hostgroup_info_done] (3): Error [12][Cannot allocate memory] [sdap_id_op_done] (9): releasing operation connection
The rest looks good. Thank you for the interdiff, indeed it made the review easier.
On Tue, 2012-08-14 at 18:12 +0200, Jakub Hrozek wrote:
On Mon, Aug 13, 2012 at 07:27:42PM -0400, Stephen Gallagher wrote:
On Mon, 2012-08-13 at 15:03 -0400, Stephen Gallagher wrote:
This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to the 1.5.x branch. It makes one major change: 1.5.x does not have support for deref/ASQ searches, so the deref lookup for hostgroups has been replaced by parallel LDAP "base" searches.
I'm attaching two patches. One is the complete patch to backport, the other is just an interdiff of the changes from Jan's original patch to support the parallel lookups.
I'm still in the process of testing this patch, but I wanted to get it out for review in the meantime.
Ok, during testing I found one bug where I forgot to initialize a loop-control/array-index variable. Oops.
My basic smoketesting is now complete. Both modes (srchost enabled and disabled) are working as expected for a small set of hosts. I don't have the infrastructure to test large numbers, but I suspect the only difference will be performance, not function.
New patches attached. As before, one is the complete patch ("Add ipa_hbac_support_srchost option to IPA provider") and the other is just the changes from Jan Zeleny's original patch (to simplify the review process).
I don't understand this part:
- if (queue_len == 0) {
/* This host is not in any hostgroups */ret = ENOMEM;goto error;- }
This seems to be preventing me from logging into a host that is not part of any host group:
[ipa_hbac_hostgroup_info_done] (3): Error [12][Cannot allocate memory] [sdap_id_op_done] (9): releasing operation connection
Thanks, there were two bugs there. The first was that this should have been ENOENT, not ENOMEM. The second was that I forgot to special-case ENOENT in ipa_hbac_hostgroup_info_done(). Both are now fixed.
New patches attached.
The rest looks good. Thank you for the interdiff, indeed it made the review easier.
On Tue, Aug 14, 2012 at 04:47:04PM -0400, Stephen Gallagher wrote:
On Tue, 2012-08-14 at 18:12 +0200, Jakub Hrozek wrote:
On Mon, Aug 13, 2012 at 07:27:42PM -0400, Stephen Gallagher wrote:
On Mon, 2012-08-13 at 15:03 -0400, Stephen Gallagher wrote:
This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to the 1.5.x branch. It makes one major change: 1.5.x does not have support for deref/ASQ searches, so the deref lookup for hostgroups has been replaced by parallel LDAP "base" searches.
I'm attaching two patches. One is the complete patch to backport, the other is just an interdiff of the changes from Jan's original patch to support the parallel lookups.
I'm still in the process of testing this patch, but I wanted to get it out for review in the meantime.
Ok, during testing I found one bug where I forgot to initialize a loop-control/array-index variable. Oops.
My basic smoketesting is now complete. Both modes (srchost enabled and disabled) are working as expected for a small set of hosts. I don't have the infrastructure to test large numbers, but I suspect the only difference will be performance, not function.
New patches attached. As before, one is the complete patch ("Add ipa_hbac_support_srchost option to IPA provider") and the other is just the changes from Jan Zeleny's original patch (to simplify the review process).
I don't understand this part:
- if (queue_len == 0) {
/* This host is not in any hostgroups */ret = ENOMEM;goto error;- }
This seems to be preventing me from logging into a host that is not part of any host group:
[ipa_hbac_hostgroup_info_done] (3): Error [12][Cannot allocate memory] [sdap_id_op_done] (9): releasing operation connection
Thanks, there were two bugs there. The first was that this should have been ENOENT, not ENOMEM. The second was that I forgot to special-case ENOENT in ipa_hbac_hostgroup_info_done(). Both are now fixed.
New patches attached.
The rest looks good. Thank you for the interdiff, indeed it made the review easier.
There's one more problem - if the host is not a member of any host group, it has no memberof attribute. However, this call would add an empty attribute in that case:
- ret = sysdb_attrs_get_el(state->hosts[0],
SYSDB_ORIG_MEMBEROF,&parent_el);- if (ret != EOK) goto error;
and later saving that host would fail:
[sysdb_store_custom] (1): Failed to store custmo entry: Constraint violation(19)[attribute originalMemberOf on name=HOSTNAME,cn=hbac_hosts,cn=custom,cn=idm.lab.bos.redhat.com,cn=sysdb specified, but with 0 values (illegal)] [sysdb_store_custom] (6): Error: 14 (Bad address) cancel ldb transaction (nesting: 2) [ipa_hbac_save_list] (1): sysdb_store_custom failed. [ipa_hbac_sysdb_save] (1): Could not save hbac_hosts. [14][Bad address]
Does anyone remember why does sysdb_attrs_get_el internally create the attribute if it doesn't exist? To ease error checking? I think we should either change it or create another getter that wouldn't create the attribute automatically.
On Wed, 2012-08-15 at 11:12 +0200, Jakub Hrozek wrote:
On Tue, Aug 14, 2012 at 04:47:04PM -0400, Stephen Gallagher wrote:
On Tue, 2012-08-14 at 18:12 +0200, Jakub Hrozek wrote:
On Mon, Aug 13, 2012 at 07:27:42PM -0400, Stephen Gallagher wrote:
On Mon, 2012-08-13 at 15:03 -0400, Stephen Gallagher wrote:
This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to the 1.5.x branch. It makes one major change: 1.5.x does not have support for deref/ASQ searches, so the deref lookup for hostgroups has been replaced by parallel LDAP "base" searches.
I'm attaching two patches. One is the complete patch to backport, the other is just an interdiff of the changes from Jan's original patch to support the parallel lookups.
I'm still in the process of testing this patch, but I wanted to get it out for review in the meantime.
Ok, during testing I found one bug where I forgot to initialize a loop-control/array-index variable. Oops.
My basic smoketesting is now complete. Both modes (srchost enabled and disabled) are working as expected for a small set of hosts. I don't have the infrastructure to test large numbers, but I suspect the only difference will be performance, not function.
New patches attached. As before, one is the complete patch ("Add ipa_hbac_support_srchost option to IPA provider") and the other is just the changes from Jan Zeleny's original patch (to simplify the review process).
I don't understand this part:
- if (queue_len == 0) {
/* This host is not in any hostgroups */ret = ENOMEM;goto error;- }
This seems to be preventing me from logging into a host that is not part of any host group:
[ipa_hbac_hostgroup_info_done] (3): Error [12][Cannot allocate memory] [sdap_id_op_done] (9): releasing operation connection
Thanks, there were two bugs there. The first was that this should have been ENOENT, not ENOMEM. The second was that I forgot to special-case ENOENT in ipa_hbac_hostgroup_info_done(). Both are now fixed.
New patches attached.
The rest looks good. Thank you for the interdiff, indeed it made the review easier.
There's one more problem - if the host is not a member of any host group, it has no memberof attribute. However, this call would add an empty attribute in that case:
- ret = sysdb_attrs_get_el(state->hosts[0],
SYSDB_ORIG_MEMBEROF,&parent_el);- if (ret != EOK) goto error;
and later saving that host would fail:
[sysdb_store_custom] (1): Failed to store custmo entry: Constraint violation(19)[attribute originalMemberOf on name=HOSTNAME,cn=hbac_hosts,cn=custom,cn=idm.lab.bos.redhat.com,cn=sysdb specified, but with 0 values (illegal)] [sysdb_store_custom] (6): Error: 14 (Bad address) cancel ldb transaction (nesting: 2) [ipa_hbac_save_list] (1): sysdb_store_custom failed. [ipa_hbac_sysdb_save] (1): Could not save hbac_hosts. [14][Bad address]
Does anyone remember why does sysdb_attrs_get_el internally create the attribute if it doesn't exist? To ease error checking? I think we should either change it or create another getter that wouldn't create the attribute automatically.
I don't remember the reasoning either, but I don't want to go through the hassle of trying to change that during this backport. I added a new patch (that will work on 1.5.x and master) that just exposes the internal get_el() routine that takes an option to create or not create the value. I then changed the above so it detects the ENOENT case appropriately.
On Wed, Aug 15, 2012 at 01:45:35PM -0400, Stephen Gallagher wrote:
On Wed, 2012-08-15 at 11:12 +0200, Jakub Hrozek wrote:
On Tue, Aug 14, 2012 at 04:47:04PM -0400, Stephen Gallagher wrote:
On Tue, 2012-08-14 at 18:12 +0200, Jakub Hrozek wrote:
On Mon, Aug 13, 2012 at 07:27:42PM -0400, Stephen Gallagher wrote:
On Mon, 2012-08-13 at 15:03 -0400, Stephen Gallagher wrote:
This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to the 1.5.x branch. It makes one major change: 1.5.x does not have support for deref/ASQ searches, so the deref lookup for hostgroups has been replaced by parallel LDAP "base" searches.
I'm attaching two patches. One is the complete patch to backport, the other is just an interdiff of the changes from Jan's original patch to support the parallel lookups.
I'm still in the process of testing this patch, but I wanted to get it out for review in the meantime.
Ok, during testing I found one bug where I forgot to initialize a loop-control/array-index variable. Oops.
My basic smoketesting is now complete. Both modes (srchost enabled and disabled) are working as expected for a small set of hosts. I don't have the infrastructure to test large numbers, but I suspect the only difference will be performance, not function.
New patches attached. As before, one is the complete patch ("Add ipa_hbac_support_srchost option to IPA provider") and the other is just the changes from Jan Zeleny's original patch (to simplify the review process).
I don't understand this part:
- if (queue_len == 0) {
/* This host is not in any hostgroups */ret = ENOMEM;goto error;- }
This seems to be preventing me from logging into a host that is not part of any host group:
[ipa_hbac_hostgroup_info_done] (3): Error [12][Cannot allocate memory] [sdap_id_op_done] (9): releasing operation connection
Thanks, there were two bugs there. The first was that this should have been ENOENT, not ENOMEM. The second was that I forgot to special-case ENOENT in ipa_hbac_hostgroup_info_done(). Both are now fixed.
New patches attached.
The rest looks good. Thank you for the interdiff, indeed it made the review easier.
There's one more problem - if the host is not a member of any host group, it has no memberof attribute. However, this call would add an empty attribute in that case:
- ret = sysdb_attrs_get_el(state->hosts[0],
SYSDB_ORIG_MEMBEROF,&parent_el);- if (ret != EOK) goto error;
and later saving that host would fail:
[sysdb_store_custom] (1): Failed to store custmo entry: Constraint violation(19)[attribute originalMemberOf on name=HOSTNAME,cn=hbac_hosts,cn=custom,cn=idm.lab.bos.redhat.com,cn=sysdb specified, but with 0 values (illegal)] [sysdb_store_custom] (6): Error: 14 (Bad address) cancel ldb transaction (nesting: 2) [ipa_hbac_save_list] (1): sysdb_store_custom failed. [ipa_hbac_sysdb_save] (1): Could not save hbac_hosts. [14][Bad address]
Does anyone remember why does sysdb_attrs_get_el internally create the attribute if it doesn't exist? To ease error checking? I think we should either change it or create another getter that wouldn't create the attribute automatically.
I don't remember the reasoning either, but I don't want to go through the hassle of trying to change that during this backport. I added a new patch (that will work on 1.5.x and master) that just exposes the internal get_el() routine that takes an option to create or not create the value. I then changed the above so it detects the ENOENT case appropriately.
Sure, this backport is not the right place for a big change to the sysdb API.
This patchset works for me.
Ack.
On Thu, 2012-08-16 at 18:16 +0200, Jakub Hrozek wrote:
On Wed, Aug 15, 2012 at 01:45:35PM -0400, Stephen Gallagher wrote:
On Wed, 2012-08-15 at 11:12 +0200, Jakub Hrozek wrote:
On Tue, Aug 14, 2012 at 04:47:04PM -0400, Stephen Gallagher wrote:
On Tue, 2012-08-14 at 18:12 +0200, Jakub Hrozek wrote:
On Mon, Aug 13, 2012 at 07:27:42PM -0400, Stephen Gallagher wrote:
On Mon, 2012-08-13 at 15:03 -0400, Stephen Gallagher wrote: > This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to > the 1.5.x branch. It makes one major change: 1.5.x does not have support > for deref/ASQ searches, so the deref lookup for hostgroups has been > replaced by parallel LDAP "base" searches. > > I'm attaching two patches. One is the complete patch to backport, the > other is just an interdiff of the changes from Jan's original patch to > support the parallel lookups. > > I'm still in the process of testing this patch, but I wanted to get it > out for review in the meantime.
Ok, during testing I found one bug where I forgot to initialize a loop-control/array-index variable. Oops.
My basic smoketesting is now complete. Both modes (srchost enabled and disabled) are working as expected for a small set of hosts. I don't have the infrastructure to test large numbers, but I suspect the only difference will be performance, not function.
New patches attached. As before, one is the complete patch ("Add ipa_hbac_support_srchost option to IPA provider") and the other is just the changes from Jan Zeleny's original patch (to simplify the review process).
I don't understand this part:
- if (queue_len == 0) {
/* This host is not in any hostgroups */ret = ENOMEM;goto error;- }
This seems to be preventing me from logging into a host that is not part of any host group:
[ipa_hbac_hostgroup_info_done] (3): Error [12][Cannot allocate memory] [sdap_id_op_done] (9): releasing operation connection
Thanks, there were two bugs there. The first was that this should have been ENOENT, not ENOMEM. The second was that I forgot to special-case ENOENT in ipa_hbac_hostgroup_info_done(). Both are now fixed.
New patches attached.
The rest looks good. Thank you for the interdiff, indeed it made the review easier.
There's one more problem - if the host is not a member of any host group, it has no memberof attribute. However, this call would add an empty attribute in that case:
- ret = sysdb_attrs_get_el(state->hosts[0],
SYSDB_ORIG_MEMBEROF,&parent_el);- if (ret != EOK) goto error;
and later saving that host would fail:
[sysdb_store_custom] (1): Failed to store custmo entry: Constraint violation(19)[attribute originalMemberOf on name=HOSTNAME,cn=hbac_hosts,cn=custom,cn=idm.lab.bos.redhat.com,cn=sysdb specified, but with 0 values (illegal)] [sysdb_store_custom] (6): Error: 14 (Bad address) cancel ldb transaction (nesting: 2) [ipa_hbac_save_list] (1): sysdb_store_custom failed. [ipa_hbac_sysdb_save] (1): Could not save hbac_hosts. [14][Bad address]
Does anyone remember why does sysdb_attrs_get_el internally create the attribute if it doesn't exist? To ease error checking? I think we should either change it or create another getter that wouldn't create the attribute automatically.
I don't remember the reasoning either, but I don't want to go through the hassle of trying to change that during this backport. I added a new patch (that will work on 1.5.x and master) that just exposes the internal get_el() routine that takes an option to create or not create the value. I then changed the above so it detects the ENOENT case appropriately.
Sure, this backport is not the right place for a big change to the sysdb API.
This patchset works for me.
Ack.
One minor correction (for master branch only). I realized this morning that on the master branch, there was one more place that needed to be converted from sysdb_attrs_get_el_int() to sysdb_attrs_get_el_ext(). Please apply the attached patch to master instead. The other patch is correct for sssd-1-5.
On Fri, Aug 17, 2012 at 08:41:59AM -0400, Stephen Gallagher wrote:
On Thu, 2012-08-16 at 18:16 +0200, Jakub Hrozek wrote:
On Wed, Aug 15, 2012 at 01:45:35PM -0400, Stephen Gallagher wrote:
On Wed, 2012-08-15 at 11:12 +0200, Jakub Hrozek wrote:
On Tue, Aug 14, 2012 at 04:47:04PM -0400, Stephen Gallagher wrote:
On Tue, 2012-08-14 at 18:12 +0200, Jakub Hrozek wrote:
On Mon, Aug 13, 2012 at 07:27:42PM -0400, Stephen Gallagher wrote: > On Mon, 2012-08-13 at 15:03 -0400, Stephen Gallagher wrote: > > This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to > > the 1.5.x branch. It makes one major change: 1.5.x does not have support > > for deref/ASQ searches, so the deref lookup for hostgroups has been > > replaced by parallel LDAP "base" searches. > > > > I'm attaching two patches. One is the complete patch to backport, the > > other is just an interdiff of the changes from Jan's original patch to > > support the parallel lookups. > > > > I'm still in the process of testing this patch, but I wanted to get it > > out for review in the meantime. > > Ok, during testing I found one bug where I forgot to initialize a > loop-control/array-index variable. Oops. > > My basic smoketesting is now complete. Both modes (srchost enabled and > disabled) are working as expected for a small set of hosts. I don't have > the infrastructure to test large numbers, but I suspect the only > difference will be performance, not function. > > New patches attached. As before, one is the complete patch ("Add > ipa_hbac_support_srchost option to IPA provider") and the other is just > the changes from Jan Zeleny's original patch (to simplify the review > process).
I don't understand this part:
> + if (queue_len == 0) { > + /* This host is not in any hostgroups */ > + ret = ENOMEM; > + goto error; > + }
This seems to be preventing me from logging into a host that is not part of any host group:
[ipa_hbac_hostgroup_info_done] (3): Error [12][Cannot allocate memory] [sdap_id_op_done] (9): releasing operation connection
Thanks, there were two bugs there. The first was that this should have been ENOENT, not ENOMEM. The second was that I forgot to special-case ENOENT in ipa_hbac_hostgroup_info_done(). Both are now fixed.
New patches attached.
The rest looks good. Thank you for the interdiff, indeed it made the review easier.
There's one more problem - if the host is not a member of any host group, it has no memberof attribute. However, this call would add an empty attribute in that case:
- ret = sysdb_attrs_get_el(state->hosts[0],
SYSDB_ORIG_MEMBEROF,&parent_el);- if (ret != EOK) goto error;
and later saving that host would fail:
[sysdb_store_custom] (1): Failed to store custmo entry: Constraint violation(19)[attribute originalMemberOf on name=HOSTNAME,cn=hbac_hosts,cn=custom,cn=idm.lab.bos.redhat.com,cn=sysdb specified, but with 0 values (illegal)] [sysdb_store_custom] (6): Error: 14 (Bad address) cancel ldb transaction (nesting: 2) [ipa_hbac_save_list] (1): sysdb_store_custom failed. [ipa_hbac_sysdb_save] (1): Could not save hbac_hosts. [14][Bad address]
Does anyone remember why does sysdb_attrs_get_el internally create the attribute if it doesn't exist? To ease error checking? I think we should either change it or create another getter that wouldn't create the attribute automatically.
I don't remember the reasoning either, but I don't want to go through the hassle of trying to change that during this backport. I added a new patch (that will work on 1.5.x and master) that just exposes the internal get_el() routine that takes an option to create or not create the value. I then changed the above so it detects the ENOENT case appropriately.
Sure, this backport is not the right place for a big change to the sysdb API.
This patchset works for me.
Ack.
One minor correction (for master branch only). I realized this morning that on the master branch, there was one more place that needed to be converted from sysdb_attrs_get_el_int() to sysdb_attrs_get_el_ext(). Please apply the attached patch to master instead. The other patch is correct for sssd-1-5.
There was one more case that needed converting in sysdb_attrs_get_uint16_t. I've fixed that and pushed the corrected patch.
The interdiff is attached.
On Tue, Aug 21, 2012 at 12:28:22PM +0200, Jakub Hrozek wrote:
On Fri, Aug 17, 2012 at 08:41:59AM -0400, Stephen Gallagher wrote:
On Thu, 2012-08-16 at 18:16 +0200, Jakub Hrozek wrote:
On Wed, Aug 15, 2012 at 01:45:35PM -0400, Stephen Gallagher wrote:
On Wed, 2012-08-15 at 11:12 +0200, Jakub Hrozek wrote:
On Tue, Aug 14, 2012 at 04:47:04PM -0400, Stephen Gallagher wrote:
On Tue, 2012-08-14 at 18:12 +0200, Jakub Hrozek wrote: > On Mon, Aug 13, 2012 at 07:27:42PM -0400, Stephen Gallagher wrote: > > On Mon, 2012-08-13 at 15:03 -0400, Stephen Gallagher wrote: > > > This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to > > > the 1.5.x branch. It makes one major change: 1.5.x does not have support > > > for deref/ASQ searches, so the deref lookup for hostgroups has been > > > replaced by parallel LDAP "base" searches. > > > > > > I'm attaching two patches. One is the complete patch to backport, the > > > other is just an interdiff of the changes from Jan's original patch to > > > support the parallel lookups. > > > > > > I'm still in the process of testing this patch, but I wanted to get it > > > out for review in the meantime. > > > > Ok, during testing I found one bug where I forgot to initialize a > > loop-control/array-index variable. Oops. > > > > My basic smoketesting is now complete. Both modes (srchost enabled and > > disabled) are working as expected for a small set of hosts. I don't have > > the infrastructure to test large numbers, but I suspect the only > > difference will be performance, not function. > > > > New patches attached. As before, one is the complete patch ("Add > > ipa_hbac_support_srchost option to IPA provider") and the other is just > > the changes from Jan Zeleny's original patch (to simplify the review > > process). > > I don't understand this part: > > > + if (queue_len == 0) { > > + /* This host is not in any hostgroups */ > > + ret = ENOMEM; > > + goto error; > > + } > > This seems to be preventing me from logging into a host that is not part > of any host group: > > [ipa_hbac_hostgroup_info_done] (3): Error [12][Cannot allocate memory] > [sdap_id_op_done] (9): releasing operation connection >
Thanks, there were two bugs there. The first was that this should have been ENOENT, not ENOMEM. The second was that I forgot to special-case ENOENT in ipa_hbac_hostgroup_info_done(). Both are now fixed.
New patches attached.
> The rest looks good. Thank you for the interdiff, indeed it made the > review easier.
There's one more problem - if the host is not a member of any host group, it has no memberof attribute. However, this call would add an empty attribute in that case:
- ret = sysdb_attrs_get_el(state->hosts[0],
SYSDB_ORIG_MEMBEROF,&parent_el);- if (ret != EOK) goto error;
and later saving that host would fail:
[sysdb_store_custom] (1): Failed to store custmo entry: Constraint violation(19)[attribute originalMemberOf on name=HOSTNAME,cn=hbac_hosts,cn=custom,cn=idm.lab.bos.redhat.com,cn=sysdb specified, but with 0 values (illegal)] [sysdb_store_custom] (6): Error: 14 (Bad address) cancel ldb transaction (nesting: 2) [ipa_hbac_save_list] (1): sysdb_store_custom failed. [ipa_hbac_sysdb_save] (1): Could not save hbac_hosts. [14][Bad address]
Does anyone remember why does sysdb_attrs_get_el internally create the attribute if it doesn't exist? To ease error checking? I think we should either change it or create another getter that wouldn't create the attribute automatically.
I don't remember the reasoning either, but I don't want to go through the hassle of trying to change that during this backport. I added a new patch (that will work on 1.5.x and master) that just exposes the internal get_el() routine that takes an option to create or not create the value. I then changed the above so it detects the ENOENT case appropriately.
Sure, this backport is not the right place for a big change to the sysdb API.
This patchset works for me.
Ack.
One minor correction (for master branch only). I realized this morning that on the master branch, there was one more place that needed to be converted from sysdb_attrs_get_el_int() to sysdb_attrs_get_el_ext(). Please apply the attached patch to master instead. The other patch is correct for sssd-1-5.
There was one more case that needed converting in sysdb_attrs_get_uint16_t. I've fixed that and pushed the corrected patch.
The interdiff is attached.
I also think we should push the patch to 1-8 when it was pushed to 1-5 and master, it would be weird if functionality was not present in one of the three supported branches.
A 1-8 backport is attached, please review.
On Tue, 2012-08-21 at 12:35 +0200, Jakub Hrozek wrote:
On Tue, Aug 21, 2012 at 12:28:22PM +0200, Jakub Hrozek wrote:
On Fri, Aug 17, 2012 at 08:41:59AM -0400, Stephen Gallagher wrote:
On Thu, 2012-08-16 at 18:16 +0200, Jakub Hrozek wrote:
On Wed, Aug 15, 2012 at 01:45:35PM -0400, Stephen Gallagher wrote:
On Wed, 2012-08-15 at 11:12 +0200, Jakub Hrozek wrote:
On Tue, Aug 14, 2012 at 04:47:04PM -0400, Stephen Gallagher wrote: > On Tue, 2012-08-14 at 18:12 +0200, Jakub Hrozek wrote: > > On Mon, Aug 13, 2012 at 07:27:42PM -0400, Stephen Gallagher wrote: > > > On Mon, 2012-08-13 at 15:03 -0400, Stephen Gallagher wrote: > > > > This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to > > > > the 1.5.x branch. It makes one major change: 1.5.x does not have support > > > > for deref/ASQ searches, so the deref lookup for hostgroups has been > > > > replaced by parallel LDAP "base" searches. > > > > > > > > I'm attaching two patches. One is the complete patch to backport, the > > > > other is just an interdiff of the changes from Jan's original patch to > > > > support the parallel lookups. > > > > > > > > I'm still in the process of testing this patch, but I wanted to get it > > > > out for review in the meantime. > > > > > > Ok, during testing I found one bug where I forgot to initialize a > > > loop-control/array-index variable. Oops. > > > > > > My basic smoketesting is now complete. Both modes (srchost enabled and > > > disabled) are working as expected for a small set of hosts. I don't have > > > the infrastructure to test large numbers, but I suspect the only > > > difference will be performance, not function. > > > > > > New patches attached. As before, one is the complete patch ("Add > > > ipa_hbac_support_srchost option to IPA provider") and the other is just > > > the changes from Jan Zeleny's original patch (to simplify the review > > > process). > > > > I don't understand this part: > > > > > + if (queue_len == 0) { > > > + /* This host is not in any hostgroups */ > > > + ret = ENOMEM; > > > + goto error; > > > + } > > > > This seems to be preventing me from logging into a host that is not part > > of any host group: > > > > [ipa_hbac_hostgroup_info_done] (3): Error [12][Cannot allocate memory] > > [sdap_id_op_done] (9): releasing operation connection > > > > Thanks, there were two bugs there. The first was that this should have > been ENOENT, not ENOMEM. The second was that I forgot to special-case > ENOENT in ipa_hbac_hostgroup_info_done(). Both are now fixed. > > New patches attached. > > > The rest looks good. Thank you for the interdiff, indeed it made the > > review easier. > >
There's one more problem - if the host is not a member of any host group, it has no memberof attribute. However, this call would add an empty attribute in that case:
> + ret = sysdb_attrs_get_el(state->hosts[0], > + SYSDB_ORIG_MEMBEROF, > + &parent_el); > + if (ret != EOK) goto error; > +
and later saving that host would fail:
[sysdb_store_custom] (1): Failed to store custmo entry: Constraint violation(19)[attribute originalMemberOf on name=HOSTNAME,cn=hbac_hosts,cn=custom,cn=idm.lab.bos.redhat.com,cn=sysdb specified, but with 0 values (illegal)] [sysdb_store_custom] (6): Error: 14 (Bad address) cancel ldb transaction (nesting: 2) [ipa_hbac_save_list] (1): sysdb_store_custom failed. [ipa_hbac_sysdb_save] (1): Could not save hbac_hosts. [14][Bad address]
Does anyone remember why does sysdb_attrs_get_el internally create the attribute if it doesn't exist? To ease error checking? I think we should either change it or create another getter that wouldn't create the attribute automatically.
I don't remember the reasoning either, but I don't want to go through the hassle of trying to change that during this backport. I added a new patch (that will work on 1.5.x and master) that just exposes the internal get_el() routine that takes an option to create or not create the value. I then changed the above so it detects the ENOENT case appropriately.
Sure, this backport is not the right place for a big change to the sysdb API.
This patchset works for me.
Ack.
One minor correction (for master branch only). I realized this morning that on the master branch, there was one more place that needed to be converted from sysdb_attrs_get_el_int() to sysdb_attrs_get_el_ext(). Please apply the attached patch to master instead. The other patch is correct for sssd-1-5.
There was one more case that needed converting in sysdb_attrs_get_uint16_t. I've fixed that and pushed the corrected patch.
The interdiff is attached.
I also think we should push the patch to 1-8 when it was pushed to 1-5 and master, it would be weird if functionality was not present in one of the three supported branches.
A 1-8 backport is attached, please review.
Ack. BTW, sorry about the earlier patch. I realized this morning that I'd forgotten to actually squash in the missing piece.
On Tue, Aug 21, 2012 at 08:37:28AM -0400, Stephen Gallagher wrote:
On Tue, 2012-08-21 at 12:35 +0200, Jakub Hrozek wrote:
On Tue, Aug 21, 2012 at 12:28:22PM +0200, Jakub Hrozek wrote:
On Fri, Aug 17, 2012 at 08:41:59AM -0400, Stephen Gallagher wrote:
On Thu, 2012-08-16 at 18:16 +0200, Jakub Hrozek wrote:
On Wed, Aug 15, 2012 at 01:45:35PM -0400, Stephen Gallagher wrote:
On Wed, 2012-08-15 at 11:12 +0200, Jakub Hrozek wrote: > On Tue, Aug 14, 2012 at 04:47:04PM -0400, Stephen Gallagher wrote: > > On Tue, 2012-08-14 at 18:12 +0200, Jakub Hrozek wrote: > > > On Mon, Aug 13, 2012 at 07:27:42PM -0400, Stephen Gallagher wrote: > > > > On Mon, 2012-08-13 at 15:03 -0400, Stephen Gallagher wrote: > > > > > This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to > > > > > the 1.5.x branch. It makes one major change: 1.5.x does not have support > > > > > for deref/ASQ searches, so the deref lookup for hostgroups has been > > > > > replaced by parallel LDAP "base" searches. > > > > > > > > > > I'm attaching two patches. One is the complete patch to backport, the > > > > > other is just an interdiff of the changes from Jan's original patch to > > > > > support the parallel lookups. > > > > > > > > > > I'm still in the process of testing this patch, but I wanted to get it > > > > > out for review in the meantime. > > > > > > > > Ok, during testing I found one bug where I forgot to initialize a > > > > loop-control/array-index variable. Oops. > > > > > > > > My basic smoketesting is now complete. Both modes (srchost enabled and > > > > disabled) are working as expected for a small set of hosts. I don't have > > > > the infrastructure to test large numbers, but I suspect the only > > > > difference will be performance, not function. > > > > > > > > New patches attached. As before, one is the complete patch ("Add > > > > ipa_hbac_support_srchost option to IPA provider") and the other is just > > > > the changes from Jan Zeleny's original patch (to simplify the review > > > > process). > > > > > > I don't understand this part: > > > > > > > + if (queue_len == 0) { > > > > + /* This host is not in any hostgroups */ > > > > + ret = ENOMEM; > > > > + goto error; > > > > + } > > > > > > This seems to be preventing me from logging into a host that is not part > > > of any host group: > > > > > > [ipa_hbac_hostgroup_info_done] (3): Error [12][Cannot allocate memory] > > > [sdap_id_op_done] (9): releasing operation connection > > > > > > > Thanks, there were two bugs there. The first was that this should have > > been ENOENT, not ENOMEM. The second was that I forgot to special-case > > ENOENT in ipa_hbac_hostgroup_info_done(). Both are now fixed. > > > > New patches attached. > > > > > The rest looks good. Thank you for the interdiff, indeed it made the > > > review easier. > > > > > > There's one more problem - if the host is not a member of any host > group, it has no memberof attribute. However, this call would add an > empty attribute in that case: > > > + ret = sysdb_attrs_get_el(state->hosts[0], > > + SYSDB_ORIG_MEMBEROF, > > + &parent_el); > > + if (ret != EOK) goto error; > > + > > and later saving that host would fail: > > [sysdb_store_custom] (1): Failed to store custmo entry: Constraint violation(19)[attribute originalMemberOf on name=HOSTNAME,cn=hbac_hosts,cn=custom,cn=idm.lab.bos.redhat.com,cn=sysdb specified, but with 0 values (illegal)] > [sysdb_store_custom] (6): Error: 14 (Bad address) > cancel ldb transaction (nesting: 2) > [ipa_hbac_save_list] (1): sysdb_store_custom failed. > [ipa_hbac_sysdb_save] (1): Could not save hbac_hosts. [14][Bad address] > > Does anyone remember why does sysdb_attrs_get_el internally create the > attribute if it doesn't exist? To ease error checking? I think we should > either change it or create another getter that wouldn't create the > attribute automatically.
I don't remember the reasoning either, but I don't want to go through the hassle of trying to change that during this backport. I added a new patch (that will work on 1.5.x and master) that just exposes the internal get_el() routine that takes an option to create or not create the value. I then changed the above so it detects the ENOENT case appropriately.
Sure, this backport is not the right place for a big change to the sysdb API.
This patchset works for me.
Ack.
One minor correction (for master branch only). I realized this morning that on the master branch, there was one more place that needed to be converted from sysdb_attrs_get_el_int() to sysdb_attrs_get_el_ext(). Please apply the attached patch to master instead. The other patch is correct for sssd-1-5.
There was one more case that needed converting in sysdb_attrs_get_uint16_t. I've fixed that and pushed the corrected patch.
The interdiff is attached.
I also think we should push the patch to 1-8 when it was pushed to 1-5 and master, it would be weird if functionality was not present in one of the three supported branches.
A 1-8 backport is attached, please review.
Ack. BTW, sorry about the earlier patch. I realized this morning that I'd forgotten to actually squash in the missing piece.
Pushed to sssd-1-8
On Thu, Aug 16, 2012 at 06:16:09PM +0200, Jakub Hrozek wrote:
On Wed, Aug 15, 2012 at 01:45:35PM -0400, Stephen Gallagher wrote:
On Wed, 2012-08-15 at 11:12 +0200, Jakub Hrozek wrote:
On Tue, Aug 14, 2012 at 04:47:04PM -0400, Stephen Gallagher wrote:
On Tue, 2012-08-14 at 18:12 +0200, Jakub Hrozek wrote:
On Mon, Aug 13, 2012 at 07:27:42PM -0400, Stephen Gallagher wrote:
On Mon, 2012-08-13 at 15:03 -0400, Stephen Gallagher wrote: > This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to > the 1.5.x branch. It makes one major change: 1.5.x does not have support > for deref/ASQ searches, so the deref lookup for hostgroups has been > replaced by parallel LDAP "base" searches. > > I'm attaching two patches. One is the complete patch to backport, the > other is just an interdiff of the changes from Jan's original patch to > support the parallel lookups. > > I'm still in the process of testing this patch, but I wanted to get it > out for review in the meantime.
Ok, during testing I found one bug where I forgot to initialize a loop-control/array-index variable. Oops.
My basic smoketesting is now complete. Both modes (srchost enabled and disabled) are working as expected for a small set of hosts. I don't have the infrastructure to test large numbers, but I suspect the only difference will be performance, not function.
New patches attached. As before, one is the complete patch ("Add ipa_hbac_support_srchost option to IPA provider") and the other is just the changes from Jan Zeleny's original patch (to simplify the review process).
I don't understand this part:
- if (queue_len == 0) {
/* This host is not in any hostgroups */ret = ENOMEM;goto error;- }
This seems to be preventing me from logging into a host that is not part of any host group:
[ipa_hbac_hostgroup_info_done] (3): Error [12][Cannot allocate memory] [sdap_id_op_done] (9): releasing operation connection
Thanks, there were two bugs there. The first was that this should have been ENOENT, not ENOMEM. The second was that I forgot to special-case ENOENT in ipa_hbac_hostgroup_info_done(). Both are now fixed.
New patches attached.
The rest looks good. Thank you for the interdiff, indeed it made the review easier.
There's one more problem - if the host is not a member of any host group, it has no memberof attribute. However, this call would add an empty attribute in that case:
- ret = sysdb_attrs_get_el(state->hosts[0],
SYSDB_ORIG_MEMBEROF,&parent_el);- if (ret != EOK) goto error;
and later saving that host would fail:
[sysdb_store_custom] (1): Failed to store custmo entry: Constraint violation(19)[attribute originalMemberOf on name=HOSTNAME,cn=hbac_hosts,cn=custom,cn=idm.lab.bos.redhat.com,cn=sysdb specified, but with 0 values (illegal)] [sysdb_store_custom] (6): Error: 14 (Bad address) cancel ldb transaction (nesting: 2) [ipa_hbac_save_list] (1): sysdb_store_custom failed. [ipa_hbac_sysdb_save] (1): Could not save hbac_hosts. [14][Bad address]
Does anyone remember why does sysdb_attrs_get_el internally create the attribute if it doesn't exist? To ease error checking? I think we should either change it or create another getter that wouldn't create the attribute automatically.
I don't remember the reasoning either, but I don't want to go through the hassle of trying to change that during this backport. I added a new patch (that will work on 1.5.x and master) that just exposes the internal get_el() routine that takes an option to create or not create the value. I then changed the above so it detects the ENOENT case appropriately.
Sure, this backport is not the right place for a big change to the sysdb API.
This patchset works for me.
Ack.
Pushed to sssd-1-5
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Stephen Gallagher