ipatests: re-add test_dnssec.py::TestInstallDNSSECFirst in gating

[flo-renaud]

The test was temporarily removed because of a known issue
but the issue is now fixed.

Related: https://pagure.io/freeipa/issue/8496
Signed-off-by: Florence Blanc-Renaud flo@redhat.com

[flo-renaud]

Also added a fix for https://pagure.io/freeipa/issue/8695. Since it's related to resolvers, @wladich could you have a look?

[rcritten]

LGTM. Can you confirm what branches this needs backports for? I see the change in ipa-4-8 and ipa-4-9.

[wladich]

@flo-renaud
Do you think the new method Resolver.uses_localhost_as_dns is expected to be of any use outside of the test_resolvconf test case, especially in such strict form - checking for localhost only, not any ip address? I prefer not to place code which is not expected to be reused to common libraries like this one.

The type of resolver can be detected in the test with isinstance() or we could add a string property "resolver_type" to all Resolver subclasses.

What do you think about changing the test from checking config files contents (which is not perfect in current state, see inline comment) to checking if ipa server is actually using localhost as a resolver? For that we could create a unique DNS record and query it with dig or host commands (or even both as they use different algorithms for obtaining system resolver).

Another thing - the name of test does not reflect the actual check as we are not checking the resolv.conf in case of Fedora 33. Maybe it is better to call it according to test goal - something like "test_localhost_resolver_used".

[flo-renaud]

@flo-renaud
Do you think the new method Resolver.uses_localhost_as_dns is expected to be of any use outside of the test_resolvconf test case, especially in such strict form - checking for localhost only, not any ip address? I prefer not to place code which is not expected to be reused to common libraries like this one.

I don't agree with this comment. Resolver is also a clean way for a test to call methods without the hastle of checking which resolver is in place. If I move the logic into the test, it means that the test would need to be updated whenever a new Resolver implementation is available.

What do you think about changing the test from checking config files contents (which is not perfect in current state, see inline comment) to checking if ipa server is actually using localhost as a resolver? For that we could create a unique DNS record and query it with dig or host commands (or even both as they use different algorithms for obtaining system resolver).

I updated the current code with a pattern and regexp search. It will probably be way faster than adding a DNS record.

Another thing - the name of test does not reflect the actual check as we are not checking the resolv.conf in case of Fedora 33. Maybe it is better to call it according to test goal - something like "test_localhost_resolver_used".

Updated.

[flo-renaud]

Link to successful run with fedora33: Details
Link to successful run woth fedora32: Details
Removing temp commit.

[wladich]

I see your point. You also mentioned that checking resolver with dig can produce a false positive in case system uses some other nameserver which has a delegation to IPA server. So the changes mostly LGTM, please see the inline comment.

[flo-renaud]

The first commit (re-enable test in gating) could be backported up to ipa-4-8, but the second one can go to ipa-4-9 only (it needs the DNS Resolver commit that was backported to ipa-4-9).

[wladich]

Thank you, ACK

[flo-renaud]

master:

• a9b4ed4 ipatests: re-add test_dnssec.py::TestInstallDNSSECFirst in gating
• fb107b9 ipatests: fix TestInstalDNSSECFirst::test_resolvconf logic