sources/azure: ensure retries on IMDS request failure (#1271)

cjp256 · web-flow · commit c1a2047cf291 · 2022-02-17T19:15:18.000-06:00
There are two issues with IMDS retries:

1. IMDS_VER_WANT will never be attempted if retries=0, such as
when fetching network metadata with infinite=True.

2. get_imds_data_with_api_fallback() will attempt one request with
IMDS_VER_WANT.  If the connection fails due to a timeout, connection
issue, or error code other than 400, an empty dictionary will be
returned without attempting the requested number of retries.

This PR:

- Updates get_imds_data_with_api_fallback() to invoke
get_metadata_from_imds() with the specified retries and infinite
parameters.

- Updates retry_on_url_exc to take a configurable set of HTTP error
codes and exception types to retry on.

- Add IMDS_RETRY_CODES set to retry with when fetching data from IMDS:

  - 404 not found (yet)
  - 410 gone / unavailable (yet)
  - 429 rate-limited/throttled
  - 500 server error

- Replace default callback with imds_readurl_exception_callback,
which configures retry_on_url_exc() with these error codes and
instances.

- Add new pytests for IMDS to eventually replace the unittest
equivalents and improve existing coverage.

Signed-off-by: Chris Patterson &lt;cpatterson@microsoft.com&gt;
diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
@@ -7,6 +7,7 @@
 import base64
 import crypt
 import datetime
+import functools
 import os
 import os.path
 import re
@@ -67,6 +68,17 @@
 IMDS_VER_MIN = "2019-06-01"
 IMDS_VER_WANT = "2021-08-01"
 IMDS_EXTENDED_VER_MIN = "2021-03-01"
+IMDS_RETRY_CODES = (
+    404,  # not found (yet)
+    410,  # gone / unavailable (yet)
+    429,  # rate-limited/throttled
+    500,  # server error
+)
+imds_readurl_exception_callback = functools.partial(
+    retry_on_url_exc,
+    retry_codes=IMDS_RETRY_CODES,
+    retry_instances=(requests.Timeout,),
+)
 
 
 class MetadataType(Enum):
@@ -725,44 +737,49 @@ def _get_data(self):
     def get_imds_data_with_api_fallback(
         self,
         *,
-        retries,
-        md_type=MetadataType.ALL,
-        exc_cb=retry_on_url_exc,
-        infinite=False,
-    ):
-        """
-        Wrapper for get_metadata_from_imds so that we can have flexibility
-        in which IMDS api-version we use. If a particular instance of IMDS
-        does not have the api version that is desired, we want to make
-        this fault tolerant and fall back to a good known minimum api
-        version.
-        """
-        for _ in range(retries):
-            try:
-                LOG.info("Attempting IMDS api-version: %s", IMDS_VER_WANT)
-                return get_metadata_from_imds(
-                    retries=0,
-                    md_type=md_type,
-                    api_version=IMDS_VER_WANT,
-                    exc_cb=exc_cb,
-                )
-            except UrlError as err:
-                LOG.info("UrlError with IMDS api-version: %s", IMDS_VER_WANT)
-                if err.code == 400:
-                    log_msg = "Fall back to IMDS api-version: {}".format(
-                        IMDS_VER_MIN
-                    )
-                    report_diagnostic_event(log_msg, logger_func=LOG.info)
-                    break
+        retries: int,
+        md_type: MetadataType = MetadataType.ALL,
+        exc_cb=imds_readurl_exception_callback,
+        infinite: bool = False,
+    ) -> dict:
+        """Fetch metadata from IMDS using IMDS_VER_WANT API version.
 
-        LOG.info("Using IMDS api-version: %s", IMDS_VER_MIN)
-        return get_metadata_from_imds(
-            retries=retries,
-            md_type=md_type,
-            api_version=IMDS_VER_MIN,
-            exc_cb=exc_cb,
-            infinite=infinite,
-        )
+        Falls back to IMDS_VER_MIN version if IMDS returns a 400 error code,
+        indicating that IMDS_VER_WANT is unsupported.
+
+        :return: Parsed metadata dictionary or empty dict on error.
+        """
+        LOG.info("Attempting IMDS api-version: %s", IMDS_VER_WANT)
+        try:
+            return get_metadata_from_imds(
+                retries=retries,
+                md_type=md_type,
+                api_version=IMDS_VER_WANT,
+                exc_cb=exc_cb,
+                infinite=infinite,
+            )
+        except UrlError as error:
+            LOG.info("UrlError with IMDS api-version: %s", IMDS_VER_WANT)
+            # Fall back if HTTP code is 400, otherwise return empty dict.
+            if error.code != 400:
+                return {}
+
+        log_msg = "Fall back to IMDS api-version: {}".format(IMDS_VER_MIN)
+        report_diagnostic_event(log_msg, logger_func=LOG.info)
+        try:
+            return get_metadata_from_imds(
+                retries=retries,
+                md_type=md_type,
+                api_version=IMDS_VER_MIN,
+                exc_cb=exc_cb,
+                infinite=infinite,
+            )
+        except UrlError as error:
+            report_diagnostic_event(
+                "Failed to fetch IMDS metadata: %s" % error,
+                logger_func=LOG.error,
+            )
+            return {}
 
     def device_name_to_device(self, name):
         return self.ds_cfg["disk_aliases"].get(name)
@@ -2271,7 +2288,7 @@ def get_metadata_from_imds(
     retries,
     md_type=MetadataType.ALL,
     api_version=IMDS_VER_MIN,
-    exc_cb=retry_on_url_exc,
+    exc_cb=imds_readurl_exception_callback,
     infinite=False,
 ):
     """Query Azure's instance metadata service, returning a dictionary.
diff --git a/cloudinit/url_helper.py b/cloudinit/url_helper.py
@@ -639,16 +639,22 @@ def oauth_headers(
     return signed_headers
 
 
-def retry_on_url_exc(msg, exc):
-    """readurl exception_cb that will retry on NOT_FOUND and Timeout.
+def retry_on_url_exc(
+    msg, exc, *, retry_codes=(NOT_FOUND,), retry_instances=(requests.Timeout,)
+):
+    """Configurable retry exception callback for readurl().
+
+    :param retry_codes: Codes to retry on. Defaults to 404.
+    :param retry_instances: Exception types to retry on. Defaults to
+      requests.Timeout.
 
-    Returns False to raise the exception from readurl, True to retry.
+    :returns: False to raise the exception from readurl(), True to retry.
     """
     if not isinstance(exc, UrlError):
         return False
-    if exc.code == NOT_FOUND:
+    if exc.code in retry_codes:
         return True
-    if exc.cause and isinstance(exc.cause, requests.Timeout):
+    if exc.cause and isinstance(exc.cause, retry_instances):
         return True
     return False
 
diff --git a/tests/unittests/sources/test_azure.py b/tests/unittests/sources/test_azure.py
