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

WIP: RESOLV: Avoid DNS search to improve fail-over reaction #5245

Closed
wants to merge 4 commits into from

Conversation

thalman
Copy link
Contributor

@thalman thalman commented Jul 15, 2020

In case of unreachable DNS server or invalid hostname sssd/c-ares tries
to search in multiple domains based on the search directive
in resolv.conf

But the hostnames in config file are fully qualified and this just
extends the time spent with DNS resolution.

This patch set the c-ares library flags to avoid DNS search

Resolves:
#5390

@thalman
Copy link
Contributor Author

thalman commented Jul 15, 2020

Despite the change is small, it changes the default behaviour and we should discuss it across the team.

Once we agree the change I will also add commit with man page update to explicitly state that servers must be IP address or FQDN.

From discussion with @sumit-bose I got that it should be safe to do it because of relation to kerberos (kerberos would not work with short hostnames) but are there any other use ceases?

CI runs fine in my environment.

@elkoniu elkoniu self-requested a review July 17, 2020 12:16
@elkoniu elkoniu self-assigned this Jul 17, 2020
@pbrezina
Copy link
Member

Does SSSD even work if the hostname or domain name is not qualified? If not then what change of behavior do you refer to?

@thalman
Copy link
Contributor Author

thalman commented Aug 24, 2020

Does SSSD even work if the hostname or domain name is not qualified? If not then what change of behavior do you refer to?

I discussed that with @sumit-bose and as he explained to me we have FQDN in configuration anyway. Kerberos requires FQDN to work and we can assume that SSSD has it in configuration. Then this patch is safe to include.

But is anyone aware of a case that I missed and short names may be there? Perhaps with LDAP provider?

@pbrezina
Copy link
Member

In theory I think it is possible to set dns_discovery_domain without the whole domain part and since it is used only in DNS it should work with the domain search. But the man page say specifies the domain part of the service discovery DNS query so I think we are safe here.

@pbrezina
Copy link
Member

pbrezina commented Sep 1, 2020

Please try to set ldap_uri to non-qualified name and see if the domain search works. If there is not Kerberos authentication there maybe no requirement for this.

@thalman
Copy link
Contributor Author

thalman commented Sep 4, 2020

my test shows that user can have NOT qualified names in ldap_uri now

ldap_uri = ldap://ldap/

and it works thanks to the domain search. With this patch users are no longer resolved.

@sumit-bose
Copy link
Contributor

Hi @thalman,

what about using your suggestion from https://bugzilla.redhat.com/show_bug.cgi?id=1608496#c26?

bye,
Sumit

@pbrezina
Copy link
Member

pbrezina commented Sep 7, 2020

'hostname.subdomain' will still work through domain search if subdomain is not a top level domain, wouldn't it?

@sumit-bose
Copy link
Contributor

'hostname.subdomain' will still work through domain search if subdomain is not a top level domain, wouldn't it?

Hi,

no, as @thalman suggested in https://bugzilla.redhat.com/show_bug.cgi?id=1608496#c26 we would check if there is a '.' in the name and since hostname.subdomain has a dot we would add a dot to the end which would skip the domain searches.

Since we cannot reliable determine if what follows the '.' is a fully-qualified domain or just the first part which has to be extended with what's available in /etc/resolv.conf I guess we cannot avoid a config option which can switch options.flags = ARES_FLAG_NOSEARCH; on and off.

bye,
Sumit

@thalman
Copy link
Contributor Author

thalman commented Sep 7, 2020

Suggested solution with "." lookup is still possible but this one is more simple, elegant and consistent.

I would rather see a new option "perform_dns_search" which will give as the possibility to keep old behaviour rather than to do the trailing dot.

I also think there will be very few installation where admins depends on domain search. Maybe some testing installation.

@sumit-bose
Copy link
Contributor

Suggested solution with "." lookup is still possible but this one is more simple, elegant and consistent.

I would rather see a new option "perform_dns_search" which will give as the possibility to keep old behaviour rather than to do the trailing dot.

I also think there will be very few installation where admins depends on domain search. Maybe some testing installation.

Hi,

I agree, but perform_dns_search sounds misleading, I would suggestion something like `dns_resolver_use_default_search_domains'.

bye,
Sumit

@thalman thalman changed the title RESOLV: Avoid DNS search to improve fail-over reaction WIP: RESOLV: Avoid DNS search to improve fail-over reaction Sep 11, 2020
@thalman thalman changed the title WIP: RESOLV: Avoid DNS search to improve fail-over reaction RESOLV: Avoid DNS search to improve fail-over reaction Nov 10, 2020
In case of unreachable DNS server or invalid hostname sssd/c-ares tries
to search in multiple domains based on the search directive
in resolv.conf

But the hostnames in config file are fully qualified and this just
extends the time spent with DNS resolution.

This patch set the c-ares library flags to avoid DNS search

Resolves:
SSSD#5390
Changing resolv_init call requires tests to be updated
This patch changes the default behaviour so DNS search
is not performed by default.
@thalman thalman changed the title RESOLV: Avoid DNS search to improve fail-over reaction WIP: RESOLV: Avoid DNS search to improve fail-over reaction Nov 18, 2020
@thalman
Copy link
Contributor Author

thalman commented Nov 18, 2020

Patch works for ldap provider but it looks like DNS search is still performed in case of AD provider. I need to investigate it more

@elkoniu
Copy link
Contributor

elkoniu commented Mar 15, 2021

@thalman If this PR is still alive and ongoing? If not maybe close it and reopen when there will be new changes? @alexey-tikhonov If I remember correct last time you run some upstream PR list cleaning to close long standing WIP PRs?

@alexey-tikhonov
Copy link
Member

This PR was discussed on a team meeting and Tomas said he plans to finish it.

@thalman
Copy link
Contributor Author

thalman commented Mar 16, 2021

I decided to close the PR due to my capacity. I will reopen it once there is a progress.

@thalman thalman closed this Mar 16, 2021
@thalman thalman deleted the dns-search branch December 9, 2022 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants