do not attempt dns resolution on ip addresses (#2040)

4 files changed, 35 insertions, 15 deletions

diff --git a/cloudinit/util.py b/cloudinit/util.py
index aca82878..fc777b82 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -34,6 +34,7 @@ import sys
 import time
 from base64 import b64decode, b64encode
 from collections import deque, namedtuple
+from contextlib import suppress
 from errno import EACCES, ENOENT
 from functools import lru_cache, total_ordering
 from pathlib import Path
@@ -44,6 +45,7 @@ from cloudinit import features, importer
 from cloudinit import log as logging
 from cloudinit import (
     mergers,
+    net,
     safeyaml,
     subp,
     temp_utils,
@@ -1233,8 +1235,8 @@ def get_fqdn_from_hosts(hostname, filename="/etc/hosts"):
     return fqdn
 
 
-def is_resolvable(name):
-    """determine if a url is resolvable, return a boolean
+def is_resolvable(url) -> bool:
+    """determine if a url's network address is resolvable, return a boolean
     This also attempts to be resilent against dns redirection.
 
     Note, that normal nsswitch resolution is used here.  So in order
@@ -1246,6 +1248,8 @@ def is_resolvable(name):
     be resolved inside the search list.
     """
     global _DNS_REDIRECT_IP
+    parsed_url = parse.urlparse(url)
+    name = parsed_url.hostname
     if _DNS_REDIRECT_IP is None:
         badips = set()
         badnames = (
@@ -1253,7 +1257,7 @@ def is_resolvable(name):
             "example.invalid.",
             "__cloud_init_expected_not_found__",
         )
-        badresults = {}
+        badresults: dict = {}
         for iname in badnames:
             try:
                 result = socket.getaddrinfo(
@@ -1270,12 +1274,14 @@ def is_resolvable(name):
             LOG.debug("detected dns redirection: %s", badresults)
 
     try:
+        # ip addresses need no resolution
+        with suppress(ValueError):
+            if net.is_ip_address(parsed_url.netloc.strip("[]")):
+                return True
         result = socket.getaddrinfo(name, None)
         # check first result's sockaddr field
         addr = result[0][4][0]
-        if addr in _DNS_REDIRECT_IP:
-            return False
-        return True
+        return addr not in _DNS_REDIRECT_IP
     except (socket.gaierror, socket.error):
         return False
 
@@ -1298,7 +1304,7 @@ def is_resolvable_url(url):
         logfunc=LOG.debug,
         msg="Resolving URL: " + url,
         func=is_resolvable,
-        args=(parse.urlparse(url).hostname,),
+        args=(url,),
     )
 
 
diff --git a/tests/unittests/config/test_apt_configure_sources_list_v1.py b/tests/unittests/config/test_apt_configure_sources_list_v1.py
index 52964e10..b0bf54f4 100644
--- a/tests/unittests/config/test_apt_configure_sources_list_v1.py
+++ b/tests/unittests/config/test_apt_configure_sources_list_v1.py
@@ -135,7 +135,7 @@ class TestAptSourceConfigSourceList(t_help.FilesystemMockingTestCase):
     @staticmethod
     def myresolve(name):
         """Fake util.is_resolvable for mirrorfail tests"""
-        if name == "does.not.exist":
+        if "does.not.exist" in name:
             print("Faking FAIL for '%s'" % name)
             return False
         else:
@@ -155,8 +155,8 @@ class TestAptSourceConfigSourceList(t_help.FilesystemMockingTestCase):
                 ],
                 "http://httpredir.debian.org/debian",
             )
-        mockresolve.assert_any_call("does.not.exist")
-        mockresolve.assert_any_call("httpredir.debian.org")
+        mockresolve.assert_any_call("http://does.not.exist")
+        mockresolve.assert_any_call("http://httpredir.debian.org/debian")
 
     def test_apt_v1_srcl_ubuntu_mirrorfail(self):
         """Test rendering of a source.list from template for ubuntu"""
@@ -168,8 +168,8 @@ class TestAptSourceConfigSourceList(t_help.FilesystemMockingTestCase):
                 ["http://does.not.exist", "http://archive.ubuntu.com/ubuntu/"],
                 "http://archive.ubuntu.com/ubuntu/",
             )
-        mockresolve.assert_any_call("does.not.exist")
-        mockresolve.assert_any_call("archive.ubuntu.com")
+        mockresolve.assert_any_call("http://does.not.exist")
+        mockresolve.assert_any_call("http://archive.ubuntu.com/ubuntu/")
 
     def test_apt_v1_srcl_custom(self):
         """Test rendering from a custom source.list template"""
diff --git a/tests/unittests/config/test_apt_source_v3.py b/tests/unittests/config/test_apt_source_v3.py
index 8d7ba5dc..1813000e 100644
--- a/tests/unittests/config/test_apt_source_v3.py
+++ b/tests/unittests/config/test_apt_source_v3.py
@@ -963,11 +963,11 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase):
 
         with mock.patch.object(util, "is_resolvable") as mockresolve:
             util.is_resolvable_url("http://1.2.3.4/ubuntu")
-        mockresolve.assert_called_with("1.2.3.4")
+        mockresolve.assert_called_with("http://1.2.3.4/ubuntu")
 
         with mock.patch.object(util, "is_resolvable") as mockresolve:
             util.is_resolvable_url("http://us.archive.ubuntu.com/ubuntu")
-        mockresolve.assert_called_with("us.archive.ubuntu.com")
+        mockresolve.assert_called_with("http://us.archive.ubuntu.com/ubuntu")
 
         # former tests can leave this set (or not if the test is ran directly)
         # do a hard reset to ensure a stable result
@@ -984,7 +984,6 @@ class TestAptSourceConfig(t_help.FilesystemMockingTestCase):
         )
         mocksock.assert_any_call("example.invalid.", None, 0, 0, 1, 2)
         mocksock.assert_any_call("us.archive.ubuntu.com", None)
-        mocksock.assert_any_call("1.2.3.4", None)
 
         self.assertTrue(ret)
         self.assertTrue(ret2)
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index ecf8cf5c..865f202a 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -3026,3 +3026,18 @@ class TestVersion:
     )
     def test_from_str(self, str_ver, cls_ver):
         assert util.Version.from_str(str_ver) == cls_ver
+
+
+@pytest.mark.allow_dns_lookup
+class TestResolvable:
+    @mock.patch.object(util, "_DNS_REDIRECT_IP", return_value=True)
+    @mock.patch.object(util.socket, "getaddrinfo")
+    def test_ips_need_not_be_resolved(self, m_getaddr, m_dns):
+        """Optimization test: dns resolution may timeout during early boot, and
+        often the urls being checked use IP addresses rather than dns names.
+        Therefore, the fast path checks if the address contains an IP and exits
+        early if the path is a valid IP.
+        """
+        assert util.is_resolvable("http://169.254.169.254/") is True
+        assert util.is_resolvable("http://[fd00:ec2::254]/") is True
+        assert not m_getaddr.called