Add connectivity_url to Oracle's EphemeralDHCPv4 (SC-395)

[TheRealFalcon]

Proposed Commit Message

Add connectivity_url to Oracle's EphemeralDHCPv4

On bionic, when trying to bring up the EphemeralDHCPv4, it's possible
that we already have a route defined, which will result in an error when
trying to add the DHCP route. Use the connectivity_url to check if we
can reach the metadata service, and if so, skip the EphemeralDHCPv4.

The has_url_connectivity function has also been modified to take either
a url as string or the dict of kwargs to send to readurl.

LP: #1939603

Additional Context

According to Oracle, the v2 endpoint will always be available, at least for the foreseeable future, anyway, so we don't need to worry about the v1 fallback.

Test Steps

Launch a bionic instance on Oracle. For me this looked like:

oci compute instance launch --availability-domain="qIZq:PHX-AD-3" --shape="VM.Standard1.1" --image-id="ocid1.image.oc1.phx.aaaaaaaai7owziu6qhbovn6a4pwnsqspehqt3vqeopytymvzawa72j4xn36q" --subnet-id="ocid1.subnet.oc1.phx.aaaaaaaaqsel74u7xoroactpxnstmx3y7vjswxildbfh3xmr34finoafsi4a" --ssh-authorized-keys-file="/home/james/.ssh/id_rsa.pub"

On the instance, change the first non-commented line in /etc/cloud/cloud.cfg.d/99-oracle-compute-infra-datasource.cfg to datasource_list: [ Oracle ].
To reproduce this issue:

sudo cloud-init clean --logs --reboot

SSH into the instance and notice the error in the logs.

Build the dpkg off this branch with packages/bddeb -d
Copy the dpkg to the instance, and install with dpkg -i cloud-init.deb (or whatever you called it).
Once again run:

sudo cloud-init clean --logs --reboot

SSH into the instance and there should be no errors, and cloud-init should have read the metadata as expected.

Checklist:

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

[blackboxsw]

@TheRealFalcon why do you think we need to support either str or MutableMapping? We only have one call-site to has_url_connectivity in cloudinit.net I think. Any reason not to just mandate a Mapping param and avoid trying to be too flexible with different param types?

[blackboxsw]

Also for background, I think Vultr is the only other datasource currently setting "connectivity_url" for the EphemeralDHCP contextmanager.

[blackboxsw]

Just for a clean/simple function signature and expectations, I'd vote to drop the "str" type option if we can. Less areas where call-sites could misinterpret what needs sending..

[blackboxsw]

I could see going either way with this, but minimally I think we probably need a return False if not 'url' is provided in connectivity_url dict value. I lean toward not providing the option to make url param a str vs dict, but your call.

here's the diff I was testing (though might need some whitespace/alignment adjustments)

diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
index b330cdaa3..80f127709 100644
--- a/cloudinit/net/__init__.py
+++ b/cloudinit/net/__init__.py
@@ -972,25 +972,19 @@ def get_ib_hwaddrs_by_interface():
     return ret
 
 
-def has_url_connectivity(
-    url_data: Union[MutableMapping[str, Any], str]
-) -> bool:
+def has_url_connectivity(url_data: MutableMapping[str, Any]) -> bool:
     """Return true when the instance has access to the provided URL.
 
     Logs a warning if url is not the expected format.
 
-    url_data is either the URL string to check, or a dictionary of kwargs
-    to send to readurl. E.g.:
+    url_data is a dictionary of kwargsto send to readurl. E.g.:
 
-    has_url_connectivity("http://example.invalid")
     has_url_connectivity({
         "url": "http://example.invalid",
         "headers": {"some": "header"},
         "timeout": 10
     })
     """
-    if isinstance(url_data, str):
-        url_data = {'url': url_data}
     if 'url' not in url_data:
         LOG.warning(
             "Ignoring connectivity check. No 'url' to check in %s", url_data)
@@ -1046,14 +1040,14 @@ class EphemeralIPv4Network(object):
 
     No operations are performed if the provided interface already has the
     specified configuration.
-    This can be verified with the connectivity_url.
+    This can be verified with the connectivity_url_kwargs.
     If unconnected, bring up the interface with valid ip, prefix and broadcast.
     If router is provided setup a default route for that interface. Upon
     context exit, clean up the interface leaving no configuration behind.
     """
 
     def __init__(self, interface, ip, prefix_or_mask, broadcast, router=None,
-                 connectivity_url=None, static_routes=None):
+                 connectivity_url_kwargs=None, static_routes=None):
         """Setup context manager and validate call signature.
 
         @param interface: Name of the network interface to bring up.
@@ -1062,7 +1056,7 @@ class EphemeralIPv4Network(object):
             prefix.
         @param broadcast: Broadcast address for the IPv4 network.
         @param router: Optionally the default gateway IP.
-        @param connectivity_url: Optionally, a URL to verify if a usable
+        @param connectivity_url_kwargs: Optionally, a URL to verify if a usable
            connection already exists.
         @param static_routes: Optionally a list of static routes from DHCP
         """
@@ -1077,7 +1071,7 @@ class EphemeralIPv4Network(object):
                 'Cannot setup network: {0}'.format(e)
             ) from e
 
-        self.connectivity_url = connectivity_url
+        self.connectivity_url_kwargs = connectivity_url_kwargs
         self.interface = interface
         self.ip = ip
         self.broadcast = broadcast
@@ -1087,11 +1081,11 @@ class EphemeralIPv4Network(object):
 
     def __enter__(self):
         """Perform ephemeral network setup if interface is not connected."""
-        if self.connectivity_url:
-            if has_url_connectivity(self.connectivity_url):
+        if self.connectivity_url_kwargs:
+            if has_url_connectivity(self.connectivity_url_kwargs):
                 LOG.debug(
                     'Skip ephemeral network setup, instance has connectivity'
-                    ' to %s', self.connectivity_url)
+                    ' to %s', self.connectivity_url_kwargs['url'])
                 return
 
         self._bringup_device()
diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py
index 9b94c9a0a..4da13a502 100644
--- a/cloudinit/net/dhcp.py
+++ b/cloudinit/net/dhcp.py
@@ -38,21 +38,21 @@ class NoDHCPLeaseError(Exception):
 
 
 class EphemeralDHCPv4(object):
-    def __init__(self, iface=None, connectivity_url=None, dhcp_log_func=None):
+    def __init__(self, iface=None, connectivity_url_kwargs=None, dhcp_log_func=None):
         self.iface = iface
         self._ephipv4 = None
         self.lease = None
         self.dhcp_log_func = dhcp_log_func
-        self.connectivity_url = connectivity_url
+        self.connectivity_url_kwargs = connectivity_url_kwargs
 
     def __enter__(self):
         """Setup sandboxed dhcp context, unless connectivity_url can already be
         reached."""
-        if self.connectivity_url:
-            if has_url_connectivity(self.connectivity_url):
+        if self.connectivity_url_kwargs:
+            if has_url_connectivity(self.connectivity_url_kwargs):
                 LOG.debug(
                     'Skip ephemeral DHCP setup, instance has connectivity'
-                    ' to %s', self.connectivity_url)
+                    ' to %s', self.connectivity_url_kwargs)
                 return
         return self.obtain_lease()
 
@@ -104,8 +104,8 @@ class EphemeralDHCPv4(object):
         if kwargs['static_routes']:
             kwargs['static_routes'] = (
                 parse_static_routes(kwargs['static_routes']))
-        if self.connectivity_url:
-            kwargs['connectivity_url'] = self.connectivity_url
+        if self.connectivity_url_kwargs:
+            kwargs['connectivity_url_kwargs'] = self.connectivity_url_kwargs
         ephipv4 = EphemeralIPv4Network(**kwargs)
         ephipv4.__enter__()
         self._ephipv4 = ephipv4
diff --git a/cloudinit/net/tests/test_init.py b/cloudinit/net/tests/test_init.py
index 38590b6c7..320ced9fd 100644
--- a/cloudinit/net/tests/test_init.py
+++ b/cloudinit/net/tests/test_init.py
@@ -622,7 +622,8 @@ class TestEphemeralIPV4Network(CiTestCase):
         params = {
             'interface': 'eth0', 'ip': '192.168.2.2',
             'prefix_or_mask': '255.255.255.0', 'broadcast': '192.168.2.255',
-            'connectivity_url': 'http://example.org/index.html'}
+            'connectivity_url_kwargs': {'url': 'http://example.org/index.html'}
+        }
 
         with net.EphemeralIPv4Network(**params):
             self.assertEqual(
@@ -852,25 +853,28 @@ class TestHasURLConnectivity(HttprettyTestCase):
     def test_url_timeout_on_connectivity_check(self, m_readurl):
         """A timeout of 5 seconds is provided when reading a url."""
         self.assertTrue(
-            net.has_url_connectivity(self.url), 'Expected True on url connect')
+                net.has_url_connectivity({'url': self.url}),
+                'Expected True on url connect')
 
     def test_true_on_url_connectivity_success(self):
         httpretty.register_uri(httpretty.GET, self.url)
         self.assertTrue(
-            net.has_url_connectivity(self.url), 'Expected True on url connect')
+            net.has_url_connectivity({'url': self.url}),
+            'Expected True on url connect')
 
     @mock.patch('requests.Session.request')
     def test_true_on_url_connectivity_timeout(self, m_request):
         """A timeout raised accessing the url will return False."""
         m_request.side_effect = requests.Timeout('Fake Connection Timeout')
         self.assertFalse(
-            net.has_url_connectivity(self.url),
+            net.has_url_connectivity({'url': self.url}),
             'Expected False on url timeout')
 
     def test_true_on_url_connectivity_failure(self):
         httpretty.register_uri(httpretty.GET, self.url, body={}, status=404)
         self.assertFalse(
-            net.has_url_connectivity(self.url), 'Expected False on url fail')
+             net.has_url_connectivity({'url': self.url}),
+             'Expected False on url fail')
 
 
 def _mk_v1_phys(mac, name, driver, device_id):
diff --git a/cloudinit/sources/DataSourceOracle.py b/cloudinit/sources/DataSourceOracle.py
index 2e591f855..5f516f00e 100644
--- a/cloudinit/sources/DataSourceOracle.py
+++ b/cloudinit/sources/DataSourceOracle.py
@@ -137,7 +137,7 @@ class DataSourceOracle(sources.DataSource):
         if not _is_iscsi_root():
             network_context = dhcp.EphemeralDHCPv4(
                 iface=net.find_fallback_nic(),
-                connectivity_url={
+                connectivity_url_kwargs={
                     "url": METADATA_PATTERN.format(version=2, path="instance"),
                     "headers": V2_HEADERS,
                 }
diff --git a/cloudinit/sources/helpers/vultr.py b/cloudinit/sources/helpers/vultr.py
index c22cd0b12..a2fd464e0 100644
--- a/cloudinit/sources/helpers/vultr.py
+++ b/cloudinit/sources/helpers/vultr.py
@@ -20,7 +20,7 @@ LOG = log.getLogger(__name__)
 def get_metadata(url, timeout, retries, sec_between):
     # Bring up interface
     try:
-        with EphemeralDHCPv4(connectivity_url=url):
+        with EphemeralDHCPv4(connectivity_url_kwargs={"url": url}):
             # Fetch the metadata
             v1 = read_metadata(url, timeout, retries, sec_between)
     except (NoDHCPLeaseError) as exc:
diff --git a/cloudinit/sources/tests/test_oracle.py b/cloudinit/sources/tests/test_oracle.py
index 70003a3a9..42e70b3b6 100644
--- a/cloudinit/sources/tests/test_oracle.py
+++ b/cloudinit/sources/tests/test_oracle.py
@@ -696,7 +696,7 @@ class TestNonIscsiRoot_GetDataBehaviour:
         assert [
             mock.call(
                 iface=m_find_fallback_nic.return_value,
-                connectivity_url={
+                connectivity_url_kwargs={
                     'headers': {
                         'Authorization': 'Bearer Oracle'
                     },

[TheRealFalcon]

Thanks @blackboxsw ! Great suggestions.

I changed your connectivity_url_kwargs to connectivity_url_data because it seems a little weird to pass something called kwargs through multiple functions. I hope that's fine. Otherwise, we can bikeshed the name 😁

[blackboxsw]

Works great. Fixed flakes and awaiting CI