Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AD/IPA: Reset subdomain service name, not domain name #721

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
2 participants
@jhrozek
Copy link
Contributor

commented Dec 20, 2018

Related: https://pagure.io/SSSD/sssd/issue/3911

Since commit 778f241 the subdomain fail
over services use the "sd_" prefix. This was done to make it easier, until
the whole failover design works better with subdomains, to see which
services belong to the main domain from tools.

However, some parts of the code would still just use the domain name for
the failover service, which meant the service was not found, notably when
trying to reset services:

(Thu Dec 13 05:29:31 2018) [sssd[be[testrelm.test]]]
[ipa_srv_ad_acct_retried] (0x0400): Subdomain re-set, will retry lookup
(Thu Dec 13 05:29:31 2018) [sssd[be[testrelm.test]]] [be_fo_reset_svc]
(0x1000): Resetting all servers in service ipaad2016.test
(Thu Dec 13 05:29:31 2018) [sssd[be[testrelm.test]]] [be_fo_reset_svc]
(0x0080): Cannot retrieve service [ipaad2016.test]

This patch switches to reading the service names from the ad_options and
the sdap_service structures that are contained within ad_options.

jhrozek added some commits Dec 19, 2018

AD/IPA: Reset subdomain service name, not domain name
Related:
https://pagure.io/SSSD/sssd/issue/3911

Since commit 778f241 the subdomain fail
over services use the "sd_" prefix. This was done to make it easier,
until the whole failover design works better with subdomains, to see
which services belong to the main domain from tools.

However, some parts of the code would still just use the domain name for
the failover service, which meant the service was not found, notably
when trying to reset services:

(Thu Dec 13 05:29:31 2018) [sssd[be[testrelm.test]]] [ipa_srv_ad_acct_retried] (0x0400): Subdomain re-set, will retry lookup
(Thu Dec 13 05:29:31 2018) [sssd[be[testrelm.test]]] [be_fo_reset_svc] (0x1000): Resetting all servers in service ipaad2016.test
(Thu Dec 13 05:29:31 2018) [sssd[be[testrelm.test]]] [be_fo_reset_svc] (0x0080): Cannot retrieve service [ipaad2016.test]

This patch switches to reading the service names from the ad_options and
the sdap_service structures that are contained within ad_options.
IPA: Add explicit return after tevent_req_error
When working on another patch I realized that we don't use explicit
return after failing a request. This could be potentially fatal as the
code would continue, perhaps with data that is not defined.

@thalman thalman self-requested a review Jan 4, 2019

@thalman thalman self-assigned this Jan 8, 2019

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

btw the only way I managed to reproduce the issue myself was frim gdb.

To test, you need a setup with IPA and an AD trusted domain. Then, on the IPA server, I set a breakpoint in ipa_srv_ad_acct_lookup_done. Then run a lookup and forcibly reset the ret returned from ipa_get_ad_acct_recv to ERR_SUBDOM_INACTIVE to go into the 'retry' branch. QE were able to reproduce in their environment, but I was not.

@thalman
Copy link
Contributor

left a comment

Looks good, tested that it works.

@thalman thalman added the Accepted label Jan 22, 2019

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

@jhrozek jhrozek closed this Jan 28, 2019

@jhrozek jhrozek added the Pushed label Jan 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.