Hi Martin, Here as per discussion on monday meeting, I removed from the patch everything unrelated to the workaround itself and added a separate test that runs a standard dnssec-enabled zone and queries it without restarting of named. The test is marked as xfail with strict=True, which means, once the bug is fixed it will start to unexpectedly pass causing the whole testrun to fail, so we will know precisely when to revert this change. This test is (apart from restarting named) an exact copy of the existing one, so it would be safe to remove it.
Patch 0038 was rebased. On 05/06/2016 04:11 PM, Martin Basti wrote: > > > On 06.05.2016 14:58, Oleg Fayans wrote: >> >> On 05/06/2016 11:42 AM, Martin Basti wrote: >>> >>> On 06.05.2016 11:14, Oleg Fayans wrote: >>>> On 05/06/2016 09:48 AM, Martin Basti wrote: >>>>> On 06.05.2016 09:36, Oleg Fayans wrote: >>>>>> Tests are finally stable: >>>>>> >>>>>> ============================= test session starts >>>>>> ============================== >>>>>> platform linux2 -- Python 2.7.11 -- py-1.4.30 -- pytest-2.7.3 >>>>>> rootdir: /usr/lib/python2.7/site-packages/ipatests, inifile: >>>>>> pytest.ini >>>>>> plugins: multihost, sourceorder >>>>>> collected 8 items >>>>>> >>>>>> test_integration/test_dnssec.py ........ >>>>>> >>>>>> ========================= 8 passed in 5561.48 seconds >>>>>> ========================== >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>> PATCH 38 LGTM >>>>> >>>>> PATCH 37 IIRC I refused to accept workaround for this issue when you >>>>> send this (almost the same) patch for first time, are you sure that we >>>>> want to hide real issues in tests, to just have green color there? >>>>> >>>> The underlying issue is 7 months old. Latest update in the issue from >>>> Peter Spacek is: "I do not have time to investigate this issue now", >>>> which means, that it will stay there for unpredictable amount of time >>>> more. If we want to have a "green" jenkins that actually tests existing >>>> features, we have to accept workarounds for such long-term issues >>>> >>>>> Martin >>> I leave decision if push this or not to different people, however I will >>> do review on this. >>> >>> >>> NACK >>> >>> 1) >>> Why do you change sleep time? How is it related to workaround? >>> >>> - time.sleep(20) # sleep a bit until LDAP changes are applied to >>> DNS >>> + time.sleep(10) # sleep a bit until LDAP changes are applied to >>> DNS >> 10 seconds proved to be enough even in our super-slow brno rhevm lab > Unrelated to workaround, send it as new patch > >> >>> >>> 2) >>> why do you removes sleep from several places? How is this related to >>> workaround? >>> @@ -281,13 +302,19 @@ class TestInstallDNSSECFirst(IntegrationTest): >>> "--a-rec=" + self.master.ip >>> ] >>> self.master.run_command(args) >>> - time.sleep(10) # sleep a bit until data are provided by >>> bind-dyndb-ldap >>> >>> args = [ >>> "ipa", "dnsrecord-add", root_zone, >>> self.master.domain.name, >>> "--ns-rec=" + self.master.hostname >>> ] >>> self.master.run_command(args) >> Because it's more reasonable to make changes on all hosts and then wait. > Now, this is NACK. > Yes, that is true wait when all changes are done, but you completely > misunderstood why that sleep is there. > Sleep is there because an A record was added, and it took time until > this change is propagated to DNS, without A record following command > (adding NS record) will fail. > Bind-dyndb-ldap feels happy so it can work today, but it may not work > tomorrow. > >> >>> 3) >>> You restart the same replica twice >>> + time.sleep(10) # sleep a bit until LDAP changes are applied to >>> DNS >>> + # A workaround for ticket N 5348 >>> + self.replicas[0].run_command(["systemctl", "restart", >>> + "named-pkcs11.service"]) >>> + self.replicas[0].run_command(["systemctl", "restart", >>> + "named-pkcs11.service"]) >>> + # End of workaround >> My bad. >> >>> 4) >>> Can you create a function doing workaround instead of copying the same >>> code several times? >>> >>> something like >>> def restart_named(*args): >>> for host in args: >>> # host$ systemctl restart named-pkcs11 >>> >>> where args are instances self.host, or self.master, or self.replica >> Now, that's a marvelous idea! Implemented. And put the sleep interval in >> it too just to keep it in one place > I wanted *args to have > restart_named(self.replicas[0], self.replicas[1]) > instead creating an additional list > restart_named([self.replicas[0], self.replicas[1]]) > but whatever it works. > >> >>> 5) >>> Why did you removed this comment? >>> - # wait until zone is signed >> Because this is kin of obvious from the name of the function: >> wait_until_record_is_signed > Unrelated to this workaround, send it as separate patch "Removing > mbasti's useless DNSSEC comments" > >> >>> 6) >>> I'm not sure, but is sleep there needed? Because restart of named will >>> download all LDAP data again. >> It turns out, without this interval the restart does not always help >> >>> If timeout is required maybe reason why >>> time is there should be rephrased, to something like, make sure the >>> dnssec keys were exported for named, or so. >> Rephrased >> >>> + time.sleep(10) # sleep a bit until LDAP changes are applied to >>> DNS >>> + # A workaround for ticket N 5348 >>> + self.master.run_command(["systemctl", "restart", >>> + "named-pkcs11.service"]) >>> + self.replicas[0].run_command(["systemctl", "restart", >>> + "named-pkcs11.service"]) >>> + # End of workaround >>> >>> Martin^2 > -- Oleg Fayans Quality Engineer FreeIPA team RedHat.
From 6662dd866083690c810003b0b7699a2722553487 Mon Sep 17 00:00:00 2001 From: Oleg Fayans <ofay...@redhat.com> Date: Wed, 11 May 2016 12:08:38 +0200 Subject: [PATCH] A workaround for ticket N 5348 A freshly created dnssec-enabled zone does not always display the signature until you restart named-pkcs11. Added restarting of this service after each dnssec-enabled zone. https://fedorahosted.org/freeipa/ticket/5348 --- ipatests/test_integration/test_dnssec.py | 97 +++++++++++++++++++++++++++++--- 1 file changed, 90 insertions(+), 7 deletions(-) diff --git a/ipatests/test_integration/test_dnssec.py b/ipatests/test_integration/test_dnssec.py index e90fb1f477ab50050f619399ee168c0e4b248ac2..35cf8636dd8841473d46fbf3322052662f4271d4 100644 --- a/ipatests/test_integration/test_dnssec.py +++ b/ipatests/test_integration/test_dnssec.py @@ -6,6 +6,7 @@ import dns.dnssec import dns.resolver import dns.name import time +import pytest from ipatests.test_integration.base import IntegrationTest from ipatests.test_integration import tasks @@ -71,6 +72,14 @@ def wait_until_record_is_signed(nameserver, record, log, rtype="SOA", return False +def restart_named(*args): + # A workaround for ticket N 5348 + time.sleep(20) # wait till dnssec key is exported to named + for host in args: + host.run_command(["systemctl", "restart", + "named-pkcs11.service"]) + + class TestInstallDNSSECLast(IntegrationTest): """Simple DNSSEC test @@ -105,6 +114,7 @@ class TestInstallDNSSECLast(IntegrationTest): ] self.master.run_command(args) + restart_named(self.master, self.replicas[0]) # test master assert wait_until_record_is_signed( self.master.ip, test_zone, self.log, timeout=100 @@ -125,6 +135,7 @@ class TestInstallDNSSECLast(IntegrationTest): ] self.replicas[0].run_command(args) + restart_named(self.replicas[0]) # test replica assert wait_until_record_is_signed( self.replicas[0].ip, test_zone_repl, self.log, timeout=300 @@ -170,8 +181,7 @@ class TestInstallDNSSECLast(IntegrationTest): ] self.master.run_command(args) - time.sleep(20) # sleep a bit until LDAP changes are applied to DNS - + restart_named(self.master) # test master assert wait_until_record_is_signed( self.master.ip, test_zone, self.log, timeout=100 @@ -219,7 +229,7 @@ class TestInstallDNSSECLast(IntegrationTest): ] self.master.run_command(args) - time.sleep(20) # sleep a bit until LDAP changes are applied to DNS + restart_named(self.master, self.replicas[0]) # test master assert wait_until_record_is_signed( @@ -235,6 +245,78 @@ class TestInstallDNSSECLast(IntegrationTest): self.log, rtype="DNSKEY").rrset assert dnskey_old != dnskey_new, "DNSKEY should be different" + +class TestZoneSigningWithoutNamedRestart(IntegrationTest): + """Test whether https://fedorahosted.org/freeipa/ticket/5348 is already + fixed. If the issue is not fixed, the test will expectedly fail. When + fixed, it will pass, which will cause the whole run to become "red" + """ + num_replicas = 1 + topology = 'star' + + @classmethod + def install(cls, mh): + tasks.install_master(cls.master, setup_dns=False) + args = [ + "ipa-dns-install", + "--dnssec-master", + "--forwarder", cls.master.config.dns_forwarder, + "-U", + ] + cls.master.run_command(args) + + tasks.install_replica(cls.master, cls.replicas[0], setup_dns=True) + + # backup trusted key + tasks.backup_file(cls.master, paths.DNSSEC_TRUSTED_KEY) + tasks.backup_file(cls.replicas[0], paths.DNSSEC_TRUSTED_KEY) + + @classmethod + def uninstall(cls, mh): + # restore trusted key + tasks.restore_files(cls.master) + tasks.restore_files(cls.replicas[0]) + + super(TestZoneSigningWithoutNamedRestart, cls).uninstall(mh) + + @pytest.mark.xfail(strict=True) + def test_sign_root_zone_no_named_restart(self): + args = [ + "ipa", "dnszone-add", root_zone, "--dnssec", "true", + "--skip-overlap-check", + ] + self.master.run_command(args) + + # make BIND happy: add the glue record and delegate zone + args = [ + "ipa", "dnsrecord-add", root_zone, self.master.hostname, + "--a-rec=" + self.master.ip + ] + self.master.run_command(args) + args = [ + "ipa", "dnsrecord-add", root_zone, self.replicas[0].hostname, + "--a-rec=" + self.replicas[0].ip + ] + self.master.run_command(args) + + time.sleep(10) # sleep a bit until data are provided by bind-dyndb-ldap + + args = [ + "ipa", "dnsrecord-add", root_zone, self.master.domain.name, + "--ns-rec=" + self.master.hostname + ] + self.master.run_command(args) + # test master + assert wait_until_record_is_signed( + self.master.ip, root_zone, self.log, timeout=100 + ), "Zone %s is not signed (master)" % root_zone + + # test replica + assert wait_until_record_is_signed( + self.replicas[0].ip, root_zone, self.log, timeout=300 + ), "Zone %s is not signed (replica)" % root_zone + + class TestInstallDNSSECFirst(IntegrationTest): """Simple DNSSEC test @@ -288,7 +370,7 @@ class TestInstallDNSSECFirst(IntegrationTest): "--ns-rec=" + self.master.hostname ] self.master.run_command(args) - + restart_named(self.master, self.replicas[0]) # test master assert wait_until_record_is_signed( self.master.ip, root_zone, self.log, timeout=100 @@ -319,7 +401,7 @@ class TestInstallDNSSECFirst(IntegrationTest): "--ns-rec=" + self.master.hostname ] self.master.run_command(args) - + restart_named(self.master, self.replicas[0]) # wait until zone is signed assert wait_until_record_is_signed( self.master.ip, example_test_zone, self.log, timeout=100 @@ -457,6 +539,7 @@ class TestMigrateDNSSECMaster(IntegrationTest): self.master.run_command(args) + restart_named(self.master, self.replicas[0]) # wait until zone is signed assert wait_until_record_is_signed( self.master.ip, example_test_zone, self.log, timeout=100 @@ -513,7 +596,7 @@ class TestMigrateDNSSECMaster(IntegrationTest): "--skip-overlap-check", ] self.replicas[0].run_command(args) - + restart_named(self.master, self.replicas[0]) # wait until zone is signed assert wait_until_record_is_signed( self.replicas[0].ip, example2_test_zone, self.log, timeout=100 @@ -546,7 +629,7 @@ class TestMigrateDNSSECMaster(IntegrationTest): "--skip-overlap-check", ] self.replicas[1].run_command(args) - + restart_named(self.replicas[0], self.replicas[1]) # wait until zone is signed assert wait_until_record_is_signed( self.replicas[1].ip, example3_test_zone, self.log, timeout=200 -- 1.8.3.1
From c7cdc9b8239d3d42ddb9c4c5bac4d1060fb4534e Mon Sep 17 00:00:00 2001 From: Oleg Fayans <ofay...@redhat.com> Date: Wed, 11 May 2016 12:11:09 +0200 Subject: [PATCH] Added necessary A record for the replica to root zone A master can only be delegated a zone authority, if this zone contains A records of the master and ALL replicas https://fedorahosted.org/freeipa/ticket/5848 --- ipatests/test_integration/test_dnssec.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ipatests/test_integration/test_dnssec.py b/ipatests/test_integration/test_dnssec.py index 35cf8636dd8841473d46fbf3322052662f4271d4..554e96c638fcac03379ed17cbc4d9ac1311ab7ea 100644 --- a/ipatests/test_integration/test_dnssec.py +++ b/ipatests/test_integration/test_dnssec.py @@ -363,6 +363,11 @@ class TestInstallDNSSECFirst(IntegrationTest): "--a-rec=" + self.master.ip ] self.master.run_command(args) + args = [ + "ipa", "dnsrecord-add", root_zone, self.replicas[0].hostname, + "--a-rec=" + self.replicas[0].ip + ] + self.master.run_command(args) time.sleep(10) # sleep a bit until data are provided by bind-dyndb-ldap args = [ -- 1.8.3.1
-- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
