From c0ea137b344a5ad1c9cff6bd5438ac9dd121bdc2 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Mon, 27 Feb 2023 10:08:15 -0700 Subject: [PATCH 1/3] do not attempt dns resolution on ip addresses --- cloudinit/util.py | 18 ++++++++++++------ .../test_apt_configure_sources_list_v1.py | 10 +++++----- tests/unittests/config/test_apt_source_v3.py | 5 ++--- tests/unittests/test_util.py | 12 ++++++++++++ 4 files changed, 31 insertions(+), 14 deletions(-) diff --git a/cloudinit/util.py b/cloudinit/util.py index aed332ec944..6ba86e4d18e 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -34,6 +34,7 @@ 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 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 = ( @@ -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 52964e10f2a..b0bf54f4ad0 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 @@ def test_apt_v1_source_list_ubuntu(self): @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 @@ def test_apt_v1_srcl_debian_mirrorfail(self): ], "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 @@ def test_apt_v1_srcl_ubuntu_mirrorfail(self): ["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 8d7ba5dcba8..1813000ec7b 100644 --- a/tests/unittests/config/test_apt_source_v3.py +++ b/tests/unittests/config/test_apt_source_v3.py @@ -963,11 +963,11 @@ def test_apt_v3_url_resolvable(self): 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 @@ def test_apt_v3_url_resolvable(self): ) 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 07142a86886..27c5dca0e2a 100644 --- a/tests/unittests/test_util.py +++ b/tests/unittests/test_util.py @@ -3026,3 +3026,15 @@ def test_to_version_and_back_to_str(self, version): ) 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: + def test_ips_need_not_be_resolved(self): + """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/") + assert util.is_resolvable("http://[fd00:ec2::254]/") From 96f7ab65b99fa17e196136f117e69c43c0316a49 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Mon, 27 Feb 2023 10:33:31 -0700 Subject: [PATCH 2/3] fmt --- cloudinit/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudinit/util.py b/cloudinit/util.py index 6ba86e4d18e..807d9649fa1 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -1257,7 +1257,7 @@ def is_resolvable(url) -> bool: "example.invalid.", "__cloud_init_expected_not_found__", ) - badresults = {} + badresults: dict = {} for iname in badnames: try: result = socket.getaddrinfo( From 37b02477dd7a434a46af17508bcab2b9b49e91bc Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Mon, 13 Mar 2023 13:39:23 -0600 Subject: [PATCH 3/3] comments --- tests/unittests/test_util.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py index 27c5dca0e2a..42d2b736e33 100644 --- a/tests/unittests/test_util.py +++ b/tests/unittests/test_util.py @@ -3030,11 +3030,14 @@ def test_from_str(self, str_ver, cls_ver): @pytest.mark.allow_dns_lookup class TestResolvable: - def test_ips_need_not_be_resolved(self): + @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/") - assert util.is_resolvable("http://[fd00:ec2::254]/") + 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