URL: https://github.com/SSSD/sssd/pull/614 Author: asheplyakov Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out Action: opened
PR body: """ Suppose the user U is a member of (AD) groups D1\A, D1\B, D2\X, and no domain controllers in the domain D2 can be reached at the moment (and there are no cached info). As of now initgroups won't assign any groups at all. To improve the behavior skip the incomplete groups so initgroup assigns at least some groups (D1\A, D1\B in the above example). """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/614/head:pr614 git checkout pr614
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-403867722
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-403867723
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
fidencio commented: """ ok to test """
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-403871241
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
fidencio commented: """ @asheplyakov, thanks for the contribution. This patch does look correct to me. I'll just fire a run of our internal CI (for the sake of the process) and as soon as I get the results the "Accepted" label will be added.
@jhrozek, please, feel free to step up if you think I'm missing something here. """
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-405158872
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
fidencio commented: """ CI results were okay, apart from the known issues.
@asheplyakov, there's one nitpick about the patch that I'd like to ask your permission to fix it locally before pushing. Our commit template is something towards the lines of: ``` component: subject
explanation
reference to the issue solved (when there's a bz/issue about that) ```
If you agree, I'd like to do a: `s/nss_protocol_fill_initgr/nss/` in the component part of your patch. Please, mind that you don't have to re-submit the patch for this change, just gimme your ack that I'll do it before pushing. """
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-405200329
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
asheplyakov commented: """ @fidencio: I'm OK with the proposed commit message change
"""
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-405263666
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
fidencio commented: """ Adding the "Accepted" label. I'll push on Wednesday, in order to give @jhrozek some time to answer as well.
Thanks a lot for your contribution and quick response @asheplyakov. """
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-405286334
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
jhrozek commented: """ I'm not against this patch (I just had no time so far to do any test) but my primary question is why do we have the incomplete groups in the cache? I thought if we could only read the SID from the tokengroups but then not resolve the SID then we would have a group name stored with name=sid.
So I don't think there's anything wrong with the patch, I just wonder if we would be papering over another bug with it. """
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-405348786
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
fidencio commented: """ Although the patch looks good, after a conversation with @jhrozek ... I have my mind made that we may be just papering over a more critical bug.
I'm removing the "Accepted" label but keeping the PR opened.
@asheplyakov, would you have a cache dump, logs or even a machine that you could give us access (via tmate) so we could nail this issue down and find out why we have incomplete groups in the first place? """
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-411658336
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
asheplyakov commented: """ @fidencio
would you have a cache dump, logs or even a machine that you could give us access
Unfortunately no. That was a client's machine, and we had no permission to copy any logs in first place. We couldn't reproduce the problem locally, perhaps it takes some tricky AD setup (and we had no permission examine their AD configuration).
"""
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-411689819
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
fidencio commented: """ I see. We'll have a phone call Today and discuss this patch within SSSD team. Although we don't have nor follow any "governance", I do believe that nowadays would be able to have an agreement based on https://libvirt.org/governance.html#roughconsensus :-)
I'll update the status of the patch based on our phone call later Today. """
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-411704304
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
fidencio commented: """ I see. We'll have a phone call Today and discuss this patch within SSSD team. Although we don't have nor follow any "governance", I do believe that nowadays we would be able to have an agreement based on https://libvirt.org/governance.html#roughconsensus :-)
I'll update the status of the patch based on our phone call later Today. """
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-411704304
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
fidencio commented: """ We agreed on merging the PR during our team meeting. """
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-411795873
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
fidencio commented: """ master: 4937f2c """
See the full comment at https://github.com/SSSD/sssd/pull/614#issuecomment-411796606
URL: https://github.com/SSSD/sssd/pull/614 Author: asheplyakov Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/614/head:pr614 git checkout pr614
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/614 Title: #614: nss_protocol_fill_initgr: skip incomplete groups instead of bailing out
Label: -Accepted
sssd-devel@lists.fedorahosted.org