Merge lp:~allenap/maas/database-locks-rererevisited into lp:maas/trunk

Proposed by Gavin Panella on 2015-07-30
Status: Merged
Approved by: Gavin Panella on 2015-08-17
Approved revision: 4154
Merged at revision: 4197
Proposed branch: lp:~allenap/maas/database-locks-rererevisited
Merge into: lp:maas/trunk
Prerequisite: lp:~allenap/maas/database-locks-rerevisited
Diff against target: 247 lines (+60/-75)
4 files modified
src/maasserver/dns/config.py (+3/-7)
src/maasserver/dns/tests/test_config.py (+16/-29)
src/maasserver/locks.py (+2/-2)
src/maasserver/security.py (+39/-37)
To merge this branch: bzr merge lp:~allenap/maas/database-locks-rererevisited
Reviewer Review Type Date Requested Status
Blake Rouse 2015-07-30 Approve on 2015-07-30
Review via email: mp+266448@code.launchpad.net

Commit message

Use non-transactional advisory locks for critical regions of DNS and security activity.

Previously locks were obtained after transactions had begun. This meant that, once the lock was acquired, the transaction may be working with stale data. This causes breakage when dealing with the shared-secret on the filesystem and when writing DNS zones with old data.

To post a comment you must log in.
Blake Rouse (blake-rouse) wrote :

Looks good as well. No bug for this either?

review: Approve
Gavin Panella (allenap) wrote :

> Looks good as well. No bug for this either?

I'm going to go with bug 1459168. Thanks again!

Gavin Panella (allenap) wrote :

FTR, I QA'ed this at home first, and it seems to work as intended.

Andres Rodriguez (andreserl) wrote :

Does it make sense to backport this to 1.8 ?

MAAS Lander (maas-lander) wrote :
Download full text (93.9 KiB)

The attempt to merge lp:~allenap/maas/database-locks-rererevisited into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://security.ubuntu.com trusty-security Release
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm pep8 phantomjs postgresql pyflakes python-apt python-bson python-bzrlib python-convoy python-coverage python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-mock python-netaddr python-netifaces python-nose python-oauth python-openssl python-paramiko python-pexpect python-pip python-pocket-lint python-psycopg2 python-pyinotify python-pyparsing python-seamicroclient python...

4154. By Gavin Panella on 2015-08-17

Remove lock tests for the dns_*_now() functions; only Changes.apply() grabs the lock now.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/dns/config.py'
2--- src/maasserver/dns/config.py 2015-07-01 18:58:34 +0000
3+++ src/maasserver/dns/config.py 2015-08-17 16:24:03 +0000
4@@ -14,11 +14,8 @@
5 __metaclass__ = type
6 __all__ = [
7 'dns_add_zones',
8- 'dns_add_zones_now',
9 'dns_update_all_zones',
10- 'dns_update_all_zones_now',
11 'dns_update_zones',
12- 'dns_update_zones_now',
13 ]
14
15 from itertools import (
16@@ -46,6 +43,7 @@
17 from maasserver.utils.orm import (
18 post_commit,
19 transactional,
20+ with_connection,
21 )
22 from provisioningserver.dns.actions import (
23 bind_reconfigure,
24@@ -96,7 +94,6 @@
25 DNS_DEFER_UPDATES = True
26
27
28-@synchronised(locks.dns)
29 def dns_add_zones_now(clusters):
30 """Add zone files for the given cluster(s), and serve them.
31
32@@ -135,7 +132,6 @@
33 return None
34
35
36-@synchronised(locks.dns)
37 def dns_update_zones_now(clusters):
38 """Update the zone files for the given cluster(s).
39
40@@ -169,7 +165,6 @@
41 return None
42
43
44-@synchronised(locks.dns)
45 def dns_update_all_zones_now(reload_retry=False, force=False):
46 """Update all zone files for all clusters.
47
48@@ -270,8 +265,9 @@
49 self.hook.addBoth(callOut, self.reset)
50 return self.hook
51
52+ @with_connection # Needed by the following lock.
53+ @synchronised(locks.dns) # Lock before beginning transaction.
54 @transactional
55- @synchronised(locks.dns)
56 def apply(self):
57 """Apply all requested changes.
58
59
60=== modified file 'src/maasserver/dns/tests/test_config.py'
61--- src/maasserver/dns/tests/test_config.py 2015-07-01 19:02:36 +0000
62+++ src/maasserver/dns/tests/test_config.py 2015-08-17 16:24:03 +0000
63@@ -187,6 +187,22 @@
64 management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
65 status=NODEGROUP_STATUS.ENABLED)
66
67+ def test__zone_changes_applied_while_holding_dns_lock(self):
68+
69+ def check_dns_is_locked(reload_retry, force):
70+ self.assertTrue(locks.dns.is_locked(), "locks.dns isn't locked")
71+ return False # Prevent dns_update_zones_now from continuing.
72+
73+ dns_update_all_zones_now = self.patch_autospec(
74+ dns_config_module, "dns_update_all_zones_now")
75+ dns_update_all_zones_now.side_effect = check_dns_is_locked
76+
77+ consolidator.update_all_zones()
78+ post_commit_hooks.fire()
79+ self.assertThat(
80+ dns_update_all_zones_now, MockCalledOnceWith(
81+ reload_retry=ANY, force=ANY))
82+
83 def test__added_zones_applied_post_commit(self):
84 dns_add_zones_now = self.patch_autospec(
85 dns_config_module, "dns_add_zones_now")
86@@ -413,35 +429,6 @@
87 fqdn, ','.join(reverse_lookup_result)))
88
89
90-class TestDNSModificationUsesLocks(TestDNSServer):
91-
92- def patch_is_dns_enabled(self):
93- def check_dns_is_locked():
94- self.assertTrue(
95- locks.dns.is_locked(), "locks.dns isn't locked")
96- return False # Prevent dns_update_zones_now from continuing.
97-
98- is_dns_enabled = self.patch_autospec(
99- dns_config_module, 'is_dns_enabled')
100- is_dns_enabled.side_effect = check_dns_is_locked
101- return is_dns_enabled
102-
103- def test_dns_update_zones_now_uses_lock(self):
104- is_dns_enabled = self.patch_is_dns_enabled()
105- dns_config_module.dns_update_zones_now(sentinel.nodegroup)
106- self.assertThat(is_dns_enabled, MockCalledOnceWith())
107-
108- def test_dns_add_zones_now_uses_lock(self):
109- is_dns_enabled = self.patch_is_dns_enabled()
110- dns_config_module.dns_add_zones_now(sentinel.nodegroup)
111- self.assertThat(is_dns_enabled, MockCalledOnceWith())
112-
113- def test_dns_update_all_zones_now_config_uses_lock(self):
114- is_dns_enabled = self.patch_is_dns_enabled()
115- dns_config_module.dns_update_all_zones_now()
116- self.assertThat(is_dns_enabled, MockCalledOnceWith())
117-
118-
119 class TestDNSConfigModifications(TestDNSServer):
120
121 def test_dns_add_zones_now_loads_dns_zone(self):
122
123=== modified file 'src/maasserver/locks.py'
124--- src/maasserver/locks.py 2015-08-17 13:22:49 +0000
125+++ src/maasserver/locks.py 2015-08-17 16:24:03 +0000
126@@ -28,7 +28,7 @@
127
128 # Lock around performing critical security-related operations, like
129 # generating or signing certificates.
130-security = DatabaseXactLock(2)
131+security = DatabaseLock(2)
132
133 # Lock used when starting up the event-loop.
134 eventloop = DatabaseLock(3)
135@@ -38,7 +38,7 @@
136 import_images = DatabaseXactLock(4)
137
138 # Lock to prevent concurrent changes to DNS configuration.
139-dns = DatabaseXactLock(6)
140+dns = DatabaseLock(6)
141
142 # Lock to prevent concurrent acquisition of nodes.
143 node_acquire = DatabaseXactLock(7)
144
145=== modified file 'src/maasserver/security.py'
146--- src/maasserver/security.py 2015-05-07 18:14:38 +0000
147+++ src/maasserver/security.py 2015-08-17 16:24:03 +0000
148@@ -22,7 +22,11 @@
149
150 from maasserver import locks
151 from maasserver.models.config import Config
152-from maasserver.utils.orm import transactional
153+from maasserver.utils import synchronised
154+from maasserver.utils.orm import (
155+ transactional,
156+ with_connection,
157+)
158 from provisioningserver.security import (
159 get_shared_secret_filesystem_path,
160 get_shared_secret_from_filesystem,
161@@ -72,52 +76,50 @@
162
163
164 @synchronous
165+@with_connection # Needed by the following lock.
166+@synchronised(locks.security) # Region-wide lock.
167 @transactional
168 def get_region_certificate():
169 cert = load_region_certificate()
170 if cert is None:
171- with locks.security:
172- # Load again, while holding the security lock.
173- cert = load_region_certificate()
174- if cert is None:
175- cert = generate_region_certificate()
176- save_region_certificate(cert)
177+ cert = generate_region_certificate()
178+ save_region_certificate(cert)
179 return cert
180
181
182 @synchronous
183+@with_connection # Needed by the following lock.
184+@synchronised(locks.security) # Region-wide lock.
185 @transactional
186 def get_shared_secret_txn():
187- # Get a full region-wide lock.
188- with locks.security:
189- # Load secret from database, if it exists.
190- secret_in_db_hex = Config.objects.get_config("rpc_shared_secret")
191- if secret_in_db_hex is None:
192- secret_in_db = None
193- else:
194- secret_in_db = to_bin(secret_in_db_hex)
195- # Load secret from the filesystem, if it exists.
196- secret_on_fs = get_shared_secret_from_filesystem()
197-
198- if secret_in_db is None and secret_on_fs is None:
199- secret = os.urandom(16) # 16-bytes of crypto-standard noise.
200- Config.objects.set_config("rpc_shared_secret", to_hex(secret))
201- set_shared_secret_on_filesystem(secret)
202- elif secret_in_db is None:
203- secret = secret_on_fs
204- Config.objects.set_config("rpc_shared_secret", to_hex(secret))
205- elif secret_on_fs is None:
206- secret = secret_in_db
207- set_shared_secret_on_filesystem(secret)
208- elif secret_in_db == secret_on_fs:
209- secret = secret_in_db # or secret_on_fs.
210- else:
211- raise AssertionError(
212- "The secret stored in the database does not match the secret "
213- "stored on the filesystem at %s. Please investigate." %
214- get_shared_secret_filesystem_path())
215-
216- return secret
217+ # Load secret from database, if it exists.
218+ secret_in_db_hex = Config.objects.get_config("rpc_shared_secret")
219+ if secret_in_db_hex is None:
220+ secret_in_db = None
221+ else:
222+ secret_in_db = to_bin(secret_in_db_hex)
223+ # Load secret from the filesystem, if it exists.
224+ secret_on_fs = get_shared_secret_from_filesystem()
225+
226+ if secret_in_db is None and secret_on_fs is None:
227+ secret = os.urandom(16) # 16-bytes of crypto-standard noise.
228+ Config.objects.set_config("rpc_shared_secret", to_hex(secret))
229+ set_shared_secret_on_filesystem(secret)
230+ elif secret_in_db is None:
231+ secret = secret_on_fs
232+ Config.objects.set_config("rpc_shared_secret", to_hex(secret))
233+ elif secret_on_fs is None:
234+ secret = secret_in_db
235+ set_shared_secret_on_filesystem(secret)
236+ elif secret_in_db == secret_on_fs:
237+ secret = secret_in_db # or secret_on_fs.
238+ else:
239+ raise AssertionError(
240+ "The secret stored in the database does not match the secret "
241+ "stored on the filesystem at %s. Please investigate." %
242+ get_shared_secret_filesystem_path())
243+
244+ return secret
245
246
247 @asynchronous(timeout=10)