Hi,
here is my branch that implements using the fully qualified names in sysdb for users and groups: https://github.com/jhrozek/sssd/commits/sysdb-fqdn
The SSSD code changes are done. There are some downstream tests that are failing (some sudo and some HBAC tests), but I think I can chase those down quickly.
But -- I still don't have the upgrade code written and I only tested the IPA overrides and IPA-AD trust code manually, so there might be a lot of bugs there. It is also a lot of stuff to review.
With that in mind, I would like to ask other developers for opinions -- should we still pursue pushing this refactoring into 1.14? Or should we release 1.14.0 first and push this patchset as the first thing into 1.15?
I'm really on the fence myself.
On 06/28/2016 06:24 PM, Jakub Hrozek wrote:
Hi,
here is my branch that implements using the fully qualified names in sysdb for users and groups: https://github.com/jhrozek/sssd/commits/sysdb-fqdn
The SSSD code changes are done. There are some downstream tests that are failing (some sudo and some HBAC tests), but I think I can chase those down quickly.
But -- I still don't have the upgrade code written and I only tested the IPA overrides and IPA-AD trust code manually, so there might be a lot of bugs there. It is also a lot of stuff to review.
With that in mind, I would like to ask other developers for opinions -- should we still pursue pushing this refactoring into 1.14? Or should we release 1.14.0 first and push this patchset as the first thing into 1.15?
Well there are few places where we told ourselves that it will be fixed as a side effect of those patches. Given the scale I'm fine with postponing it to 1.15 but than we need to fix those.
However, it still may be better to do it for 1.14 if doable.
I'm really on the fence myself.
On (29/06/16 11:54), Pavel Březina wrote:
On 06/28/2016 06:24 PM, Jakub Hrozek wrote:
Hi,
here is my branch that implements using the fully qualified names in sysdb for users and groups: https://github.com/jhrozek/sssd/commits/sysdb-fqdn
The SSSD code changes are done. There are some downstream tests that are failing (some sudo and some HBAC tests), but I think I can chase those down quickly.
But -- I still don't have the upgrade code written and I only tested the IPA overrides and IPA-AD trust code manually, so there might be a lot of bugs there. It is also a lot of stuff to review.
With that in mind, I would like to ask other developers for opinions -- should we still pursue pushing this refactoring into 1.14? Or should we release 1.14.0 first and push this patchset as the first thing into 1.15?
Well there are few places where we told ourselves that it will be fixed as a side effect of those patches. Given the scale I'm fine with postponing it to 1.15 but than we need to fix those.
However, it still may be better to do it for 1.14 if doable.
+1 for 1.14 if possible
LS
On Wed, Jun 29, 2016 at 12:05:42PM +0200, Lukas Slebodnik wrote:
On (29/06/16 11:54), Pavel Březina wrote:
On 06/28/2016 06:24 PM, Jakub Hrozek wrote:
Hi,
here is my branch that implements using the fully qualified names in sysdb for users and groups: https://github.com/jhrozek/sssd/commits/sysdb-fqdn
The SSSD code changes are done. There are some downstream tests that are failing (some sudo and some HBAC tests), but I think I can chase those down quickly.
But -- I still don't have the upgrade code written and I only tested the IPA overrides and IPA-AD trust code manually, so there might be a lot of bugs there. It is also a lot of stuff to review.
With that in mind, I would like to ask other developers for opinions -- should we still pursue pushing this refactoring into 1.14? Or should we release 1.14.0 first and push this patchset as the first thing into 1.15?
Well there are few places where we told ourselves that it will be fixed as a side effect of those patches. Given the scale I'm fine with postponing it to 1.15 but than we need to fix those.
However, it still may be better to do it for 1.14 if doable.
+1 for 1.14 if possible
I would prefer this as well. My first tests didn't went too bad. Besides other things I tested Smartcard and 2-factor authentication which both worked as expected.
I added 3 fixes to my copy at https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=jhrozek_sysdb...
This first is needed if there is no home-directory defined in AD. The other 2 I came across while checking if lookups by UPN with alternative suffix are working. The lookups were working but the output changed depending on the lookup history. The functionality from the patches is not present in current master as well so chances are that there might similar issues here.
About cache updates. I think this is the critical part in the decision here. If updates are not working or are too complex we cannot commit the refactoring.
Maybe we should save a copy of the old cache to allow downgrades without loosing the cache content if something goes wrong with the update?
Maybe we can move the update code to sssctl and call it like 'sssctl update_cache old_file new_file' from SSSD. If the tool reports an error SSSD might start with an empty cache instead and the copy of the old cache can be used to update the cache later manually or with a fixed version of sssctl. Maybe it would be even possible to call the tool during the upgrade of the rpm package and abort the rpm transaction if the cache cannot be updated? Even if this won't work the careful/worried admin can install the latest version of SSSD with the update tool to a new system and use it to check if the tool can update existing caches by copying them and running the tool on them.
bye, Sumit
On Wed, Jun 29, 2016 at 04:36:23PM +0200, Sumit Bose wrote:
On Wed, Jun 29, 2016 at 12:05:42PM +0200, Lukas Slebodnik wrote:
On (29/06/16 11:54), Pavel Březina wrote:
On 06/28/2016 06:24 PM, Jakub Hrozek wrote:
Hi,
here is my branch that implements using the fully qualified names in sysdb for users and groups: https://github.com/jhrozek/sssd/commits/sysdb-fqdn
The SSSD code changes are done. There are some downstream tests that are failing (some sudo and some HBAC tests), but I think I can chase those down quickly.
But -- I still don't have the upgrade code written and I only tested the IPA overrides and IPA-AD trust code manually, so there might be a lot of bugs there. It is also a lot of stuff to review.
With that in mind, I would like to ask other developers for opinions -- should we still pursue pushing this refactoring into 1.14? Or should we release 1.14.0 first and push this patchset as the first thing into 1.15?
Well there are few places where we told ourselves that it will be fixed as a side effect of those patches. Given the scale I'm fine with postponing it to 1.15 but than we need to fix those.
However, it still may be better to do it for 1.14 if doable.
+1 for 1.14 if possible
I would prefer this as well. My first tests didn't went too bad. Besides other things I tested Smartcard and 2-factor authentication which both worked as expected.
I added 3 fixes to my copy at https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=jhrozek_sysdb...
Thank you for the fixes and checking the code in general, I will check these out later.
This first is needed if there is no home-directory defined in AD. The other 2 I came across while checking if lookups by UPN with alternative suffix are working. The lookups were working but the output changed depending on the lookup history. The functionality from the patches is not present in current master as well so chances are that there might similar issues here.
About cache updates. I think this is the critical part in the decision here. If updates are not working or are too complex we cannot commit the refactoring.
Yes. The most complex part is parsing the qualified names which might be in any format, given by the full_name_format option. To solve this, I just pass a name context to the upgrade function.
The upgrade so far doesn't look too bad to me, at least users and groups from main and trusted domains work: https://github.com/jhrozek/sssd/commit/8492cfeaa3ca789c204e78bfdaebdfdb38c81... I still need to upgrade override objects and sudo rules because these contain usernames as well.
The memberof plugin is forcibly disabled during the upgrade and all attributes are written in a single transaction from the upgrade. I think this is safer, especially with complex group memberships, where the memberof plugin might otherwise process a single entry multiple times.
Maybe we should save a copy of the old cache to allow downgrades without loosing the cache content if something goes wrong with the update?
Yes. We can call sssctl to do this.
Maybe we can move the update code to sssctl and call it like 'sssctl update_cache old_file new_file' from SSSD. If the tool reports an error SSSD might start with an empty cache instead and the copy of the old cache can be used to update the cache later manually or with a fixed version of sssctl.
Yes, I was thinking about this as well. Also for performance reasons, I was thinking about doing the upgrade potentially in tmpfs. But I didn't time any upgrades with huge domains yet.
The other thing I was considering is marking all objects as expired during upgrade. The next lookups would then overwrite whatever is in the cache, so hopefully the only broken scenarios would be those where the ugprade is done offline and something goes bad.
Maybe it would be even possible to call the tool during the upgrade of the rpm package and abort the rpm transaction if the cache cannot be updated? Even if this won't work the careful/worried admin can install the latest version of SSSD with the update tool to a new system and use it to check if the tool can update existing caches by copying them and running the tool on them.
Thanks, I'll try to continue to finish the upgrade either today in the evening or tomorrow early, so we can all review it (today I will hopefully be releasing the Beta..)
btw as a general warning - I keep force-pushing my branch.
On Wed, Jun 29, 2016 at 07:00:13PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 04:36:23PM +0200, Sumit Bose wrote:
On Wed, Jun 29, 2016 at 12:05:42PM +0200, Lukas Slebodnik wrote:
On (29/06/16 11:54), Pavel Březina wrote:
On 06/28/2016 06:24 PM, Jakub Hrozek wrote:
Hi,
here is my branch that implements using the fully qualified names in sysdb for users and groups: https://github.com/jhrozek/sssd/commits/sysdb-fqdn
The SSSD code changes are done. There are some downstream tests that are failing (some sudo and some HBAC tests), but I think I can chase those down quickly.
But -- I still don't have the upgrade code written and I only tested the IPA overrides and IPA-AD trust code manually, so there might be a lot of bugs there. It is also a lot of stuff to review.
With that in mind, I would like to ask other developers for opinions -- should we still pursue pushing this refactoring into 1.14? Or should we release 1.14.0 first and push this patchset as the first thing into 1.15?
Well there are few places where we told ourselves that it will be fixed as a side effect of those patches. Given the scale I'm fine with postponing it to 1.15 but than we need to fix those.
However, it still may be better to do it for 1.14 if doable.
+1 for 1.14 if possible
I would prefer this as well. My first tests didn't went too bad. Besides other things I tested Smartcard and 2-factor authentication which both worked as expected.
I added 3 fixes to my copy at https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=jhrozek_sysdb...
Thank you for the fixes and checking the code in general, I will check these out later.
Thanks, merged to the branch.
This first is needed if there is no home-directory defined in AD. The other 2 I came across while checking if lookups by UPN with alternative suffix are working. The lookups were working but the output changed depending on the lookup history. The functionality from the patches is not present in current master as well so chances are that there might similar issues here.
About cache updates. I think this is the critical part in the decision here. If updates are not working or are too complex we cannot commit the refactoring.
Yes. The most complex part is parsing the qualified names which might be in any format, given by the full_name_format option. To solve this, I just pass a name context to the upgrade function.
The upgrade so far doesn't look too bad to me, at least users and groups from main and trusted domains work: https://github.com/jhrozek/sssd/commit/8492cfeaa3ca789c204e78bfdaebdfdb38c81... I still need to upgrade override objects and sudo rules because these contain usernames as well.
The memberof plugin is forcibly disabled during the upgrade and all attributes are written in a single transaction from the upgrade. I think this is safer, especially with complex group memberships, where the memberof plugin might otherwise process a single entry multiple times.
The upgrade is done now. Please review..I tested user and group lookups also with overrides. I force-pushed the code into my github branch.
I also fixed the timestamp integration tests.
Maybe we should save a copy of the old cache to allow downgrades without loosing the cache content if something goes wrong with the update?
Yes. We can call sssctl to do this.
sssctl can do the upgrade now, I just need to add the upgrade to the specfile..
The only thing remaining in this branch is to fix the failing downstream tests IMO..
On Fri, Jul 01, 2016 at 03:24:39PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 07:00:13PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 04:36:23PM +0200, Sumit Bose wrote:
On Wed, Jun 29, 2016 at 12:05:42PM +0200, Lukas Slebodnik wrote:
On (29/06/16 11:54), Pavel Březina wrote:
On 06/28/2016 06:24 PM, Jakub Hrozek wrote:
Hi,
here is my branch that implements using the fully qualified names in sysdb for users and groups: https://github.com/jhrozek/sssd/commits/sysdb-fqdn
The SSSD code changes are done. There are some downstream tests that are failing (some sudo and some HBAC tests), but I think I can chase those down quickly.
But -- I still don't have the upgrade code written and I only tested the IPA overrides and IPA-AD trust code manually, so there might be a lot of bugs there. It is also a lot of stuff to review.
With that in mind, I would like to ask other developers for opinions -- should we still pursue pushing this refactoring into 1.14? Or should we release 1.14.0 first and push this patchset as the first thing into 1.15?
Well there are few places where we told ourselves that it will be fixed as a side effect of those patches. Given the scale I'm fine with postponing it to 1.15 but than we need to fix those.
However, it still may be better to do it for 1.14 if doable.
+1 for 1.14 if possible
I would prefer this as well. My first tests didn't went too bad. Besides other things I tested Smartcard and 2-factor authentication which both worked as expected.
I added 3 fixes to my copy at https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=jhrozek_sysdb...
Thank you for the fixes and checking the code in general, I will check these out later.
Thanks, merged to the branch.
This first is needed if there is no home-directory defined in AD. The other 2 I came across while checking if lookups by UPN with alternative suffix are working. The lookups were working but the output changed depending on the lookup history. The functionality from the patches is not present in current master as well so chances are that there might similar issues here.
About cache updates. I think this is the critical part in the decision here. If updates are not working or are too complex we cannot commit the refactoring.
Yes. The most complex part is parsing the qualified names which might be in any format, given by the full_name_format option. To solve this, I just pass a name context to the upgrade function.
The upgrade so far doesn't look too bad to me, at least users and groups from main and trusted domains work: https://github.com/jhrozek/sssd/commit/8492cfeaa3ca789c204e78bfdaebdfdb38c81... I still need to upgrade override objects and sudo rules because these contain usernames as well.
The memberof plugin is forcibly disabled during the upgrade and all attributes are written in a single transaction from the upgrade. I think this is safer, especially with complex group memberships, where the memberof plugin might otherwise process a single entry multiple times.
The upgrade is done now. Please review..I tested user and group lookups also with overrides. I force-pushed the code into my github branch.
I also fixed the timestamp integration tests.
Maybe we should save a copy of the old cache to allow downgrades without loosing the cache content if something goes wrong with the update?
Yes. We can call sssctl to do this.
sssctl can do the upgrade now, I just need to add the upgrade to the specfile..
The only thing remaining in this branch is to fix the failing downstream tests IMO..
Hi,
I fixed one rather embarassing issue in sudo code related to handling of the special 'ALL' case. There are issues looking up service accounts, but I'm not sure if they are related to this patchset or broken in master., I will investigate more.
There are some failures in downstram HBAC test, but I can't reproduce them so far locally. I will keep digging.
Have a nice weekend!
On Fri, Jul 01, 2016 at 11:01:53PM +0200, Jakub Hrozek wrote:
On Fri, Jul 01, 2016 at 03:24:39PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 07:00:13PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 04:36:23PM +0200, Sumit Bose wrote:
On Wed, Jun 29, 2016 at 12:05:42PM +0200, Lukas Slebodnik wrote:
On (29/06/16 11:54), Pavel Březina wrote:
On 06/28/2016 06:24 PM, Jakub Hrozek wrote: > Hi, > > here is my branch that implements using the fully qualified names in sysdb > for users and groups: > https://github.com/jhrozek/sssd/commits/sysdb-fqdn > > The SSSD code changes are done. There are some downstream tests that are > failing (some sudo and some HBAC tests), but I think I can chase those > down quickly. > > But -- I still don't have the upgrade code written and I only tested the > IPA overrides and IPA-AD trust code manually, so there might be a lot of > bugs there. It is also a lot of stuff to review. > > With that in mind, I would like to ask other developers for opinions -- > should we still pursue pushing this refactoring into 1.14? Or should we > release 1.14.0 first and push this patchset as the first thing into > 1.15?
Well there are few places where we told ourselves that it will be fixed as a side effect of those patches. Given the scale I'm fine with postponing it to 1.15 but than we need to fix those.
However, it still may be better to do it for 1.14 if doable.
+1 for 1.14 if possible
I would prefer this as well. My first tests didn't went too bad. Besides other things I tested Smartcard and 2-factor authentication which both worked as expected.
I added 3 fixes to my copy at https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=jhrozek_sysdb...
Thank you for the fixes and checking the code in general, I will check these out later.
Thanks, merged to the branch.
This first is needed if there is no home-directory defined in AD. The other 2 I came across while checking if lookups by UPN with alternative suffix are working. The lookups were working but the output changed depending on the lookup history. The functionality from the patches is not present in current master as well so chances are that there might similar issues here.
About cache updates. I think this is the critical part in the decision here. If updates are not working or are too complex we cannot commit the refactoring.
Yes. The most complex part is parsing the qualified names which might be in any format, given by the full_name_format option. To solve this, I just pass a name context to the upgrade function.
The upgrade so far doesn't look too bad to me, at least users and groups from main and trusted domains work: https://github.com/jhrozek/sssd/commit/8492cfeaa3ca789c204e78bfdaebdfdb38c81... I still need to upgrade override objects and sudo rules because these contain usernames as well.
The memberof plugin is forcibly disabled during the upgrade and all attributes are written in a single transaction from the upgrade. I think this is safer, especially with complex group memberships, where the memberof plugin might otherwise process a single entry multiple times.
The upgrade is done now. Please review..I tested user and group lookups also with overrides. I force-pushed the code into my github branch.
I also fixed the timestamp integration tests.
Maybe we should save a copy of the old cache to allow downgrades without loosing the cache content if something goes wrong with the update?
Yes. We can call sssctl to do this.
sssctl can do the upgrade now, I just need to add the upgrade to the specfile..
The only thing remaining in this branch is to fix the failing downstream tests IMO..
Hi,
I fixed one rather embarassing issue in sudo code related to handling of the special 'ALL' case. There are issues looking up service accounts, but I'm not sure if they are related to this patchset or broken in master., I will investigate more.
There are some failures in downstram HBAC test, but I can't reproduce them so far locally. I will keep digging.
Have a nice weekend!
Hi, I found some other bugs. To ease the review, I pushed them to: https://github.com/jhrozek/sssd/commits/sysdb-fqdn I included Pavel's sudo patch as well because otherwise I would do some changes and then he would overwrite them back :)
So far I kept the patches separate mostly to make it clear where the bugs were, but I would like to squash them later.
More downstream tests were fixed with these patches, but a new set is running at the moment.
On (03/07/16 12:50), Jakub Hrozek wrote:
On Fri, Jul 01, 2016 at 11:01:53PM +0200, Jakub Hrozek wrote:
On Fri, Jul 01, 2016 at 03:24:39PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 07:00:13PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 04:36:23PM +0200, Sumit Bose wrote:
On Wed, Jun 29, 2016 at 12:05:42PM +0200, Lukas Slebodnik wrote:
On (29/06/16 11:54), Pavel Březina wrote: >On 06/28/2016 06:24 PM, Jakub Hrozek wrote: >> Hi, >> >> here is my branch that implements using the fully qualified names in sysdb >> for users and groups: >> https://github.com/jhrozek/sssd/commits/sysdb-fqdn >> >> The SSSD code changes are done. There are some downstream tests that are >> failing (some sudo and some HBAC tests), but I think I can chase those >> down quickly. >> >> But -- I still don't have the upgrade code written and I only tested the >> IPA overrides and IPA-AD trust code manually, so there might be a lot of >> bugs there. It is also a lot of stuff to review. >> >> With that in mind, I would like to ask other developers for opinions -- >> should we still pursue pushing this refactoring into 1.14? Or should we >> release 1.14.0 first and push this patchset as the first thing into >> 1.15? > >Well there are few places where we told ourselves that it will be fixed as a >side effect of those patches. Given the scale I'm fine with postponing it to >1.15 but than we need to fix those. > >However, it still may be better to do it for 1.14 if doable. > +1 for 1.14 if possible
I would prefer this as well. My first tests didn't went too bad. Besides other things I tested Smartcard and 2-factor authentication which both worked as expected.
I added 3 fixes to my copy at https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=jhrozek_sysdb...
Thank you for the fixes and checking the code in general, I will check these out later.
Thanks, merged to the branch.
This first is needed if there is no home-directory defined in AD. The other 2 I came across while checking if lookups by UPN with alternative suffix are working. The lookups were working but the output changed depending on the lookup history. The functionality from the patches is not present in current master as well so chances are that there might similar issues here.
About cache updates. I think this is the critical part in the decision here. If updates are not working or are too complex we cannot commit the refactoring.
Yes. The most complex part is parsing the qualified names which might be in any format, given by the full_name_format option. To solve this, I just pass a name context to the upgrade function.
The upgrade so far doesn't look too bad to me, at least users and groups from main and trusted domains work: https://github.com/jhrozek/sssd/commit/8492cfeaa3ca789c204e78bfdaebdfdb38c81... I still need to upgrade override objects and sudo rules because these contain usernames as well.
The memberof plugin is forcibly disabled during the upgrade and all attributes are written in a single transaction from the upgrade. I think this is safer, especially with complex group memberships, where the memberof plugin might otherwise process a single entry multiple times.
The upgrade is done now. Please review..I tested user and group lookups also with overrides. I force-pushed the code into my github branch.
I also fixed the timestamp integration tests.
Maybe we should save a copy of the old cache to allow downgrades without loosing the cache content if something goes wrong with the update?
Yes. We can call sssctl to do this.
sssctl can do the upgrade now, I just need to add the upgrade to the specfile..
The only thing remaining in this branch is to fix the failing downstream tests IMO..
Hi,
I fixed one rather embarassing issue in sudo code related to handling of the special 'ALL' case. There are issues looking up service accounts, but I'm not sure if they are related to this patchset or broken in master., I will investigate more.
There are some failures in downstram HBAC test, but I can't reproduce them so far locally. I will keep digging.
Have a nice weekend!
Hi, I found some other bugs. To ease the review, I pushed them to: https://github.com/jhrozek/sssd/commits/sysdb-fqdn I included Pavel's sudo patch as well because otherwise I would do some changes and then he would overwrite them back :)
So far I kept the patches separate mostly to make it clear where the bugs were, but I would like to squash them later.
More downstream tests were fixed with these patches, but a new set is running at the moment.
There are some failures in nss-srv unit test [ RUN ] test_nss_getorigbyname "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2125: error: Failure!
[ FAILED ] test_nss_getorigbyname [ RUN ] test_nss_getorigbyname_extra_attrs "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2225: error: Failure!
[ FAILED ] test_nss_getorigbyname_extra_attrs [ RUN ] test_nss_getorigbyname_multi_value_attrs "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2353: error: Failure!
[ FAILED ] test_nss_getorigbyname_multi_value_attrs
and you need to rebase patches on top of current master because there is a small conflict in error codes.
It would be good to push patches with thisrefactoring before 1.14.0. We can fix small bugs later (1.14.1)
LS
On Wed, Jul 06, 2016 at 11:13:02AM +0200, Lukas Slebodnik wrote:
On (03/07/16 12:50), Jakub Hrozek wrote:
On Fri, Jul 01, 2016 at 11:01:53PM +0200, Jakub Hrozek wrote:
On Fri, Jul 01, 2016 at 03:24:39PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 07:00:13PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 04:36:23PM +0200, Sumit Bose wrote:
On Wed, Jun 29, 2016 at 12:05:42PM +0200, Lukas Slebodnik wrote: > On (29/06/16 11:54), Pavel Březina wrote: > >On 06/28/2016 06:24 PM, Jakub Hrozek wrote: > >> Hi, > >> > >> here is my branch that implements using the fully qualified names in sysdb > >> for users and groups: > >> https://github.com/jhrozek/sssd/commits/sysdb-fqdn > >> > >> The SSSD code changes are done. There are some downstream tests that are > >> failing (some sudo and some HBAC tests), but I think I can chase those > >> down quickly. > >> > >> But -- I still don't have the upgrade code written and I only tested the > >> IPA overrides and IPA-AD trust code manually, so there might be a lot of > >> bugs there. It is also a lot of stuff to review. > >> > >> With that in mind, I would like to ask other developers for opinions -- > >> should we still pursue pushing this refactoring into 1.14? Or should we > >> release 1.14.0 first and push this patchset as the first thing into > >> 1.15? > > > >Well there are few places where we told ourselves that it will be fixed as a > >side effect of those patches. Given the scale I'm fine with postponing it to > >1.15 but than we need to fix those. > > > >However, it still may be better to do it for 1.14 if doable. > > > +1 for 1.14 if possible
I would prefer this as well. My first tests didn't went too bad. Besides other things I tested Smartcard and 2-factor authentication which both worked as expected.
I added 3 fixes to my copy at https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=jhrozek_sysdb...
Thank you for the fixes and checking the code in general, I will check these out later.
Thanks, merged to the branch.
This first is needed if there is no home-directory defined in AD. The other 2 I came across while checking if lookups by UPN with alternative suffix are working. The lookups were working but the output changed depending on the lookup history. The functionality from the patches is not present in current master as well so chances are that there might similar issues here.
About cache updates. I think this is the critical part in the decision here. If updates are not working or are too complex we cannot commit the refactoring.
Yes. The most complex part is parsing the qualified names which might be in any format, given by the full_name_format option. To solve this, I just pass a name context to the upgrade function.
The upgrade so far doesn't look too bad to me, at least users and groups from main and trusted domains work: https://github.com/jhrozek/sssd/commit/8492cfeaa3ca789c204e78bfdaebdfdb38c81... I still need to upgrade override objects and sudo rules because these contain usernames as well.
The memberof plugin is forcibly disabled during the upgrade and all attributes are written in a single transaction from the upgrade. I think this is safer, especially with complex group memberships, where the memberof plugin might otherwise process a single entry multiple times.
The upgrade is done now. Please review..I tested user and group lookups also with overrides. I force-pushed the code into my github branch.
I also fixed the timestamp integration tests.
Maybe we should save a copy of the old cache to allow downgrades without loosing the cache content if something goes wrong with the update?
Yes. We can call sssctl to do this.
sssctl can do the upgrade now, I just need to add the upgrade to the specfile..
The only thing remaining in this branch is to fix the failing downstream tests IMO..
Hi,
I fixed one rather embarassing issue in sudo code related to handling of the special 'ALL' case. There are issues looking up service accounts, but I'm not sure if they are related to this patchset or broken in master., I will investigate more.
There are some failures in downstram HBAC test, but I can't reproduce them so far locally. I will keep digging.
Have a nice weekend!
Hi, I found some other bugs. To ease the review, I pushed them to: https://github.com/jhrozek/sssd/commits/sysdb-fqdn I included Pavel's sudo patch as well because otherwise I would do some changes and then he would overwrite them back :)
So far I kept the patches separate mostly to make it clear where the bugs were, but I would like to squash them later.
More downstream tests were fixed with these patches, but a new set is running at the moment.
There are some failures in nss-srv unit test [ RUN ] test_nss_getorigbyname "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2125: error: Failure!
[ FAILED ] test_nss_getorigbyname [ RUN ] test_nss_getorigbyname_extra_attrs "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2225: error: Failure!
[ FAILED ] test_nss_getorigbyname_extra_attrs [ RUN ] test_nss_getorigbyname_multi_value_attrs "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2353: error: Failure!
[ FAILED ] test_nss_getorigbyname_multi_value_attrs
and you need to rebase patches on top of current master because there is a small conflict in error codes.
It would be good to push patches with thisrefactoring before 1.14.0. We can fix small bugs later (1.14.1)
I pushed another patchset into my branch that includes one fix for HBAC, one fix for SUDO (both squashed and both found by downstream tests) and several patches from Sumit.
I'm still running additional tests and I would like to work on the upgrade from the specfile, but I think this patchset can be merged already.
On Wed, Jul 06, 2016 at 06:34:32PM +0200, Jakub Hrozek wrote:
On Wed, Jul 06, 2016 at 11:13:02AM +0200, Lukas Slebodnik wrote:
On (03/07/16 12:50), Jakub Hrozek wrote:
On Fri, Jul 01, 2016 at 11:01:53PM +0200, Jakub Hrozek wrote:
On Fri, Jul 01, 2016 at 03:24:39PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 07:00:13PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 04:36:23PM +0200, Sumit Bose wrote: > On Wed, Jun 29, 2016 at 12:05:42PM +0200, Lukas Slebodnik wrote: > > On (29/06/16 11:54), Pavel Březina wrote: > > >On 06/28/2016 06:24 PM, Jakub Hrozek wrote: > > >> Hi, > > >> > > >> here is my branch that implements using the fully qualified names in sysdb > > >> for users and groups: > > >> https://github.com/jhrozek/sssd/commits/sysdb-fqdn > > >> > > >> The SSSD code changes are done. There are some downstream tests that are > > >> failing (some sudo and some HBAC tests), but I think I can chase those > > >> down quickly. > > >> > > >> But -- I still don't have the upgrade code written and I only tested the > > >> IPA overrides and IPA-AD trust code manually, so there might be a lot of > > >> bugs there. It is also a lot of stuff to review. > > >> > > >> With that in mind, I would like to ask other developers for opinions -- > > >> should we still pursue pushing this refactoring into 1.14? Or should we > > >> release 1.14.0 first and push this patchset as the first thing into > > >> 1.15? > > > > > >Well there are few places where we told ourselves that it will be fixed as a > > >side effect of those patches. Given the scale I'm fine with postponing it to > > >1.15 but than we need to fix those. > > > > > >However, it still may be better to do it for 1.14 if doable. > > > > > +1 for 1.14 if possible > > I would prefer this as well. My first tests didn't went too bad. Besides > other things I tested Smartcard and 2-factor authentication which both > worked as expected. > > I added 3 fixes to my copy at > https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=jhrozek_sysdb...
Thank you for the fixes and checking the code in general, I will check these out later.
Thanks, merged to the branch.
> > This first is needed if there is no home-directory defined in AD. The > other 2 I came across while checking if lookups by UPN with alternative > suffix are working. The lookups were working but the output changed > depending on the lookup history. The functionality from the patches is > not present in current master as well so chances are that there might > similar issues here. > > > > About cache updates. I think this is the critical part in the decision > here. If updates are not working or are too complex we cannot commit the > refactoring.
Yes. The most complex part is parsing the qualified names which might be in any format, given by the full_name_format option. To solve this, I just pass a name context to the upgrade function.
The upgrade so far doesn't look too bad to me, at least users and groups from main and trusted domains work: https://github.com/jhrozek/sssd/commit/8492cfeaa3ca789c204e78bfdaebdfdb38c81... I still need to upgrade override objects and sudo rules because these contain usernames as well.
The memberof plugin is forcibly disabled during the upgrade and all attributes are written in a single transaction from the upgrade. I think this is safer, especially with complex group memberships, where the memberof plugin might otherwise process a single entry multiple times.
The upgrade is done now. Please review..I tested user and group lookups also with overrides. I force-pushed the code into my github branch.
I also fixed the timestamp integration tests.
> > Maybe we should save a copy of the old cache to allow downgrades > without loosing the cache content if something goes wrong with the > update?
Yes. We can call sssctl to do this.
sssctl can do the upgrade now, I just need to add the upgrade to the specfile..
The only thing remaining in this branch is to fix the failing downstream tests IMO..
Hi,
I fixed one rather embarassing issue in sudo code related to handling of the special 'ALL' case. There are issues looking up service accounts, but I'm not sure if they are related to this patchset or broken in master., I will investigate more.
There are some failures in downstram HBAC test, but I can't reproduce them so far locally. I will keep digging.
Have a nice weekend!
Hi, I found some other bugs. To ease the review, I pushed them to: https://github.com/jhrozek/sssd/commits/sysdb-fqdn I included Pavel's sudo patch as well because otherwise I would do some changes and then he would overwrite them back :)
So far I kept the patches separate mostly to make it clear where the bugs were, but I would like to squash them later.
More downstream tests were fixed with these patches, but a new set is running at the moment.
There are some failures in nss-srv unit test [ RUN ] test_nss_getorigbyname "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2125: error: Failure!
[ FAILED ] test_nss_getorigbyname [ RUN ] test_nss_getorigbyname_extra_attrs "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2225: error: Failure!
[ FAILED ] test_nss_getorigbyname_extra_attrs [ RUN ] test_nss_getorigbyname_multi_value_attrs "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2353: error: Failure!
[ FAILED ] test_nss_getorigbyname_multi_value_attrs
and you need to rebase patches on top of current master because there is a small conflict in error codes.
It would be good to push patches with thisrefactoring before 1.14.0. We can fix small bugs later (1.14.1)
I pushed another patchset into my branch that includes one fix for HBAC, one fix for SUDO (both squashed and both found by downstream tests) and several patches from Sumit.
I'm still running additional tests and I would like to work on the upgrade from the specfile, but I think this patchset can be merged already.
Of course I forgot to cherry-pick one Sumit's patch. I included it now and re-ran tests again. I will send a report to our internal list (because the tests are downstream..) when they finish..
On Wed, Jul 06, 2016 at 09:02:05PM +0200, Jakub Hrozek wrote:
On Wed, Jul 06, 2016 at 06:34:32PM +0200, Jakub Hrozek wrote:
On Wed, Jul 06, 2016 at 11:13:02AM +0200, Lukas Slebodnik wrote:
On (03/07/16 12:50), Jakub Hrozek wrote:
On Fri, Jul 01, 2016 at 11:01:53PM +0200, Jakub Hrozek wrote:
On Fri, Jul 01, 2016 at 03:24:39PM +0200, Jakub Hrozek wrote:
On Wed, Jun 29, 2016 at 07:00:13PM +0200, Jakub Hrozek wrote: > On Wed, Jun 29, 2016 at 04:36:23PM +0200, Sumit Bose wrote: > > On Wed, Jun 29, 2016 at 12:05:42PM +0200, Lukas Slebodnik wrote: > > > On (29/06/16 11:54), Pavel Březina wrote: > > > >On 06/28/2016 06:24 PM, Jakub Hrozek wrote: > > > >> Hi, > > > >> > > > >> here is my branch that implements using the fully qualified names in sysdb > > > >> for users and groups: > > > >> https://github.com/jhrozek/sssd/commits/sysdb-fqdn > > > >> > > > >> The SSSD code changes are done. There are some downstream tests that are > > > >> failing (some sudo and some HBAC tests), but I think I can chase those > > > >> down quickly. > > > >> > > > >> But -- I still don't have the upgrade code written and I only tested the > > > >> IPA overrides and IPA-AD trust code manually, so there might be a lot of > > > >> bugs there. It is also a lot of stuff to review. > > > >> > > > >> With that in mind, I would like to ask other developers for opinions -- > > > >> should we still pursue pushing this refactoring into 1.14? Or should we > > > >> release 1.14.0 first and push this patchset as the first thing into > > > >> 1.15? > > > > > > > >Well there are few places where we told ourselves that it will be fixed as a > > > >side effect of those patches. Given the scale I'm fine with postponing it to > > > >1.15 but than we need to fix those. > > > > > > > >However, it still may be better to do it for 1.14 if doable. > > > > > > > +1 for 1.14 if possible > > > > I would prefer this as well. My first tests didn't went too bad. Besides > > other things I tested Smartcard and 2-factor authentication which both > > worked as expected. > > > > I added 3 fixes to my copy at > > https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=jhrozek_sysdb... > > Thank you for the fixes and checking the code in general, I will check > these out later.
Thanks, merged to the branch.
> > > > > This first is needed if there is no home-directory defined in AD. The > > other 2 I came across while checking if lookups by UPN with alternative > > suffix are working. The lookups were working but the output changed > > depending on the lookup history. The functionality from the patches is > > not present in current master as well so chances are that there might > > similar issues here. > > > > > > > > About cache updates. I think this is the critical part in the decision > > here. If updates are not working or are too complex we cannot commit the > > refactoring. > > Yes. The most complex part is parsing the qualified names which might > be in any format, given by the full_name_format option. To solve this, > I just pass a name context to the upgrade function. > > The upgrade so far doesn't look too bad to me, at least users and groups > from main and trusted domains work: > https://github.com/jhrozek/sssd/commit/8492cfeaa3ca789c204e78bfdaebdfdb38c81... > I still need to upgrade override objects and sudo rules because these > contain usernames as well. > > The memberof plugin is forcibly disabled during the upgrade and all > attributes are written in a single transaction from the upgrade. I think > this is safer, especially with complex group memberships, where the > memberof plugin might otherwise process a single entry multiple times.
The upgrade is done now. Please review..I tested user and group lookups also with overrides. I force-pushed the code into my github branch.
I also fixed the timestamp integration tests.
> > > > > Maybe we should save a copy of the old cache to allow downgrades > > without loosing the cache content if something goes wrong with the > > update? > > Yes. We can call sssctl to do this.
sssctl can do the upgrade now, I just need to add the upgrade to the specfile..
The only thing remaining in this branch is to fix the failing downstream tests IMO..
Hi,
I fixed one rather embarassing issue in sudo code related to handling of the special 'ALL' case. There are issues looking up service accounts, but I'm not sure if they are related to this patchset or broken in master., I will investigate more.
There are some failures in downstram HBAC test, but I can't reproduce them so far locally. I will keep digging.
Have a nice weekend!
Hi, I found some other bugs. To ease the review, I pushed them to: https://github.com/jhrozek/sssd/commits/sysdb-fqdn I included Pavel's sudo patch as well because otherwise I would do some changes and then he would overwrite them back :)
So far I kept the patches separate mostly to make it clear where the bugs were, but I would like to squash them later.
More downstream tests were fixed with these patches, but a new set is running at the moment.
There are some failures in nss-srv unit test [ RUN ] test_nss_getorigbyname "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2125: error: Failure!
[ FAILED ] test_nss_getorigbyname [ RUN ] test_nss_getorigbyname_extra_attrs "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2225: error: Failure!
[ FAILED ] test_nss_getorigbyname_extra_attrs [ RUN ] test_nss_getorigbyname_multi_value_attrs "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2353: error: Failure!
[ FAILED ] test_nss_getorigbyname_multi_value_attrs
and you need to rebase patches on top of current master because there is a small conflict in error codes.
It would be good to push patches with thisrefactoring before 1.14.0. We can fix small bugs later (1.14.1)
I pushed another patchset into my branch that includes one fix for HBAC, one fix for SUDO (both squashed and both found by downstream tests) and several patches from Sumit.
I'm still running additional tests and I would like to work on the upgrade from the specfile, but I think this patchset can be merged already.
Of course I forgot to cherry-pick one Sumit's patch. I included it now and re-ran tests again. I will send a report to our internal list (because the tests are downstream..) when they finish..
I did some testing as well which now all went well, including the update of a cache with more then 500 entries on a offline system. I think as well the patches are ready to be pushed. I'm just waiting for a CI run to finish before giving my ACK.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On Wed, Jul 06, 2016 at 10:24:26PM +0200, Sumit Bose wrote:
On Wed, Jul 06, 2016 at 09:02:05PM +0200, Jakub Hrozek wrote:
On Wed, Jul 06, 2016 at 06:34:32PM +0200, Jakub Hrozek wrote:
On Wed, Jul 06, 2016 at 11:13:02AM +0200, Lukas Slebodnik wrote:
On (03/07/16 12:50), Jakub Hrozek wrote:
On Fri, Jul 01, 2016 at 11:01:53PM +0200, Jakub Hrozek wrote:
On Fri, Jul 01, 2016 at 03:24:39PM +0200, Jakub Hrozek wrote: > On Wed, Jun 29, 2016 at 07:00:13PM +0200, Jakub Hrozek wrote: > > On Wed, Jun 29, 2016 at 04:36:23PM +0200, Sumit Bose wrote: > > > On Wed, Jun 29, 2016 at 12:05:42PM +0200, Lukas Slebodnik wrote: > > > > On (29/06/16 11:54), Pavel Březina wrote: > > > > >On 06/28/2016 06:24 PM, Jakub Hrozek wrote: > > > > >> Hi, > > > > >> > > > > >> here is my branch that implements using the fully qualified names in sysdb > > > > >> for users and groups: > > > > >> https://github.com/jhrozek/sssd/commits/sysdb-fqdn > > > > >> > > > > >> The SSSD code changes are done. There are some downstream tests that are > > > > >> failing (some sudo and some HBAC tests), but I think I can chase those > > > > >> down quickly. > > > > >> > > > > >> But -- I still don't have the upgrade code written and I only tested the > > > > >> IPA overrides and IPA-AD trust code manually, so there might be a lot of > > > > >> bugs there. It is also a lot of stuff to review. > > > > >> > > > > >> With that in mind, I would like to ask other developers for opinions -- > > > > >> should we still pursue pushing this refactoring into 1.14? Or should we > > > > >> release 1.14.0 first and push this patchset as the first thing into > > > > >> 1.15? > > > > > > > > > >Well there are few places where we told ourselves that it will be fixed as a > > > > >side effect of those patches. Given the scale I'm fine with postponing it to > > > > >1.15 but than we need to fix those. > > > > > > > > > >However, it still may be better to do it for 1.14 if doable. > > > > > > > > > +1 for 1.14 if possible > > > > > > I would prefer this as well. My first tests didn't went too bad. Besides > > > other things I tested Smartcard and 2-factor authentication which both > > > worked as expected. > > > > > > I added 3 fixes to my copy at > > > https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=jhrozek_sysdb... > > > > Thank you for the fixes and checking the code in general, I will check > > these out later. > > Thanks, merged to the branch. > > > > > > > > > This first is needed if there is no home-directory defined in AD. The > > > other 2 I came across while checking if lookups by UPN with alternative > > > suffix are working. The lookups were working but the output changed > > > depending on the lookup history. The functionality from the patches is > > > not present in current master as well so chances are that there might > > > similar issues here. > > > > > > > > > > > > About cache updates. I think this is the critical part in the decision > > > here. If updates are not working or are too complex we cannot commit the > > > refactoring. > > > > Yes. The most complex part is parsing the qualified names which might > > be in any format, given by the full_name_format option. To solve this, > > I just pass a name context to the upgrade function. > > > > The upgrade so far doesn't look too bad to me, at least users and groups > > from main and trusted domains work: > > https://github.com/jhrozek/sssd/commit/8492cfeaa3ca789c204e78bfdaebdfdb38c81... > > I still need to upgrade override objects and sudo rules because these > > contain usernames as well. > > > > The memberof plugin is forcibly disabled during the upgrade and all > > attributes are written in a single transaction from the upgrade. I think > > this is safer, especially with complex group memberships, where the > > memberof plugin might otherwise process a single entry multiple times. > > The upgrade is done now. Please review..I tested user and group lookups > also with overrides. I force-pushed the code into my github branch. > > I also fixed the timestamp integration tests. > > > > > > > > > Maybe we should save a copy of the old cache to allow downgrades > > > without loosing the cache content if something goes wrong with the > > > update? > > > > Yes. We can call sssctl to do this. > > sssctl can do the upgrade now, I just need to add the upgrade to the > specfile.. > > The only thing remaining in this branch is to fix the failing downstream > tests IMO..
Hi,
I fixed one rather embarassing issue in sudo code related to handling of the special 'ALL' case. There are issues looking up service accounts, but I'm not sure if they are related to this patchset or broken in master., I will investigate more.
There are some failures in downstram HBAC test, but I can't reproduce them so far locally. I will keep digging.
Have a nice weekend!
Hi, I found some other bugs. To ease the review, I pushed them to: https://github.com/jhrozek/sssd/commits/sysdb-fqdn I included Pavel's sudo patch as well because otherwise I would do some changes and then he would overwrite them back :)
So far I kept the patches separate mostly to make it clear where the bugs were, but I would like to squash them later.
More downstream tests were fixed with these patches, but a new set is running at the moment.
There are some failures in nss-srv unit test [ RUN ] test_nss_getorigbyname "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2125: error: Failure!
[ FAILED ] test_nss_getorigbyname [ RUN ] test_nss_getorigbyname_extra_attrs "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2225: error: Failure!
[ FAILED ] test_nss_getorigbyname_extra_attrs [ RUN ] test_nss_getorigbyname_multi_value_attrs "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2353: error: Failure!
[ FAILED ] test_nss_getorigbyname_multi_value_attrs
and you need to rebase patches on top of current master because there is a small conflict in error codes.
It would be good to push patches with thisrefactoring before 1.14.0. We can fix small bugs later (1.14.1)
I pushed another patchset into my branch that includes one fix for HBAC, one fix for SUDO (both squashed and both found by downstream tests) and several patches from Sumit.
I'm still running additional tests and I would like to work on the upgrade from the specfile, but I think this patchset can be merged already.
Of course I forgot to cherry-pick one Sumit's patch. I included it now and re-ran tests again. I will send a report to our internal list (because the tests are downstream..) when they finish..
I did some testing as well which now all went well, including the update of a cache with more then 500 entries on a offline system. I think as well the patches are ready to be pushed. I'm just waiting for a CI run to finish before giving my ACK.
ok, CI passed http://sssd-ci.duckdns.org/logs/job/47/41/summary.html
ACK
bye, Sumit
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On 07/06/2016 11:21 PM, Sumit Bose wrote:
On Wed, Jul 06, 2016 at 10:24:26PM +0200, Sumit Bose wrote:
On Wed, Jul 06, 2016 at 09:02:05PM +0200, Jakub Hrozek wrote:
On Wed, Jul 06, 2016 at 06:34:32PM +0200, Jakub Hrozek wrote:
On Wed, Jul 06, 2016 at 11:13:02AM +0200, Lukas Slebodnik wrote:
On (03/07/16 12:50), Jakub Hrozek wrote:
On Fri, Jul 01, 2016 at 11:01:53PM +0200, Jakub Hrozek wrote: > On Fri, Jul 01, 2016 at 03:24:39PM +0200, Jakub Hrozek wrote: >> On Wed, Jun 29, 2016 at 07:00:13PM +0200, Jakub Hrozek wrote: >>> On Wed, Jun 29, 2016 at 04:36:23PM +0200, Sumit Bose wrote: >>>> On Wed, Jun 29, 2016 at 12:05:42PM +0200, Lukas Slebodnik wrote: >>>>> On (29/06/16 11:54), Pavel Březina wrote: >>>>>> On 06/28/2016 06:24 PM, Jakub Hrozek wrote: >>>>>>> Hi, >>>>>>> >>>>>>> here is my branch that implements using the fully qualified names in sysdb >>>>>>> for users and groups: >>>>>>> https://github.com/jhrozek/sssd/commits/sysdb-fqdn >>>>>>> >>>>>>> The SSSD code changes are done. There are some downstream tests that are >>>>>>> failing (some sudo and some HBAC tests), but I think I can chase those >>>>>>> down quickly. >>>>>>> >>>>>>> But -- I still don't have the upgrade code written and I only tested the >>>>>>> IPA overrides and IPA-AD trust code manually, so there might be a lot of >>>>>>> bugs there. It is also a lot of stuff to review. >>>>>>> >>>>>>> With that in mind, I would like to ask other developers for opinions -- >>>>>>> should we still pursue pushing this refactoring into 1.14? Or should we >>>>>>> release 1.14.0 first and push this patchset as the first thing into >>>>>>> 1.15? >>>>>> >>>>>> Well there are few places where we told ourselves that it will be fixed as a >>>>>> side effect of those patches. Given the scale I'm fine with postponing it to >>>>>> 1.15 but than we need to fix those. >>>>>> >>>>>> However, it still may be better to do it for 1.14 if doable. >>>>>> >>>>> +1 for 1.14 if possible >>>> >>>> I would prefer this as well. My first tests didn't went too bad. Besides >>>> other things I tested Smartcard and 2-factor authentication which both >>>> worked as expected. >>>> >>>> I added 3 fixes to my copy at >>>> https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=jhrozek_sysdb... >>> >>> Thank you for the fixes and checking the code in general, I will check >>> these out later. >> >> Thanks, merged to the branch. >> >>> >>>> >>>> This first is needed if there is no home-directory defined in AD. The >>>> other 2 I came across while checking if lookups by UPN with alternative >>>> suffix are working. The lookups were working but the output changed >>>> depending on the lookup history. The functionality from the patches is >>>> not present in current master as well so chances are that there might >>>> similar issues here. >>>> >>>> >>>> >>>> About cache updates. I think this is the critical part in the decision >>>> here. If updates are not working or are too complex we cannot commit the >>>> refactoring. >>> >>> Yes. The most complex part is parsing the qualified names which might >>> be in any format, given by the full_name_format option. To solve this, >>> I just pass a name context to the upgrade function. >>> >>> The upgrade so far doesn't look too bad to me, at least users and groups >>> from main and trusted domains work: >>> https://github.com/jhrozek/sssd/commit/8492cfeaa3ca789c204e78bfdaebdfdb38c81... >>> I still need to upgrade override objects and sudo rules because these >>> contain usernames as well. >>> >>> The memberof plugin is forcibly disabled during the upgrade and all >>> attributes are written in a single transaction from the upgrade. I think >>> this is safer, especially with complex group memberships, where the >>> memberof plugin might otherwise process a single entry multiple times. >> >> The upgrade is done now. Please review..I tested user and group lookups >> also with overrides. I force-pushed the code into my github branch. >> >> I also fixed the timestamp integration tests. >> >>> >>>> >>>> Maybe we should save a copy of the old cache to allow downgrades >>>> without loosing the cache content if something goes wrong with the >>>> update? >>> >>> Yes. We can call sssctl to do this. >> >> sssctl can do the upgrade now, I just need to add the upgrade to the >> specfile.. >> >> The only thing remaining in this branch is to fix the failing downstream >> tests IMO.. > > Hi, > > I fixed one rather embarassing issue in sudo code related to handling of > the special 'ALL' case. There are issues looking up service accounts, but > I'm not sure if they are related to this patchset or broken in master., > I will investigate more. > > There are some failures in downstram HBAC test, but I can't reproduce > them so far locally. I will keep digging. > > Have a nice weekend!
Hi, I found some other bugs. To ease the review, I pushed them to: https://github.com/jhrozek/sssd/commits/sysdb-fqdn I included Pavel's sudo patch as well because otherwise I would do some changes and then he would overwrite them back :)
So far I kept the patches separate mostly to make it clear where the bugs were, but I would like to squash them later.
More downstream tests were fixed with these patches, but a new set is running at the moment.
There are some failures in nss-srv unit test [ RUN ] test_nss_getorigbyname "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2125: error: Failure!
[ FAILED ] test_nss_getorigbyname [ RUN ] test_nss_getorigbyname_extra_attrs "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2225: error: Failure!
[ FAILED ] test_nss_getorigbyname_extra_attrs [ RUN ] test_nss_getorigbyname_multi_value_attrs "orig_name@nss_test" != "orig_name" ../sssd/src/tests/cmocka/test_nss_srv.c:2353: error: Failure!
[ FAILED ] test_nss_getorigbyname_multi_value_attrs
and you need to rebase patches on top of current master because there is a small conflict in error codes.
It would be good to push patches with thisrefactoring before 1.14.0. We can fix small bugs later (1.14.1)
I pushed another patchset into my branch that includes one fix for HBAC, one fix for SUDO (both squashed and both found by downstream tests) and several patches from Sumit.
I'm still running additional tests and I would like to work on the upgrade from the specfile, but I think this patchset can be merged already.
Of course I forgot to cherry-pick one Sumit's patch. I included it now and re-ran tests again. I will send a report to our internal list (because the tests are downstream..) when they finish..
I did some testing as well which now all went well, including the update of a cache with more then 500 entries on a offline system. I think as well the patches are ready to be pushed. I'm just waiting for a CI run to finish before giving my ACK.
ok, CI passed http://sssd-ci.duckdns.org/logs/job/47/41/summary.html
ACK
I went through the code changes and it looks good to me. I have just one nitpick for the changes in sssctl, can you move filter construction and the whole magic around fqn in sssctl_find_object() to a separate function?
On (07/07/16 11:39), Pavel Březina wrote:
On 07/06/2016 11:21 PM, Sumit Bose wrote:
ok, CI passed http://sssd-ci.duckdns.org/logs/job/47/41/summary.html
ACK
I went through the code changes and it looks good to me. I have just one nitpick for the changes in sssctl, can you move filter construction and the whole magic around fqn in sssctl_find_object() to a separate function?
too late for review :-)
Patches have been already pushed to master (Thu, 7 Jul 2016 08:53:57 +0000 (UTC)) Jakub just forgot to send a mail.
Feel free to send patch with your proposed change.
LS
On Thu, Jul 07, 2016 at 11:45:42AM +0200, Lukas Slebodnik wrote:
On (07/07/16 11:39), Pavel Březina wrote:
On 07/06/2016 11:21 PM, Sumit Bose wrote:
ok, CI passed http://sssd-ci.duckdns.org/logs/job/47/41/summary.html
ACK
I went through the code changes and it looks good to me. I have just one nitpick for the changes in sssctl, can you move filter construction and the whole magic around fqn in sssctl_find_object() to a separate function?
too late for review :-)
Patches have been already pushed to master (Thu, 7 Jul 2016 08:53:57 +0000 (UTC)) Jakub just forgot to send a mail.
Yes, please, send a follow-up patch.
This thread was pushed as e6b6b9f..c88b63b.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Sumit Bose