cloud-init

Merge ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master

  1. Git
  2. lp:~smoser/cloud-init
  3. bug/1683038-ec2-no-warn-on-explicit
  4. Merge into master
Proposed by Scott Moser on 2017-05-18
Status: Work in progress
Proposed branch: ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit
Merge into: cloud-init:master
Diff against target: 373 lines (+318/-10)
2 files modified
cloudinit/sources/DataSourceEc2.py (+44/-10)
tests/unittests/test_datasource/test_ec2.py (+274/-0)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve on 2017-05-31
Chad Smith Approve on 2017-05-24
cloud-init commiters 2017-05-18 Pending
Review via email: mp+324274@code.launchpad.net

Commit Message

Ec2: do not warn user of strict if Ec2 is explicitly set.

If the system is configured with datasource_list of either just
'Ec2' or 'Ec2' and 'None', then consider this as if they had set
strict_id = true. A change is made to the signature of
'warn_if_necessary' to make unit testing easier.

Additionally, fix a bug in ds-identify. If the user configured:
  datasource_list: ["Ec2", "None"]
then ds-identify would write
  datasource_list: ["Ec2", "None", "None"]
which would break the logic to avoid warning.

LP: #1683038

To post a comment you must log in.
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Reply
Ryan Harper (raharper) wrote :

Generally looks fine, one question w.r.t confirming explict_dslist check

Reply
Chad Smith (chad.smith) :
Reply
Chad Smith (chad.smith) :
Reply
Chad Smith (chad.smith) :
Reply
Scott Moser (smoser) wrote :

On Mon, 22 May 2017, Ryan Harper wrote:

> Generally looks fine, one question w.r.t confirming explict_dslist check
>
> Diff comments:
>
> > diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py
> > index 2f9c7ed..818a639 100644
> > --- a/cloudinit/sources/DataSourceEc2.py
> > +++ b/cloudinit/sources/DataSourceEc2.py
> > @@ -266,6 +267,12 @@ def parse_strict_mode(cfgval):
> > return mode, sleep
> >
> >
> > +def _is_explicit_dslist(dslist):
> > + if 'Ec2' not in dslist:
> > + return False
> > + return (len(dslist) == 1 or (len(dslist) == 2 and 'None' in dslist))
>
> if we know dslist is len == 2, then We could ensure it looks like:
> ['Ec2', None]
>
> Right? That's exactly what we're looking for? Verus this [None, 'Ec2']
> which would pass this test but not be what we're looking for IIUC

Yeah, I avoided checking for 'Ec2' because if this datasoruce is running,
then Ec2 is in the list. But I think it makes more sense to be explicit
Will cahnge as you sigguested.
  if dslist == ['Ec2', 'None'] or dslist == ['Ec2']

sm

Reply
Scott Moser (smoser) wrote :

some responses in line. i'll fix some things too. thanks for review.

Reply
Scott Moser (smoser) wrote :

I've pushed some new commits, please see and review.
thanks.

Reply
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Reply
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Reply
Chad Smith (chad.smith) :
review: Approve
Reply
Chad Smith (chad.smith) wrote :

pre-approved on assumption of a new unit test for behavior, if the following branch[1] lands before this one you could create a CiTestCase setting with_logs = True and self.assertIn(" not warning", self.logs.getvalue())

References:
[1] https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324450

Reply
Scott Moser (smoser) wrote :

I split the ds-identify portion out into
 https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324557
that way i can get it in now, and fix the ec2 warn portion itself with a nice test.

Reply
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Reply
~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit updated on 2017-05-30
e8fcbfd... by Scott Moser on 2017-05-30

Add a docstring for _collect_platform_data.

c4d0d40... by Scott Moser on 2017-05-30

initial test_ec2.py

Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Reply
~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit updated on 2017-05-31
092ea40... by Scott Moser on 2017-05-31

Add test for warn_if_necessary.

Change here also changes function signature of warn_if_necessary
to allow for more direct testing without using 'activate.'

Scott Moser (smoser) wrote :

I have to think some more on the 'FIXME' bits here.
But i'd like feedback on teh added unit tests.

Reply
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Reply
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Reply
~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit updated on 2017-05-31
a247285... by Scott Moser on 2017-05-31

add warning on unexpected situation strict_id=true and platform unknown.

Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Reply
Scott Moser (smoser) wrote :

I think that the signature change is fine.
the other fixme I think is best fixed by just having ds-identify write an entry in the config that says that it wrote a datasource_list, and how it did that.

Ie, it could declare that it essentially copied the datasource list like this:
   di_datasource_list_method: copy

or something to that affect. Then cloud-init could read that. A 'copy' there would be different than 'search'. The copy would show no warning, but the search would.

I'm not sure stil..
We *should* grab the other parts of this merge proposal and get them into main.

Reply
Scott Moser (smoser) wrote :

I grabbed the initial test portions of this merge proposal and put them into another
at https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/327534
note, those do not have the tests for WarnIfNecessary

Reply

Unmerged commits

a247285... by Scott Moser on 2017-05-31

add warning on unexpected situation strict_id=true and platform unknown.

092ea40... by Scott Moser on 2017-05-31

Add test for warn_if_necessary.

Change here also changes function signature of warn_if_necessary
to allow for more direct testing without using 'activate.'

c4d0d40... by Scott Moser on 2017-05-30

initial test_ec2.py

e8fcbfd... by Scott Moser on 2017-05-30

Add a docstring for _collect_platform_data.

e358e2f... by Scott Moser on 2017-05-18

Ec2: do not warn user of strict if Ec2 is explicitly set.

If the system is configured with datasource_list of either just
'Ec2' or 'Ec2' and 'None', then consider this as if they had set
strict_id = true.

LP: #1683038

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py
2index 9e2fdc0..d0a2f8e 100644
3--- a/cloudinit/sources/DataSourceEc2.py
4+++ b/cloudinit/sources/DataSourceEc2.py
5@@ -69,7 +69,8 @@ class DataSourceEc2(sources.DataSource):
6 LOG.debug("strict_mode: %s, cloud_platform=%s",
7 strict_mode, self.cloud_platform)
8 if strict_mode == "true" and self.cloud_platform == Platforms.UNKNOWN:
9- return False
10+ if not _is_explicit_dslist(self.sys_cfg.get('datasource_list')):
11+ return False
12 elif self.cloud_platform == Platforms.NO_EC2_METADATA:
13 return False
14
15@@ -229,10 +230,15 @@ class DataSourceEc2(sources.DataSource):
16 def activate(self, cfg, is_new_instance):
17 if not is_new_instance:
18 return
19- if self.cloud_platform == Platforms.UNKNOWN:
20- warn_if_necessary(
21- util.get_cfg_by_path(cfg, STRICT_ID_PATH, STRICT_ID_DEFAULT),
22- cfg)
23+ # FIXME: Think about this function signature change.
24+ # This change allows 'warn_if_necessary' unit test to do more
25+ # without testing 'activate'. I need to test though the upgrade
26+ # adn reboot path where an old obj.pkl file has 'activate' called
27+ # on it. I think it is fine... as the new code will have a
28+ # cloud_platform on it already.
29+ warn_if_necessary(
30+ util.get_cfg_by_path(cfg, STRICT_ID_PATH, STRICT_ID_DEFAULT),
31+ cfg, platform=self.cloud_platform)
32
33
34 def read_strict_mode(cfgval, default):
35@@ -273,7 +279,17 @@ def parse_strict_mode(cfgval):
36 return mode, sleep
37
38
39-def warn_if_necessary(cfgval, cfg):
40+def _is_explicit_dslist(dslist):
41+ """Is dslist explicitly configured to search Ec2 datasource only?"""
42+ if not dslist:
43+ return False
44+ return dslist in (['Ec2'], ['Ec2', 'None'])
45+
46+
47+def warn_if_necessary(cfgval, cfg, platform):
48+ if platform != Platforms.UNKNOWN:
49+ return
50+
51 try:
52 mode, sleep = parse_strict_mode(cfgval)
53 except ValueError as e:
54@@ -283,6 +299,18 @@ def warn_if_necessary(cfgval, cfg):
55 if mode == "false":
56 return
57
58+ dslist = cfg.get('datasource_list')
59+ if _is_explicit_dslist(cfg.get('datasource_list')):
60+ LOG.debug("mode=%s but explicit dslist (%s), not warning.",
61+ mode, dslist)
62+ return
63+
64+ if mode == "true":
65+ # Unknown platform and strict is true. get_data should have
66+ # returned false.
67+ LOG.warning("Unexpected error. strict_mode=%s platform=%s (%s)",
68+ mode, platform, cfgval)
69+
70 warnings.show_warning('non_ec2_md', cfg, mode=True, sleep=sleep)
71
72
73@@ -316,10 +344,16 @@ def identify_platform():
74
75
76 def _collect_platform_data():
77- # returns a dictionary with all lower case values:
78- # uuid: system-uuid from dmi or /sys/hypervisor
79- # uuid_source: 'hypervisor' (/sys/hypervisor/uuid) or 'dmi'
80- # serial: dmi 'system-serial-number' (/sys/.../product_serial)
81+ """Returns a dictionary of platform info from dmi or /sys/hypervisor.
82+
83+ Keys in the dictionary are as follows:
84+ uuid: system-uuid from dmi or /sys/hypervisor
85+ uuid_source: 'hypervisor' (/sys/hypervisor/uuid) or 'dmi'
86+ serial: dmi 'system-serial-number' (/sys/.../product_serial)
87+
88+ On Ec2 instances experimentation is that product_serial is upper case,
89+ and product_uuid is lower case. This returns lower case values for both.
90+ """
91 data = {}
92 try:
93 uuid = util.load_file("/sys/hypervisor/uuid").strip()
94diff --git a/tests/unittests/test_datasource/test_ec2.py b/tests/unittests/test_datasource/test_ec2.py
95new file mode 100644
96index 0000000..443eda9
97--- /dev/null
98+++ b/tests/unittests/test_datasource/test_ec2.py
99@@ -0,0 +1,274 @@
100+# This file is part of cloud-init. See LICENSE file for license information.
101+
102+import httpretty
103+import mock
104+
105+from .. import helpers as test_helpers
106+from cloudinit import helpers
107+from cloudinit.sources import DataSourceEc2 as ec2
108+
109+
110+# collected from api version 2009-04-04/ with
111+# python3 -c 'import json
112+# from cloudinit.ec2_utils import get_instance_metadata as gm
113+# print(json.dumps(gm("2009-04-04"), indent=1, sort_keys=True))'
114+DEFAULT_METADATA = {
115+ "ami-id": "ami-80861296",
116+ "ami-launch-index": "0",
117+ "ami-manifest-path": "(unknown)",
118+ "block-device-mapping": {"ami": "/dev/sda1", "root": "/dev/sda1"},
119+ "hostname": "ip-10-0-0-149",
120+ "instance-action": "none",
121+ "instance-id": "i-0052913950685138c",
122+ "instance-type": "t2.micro",
123+ "local-hostname": "ip-10-0-0-149",
124+ "local-ipv4": "10.0.0.149",
125+ "placement": {"availability-zone": "us-east-1b"},
126+ "profile": "default-hvm",
127+ "public-hostname": "",
128+ "public-ipv4": "107.23.188.247",
129+ "public-keys": {"brickies": ["ssh-rsa AAAAB3Nz....w== brickies"]},
130+ "reservation-id": "r-00a2c173fb5782a08",
131+ "security-groups": "wide-open"
132+}
133+
134+
135+def register_ssh_keys(rfunc, base_url, keys_data):
136+ """handle ssh key inconsistencies.
137+
138+ public-keys in the ec2 metadata is inconsistently formated compared
139+ to other entries.
140+ Given keys_data of {name1: pubkey1, name2: pubkey2}
141+
142+ This registers the following urls:
143+ base_url 0={name1}\n1={name2} # (for each name)
144+ base_url/ 0={name1}\n1={name2} # (for each name)
145+ base_url/0 openssh-key
146+ base_url/0/ openssh-key
147+ base_url/0/openssh-key {pubkey1}
148+ base_url/0/openssh-key/ {pubkey1}
149+ ...
150+ """
151+
152+ base_url = base_url.rstrip("/")
153+ odd_index = '\n'.join(
154+ ["{0}={1}".format(n, name)
155+ for n, name in enumerate(sorted(keys_data))])
156+
157+ rfunc(base_url, odd_index)
158+ rfunc(base_url + "/", odd_index)
159+
160+ for n, name in enumerate(sorted(keys_data)):
161+ val = keys_data[name]
162+ if isinstance(val, list):
163+ val = '\n'.join(val)
164+ burl = base_url + "/%s" % n
165+ rfunc(burl, "openssh-key")
166+ rfunc(burl + "/", "openssh-key")
167+ rfunc(burl + "/%s/openssh-key" % name, val)
168+ rfunc(burl + "/%s/openssh-key/" % name, val)
169+
170+
171+def register_mock_metaserver(base_url, data):
172+ """Register with httpretty a ec2 metadata like service serving 'data'.
173+
174+ If given a dictionary, it will populate urls under base_url for
175+ that dictionary. For example, input of
176+ {"instance-id": "i-abc", "mac": "00:16:3e:00:00:00"}
177+ populates
178+ base_url with 'instance-id\nmac'
179+ base_url/ with 'instance-id\nmac'
180+ base_url/instance-id with i-abc
181+ base_url/mac with 00:16:3e:00:00:00
182+ """
183+ def register_helper(register, base_url, body):
184+ if isinstance(body, str):
185+ register(base_url, body)
186+ elif isinstance(body, list):
187+ register(base_url.rstrip('/'), '\n'.join(body) + '\n')
188+ register(base_url.rstrip('/') + '/', '\n'.join(body) + '\n')
189+ elif isinstance(body, dict):
190+ vals = []
191+ for k, v in body.items():
192+ if k == 'public-keys':
193+ register_ssh_keys(
194+ register, base_url.rstrip('/') + '/public-keys/', v)
195+ continue
196+ if isinstance(v, (str, list)):
197+ suffix = k.rstrip('/')
198+ else:
199+ suffix = k.rstrip('/') + '/'
200+ vals.append(suffix)
201+ url = base_url.rstrip('/') + '/' + suffix
202+ register_helper(register, url, v)
203+ register(base_url.rstrip('/'), '\n'.join(vals) + '\n')
204+ register(base_url.rstrip('/') + '/', '\n'.join(vals) + '\n')
205+ elif body is None:
206+ register(base_url.rstrip('/'), 'not found', status_code=404)
207+
208+ def myreg(*argc, **kwargs):
209+ # print("register_url(%s, %s)" % (argc, kwargs))
210+ return httpretty.register_uri(httpretty.GET, *argc, **kwargs)
211+
212+ register_helper(myreg, base_url, data)
213+
214+
215+class TestEc2(test_helpers.HttprettyTestCase):
216+ valid_platform_data = {
217+ 'uuid': 'ec212f79-87d1-2f1d-588f-d86dc0fd5412',
218+ 'uuid_source': 'dmi',
219+ 'serial': 'ec212f79-87d1-2f1d-588f-d86dc0fd5412',
220+ }
221+
222+ def setUp(self):
223+ super(TestEc2, self).setUp()
224+ self.metadata_addr = ec2.DataSourceEc2.metadata_urls[0]
225+ self.api_ver = '2009-04-04'
226+
227+ @property
228+ def metadata_url(self):
229+ return '/'.join([self.metadata_addr, self.api_ver, 'meta-data', ''])
230+
231+ @property
232+ def userdata_url(self):
233+ return '/'.join([self.metadata_addr, self.api_ver, 'user-data'])
234+
235+ def _patch_add_cleanup(self, mpath, *args, **kwargs):
236+ p = mock.patch(mpath, *args, **kwargs)
237+ p.start()
238+ self.addCleanup(p.stop)
239+
240+ def _setup_ds(self, sys_cfg=None, platform_data=None, md=None, ud=None):
241+ distro = {}
242+ paths = helpers.Paths({})
243+ if sys_cfg is None:
244+ sys_cfg = {}
245+ ds = ec2.DataSourceEc2(sys_cfg=sys_cfg, distro=distro, paths=paths)
246+ if platform_data is not None:
247+ self._patch_add_cleanup(
248+ "cloudinit.sources.DataSourceEc2._collect_platform_data",
249+ return_value=platform_data)
250+
251+ if md:
252+ register_mock_metaserver(self.metadata_url, md)
253+ register_mock_metaserver(self.userdata_url, ud)
254+
255+ return ds
256+
257+ @httpretty.activate
258+ def test_valid_platform_with_strict_true(self):
259+ """Valid platform data should return true with strict_id true."""
260+ ds = self._setup_ds(
261+ platform_data=self.valid_platform_data,
262+ sys_cfg={'datasource': {'Ec2': {'strict_id': True}}},
263+ md=DEFAULT_METADATA)
264+ ret = ds.get_data()
265+ self.assertEqual(True, ret)
266+
267+ @httpretty.activate
268+ def test_valid_platform_with_strict_false(self):
269+ """Valid platform data should return true with strict_id false."""
270+ ds = self._setup_ds(
271+ platform_data=self.valid_platform_data,
272+ sys_cfg={'datasource': {'Ec2': {'strict_id': False}}},
273+ md=DEFAULT_METADATA)
274+ ret = ds.get_data()
275+ self.assertEqual(True, ret)
276+
277+ @httpretty.activate
278+ def test_unknown_platform_with_strict_true(self):
279+ """Unknown platform data with strict_id true should return False."""
280+ uuid = 'ab439480-72bf-11d3-91fc-b8aded755F9a'
281+ ds = self._setup_ds(
282+ platform_data={'uuid': uuid, 'uuid_source': 'dmi', 'serial': ''},
283+ sys_cfg={'datasource': {'Ec2': {'strict_id': True}}},
284+ md=DEFAULT_METADATA)
285+ ret = ds.get_data()
286+ self.assertEqual(False, ret)
287+
288+ @httpretty.activate
289+ def test_unknown_platform_with_strict_false(self):
290+ """Unknown platform data with strict_id false should return True."""
291+ uuid = 'ab439480-72bf-11d3-91fc-b8aded755F9a'
292+ ds = self._setup_ds(
293+ platform_data={'uuid': uuid, 'uuid_source': 'dmi', 'serial': ''},
294+ sys_cfg={'datasource': {'Ec2': {'strict_id': False}}},
295+ md=DEFAULT_METADATA)
296+ ret = ds.get_data()
297+ self.assertEqual(True, ret)
298+
299+
300+class TestWarnIfNecessary(test_helpers.CiTestCase):
301+ """Test warn_if_necessary behaves as expected.
302+
303+ warn_if_necessary is called with the value of the 'strict_id' setting.
304+ It should call show_warning if a warning should be shown."""
305+
306+ with_logs = True
307+
308+ @mock.patch("cloudinit.sources.DataSourceEc2.warnings.show_warning")
309+ def test_no_warnings_if_platform_identified(self, m_show_warning):
310+ """If platform is something known, then no warninings expected."""
311+ ec2.warn_if_necessary("warn", cfg={}, platform=ec2.Platforms.AWS)
312+ ec2.warn_if_necessary("warn", cfg={}, platform=ec2.Platforms.BRIGHTBOX)
313+ ec2.warn_if_necessary("warn", cfg={}, platform=ec2.Platforms.SEEDED)
314+ ec2.warn_if_necessary("warn", cfg={}, platform=ec2.Platforms.ALIYUN)
315+ self.assertEqual(0, m_show_warning.call_count)
316+
317+ @mock.patch("cloudinit.sources.DataSourceEc2.warnings.show_warning")
318+ def test_warn_if_platform_unknown_and_strict_true(self, m_show_warning):
319+ """If platform is unknown and strict is warn, warnings expected."""
320+ ec2.warn_if_necessary("warn", cfg={}, platform=ec2.Platforms.UNKNOWN)
321+ self.assertEqual(1, m_show_warning.call_count)
322+
323+ @mock.patch("cloudinit.sources.DataSourceEc2.warnings.show_warning")
324+ def test_no_warn_if_platform_unknown_and_not_strict(self, m_show_warning):
325+ """If platform is unknown but strict is false, no warnings."""
326+ ec2.warn_if_necessary("false", cfg={}, platform=ec2.Platforms.UNKNOWN)
327+ self.assertEqual(0, m_show_warning.call_count)
328+
329+ @mock.patch("cloudinit.sources.DataSourceEc2.warnings.show_warning")
330+ def test_no_warn_if_unknown_strict_true_explicit_ds(self, m_show_warning):
331+ """No warnings on strict_id=warn, platform=unknown, ec2 only."""
332+ ec2.warn_if_necessary("false", cfg={'datasource_list': ['Ec2']},
333+ platform=ec2.Platforms.UNKNOWN)
334+ self.assertEqual(0, m_show_warning.call_count)
335+
336+ @mock.patch("cloudinit.sources.DataSourceEc2.warnings.show_warning")
337+ def test_no_warn_if_unknown_strict_true_exp_and_none(self, m_show_warning):
338+ """No warnings on strict_id=warn, platform=unknown, Ec2, None.
339+
340+ FIXME: If ds-identify writes datasource_list due to
341+ strict_id = warn and DS_MAYBE, will this code path
342+ be taken? That *should* get a warning.
343+
344+ If datasource_list contains ['Ec2', 'None'], then no warning
345+ should be seen as this is explicit configuration of Ec2."""
346+ ec2.warn_if_necessary(
347+ "warn", cfg={'datasource_list': ['Ec2', 'None']},
348+ platform=ec2.Platforms.UNKNOWN)
349+ self.assertEqual(0, m_show_warning.call_count)
350+
351+ @mock.patch("cloudinit.sources.DataSourceEc2.warnings.show_warning")
352+ def test_warn_if_unknown_strict_true_noexp_and_none(self, m_show_warning):
353+ """Warn if strict_id=warn, platform=unknown, but not explicit list.
354+
355+ This is not an 'explicit' configuration because it has extra
356+ entry in its datasource_list.
357+ """
358+ ec2.warn_if_necessary(
359+ "warn", cfg={'datasource_list': ['OpenStack', 'Ec2', 'None']},
360+ platform=ec2.Platforms.UNKNOWN)
361+ self.assertNotIn('Unexpected error', self.logs.getvalue())
362+ self.assertEqual(1, m_show_warning.call_count)
363+
364+ @mock.patch("cloudinit.sources.DataSourceEc2.warnings.show_warning")
365+ def test_additional_warn_on_strict_and_unknown(self, m_show_warning):
366+ """If strict_id is true and platform is unknown, unexpected error."""
367+ ec2.warn_if_necessary(
368+ "true", cfg={}, platform=ec2.Platforms.UNKNOWN)
369+ self.assertIn('Unexpected error', self.logs.getvalue())
370+ self.assertEqual(1, m_show_warning.call_count)
371+
372+
373+# vi: ts=4 expandtab

Subscribers

People subscribed via source and target branches

to all changes: