URL: https://github.com/SSSD/sssd/pull/411 Author: pbrezina Title: #411: AD: Remember last site discovered Action: opened
PR body: """ To discover Active Directory site for a client we must first contact any directory controller for an LDAP ping. This is done by searching domain-wide DNS tree which may however contain servers that are not reachable from current site and than we face long timeouts or failure.
This patch makes sssd remember the last successfuly discovered site and use this for DNS search to lookup a site and forest again similar to what we do when ad_site option is set.
Resolves: https://pagure.io/SSSD/sssd/issue/3265 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/411/head:pr411 git checkout pr411
URL: https://github.com/SSSD/sssd/pull/411 Title: #411: AD: Remember last site discovered
fidencio commented: """ @pbrezina, from a quick look in the code (no tests have been done on my side) there's something that bothers me a little bit.
This part of the code: ``` + /* Remember current site so it can be used during next lookup so + * we can contact directory controllers within a known reachable + * site first. */ + ret = ad_srv_plugin_ctx_switch_site(state->ctx, state->site); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, "Unable to set site [%d]: %s\n", + ret, sss_strerror(ret)); + goto done; + } ```
Although I'd raise a loud debug message, I personally wouldn't fail ad_srv_plugin_site_done() because of a ad_srv_plugin_ctx_switch_site() failure.
Keep in mind that I don't have any strong feelings about what will be your preference. (IOW, feel free to ignore this comment). """
See the full comment at https://github.com/SSSD/sssd/pull/411#issuecomment-337614191
URL: https://github.com/SSSD/sssd/pull/411 Title: #411: AD: Remember last site discovered
pbrezina commented: """ It can fail only on lack of memory so I think it is fine to fail. """
See the full comment at https://github.com/SSSD/sssd/pull/411#issuecomment-337820126
URL: https://github.com/SSSD/sssd/pull/411 Title: #411: AD: Remember last site discovered
fidencio commented: """ Okay, code-wise it looks fine then. Although, I'd have to properly test the patch and I don't think I'll have the capacity to do so for *this* *Friday* release.
If someone else has the capacity, please, go for it. Otherwise I'll do the tests and add the "Accepted" label next week. """
See the full comment at https://github.com/SSSD/sssd/pull/411#issuecomment-337823980
URL: https://github.com/SSSD/sssd/pull/411 Title: #411: AD: Remember last site discovered
jhrozek commented: """ This patch looks easy enough for the case where SSSD keeps running, but wouldn't it be even nicer to save the site persistently to confdb? """
See the full comment at https://github.com/SSSD/sssd/pull/411#issuecomment-337848744
URL: https://github.com/SSSD/sssd/pull/411 Title: #411: AD: Remember last site discovered
pbrezina commented: """ Ok, I can add the functionality. """
See the full comment at https://github.com/SSSD/sssd/pull/411#issuecomment-337852455
URL: https://github.com/SSSD/sssd/pull/411 Title: #411: AD: Remember last site discovered
jhrozek commented: """ It was a suggestion, I'm not 100% sure if it makes sense. But if it does, it would be nice to implement this by extending the code that already handles saving to and reading from sysdb which is e.g. used in the subdomains provider. """
See the full comment at https://github.com/SSSD/sssd/pull/411#issuecomment-337853946
URL: https://github.com/SSSD/sssd/pull/411 Title: #411: AD: Remember last site discovered
pbrezina commented: """ Yes, I think it does make sense. The site is not likely going to change so contacting domain controllers inside known side is better. And if the site will not be reachable, well we will failover to dns discovery domain.
Exactly, what code do you have in mind? """
See the full comment at https://github.com/SSSD/sssd/pull/411#issuecomment-337857741
URL: https://github.com/SSSD/sssd/pull/411 Title: #411: AD: Remember last site discovered
jhrozek commented: """ I had `sysdb_master_domain_update` in mind, mostly to keep all the code that updates the domain on the same place and we already handle e.g. the realm there. I wanted to avoid storing data to the confdb directly, but reuse or extend some API that already exists.
But you'll see during development if it makes sense or not :) """
See the full comment at https://github.com/SSSD/sssd/pull/411#issuecomment-337860448
URL: https://github.com/SSSD/sssd/pull/411 Title: #411: AD: Remember last site discovered
jhrozek commented: """ btw I'm setting the changes requested label to indicate that no review is needed at this point and that we're instead waiting for a new version of the patch. """
See the full comment at https://github.com/SSSD/sssd/pull/411#issuecomment-338199229
URL: https://github.com/SSSD/sssd/pull/411 Title: #411: AD: Remember last site discovered
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/411 Title: #411: AD: Remember last site discovered
pbrezina commented: """ If you want to include it in this release, please, go ahead with this patch. I had to put focus on other places today. """
See the full comment at https://github.com/SSSD/sssd/pull/411#issuecomment-338206451
URL: https://github.com/SSSD/sssd/pull/411 Title: #411: AD: Remember last site discovered
jhrozek commented: """ I would prefer to have the final patch as long as you think it can be done during next week. """
See the full comment at https://github.com/SSSD/sssd/pull/411#issuecomment-338209691
sssd-devel@lists.fedorahosted.org
-
fidencio -
jhrozek
-
pbrezina