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_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..
>
> 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?