Merge ~ajorgens/cloud-init:instance-identity into cloud-init:master
- Git
- lp:~ajorgens/cloud-init
- instance-identity
- Merge into master
Status: | Merged |
---|---|
Approved by: | Scott Moser |
Approved revision: | 0aa9fe8c552cc95335fc112ba2a9d2e52dea84ba |
Merged at revision: | 703241a3c50f2cfec21e7c8e90616c3378ebbea2 |
Proposed branch: | ~ajorgens/cloud-init:instance-identity |
Merge into: | cloud-init:master |
Diff against target: |
198 lines (+78/-22) 4 files modified
cloudinit/distros/__init__.py (+7/-8) cloudinit/ec2_utils.py (+30/-9) cloudinit/sources/DataSourceEc2.py (+25/-5) tests/unittests/test_datasource/test_aliyun.py (+16/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Chad Smith | Approve | ||
Scott Moser | Approve | ||
Server Team CI bot | continuous-integration | Approve | |
Review via email: mp+329657@code.launchpad.net |
Commit message
ec2: Use instance-identity doc for region and instance-id
The instance identity document is a better source for region information, partly because region isn't actually in meta-data at all, only availability-zone, which happens to be named similarly.
http://
Description of the change
Server Team CI bot (server-team-bot) wrote : | # |
Scott Moser (smoser) wrote : | # |
I'm not opposed to this in principal, I just have a few concerns. We need to make sure we continue to work on "clones" of ec2 and do not significantly affect them negatively. Chad's recent work to use a newer version of the metadata service and also our identifying the platform (Platforms.AWS) might allow us to more aggressively take advantage of "Genuine AWS".
Someo comments in line.
At very least we need some information and more thought at this point.
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
Andrew Jorgensen (ajorgens) wrote : | # |
I made a point of ensuring that the unit tests for Aliyun still worked. Lars had a PR for better mocking of meta-data service that might be helpful here.
If you have advice on which clones this needs to be tested against I'm happy to try.
Scott Moser (smoser) : | # |
Andrew Jorgensen (ajorgens) : | # |
Scott Moser (smoser) wrote : | # |
Can you adjust this to search under ec2 only if on Platforms.aws ?
see comment.
- 0aa9fe8... by Andrew Jorgensen
-
ec2: Use instance-identity doc for region and instance-id
The instance identity document is a better source for region information,
partly because region isn't actually in meta-data at all, only
availability-zone, which happens to be named similarly.Reviewed-by: Ethan Faust <email address hidden>
Reviewed-by: Cyle Riggs <email address hidden>
Reviewed-by: Tom Kirchner <email address hidden>
Reviewed-by: Matt Nierzwicki <email address hidden>
[<email address hidden>: rebase onto 0.7.9]
[<email address hidden>: changes per merge proposal discussions]
Andrew Jorgensen (ajorgens) wrote : | # |
Is this what you're looking for?
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:0aa9fe8c552
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
Ryan Harper (raharper) : | # |
Scott Moser (smoser) wrote : | # |
I approve.
Ryan was just suggesting your use of the word 'current' be replaced with 'current as of 2017-12-11' or something.
i'm capable of doing that change.
Chad Smith (chad.smith) wrote : | # |
+1 on this branch, tested on
As a follow-up. I'd like to see us persist this new dynamic/identity metadata route in the datasource's self.metadata dict because that is written to instance-data.json on which other consumers will eventually rely. The datasource should be persisting any metadata on which it relies to introspect the platform so that others could consume that same data.
Chad Smith (chad.smith) wrote : | # |
sorry tested on non-ec2 openstack 'lookalike' platform as well as Ec2 proper instances. Looks good and good fallback configuration if instance-identity doesn't exist as expected.
Preview Diff
1 | diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py | |||
2 | index d5becd1..e576c24 100755 | |||
3 | --- a/cloudinit/distros/__init__.py | |||
4 | +++ b/cloudinit/distros/__init__.py | |||
5 | @@ -45,6 +45,10 @@ OSFAMILIES = { | |||
6 | 45 | 45 | ||
7 | 46 | LOG = logging.getLogger(__name__) | 46 | LOG = logging.getLogger(__name__) |
8 | 47 | 47 | ||
9 | 48 | # This is a best guess regex, based on current EC2 AZs, it could break when | ||
10 | 49 | # Amazon adds new regions and new AZs. | ||
11 | 50 | _EC2_AZ_RE = re.compile('^[a-z][a-z]-(?:[a-z]+-)+[0-9][a-z]$') | ||
12 | 51 | |||
13 | 48 | 52 | ||
14 | 49 | @six.add_metaclass(abc.ABCMeta) | 53 | @six.add_metaclass(abc.ABCMeta) |
15 | 50 | class Distro(object): | 54 | class Distro(object): |
16 | @@ -686,18 +690,13 @@ def _get_package_mirror_info(mirror_info, data_source=None, | |||
17 | 686 | if not mirror_info: | 690 | if not mirror_info: |
18 | 687 | mirror_info = {} | 691 | mirror_info = {} |
19 | 688 | 692 | ||
20 | 689 | # ec2 availability zones are named cc-direction-[0-9][a-d] (us-east-1b) | ||
21 | 690 | # the region is us-east-1. so region = az[0:-1] | ||
22 | 691 | directions_re = '|'.join([ | ||
23 | 692 | 'central', 'east', 'north', 'northeast', 'northwest', | ||
24 | 693 | 'south', 'southeast', 'southwest', 'west']) | ||
25 | 694 | ec2_az_re = ("^[a-z][a-z]-(%s)-[1-9][0-9]*[a-z]$" % directions_re) | ||
26 | 695 | |||
27 | 696 | subst = {} | 693 | subst = {} |
28 | 697 | if data_source and data_source.availability_zone: | 694 | if data_source and data_source.availability_zone: |
29 | 698 | subst['availability_zone'] = data_source.availability_zone | 695 | subst['availability_zone'] = data_source.availability_zone |
30 | 699 | 696 | ||
32 | 700 | if re.match(ec2_az_re, data_source.availability_zone): | 697 | # ec2 availability zones are named cc-direction-[0-9][a-d] (us-east-1b) |
33 | 698 | # the region is us-east-1. so region = az[0:-1] | ||
34 | 699 | if _EC2_AZ_RE.match(data_source.availability_zone): | ||
35 | 701 | subst['ec2_region'] = "%s" % data_source.availability_zone[0:-1] | 700 | subst['ec2_region'] = "%s" % data_source.availability_zone[0:-1] |
36 | 702 | 701 | ||
37 | 703 | if data_source and data_source.region: | 702 | if data_source and data_source.region: |
38 | diff --git a/cloudinit/ec2_utils.py b/cloudinit/ec2_utils.py | |||
39 | index 723d6bd..d6c61e4 100644 | |||
40 | --- a/cloudinit/ec2_utils.py | |||
41 | +++ b/cloudinit/ec2_utils.py | |||
42 | @@ -1,6 +1,8 @@ | |||
43 | 1 | # Copyright (C) 2012 Yahoo! Inc. | 1 | # Copyright (C) 2012 Yahoo! Inc. |
44 | 2 | # Copyright (C) 2014 Amazon.com, Inc. or its affiliates. | ||
45 | 2 | # | 3 | # |
46 | 3 | # Author: Joshua Harlow <harlowja@yahoo-inc.com> | 4 | # Author: Joshua Harlow <harlowja@yahoo-inc.com> |
47 | 5 | # Author: Andrew Jorgensen <ajorgens@amazon.com> | ||
48 | 4 | # | 6 | # |
49 | 5 | # This file is part of cloud-init. See LICENSE file for license information. | 7 | # This file is part of cloud-init. See LICENSE file for license information. |
50 | 6 | 8 | ||
51 | @@ -164,14 +166,11 @@ def get_instance_userdata(api_version='latest', | |||
52 | 164 | return user_data | 166 | return user_data |
53 | 165 | 167 | ||
54 | 166 | 168 | ||
63 | 167 | def get_instance_metadata(api_version='latest', | 169 | def _get_instance_metadata(tree, api_version='latest', |
64 | 168 | metadata_address='http://169.254.169.254', | 170 | metadata_address='http://169.254.169.254', |
65 | 169 | ssl_details=None, timeout=5, retries=5, | 171 | ssl_details=None, timeout=5, retries=5, |
66 | 170 | leaf_decoder=None): | 172 | leaf_decoder=None): |
67 | 171 | md_url = url_helper.combine_url(metadata_address, api_version) | 173 | md_url = url_helper.combine_url(metadata_address, api_version, tree) |
60 | 172 | # Note, 'meta-data' explicitly has trailing /. | ||
61 | 173 | # this is required for CloudStack (LP: #1356855) | ||
62 | 174 | md_url = url_helper.combine_url(md_url, 'meta-data/') | ||
68 | 175 | caller = functools.partial(util.read_file_or_url, | 174 | caller = functools.partial(util.read_file_or_url, |
69 | 176 | ssl_details=ssl_details, timeout=timeout, | 175 | ssl_details=ssl_details, timeout=timeout, |
70 | 177 | retries=retries) | 176 | retries=retries) |
71 | @@ -189,7 +188,29 @@ def get_instance_metadata(api_version='latest', | |||
72 | 189 | md = {} | 188 | md = {} |
73 | 190 | return md | 189 | return md |
74 | 191 | except Exception: | 190 | except Exception: |
76 | 192 | util.logexc(LOG, "Failed fetching metadata from url %s", md_url) | 191 | util.logexc(LOG, "Failed fetching %s from url %s", tree, md_url) |
77 | 193 | return {} | 192 | return {} |
78 | 194 | 193 | ||
79 | 194 | |||
80 | 195 | def get_instance_metadata(api_version='latest', | ||
81 | 196 | metadata_address='http://169.254.169.254', | ||
82 | 197 | ssl_details=None, timeout=5, retries=5, | ||
83 | 198 | leaf_decoder=None): | ||
84 | 199 | # Note, 'meta-data' explicitly has trailing /. | ||
85 | 200 | # this is required for CloudStack (LP: #1356855) | ||
86 | 201 | return _get_instance_metadata(tree='meta-data/', api_version=api_version, | ||
87 | 202 | metadata_address=metadata_address, | ||
88 | 203 | ssl_details=ssl_details, timeout=timeout, | ||
89 | 204 | retries=retries, leaf_decoder=leaf_decoder) | ||
90 | 205 | |||
91 | 206 | |||
92 | 207 | def get_instance_identity(api_version='latest', | ||
93 | 208 | metadata_address='http://169.254.169.254', | ||
94 | 209 | ssl_details=None, timeout=5, retries=5, | ||
95 | 210 | leaf_decoder=None): | ||
96 | 211 | return _get_instance_metadata(tree='dynamic/instance-identity', | ||
97 | 212 | api_version=api_version, | ||
98 | 213 | metadata_address=metadata_address, | ||
99 | 214 | ssl_details=ssl_details, timeout=timeout, | ||
100 | 215 | retries=retries, leaf_decoder=leaf_decoder) | ||
101 | 195 | # vi: ts=4 expandtab | 216 | # vi: ts=4 expandtab |
102 | diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py | |||
103 | index 7bbbfb6..fd4b73b 100644 | |||
104 | --- a/cloudinit/sources/DataSourceEc2.py | |||
105 | +++ b/cloudinit/sources/DataSourceEc2.py | |||
106 | @@ -148,7 +148,12 @@ class DataSourceEc2(sources.DataSource): | |||
107 | 148 | return self.min_metadata_version | 148 | return self.min_metadata_version |
108 | 149 | 149 | ||
109 | 150 | def get_instance_id(self): | 150 | def get_instance_id(self): |
111 | 151 | return self.metadata['instance-id'] | 151 | if self.cloud_platform == Platforms.AWS: |
112 | 152 | # Prefer the ID from the instance identity document, but fall back | ||
113 | 153 | return self.identity.get( | ||
114 | 154 | 'instanceId', self.metadata['instance-id']) | ||
115 | 155 | else: | ||
116 | 156 | return self.metadata['instance-id'] | ||
117 | 152 | 157 | ||
118 | 153 | def _get_url_settings(self): | 158 | def _get_url_settings(self): |
119 | 154 | mcfg = self.ds_cfg | 159 | mcfg = self.ds_cfg |
120 | @@ -262,15 +267,27 @@ class DataSourceEc2(sources.DataSource): | |||
121 | 262 | @property | 267 | @property |
122 | 263 | def availability_zone(self): | 268 | def availability_zone(self): |
123 | 264 | try: | 269 | try: |
125 | 265 | return self.metadata['placement']['availability-zone'] | 270 | if self.cloud_platform == Platforms.AWS: |
126 | 271 | return self.identity.get( | ||
127 | 272 | 'availabilityZone', | ||
128 | 273 | self.metadata['placement']['availability-zone']) | ||
129 | 274 | else: | ||
130 | 275 | return self.metadata['placement']['availability-zone'] | ||
131 | 266 | except KeyError: | 276 | except KeyError: |
132 | 267 | return None | 277 | return None |
133 | 268 | 278 | ||
134 | 269 | @property | 279 | @property |
135 | 270 | def region(self): | 280 | def region(self): |
139 | 271 | az = self.availability_zone | 281 | if self.cloud_platform == Platforms.AWS: |
140 | 272 | if az is not None: | 282 | region = self.identity.get('region') |
141 | 273 | return az[:-1] | 283 | # Fallback to trimming the availability zone if region is missing |
142 | 284 | if self.availability_zone and not region: | ||
143 | 285 | region = self.availability_zone[:-1] | ||
144 | 286 | return region | ||
145 | 287 | else: | ||
146 | 288 | az = self.availability_zone | ||
147 | 289 | if az is not None: | ||
148 | 290 | return az[:-1] | ||
149 | 274 | return None | 291 | return None |
150 | 275 | 292 | ||
151 | 276 | @property | 293 | @property |
152 | @@ -351,6 +368,9 @@ class DataSourceEc2(sources.DataSource): | |||
153 | 351 | api_version, self.metadata_address) | 368 | api_version, self.metadata_address) |
154 | 352 | self.metadata = ec2.get_instance_metadata( | 369 | self.metadata = ec2.get_instance_metadata( |
155 | 353 | api_version, self.metadata_address) | 370 | api_version, self.metadata_address) |
156 | 371 | if self.cloud_platform == Platforms.AWS: | ||
157 | 372 | self.identity = ec2.get_instance_identity( | ||
158 | 373 | api_version, self.metadata_address).get('document', {}) | ||
159 | 354 | except Exception: | 374 | except Exception: |
160 | 355 | util.logexc( | 375 | util.logexc( |
161 | 356 | LOG, "Failed reading from metadata address %s", | 376 | LOG, "Failed reading from metadata address %s", |
162 | diff --git a/tests/unittests/test_datasource/test_aliyun.py b/tests/unittests/test_datasource/test_aliyun.py | |||
163 | index 82ee971..dc548eb 100644 | |||
164 | --- a/tests/unittests/test_datasource/test_aliyun.py | |||
165 | +++ b/tests/unittests/test_datasource/test_aliyun.py | |||
166 | @@ -47,6 +47,9 @@ def register_mock_metaserver(base_url, data): | |||
167 | 47 | elif isinstance(body, list): | 47 | elif isinstance(body, list): |
168 | 48 | register(base_url.rstrip('/'), '\n'.join(body) + '\n') | 48 | register(base_url.rstrip('/'), '\n'.join(body) + '\n') |
169 | 49 | elif isinstance(body, dict): | 49 | elif isinstance(body, dict): |
170 | 50 | if not body: | ||
171 | 51 | register(base_url.rstrip('/') + '/', 'not found', | ||
172 | 52 | status_code=404) | ||
173 | 50 | vals = [] | 53 | vals = [] |
174 | 51 | for k, v in body.items(): | 54 | for k, v in body.items(): |
175 | 52 | if isinstance(v, (str, list)): | 55 | if isinstance(v, (str, list)): |
176 | @@ -91,9 +94,22 @@ class TestAliYunDatasource(test_helpers.HttprettyTestCase): | |||
177 | 91 | self.metadata_address, | 94 | self.metadata_address, |
178 | 92 | self.ds.min_metadata_version, 'user-data') | 95 | self.ds.min_metadata_version, 'user-data') |
179 | 93 | 96 | ||
180 | 97 | # EC2 provides an instance-identity document which must return 404 here | ||
181 | 98 | # for this test to pass. | ||
182 | 99 | @property | ||
183 | 100 | def default_identity(self): | ||
184 | 101 | return {} | ||
185 | 102 | |||
186 | 103 | @property | ||
187 | 104 | def identity_url(self): | ||
188 | 105 | return os.path.join(self.metadata_address, | ||
189 | 106 | self.ds.min_metadata_version, | ||
190 | 107 | 'dynamic', 'instance-identity') | ||
191 | 108 | |||
192 | 94 | def regist_default_server(self): | 109 | def regist_default_server(self): |
193 | 95 | register_mock_metaserver(self.metadata_url, self.default_metadata) | 110 | register_mock_metaserver(self.metadata_url, self.default_metadata) |
194 | 96 | register_mock_metaserver(self.userdata_url, self.default_userdata) | 111 | register_mock_metaserver(self.userdata_url, self.default_userdata) |
195 | 112 | register_mock_metaserver(self.identity_url, self.default_identity) | ||
196 | 97 | 113 | ||
197 | 98 | def _test_get_data(self): | 114 | def _test_get_data(self): |
198 | 99 | self.assertEqual(self.ds.metadata, self.default_metadata) | 115 | self.assertEqual(self.ds.metadata, self.default_metadata) |
FAILED: Continuous integration, rev:0af07e16ac1 be5a98f83176012 e5e1af3a6d9b2d /code.launchpad .net/~ajorgens/ cloud-init/ +git/cloud- init/+merge/ 329657/ +edit-commit- message
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https:/
https:/ /jenkins. ubuntu. com/server/ job/cloud- init-ci/ 212/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild: /jenkins. ubuntu. com/server/ job/cloud- init-ci/ 212/rebuild
https:/