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_fqdn > > > > >> > > > > > > >> > > 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/8492cfeaa3ca789c204e78bfdaebdfdb38c81146 > > > > >> > > 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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
