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_sy...
> >> > >
> >> > > 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/8492cfeaa3ca789c204e78bfdaebdfdb38...
> >> > > 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..