Hi!
When reviewing Petr's netgroup patch I found some issues with netgroups when using IPA provider.
Attached patch fixes one of them.
I filed ticket for the other issue here: https://fedorahosted.org/sssd/ticket/3117
Reviewing this is not priority for this week, but I already had the patch and wanted to put the list.
Thanks, Michal
On 08/02/2016 11:09 AM, Michal Židek wrote:
Hi!
When reviewing Petr's netgroup patch I found some issues with netgroups when using IPA provider.
Attached patch fixes one of them.
I filed ticket for the other issue here: https://fedorahosted.org/sssd/ticket/3117
Reviewing this is not priority for this week, but I already had the patch and wanted to put the list.
Thanks, Michal
0001-ipa_netgroups-Lowercase-key-to-htable.patch
From 2e5022452d7002e44ad17a59b4d5b12958721997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Thu, 28 Jul 2016 12:55:46 +0200 Subject: [PATCH] ipa_netgroups: Lowercase key to htable
Fixes: https://fedorahosted.org/sssd/ticket/3116
We lowercase the search key when storing entries into the hash table, but do not do it when we search for them. As a result we were not able to find netgroup by DN.
Hi Michal,
it looks good to me (aka LGTM). I am waiting for CI.
Regards
On 08/02/2016 12:41 PM, Petr Cech wrote:
On 08/02/2016 11:09 AM, Michal Židek wrote:
Hi!
When reviewing Petr's netgroup patch I found some issues with netgroups when using IPA provider.
Attached patch fixes one of them.
I filed ticket for the other issue here: https://fedorahosted.org/sssd/ticket/3117
Reviewing this is not priority for this week, but I already had the patch and wanted to put the list.
Thanks, Michal
0001-ipa_netgroups-Lowercase-key-to-htable.patch
From 2e5022452d7002e44ad17a59b4d5b12958721997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Thu, 28 Jul 2016 12:55:46 +0200 Subject: [PATCH] ipa_netgroups: Lowercase key to htable
Fixes: https://fedorahosted.org/sssd/ticket/3116
We lowercase the search key when storing entries into the hash table, but do not do it when we search for them. As a result we were not able to find netgroup by DN.
Hi Michal,
it looks good to me (aka LGTM). I am waiting for CI.
CI passed: http://sssd-ci.duckdns.org/logs/job/50/67/summary.html
ACK
And for test... I think you might file a ticket.
On (02/08/16 13:50), Petr Cech wrote:
On 08/02/2016 12:41 PM, Petr Cech wrote:
On 08/02/2016 11:09 AM, Michal Židek wrote:
Hi!
When reviewing Petr's netgroup patch I found some issues with netgroups when using IPA provider.
Attached patch fixes one of them.
I filed ticket for the other issue here: https://fedorahosted.org/sssd/ticket/3117
Reviewing this is not priority for this week, but I already had the patch and wanted to put the list.
Thanks, Michal
0001-ipa_netgroups-Lowercase-key-to-htable.patch
From 2e5022452d7002e44ad17a59b4d5b12958721997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Thu, 28 Jul 2016 12:55:46 +0200 Subject: [PATCH] ipa_netgroups: Lowercase key to htable
Fixes: https://fedorahosted.org/sssd/ticket/3116
We lowercase the search key when storing entries into the hash table, but do not do it when we search for them. As a result we were not able to find netgroup by DN.
Hi Michal,
it looks good to me (aka LGTM). I am waiting for CI.
CI passed: http://sssd-ci.duckdns.org/logs/job/50/67/summary.html
ACK
And for test... I think you might file a ticket.
I'm sorry I have a different opinion. Michal tried to resolve nested netgroup membership which should have been fixed in #2275 This patch is just a fix to the partial problem.
a) it does not fix a bug b) it does not have an integration test c) id does not have an unit test
Many people consider a code without code coverage as a legacy code. We need to avoid such situations and moreover we need to avoid possible regressions in future. Therefore nested netgroup membership in IPA need to be fixed (and not just a partially). And we should have a test for this bug. I don't might which king of test. But IMHO the simplest would be to write an integration test to freeipa project.
=> Conditional NACK untill resolvimng previously mentioned issues.
LS
On 08/02/2016 03:46 PM, Lukas Slebodnik wrote:
On (02/08/16 13:50), Petr Cech wrote:
On 08/02/2016 12:41 PM, Petr Cech wrote:
On 08/02/2016 11:09 AM, Michal Židek wrote:
Hi!
When reviewing Petr's netgroup patch I found some issues with netgroups when using IPA provider.
Attached patch fixes one of them.
I filed ticket for the other issue here: https://fedorahosted.org/sssd/ticket/3117
Reviewing this is not priority for this week, but I already had the patch and wanted to put the list.
Thanks, Michal
0001-ipa_netgroups-Lowercase-key-to-htable.patch
From 2e5022452d7002e44ad17a59b4d5b12958721997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Thu, 28 Jul 2016 12:55:46 +0200 Subject: [PATCH] ipa_netgroups: Lowercase key to htable
Fixes: https://fedorahosted.org/sssd/ticket/3116
We lowercase the search key when storing entries into the hash table, but do not do it when we search for them. As a result we were not able to find netgroup by DN.
Hi Michal,
it looks good to me (aka LGTM). I am waiting for CI.
CI passed: http://sssd-ci.duckdns.org/logs/job/50/67/summary.html
ACK
And for test... I think you might file a ticket.
I'm sorry I have a different opinion. Michal tried to resolve nested netgroup membership which should have been fixed in #2275 This patch is just a fix to the partial problem.
a) it does not fix a bug
It does fix one.
b) it does not have an integration test c) id does not have an unit test
^ Yes we do not have upstream tests for netgroups. We should implement them.
Many people consider a code without code coverage as a legacy code. We need to avoid such situations and moreover we need to avoid possible regressions in future. Therefore nested netgroup membership in IPA need to be fixed (and not just a partially). And we should have a test for this bug. I don't might which king of test. But IMHO the simplest would be to write an integration test to freeipa project.
=> Conditional NACK untill resolvimng previously mentioned issues.
LS
The purpose of this patch was to fix one particular bug in code that I found when looking into the netgroups. It fixes one issue with netgroups, not all of them. I sent it to list and filed the tickets so that I do not need to keep the information about it in my head.
It does not have high priority (now) and I am OK if the two tickets are solved together later.
Michal
On (02/08/16 16:39), Michal Židek wrote:
On 08/02/2016 03:46 PM, Lukas Slebodnik wrote:
On (02/08/16 13:50), Petr Cech wrote:
On 08/02/2016 12:41 PM, Petr Cech wrote:
On 08/02/2016 11:09 AM, Michal Židek wrote:
Hi!
When reviewing Petr's netgroup patch I found some issues with netgroups when using IPA provider.
Attached patch fixes one of them.
I filed ticket for the other issue here: https://fedorahosted.org/sssd/ticket/3117
Reviewing this is not priority for this week, but I already had the patch and wanted to put the list.
Thanks, Michal
0001-ipa_netgroups-Lowercase-key-to-htable.patch
From 2e5022452d7002e44ad17a59b4d5b12958721997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Thu, 28 Jul 2016 12:55:46 +0200 Subject: [PATCH] ipa_netgroups: Lowercase key to htable
Fixes: https://fedorahosted.org/sssd/ticket/3116
We lowercase the search key when storing entries into the hash table, but do not do it when we search for them. As a result we were not able to find netgroup by DN.
Hi Michal,
it looks good to me (aka LGTM). I am waiting for CI.
CI passed: http://sssd-ci.duckdns.org/logs/job/50/67/summary.html
ACK
And for test... I think you might file a ticket.
I'm sorry I have a different opinion. Michal tried to resolve nested netgroup membership which should have been fixed in #2275 This patch is just a fix to the partial problem.
a) it does not fix a bug
It does fix one.
b) it does not have an integration test c) id does not have an unit test
^
Yes we do not have upstream tests for netgroups. We should implement them.
Many people consider a code without code coverage as a legacy code. We need to avoid such situations and moreover we need to avoid possible regressions in future. Therefore nested netgroup membership in IPA need to be fixed (and not just a partially). And we should have a test for this bug. I don't might which king of test. But IMHO the simplest would be to write an integration test to freeipa project.
=> Conditional NACK untill resolvimng previously mentioned issues.
LS
The purpose of this patch was to fix one particular bug in code that I found when looking into the netgroups. It fixes one issue with netgroups, not all of them. I sent it to list and filed the tickets so that I do not need to keep the information about it in my head.
Let me use a different example. There is a crash[1] in current master if group or use contains brackets. There were two "sub-bugs" in code. One in responder and other in back-end. It does not make a sense to fix just one part. Because from user point of view it still does not work. It only make a sense to split fixes into two patches; but that's the developer point of view.
It does not have high priority (now) and I am OK if the two tickets are solved together later.
That is not true. #2275 should have been fixed in 1.13.1 but it does not work. It is not important whether it's a regression or it was fixed just a partially in #2275. It has to be fixed very soon because there is a related downstream ticket BZ1118257 and we do not have a lot of time. So it has the highest priority.
BTW the best way how to fix a bugs is to firstly write a test. manual reproducer is just a wasting of time. I did it in [1] :-)
LS
[1] https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.o...
On (02/08/16 11:09), Michal Židek wrote:
Hi!
When reviewing Petr's netgroup patch I found some issues with netgroups when using IPA provider.
Attached patch fixes one of them.
I filed ticket for the other issue here: https://fedorahosted.org/sssd/ticket/3117
Reviewing this is not priority for this week, but I already had the patch and wanted to put the list.
Thanks, Michal
From 2e5022452d7002e44ad17a59b4d5b12958721997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Thu, 28 Jul 2016 12:55:46 +0200 Subject: [PATCH] ipa_netgroups: Lowercase key to htable
Fixes: https://fedorahosted.org/sssd/ticket/3116
We lowercase the search key when storing entries into the hash table, but do not do it when we search for them. As a result we were not able to find netgroup by DN.
src/providers/ipa/ipa_netgroups.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/providers/ipa/ipa_netgroups.c b/src/providers/ipa/ipa_netgroups.c index a19e5e0..38b4b5d 100644 --- a/src/providers/ipa/ipa_netgroups.c +++ b/src/providers/ipa/ipa_netgroups.c @@ -888,7 +888,13 @@ static int ipa_netgr_process_all(struct ipa_get_netgroups_state *state) j = 0; if (ret == EOK) { for (j = 0; members[j]; j++) {
key.str = discard_const(members[j]);
/* Lower case the search key (we do this also when storing
* the entries in the hash table). */
key.str = talloc_strdup(state, discard_const(members[j]));
for (int p = 0; key.str[p] != '\0'; p++) {
key.str[p] = tolower(key.str[p]);
}
Currently it's not possible to write an integration test using cwrap But it would be good to add test into freeipa tests. https://git.fedorahosted.org/cgit/freeipa.git/tree/ipatests/test_integration...
It should not be complicated. You can reuse 99% of current upstream freeipa tests. You will need to jsut add netgroups/hostgroups to IPA and call "getent netgroup"
It's better to write integration tests in other project rather than manually testing with each release.
LS
sssd-devel@lists.fedorahosted.org