Repository: adamchainz/ec2-metadata

Issue:
Filter out non-200 responses
Currently we don't filter out error responses from the metadata API, which means it can return data from error responses. There should be a `resp.raise_for_status()` in there, or even filter for exactly `resp.status_code == 200`

Discussion:

Patch:
diff --git a/HISTORY.rst b/HISTORY.rst
index 2f30547..1d3dfdf 100644
--- a/HISTORY.rst
+++ b/HISTORY.rst
@@ -8,6 +8,10 @@ Pending Release
 
 .. Insert new release notes below this line
 
+* All methods can now raise ``requests.exceptions.HTTPError`` if the metadata
+  API returns a bad response, rather than failing during parsing or silently
+  returning data from non-200 responses.
+
 1.2.1 (2017-08-31)
 ------------------
 
diff --git a/ec2_metadata.py b/ec2_metadata.py
index e087b14..ce62284 100644
--- a/ec2_metadata.py
+++ b/ec2_metadata.py
@@ -29,59 +29,65 @@ class BaseLazyObject(object):
 
 class EC2Metadata(BaseLazyObject):
 
+    def _get_url(self, url, raise_for_status=True):
+        resp = requests.get(url)
+        if raise_for_status:
+            resp.raise_for_status()
+        return resp
+
     @property
     def account_id(self):
         return self.instance_identity_document['accountId']
 
     @cached_property
     def ami_id(self):
-        return requests.get(METADATA_URL + 'ami-id').text
+        return self._get_url(METADATA_URL + 'ami-id').text
 
     @cached_property
     def availability_zone(self):
-        return requests.get(METADATA_URL + 'placement/availability-zone').text
+        return self._get_url(METADATA_URL + 'placement/availability-zone').text
 
     @cached_property
     def ami_launch_index(self):
-        return int(requests.get(METADATA_URL + 'ami-launch-index').text)
+        return int(self._get_url(METADATA_URL + 'ami-launch-index').text)
 
     @cached_property
     def ami_manifest_path(self):
-        return requests.get(METADATA_URL + 'ami-manifest-path').text
+        return self._get_url(METADATA_URL + 'ami-manifest-path').text
 
     @cached_property
     def instance_id(self):
-        return requests.get(METADATA_URL + 'instance-id').text
+        return self._get_url(METADATA_URL + 'instance-id').text
 
     @cached_property
     def instance_identity_document(self):
-        return requests.get(DYNAMIC_URL + 'instance-identity/document').json()
+        return self._get_url(DYNAMIC_URL + 'instance-identity/document').json()
 
     @cached_property
     def instance_type(self):
-        return requests.get(METADATA_URL + 'instance-type').text
+        return self._get_url(METADATA_URL + 'instance-type').text
 
     @cached_property
     def mac(self):
-        return requests.get(METADATA_URL + 'mac').text
+        return self._get_url(METADATA_URL + 'mac').text
 
     @cached_property
     def network_interfaces(self):
-        macs_text = requests.get(METADATA_URL + 'network/interfaces/macs/').text
+        macs_text = self._get_url(METADATA_URL + 'network/interfaces/macs/').text
         macs = [line.rstrip('/') for line in macs_text.splitlines()]
-        return {mac: NetworkInterface(mac) for mac in macs}
+        return {mac: NetworkInterface(mac, self) for mac in macs}
 
     @cached_property
     def private_hostname(self):
-        return requests.get(METADATA_URL + 'local-hostname').text
+        return self._get_url(METADATA_URL + 'local-hostname').text
 
     @cached_property
     def private_ipv4(self):
-        return requests.get(METADATA_URL + 'local-ipv4').text
+        return self._get_url(METADATA_URL + 'local-ipv4').text
 
     @cached_property
     def public_hostname(self):
-        resp = requests.get(METADATA_URL + 'public-hostname')
+        resp = self._get_url(METADATA_URL + 'public-hostname', raise_for_status=False)
         if resp.status_code == 404:
             return None
         else:
@@ -89,7 +95,7 @@ class EC2Metadata(BaseLazyObject):
 
     @cached_property
     def public_ipv4(self):
-        resp = requests.get(METADATA_URL + 'public-ipv4')
+        resp = self._get_url(METADATA_URL + 'public-ipv4', raise_for_status=False)
         if resp.status_code == 404:
             return None
         else:
@@ -101,15 +107,15 @@ class EC2Metadata(BaseLazyObject):
 
     @cached_property
     def reservation_id(self):
-        return requests.get(METADATA_URL + 'reservation-id').text
+        return self._get_url(METADATA_URL + 'reservation-id').text
 
     @cached_property
     def security_groups(self):
-        return requests.get(METADATA_URL + 'security-groups').text.splitlines()
+        return self._get_url(METADATA_URL + 'security-groups').text.splitlines()
 
     @cached_property
     def user_data(self):
-        resp = requests.get(USERDATA_URL)
+        resp = self._get_url(USERDATA_URL, raise_for_status=False)
         if resp.status_code == 404:
             return None
         else:
@@ -118,14 +124,22 @@ class EC2Metadata(BaseLazyObject):
 
 class NetworkInterface(BaseLazyObject):
 
-    def __init__(self, mac):
+    def __init__(self, mac, parent=None):
         self.mac = mac
+        if parent is None:
+            self.parent = ec2_metadata
+        else:
+            self.parent = parent
 
     def __repr__(self):
         return 'NetworkInterface({mac})'.format(mac=repr(self.mac))
 
     def __eq__(self, other):
-        return isinstance(other, NetworkInterface) and self.mac == other.mac
+        return (
+            isinstance(other, NetworkInterface) and
+            self.mac == other.mac and
+            self.parent == other.parent
+        )
 
     def _url(self, item):
         return '{base}network/interfaces/macs/{mac}/{item}'.format(
@@ -136,14 +150,14 @@ class NetworkInterface(BaseLazyObject):
 
     @cached_property
     def device_number(self):
-        return int(requests.get(self._url('device-number')).text)
+        return int(self.parent._get_url(self._url('device-number')).text)
 
     @cached_property
     def ipv4_associations(self):
         associations = {}
         for public_ip in self.public_ipv4s:
-            resp = requests.get(self._url('ipv4-associations/{}'.format(public_ip)))
-            resp.raise_for_status()
+            url = self._url('ipv4-associations/{}'.format(public_ip))
+            resp = self.parent._get_url(url)
             private_ips = resp.text.splitlines()
             associations[public_ip] = private_ips
         return associations
@@ -156,19 +170,19 @@ class NetworkInterface(BaseLazyObject):
 
     @cached_property
     def owner_id(self):
-        return requests.get(self._url('owner-id')).text
+        return self.parent._get_url(self._url('owner-id')).text
 
     @cached_property
     def private_hostname(self):
-        return requests.get(self._url('local-hostname')).text
+        return self.parent._get_url(self._url('local-hostname')).text
 
     @cached_property
     def private_ipv4s(self):
-        return requests.get(self._url('local-ipv4s')).text.splitlines()
+        return self.parent._get_url(self._url('local-ipv4s')).text.splitlines()
 
     @cached_property
     def public_hostname(self):
-        resp = requests.get(self._url('public-hostname'))
+        resp = self.parent._get_url(self._url('public-hostname'), raise_for_status=False)
         if resp.status_code == 404:
             return None
         else:
@@ -176,7 +190,7 @@ class NetworkInterface(BaseLazyObject):
 
     @cached_property
     def public_ipv4s(self):
-        resp = requests.get(self._url('public-ipv4s'))
+        resp = self.parent._get_url(self._url('public-ipv4s'), raise_for_status=False)
         if resp.status_code == 404:
             return []
         else:
@@ -184,19 +198,19 @@ class NetworkInterface(BaseLazyObject):
 
     @cached_property
     def security_groups(self):
-        return requests.get(self._url('security-groups')).text.splitlines()
+        return self.parent._get_url(self._url('security-groups')).text.splitlines()
 
     @cached_property
     def security_group_ids(self):
-        return requests.get(self._url('security-group-ids')).text.splitlines()
+        return self.parent._get_url(self._url('security-group-ids')).text.splitlines()
 
     @cached_property
     def subnet_id(self):
-        return requests.get(self._url('subnet-id')).text
+        return self.parent._get_url(self._url('subnet-id')).text
 
     @cached_property
     def subnet_ipv4_cidr_block(self):
-        resp = requests.get(self._url('subnet-ipv4-cidr-block'))
+        resp = self.parent._get_url(self._url('subnet-ipv4-cidr-block'), raise_for_status=False)
         if resp.status_code == 404:
             return None
         else:
@@ -210,11 +224,11 @@ class NetworkInterface(BaseLazyObject):
 
     @cached_property
     def vpc_id(self):
-        return requests.get(self._url('vpc-id')).text
+        return self.parent._get_url(self._url('vpc-id')).text
 
     @cached_property
     def vpc_ipv4_cidr_block(self):
-        resp = requests.get(self._url('vpc-ipv4-cidr-block'))
+        resp = self.parent._get_url(self._url('vpc-ipv4-cidr-block'), raise_for_status=False)
         if resp.status_code == 404:
             return None
         else:
@@ -222,7 +236,7 @@ class NetworkInterface(BaseLazyObject):
 
     @cached_property
     def vpc_ipv4_cidr_blocks(self):
-        resp = requests.get(self._url('vpc-ipv4-cidr-blocks'))
+        resp = self.parent._get_url(self._url('vpc-ipv4-cidr-blocks'), raise_for_status=False)
         if resp.status_code == 404:
             return []
         else:


Test patch:
diff --git a/test_ec2_metadata.py b/test_ec2_metadata.py
index 5bd65a5..73707ee 100644
--- a/test_ec2_metadata.py
+++ b/test_ec2_metadata.py
@@ -4,6 +4,7 @@ from __future__ import absolute_import, division, print_function, unicode_litera
 import json
 
 import pytest
+import requests
 import responses
 
 from ec2_metadata import DYNAMIC_URL, METADATA_URL, USERDATA_URL, NetworkInterface, ec2_metadata
@@ -61,6 +62,12 @@ def test_account_id(resps):
     assert ec2_metadata.account_id == '1234'
 
 
+def test_account_id_error(resps):
+    add_response(resps, DYNAMIC_URL + 'instance-identity/document', status=500)
+    with pytest.raises(requests.exceptions.HTTPError):
+        ec2_metadata.account_id
+
+
 def test_ami_id(resps):
     add_response(resps, 'ami-id', 'ami-12345678')
     assert ec2_metadata.ami_id == 'ami-12345678'
@@ -121,7 +128,7 @@ def test_mac(resps):
 
 def test_network_interfaces(resps):
     add_response(resps, 'network/interfaces/macs/', example_mac + '/')
-    assert ec2_metadata.network_interfaces == {example_mac: NetworkInterface(example_mac)}
+    assert ec2_metadata.network_interfaces == {example_mac: NetworkInterface(example_mac, ec2_metadata)}
 
 
 def test_private_hostname(resps):