URL: https://github.com/SSSD/sssd/pull/579 Author: sumit-bose Title: #579: krb5 locator: add support for multiple addresses and sub-domains Action: opened
PR body: """ Related to https://pagure.io/SSSD/sssd/issue/3652 and https://pagure.io/SSSD/sssd/issue/941. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/579/head:pr579 git checkout pr579
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
jhrozek commented: """ retest this please """
See the full comment at https://github.com/SSSD/sssd/pull/579#issuecomment-392011228
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
fidencio commented: """ retest this please """
See the full comment at https://github.com/SSSD/sssd/pull/579#issuecomment-392458475
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
jhrozek commented: """ The only issue I found is that the kdcinfo files are note removed when sssd exits.
One other thing we might consider is that the krb5_use_kdcinfo option can only be set in the main domain and affects all subdomains. I /think/ that's even ultimately the right thing to do, I can't imagine a scenario where you want the kdcinfo files for one domain but not the other, but I think it should be mentioned during the review.
Finally, about testing, I checked that the kdcinfo files are created for all the domains and I checked (using strace kinit) that libkrb5 is able to read all the addresses and fall back to the next addresses in the list in case the first one does not work.
The code looks good to me, Coverity was happy as well. """
See the full comment at https://github.com/SSSD/sssd/pull/579#issuecomment-393860677
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/579 Author: sumit-bose Title: #579: krb5 locator: add support for multiple addresses and sub-domains Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/579/head:pr579 git checkout pr579
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
sumit-bose commented: """ Hi Jakub, thank you for the review I added a new patch which make the removal of the krb5info files more flexible and takes care of the removal of the sub-domain files as well.
There is even the chance to simplify this even more, but currently I do not completely understand the be_fo_run_callbacks_at_next_request() calls in remove_krb5_info_files_callback(). So I kept them and might try to improve this later. """
See the full comment at https://github.com/SSSD/sssd/pull/579#issuecomment-394817217
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
jhrozek commented: """ I think the general intention of the patch is good and I like destructors being used in general, but there seems to be a bug. When I send SIGUSR1 to sssd to force it to go offline, sssd_be crashes: ``` Program received signal SIGUSR1, User defined signal 1. 0x00007f8562e72aa0 in epoll_pwait () from /lib64/libc.so.6 (gdb) c Continuing.
Program received signal SIGABRT, Aborted. 0x00007f8562d9266b in raise () from /lib64/libc.so.6 (gdb) bt #0 0x00007f8562d9266b in raise () from /lib64/libc.so.6 #1 0x00007f8562d94381 in abort () from /lib64/libc.so.6 #2 0x00007f85635d030c in talloc_abort () from /lib64/libtalloc.so.2 #3 0x00007f85635d06d3 in talloc_check_name () from /lib64/libtalloc.so.2 #4 0x00007f855cacdade in remove_krb5_info_files_callback (pvt=0xf4a130) at /sssd/src/providers/krb5/krb5_common.c:921 #5 0x000000000040ea69 in be_run_cb_step (ev=0xe70960, te=0xedfac0, current_time=..., pvt=0xf6b4c0) at /sssd/src/providers/data_provider_callbacks.c:96 #6 0x00007f85637ea491 in tevent_common_loop_timer_delay () from /lib64/libtevent.so.0 #7 0x00007f85637eb751 in epoll_event_loop_once () from /lib64/libtevent.so.0 #8 0x00007f85637e9af7 in std_event_loop_once () from /lib64/libtevent.so.0 #9 0x00007f85637e5f5d in _tevent_loop_once () from /lib64/libtevent.so.0 #10 0x00007f85637e617b in tevent_common_loop_wait () from /lib64/libtevent.so.0 #11 0x00007f85637e9a97 in std_event_loop_wait () from /lib64/libtevent.so.0 #12 0x00007f8567346420 in server_loop (main_ctx=0xe71df0) at /sssd/src/util/server.c:725 #13 0x0000000000409e96 in main (argc=8, argv=0x7ffe71409c18) at /sssd/src/providers/data_provider_be.c:639 (gdb) ``` """
See the full comment at https://github.com/SSSD/sssd/pull/579#issuecomment-395111558
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
jhrozek commented: """ Please let me know if you're too busy with adcli or realmd work and would prefer me to investigate, for now, I'm just adding Changes Requested and will move on to the next PR which needs review.. """
See the full comment at https://github.com/SSSD/sssd/pull/579#issuecomment-395111871
URL: https://github.com/SSSD/sssd/pull/579 Author: sumit-bose Title: #579: krb5 locator: add support for multiple addresses and sub-domains Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/579/head:pr579 git checkout pr579
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
sumit-bose commented: """ I hope the latest version helps to fix/avoid the crash by making sure the offline callbacks are only run once (as the online callbacks already are). There is another patch which tries to make sure the callback is only added once for each file. I can squash this one in later but I wanted to make the additions clear for now. """
See the full comment at https://github.com/SSSD/sssd/pull/579#issuecomment-395830853
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
jhrozek commented: """ retest this please """
See the full comment at https://github.com/SSSD/sssd/pull/579#issuecomment-395842120
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
jhrozek commented: """ Thank you, I ran a quick test and I no longer see the crashes. I will continue with the full review later, though. """
See the full comment at https://github.com/SSSD/sssd/pull/579#issuecomment-395855371
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
jhrozek commented: """ I think the commit that runs the offline callbacks is correct. The kdcinfo files are now removed. The code looks OK, Coverity is not complaining so I will add the ack once CI finishes. """
See the full comment at https://github.com/SSSD/sssd/pull/579#issuecomment-396228958
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
jhrozek commented: """ CI: http://vm-031.$ABC/logs/job/90/34/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/579#issuecomment-396587573
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
fidencio commented: """ master: efae950 9f68324 c1fbc6b 2124275 cc79227 d91661e 4759a48 f28d995 """
See the full comment at https://github.com/SSSD/sssd/pull/579#issuecomment-397388098
URL: https://github.com/SSSD/sssd/pull/579 Author: sumit-bose Title: #579: krb5 locator: add support for multiple addresses and sub-domains Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/579/head:pr579 git checkout pr579
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/579 Title: #579: krb5 locator: add support for multiple addresses and sub-domains
Label: -Accepted
sssd-devel@lists.fedorahosted.org
-
fidencio -
jhrozek
-
sumit-bose