pgsql Interface for charms.reactive

Merge ~cjwatson/interface-pgsql:fix-readiness-check-v2 into interface-pgsql:master

  1. Git
  2. lp:~cjwatson/interface-pgsql
  3. fix-readiness-check-v2
  4. Merge into master
Proposed by Colin Watson
Status: Merged
Approved by: Tom Haddon
Approved revision: 70d2da57d24b207797b8edfc66680f40a0526fbd
Merged at revision: 70d2da57d24b207797b8edfc66680f40a0526fbd
Proposed branch: ~cjwatson/interface-pgsql:fix-readiness-check-v2
Merge into: interface-pgsql:master
Diff against target: 160 lines (+45/-26)
3 files modified
requires.py (+14/-10)
tox.ini (+4/-0)
unit_tests/test_requires.py (+27/-16)
Reviewer Review Type Date Requested Status
Tom Haddon Approve
Guillermo Gonzalez (community) Approve
Review via email: mp+438333@code.launchpad.net

Commit message

Fix unit readiness check for v2 protocol

Description of the change

Commit 133726c646b31d63cf5c33e73322598ba1012b07 broke the readiness check in `ConnectionStrings._authorized` when using the v2 protocol (used particularly by proxies), because `self[name]` is only populated usefully for units using the v1 protocol. The previous code essentially seemed to be checking the bottom half of the `_cs` function, so I split that out into a new function and arranged to call it directly to avoid them getting out of sync again.

The unit tests seem to have bitrotted and I couldn't figure out how to get them to run, but I've tested this with direct relations to the `postgresql` charm, and Guillermo Gonzalez has tested this using the `external-services` charm which acts as a proxy.

To post a comment you must log in.
Revision history for this message
Guillermo Gonzalez (verterok) wrote :

Confirmed it work with a staging deploy of a store service
Thanks!

review: Approve
Revision history for this message
Tom Haddon (mthaddon) wrote :

In terms of the unit tests if I run `tox -e py3` I get the following: https://paste.ubuntu.com/p/dCm4k3DTwN/. However, if I set an env variable of CHARM_DIR to be the location of a local copy of the postgresql charm and run `TOX_TESTENV_PASSENV=CHARM_DIR tox -e py3` I get https://paste.ubuntu.com/p/jqW5NsGHSY/. The failing tests seem to have also been introduced in https://git.launchpad.net/interface-pgsql/commit/?id=133726c646b31d63cf5c33e73322598ba1012b07. I was able to get all tests passing (on current master) with this diff: https://paste.ubuntu.com/p/gsJwtK8ShX/. I'm not sure if that's the right approach though, let me know what you think?

Revision history for this message
Colin Watson (cjwatson) wrote :

@mthaddon Thanks for these suggestions - those were definitely useful clues. I went for a slightly different approach which I think is cleaner, since it means the tests are calling `_cs` in a more realistic way. What do you think?

Revision history for this message
Tom Haddon (mthaddon) wrote :

LGTM, thx

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/requires.py b/requires.py
2index 603591a..a141c85 100644
3--- a/requires.py
4+++ b/requires.py
5@@ -316,10 +316,7 @@ class ConnectionStrings(OrderedDict):
6 if 'master' not in d and 'standbys' not in d:
7 continue
8
9- # If we don't have a connection string for this unit, it
10- # isn't ready for us. We should wait until all units are
11- # ready.
12- if self[name] is None:
13+ if not _authorized(d, unit.relation.to_publish_raw):
14 return False
15
16 return True
17@@ -558,24 +555,31 @@ def _cs(unit):
18 if not all(d.values()):
19 return None
20
21+ if not _authorized(reldata, locdata):
22+ return None
23+ return ConnectionString(**d)
24+
25+
26+def _authorized(reldata, locdata):
27 # Cannot connect if egress subnets have not been authorized.
28 allowed_subnets = set(_csplit(reldata.get('allowed-subnets')))
29 if allowed_subnets:
30 my_egress = set(_csplit(locdata.get('egress-subnets')))
31 if not (my_egress <= allowed_subnets):
32- return None
33+ return False
34 else:
35 # If unit name has not been authorized. This is a legacy protocol,
36 # deprecated with Juju 2.3 and cross model relation support.
37 local_unit = hookenv.local_unit()
38 allowed_units = set(_csplit(reldata.get('allowed-units')))
39 if local_unit not in allowed_units:
40- return None # Not yet authorized
41+ return False # Not yet authorized
42
43 if locdata.get('database') and locdata.get('database', '') != reldata.get('database', ''):
44- return None # Requested database does not match yet
45+ return False # Requested database does not match yet
46 if locdata.get('roles') and locdata.get('roles', '') != reldata.get('roles', ''):
47- return None # Requested roles have not yet been assigned
48+ return False # Requested roles have not yet been assigned
49 if locdata.get('extensions') and locdata.get('extensions', '') != reldata.get('extensions', ''):
50- return None # Requested extensions have not yet been installed
51- return ConnectionString(**d)
52+ return False # Requested extensions have not yet been installed
53+
54+ return True
55diff --git a/tox.ini b/tox.ini
56index 607907a..a989253 100644
57--- a/tox.ini
58+++ b/tox.ini
59@@ -12,7 +12,11 @@ deps =
60 charmhelpers
61 charms.reactive
62 pytest
63+allowlist_externals = sh
64+commands_pre = sh -c '[ "$CHARM_DIR" ] || (echo "Set CHARM_DIR to a local clone of https://git.launchpad.net/postgresql-charm." >&2; exit 1)'
65 commands = py.test {posargs:-v unit_tests/}
66+passenv =
67+ CHARM_DIR
68
69 [testenv:lint]
70 deps =
71diff --git a/unit_tests/test_requires.py b/unit_tests/test_requires.py
72index eb55514..9a635ff 100644
73--- a/unit_tests/test_requires.py
74+++ b/unit_tests/test_requires.py
75@@ -13,12 +13,13 @@
76 # You should have received a copy of the GNU General Public License
77 # along with this program. If not, see <http://www.gnu.org/licenses/>.
78
79-from collections import UserDict
80 import os.path
81 import sys
82 import unittest
83 from unittest.mock import patch
84
85+from charms.reactive.endpoints import RelatedUnit, Relation
86+
87 sys.path.append(os.path.join(os.path.dirname(__file__), os.pardir))
88
89 import requires
90@@ -27,20 +28,30 @@ from requires import ConnectionString
91
92 class TestConnectionStringConstructor(unittest.TestCase):
93 def setUp(self):
94- self.reldata = UserDict({'allowed-units': 'client/0 client/9 client/8',
95- 'host': '10.9.8.7',
96- 'port': '5433',
97- 'database': 'mydata',
98- 'user': 'mememe',
99- 'password': 'secret'})
100- self.reldata.relname = 'relname'
101- self.reldata.relid = 'relname:42'
102+ self.reldata = {
103+ 'allowed-units': 'client/0,client/9,client/8',
104+ 'host': '10.9.8.7',
105+ 'port': '5433',
106+ 'database': 'mydata',
107+ 'user': 'mememe',
108+ 'password': 'secret',
109+ }
110
111 local_unit = self.patch('charmhelpers.core.hookenv.local_unit')
112 local_unit.return_value = 'client/9'
113
114- rels = self.patch('charmhelpers.context.Relations')
115- rels()['relname']['relname:42'].local = {'database': 'mydata'}
116+ def mock_relation_get(attribute=None, unit=None, rid=None, app=None):
117+ if unit == 'client/9':
118+ return {'database': 'mydata'}
119+ elif unit == 'postgresql/0':
120+ return self.reldata
121+ else:
122+ raise Exception('Unknown unit: %s' % unit)
123+
124+ relation_get = self.patch('charmhelpers.core.hookenv.relation_get')
125+ relation_get.side_effect = mock_relation_get
126+
127+ self.unit = RelatedUnit(Relation('relname:42'), 'postgresql/0')
128
129 def patch(self, dotpath):
130 patcher = patch(dotpath, autospec=True)
131@@ -49,7 +60,7 @@ class TestConnectionStringConstructor(unittest.TestCase):
132 return mock
133
134 def test_normal(self):
135- conn_str = requires._cs(self.reldata)
136+ conn_str = requires._cs(self.unit)
137 self.assertIsNotNone(conn_str)
138 self.assertIsInstance(conn_str, ConnectionString)
139 self.assertEqual(conn_str,
140@@ -58,16 +69,16 @@ class TestConnectionStringConstructor(unittest.TestCase):
141
142 def test_missing_attr(self):
143 del self.reldata['port']
144- self.assertIsNone(requires._cs(self.reldata))
145+ self.assertIsNone(requires._cs(self.unit))
146
147 def test_incorrect_database(self):
148 self.reldata['database'] = 'notherdb'
149- self.assertIsNone(requires._cs(self.reldata))
150+ self.assertIsNone(requires._cs(self.unit))
151
152 def test_unauthorized(self):
153 self.reldata['allowed-units'] = 'client/90'
154- self.assertIsNone(requires._cs(self.reldata))
155+ self.assertIsNone(requires._cs(self.unit))
156
157 def test_no_auth(self):
158 del self.reldata['allowed-units']
159- self.assertIsNone(requires._cs(self.reldata))
160+ self.assertIsNone(requires._cs(self.unit))

Subscribers

People subscribed via source and target branches