Merge ~ajorgens/cloud-init:instance-identity into cloud-init:master

[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.

http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instance-identity-documents.html

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:0af07e16ac1be5a98f83176012e5e1af3a6d9b2d
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://code.launchpad.net/~ajorgens/cloud-init/+git/cloud-init/+merge/329657/+edit-commit-message

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:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/212/rebuild

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

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.

review: Needs Information

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/270/
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://jenkins.ubuntu.com/server/job/cloud-init-ci/270/rebuild

review: Approve (continuous-integration)

[Andrew Jorgensen (ajorgens)]

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)]

Can you adjust this to search under ec2 only if on Platforms.aws ?

see comment.

0aa9fe8... by Andrew Jorgensen on 2017-11-27

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)]

Is this what you're looking for?

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:0aa9fe8c552cc95335fc112ba2a9d2e52dea84ba
https://jenkins.ubuntu.com/server/job/cloud-init-ci/549/
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://jenkins.ubuntu.com/server/job/cloud-init-ci/549/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

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.

review: Approve

[Chad Smith (chad.smith)]

+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.

review: Approve

[Chad Smith (chad.smith)]

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.

diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
index d5becd1..e576c24 100755
--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -45,6 +45,10 @@ OSFAMILIES = {
 
 LOG = logging.getLogger(__name__)
 
+# This is a best guess regex, based on current EC2 AZs, it could break when
+# Amazon adds new regions and new AZs.
+_EC2_AZ_RE = re.compile('^[a-z][a-z]-(?:[a-z]+-)+[0-9][a-z]$')
+
 
 @six.add_metaclass(abc.ABCMeta)
 class Distro(object):
@@ -686,18 +690,13 @@ def _get_package_mirror_info(mirror_info, data_source=None,
     if not mirror_info:
         mirror_info = {}
 
-    # ec2 availability zones are named cc-direction-[0-9][a-d] (us-east-1b)
-    # the region is us-east-1. so region = az[0:-1]
-    directions_re = '|'.join([
-        'central', 'east', 'north', 'northeast', 'northwest',
-        'south', 'southeast', 'southwest', 'west'])
-    ec2_az_re = ("^[a-z][a-z]-(%s)-[1-9][0-9]*[a-z]$" % directions_re)
-
     subst = {}
     if data_source and data_source.availability_zone:
         subst['availability_zone'] = data_source.availability_zone
 
-        if re.match(ec2_az_re, data_source.availability_zone):
+        # ec2 availability zones are named cc-direction-[0-9][a-d] (us-east-1b)
+        # the region is us-east-1. so region = az[0:-1]
+        if _EC2_AZ_RE.match(data_source.availability_zone):
             subst['ec2_region'] = "%s" % data_source.availability_zone[0:-1]
 
     if data_source and data_source.region:
diff --git a/cloudinit/ec2_utils.py b/cloudinit/ec2_utils.py
index 723d6bd..d6c61e4 100644
--- a/cloudinit/ec2_utils.py
+++ b/cloudinit/ec2_utils.py
@@ -1,6 +1,8 @@
 # Copyright (C) 2012 Yahoo! Inc.
+# Copyright (C) 2014 Amazon.com, Inc. or its affiliates.
 #
 # Author: Joshua Harlow <[e-mail]>
+# Author: Andrew Jorgensen <[e-mail]>
 #
 # This file is part of cloud-init. See LICENSE file for license information.
 
@@ -164,14 +166,11 @@ def get_instance_userdata(api_version='latest',
     return user_data
 
 
-def get_instance_metadata(api_version='latest',
-                          metadata_address='http://169.254.169.254',
-                          ssl_details=None, timeout=5, retries=5,
-                          leaf_decoder=None):
-    md_url = url_helper.combine_url(metadata_address, api_version)
-    # Note, 'meta-data' explicitly has trailing /.
-    # this is required for CloudStack (LP: #1356855)
-    md_url = url_helper.combine_url(md_url, 'meta-data/')
+def _get_instance_metadata(tree, api_version='latest',
+                           metadata_address='http://169.254.169.254',
+                           ssl_details=None, timeout=5, retries=5,
+                           leaf_decoder=None):
+    md_url = url_helper.combine_url(metadata_address, api_version, tree)
     caller = functools.partial(util.read_file_or_url,
                                ssl_details=ssl_details, timeout=timeout,
                                retries=retries)
@@ -189,7 +188,29 @@ def get_instance_metadata(api_version='latest',
             md = {}
         return md
     except Exception:
-        util.logexc(LOG, "Failed fetching metadata from url %s", md_url)
+        util.logexc(LOG, "Failed fetching %s from url %s", tree, md_url)
         return {}
 
+
+def get_instance_metadata(api_version='latest',
+                          metadata_address='http://169.254.169.254',
+                          ssl_details=None, timeout=5, retries=5,
+                          leaf_decoder=None):
+    # Note, 'meta-data' explicitly has trailing /.
+    # this is required for CloudStack (LP: #1356855)
+    return _get_instance_metadata(tree='meta-data/', api_version=api_version,
+                                  metadata_address=metadata_address,
+                                  ssl_details=ssl_details, timeout=timeout,
+                                  retries=retries, leaf_decoder=leaf_decoder)
+
+
+def get_instance_identity(api_version='latest',
+                          metadata_address='http://169.254.169.254',
+                          ssl_details=None, timeout=5, retries=5,
+                          leaf_decoder=None):
+    return _get_instance_metadata(tree='dynamic/instance-identity',
+                                  api_version=api_version,
+                                  metadata_address=metadata_address,
+                                  ssl_details=ssl_details, timeout=timeout,
+                                  retries=retries, leaf_decoder=leaf_decoder)
 # vi: ts=4 expandtab
diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py
index 7bbbfb6..fd4b73b 100644
--- a/cloudinit/sources/DataSourceEc2.py
+++ b/cloudinit/sources/DataSourceEc2.py
@@ -148,7 +148,12 @@ class DataSourceEc2(sources.DataSource):
         return self.min_metadata_version
 
     def get_instance_id(self):
-        return self.metadata['instance-id']
+        if self.cloud_platform == Platforms.AWS:
+            # Prefer the ID from the instance identity document, but fall back
+            return self.identity.get(
+                'instanceId', self.metadata['instance-id'])
+        else:
+            return self.metadata['instance-id']
 
     def _get_url_settings(self):
         mcfg = self.ds_cfg
@@ -262,15 +267,27 @@ class DataSourceEc2(sources.DataSource):
     @property
     def availability_zone(self):
         try:
-            return self.metadata['placement']['availability-zone']
+            if self.cloud_platform == Platforms.AWS:
+                return self.identity.get(
+                    'availabilityZone',
+                    self.metadata['placement']['availability-zone'])
+            else:
+                return self.metadata['placement']['availability-zone']
         except KeyError:
             return None
 
     @property
     def region(self):
-        az = self.availability_zone
-        if az is not None:
-            return az[:-1]
+        if self.cloud_platform == Platforms.AWS:
+            region = self.identity.get('region')
+            # Fallback to trimming the availability zone if region is missing
+            if self.availability_zone and not region:
+                region = self.availability_zone[:-1]
+            return region
+        else:
+            az = self.availability_zone
+            if az is not None:
+                return az[:-1]
         return None
 
     @property
@@ -351,6 +368,9 @@ class DataSourceEc2(sources.DataSource):
                 api_version, self.metadata_address)
             self.metadata = ec2.get_instance_metadata(
                 api_version, self.metadata_address)
+            if self.cloud_platform == Platforms.AWS:
+                self.identity = ec2.get_instance_identity(
+                    api_version, self.metadata_address).get('document', {})
         except Exception:
             util.logexc(
                 LOG, "Failed reading from metadata address %s",
diff --git a/tests/unittests/test_datasource/test_aliyun.py b/tests/unittests/test_datasource/test_aliyun.py
index 82ee971..dc548eb 100644
--- a/tests/unittests/test_datasource/test_aliyun.py
+++ b/tests/unittests/test_datasource/test_aliyun.py
@@ -47,6 +47,9 @@ def register_mock_metaserver(base_url, data):
         elif isinstance(body, list):
             register(base_url.rstrip('/'), '\n'.join(body) + '\n')
         elif isinstance(body, dict):
+            if not body:
+                register(base_url.rstrip('/') + '/', 'not found',
+                         status_code=404)
             vals = []
             for k, v in body.items():
                 if isinstance(v, (str, list)):
@@ -91,9 +94,22 @@ class TestAliYunDatasource(test_helpers.HttprettyTestCase):
             self.metadata_address,
             self.ds.min_metadata_version, 'user-data')
 
+    # EC2 provides an instance-identity document which must return 404 here
+    # for this test to pass.
+    @property
+    def default_identity(self):
+        return {}
+
+    @property
+    def identity_url(self):
+        return os.path.join(self.metadata_address,
+                            self.ds.min_metadata_version,
+                            'dynamic', 'instance-identity')
+
     def regist_default_server(self):
         register_mock_metaserver(self.metadata_url, self.default_metadata)
         register_mock_metaserver(self.userdata_url, self.default_userdata)
+        register_mock_metaserver(self.identity_url, self.default_identity)
 
     def _test_get_data(self):
         self.assertEqual(self.ds.metadata, self.default_metadata)