Merge ~minagalic/cloud-init:feax/ephemeral_connectivity into cloud-init:master

[Mina Galić]

net: Ephemeral*Network: add connectivity check via URL

We add a new Optional parameter: connectivity_url
This is used in __enter__ to verify if a connection already exists.
If it does exist, no operations are performed.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:3f711fd9cbde2e4fb62a02d019e21770dd3e242c
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~i.galic/cloud-init/+git/cloud-init/+merge/358876/+edit-commit-message

https://jenkins.ubuntu.com/server/job/cloud-init-ci/446/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/446/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:0bb25f3a74af9d2cf21c5e657f0b3325ab79c0e8
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~i.galic/cloud-init/+git/cloud-init/+merge/358876/+edit-commit-message

https://jenkins.ubuntu.com/server/job/cloud-init-ci/448/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/448/rebuild

review: Needs Fixing (continuous-integration)

f3c4223... by =?utf-8?q?Igor_Gali=C4=87?= <email address hidden> on 2018-11-15

EphemeralDHCPv4: add connectivity_url parameter

..and pass it on to EphemeralIpv4Network

[Mina Galić (minagalic)]

I'm gonna need some help here

review: Needs Fixing

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:119058aadd56a91c7889e861b7d7903fde164f3e
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~i.galic/cloud-init/+git/cloud-init/+merge/358876/+edit-commit-message

https://jenkins.ubuntu.com/server/job/cloud-init-ci/450/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/450/rebuild

review: Needs Fixing (continuous-integration)

68046f6... by =?utf-8?q?Igor_Gali=C4=87?= <email address hidden> on 2018-11-16

Thanks to tests, discovered it's better to test connectivity ourselves

in __enter__(), *before* calling self.obtain_lease()
Otherwise we're performing too much discovery before trying to actually test
connectivity by instantiating EphemeralIPv4Network

The question left now, is whether we should still pass connectivity_url to
EphemeralIPv4Network. Chances are, nothing has changed in the last sub-second,
and if it hasn't, then we're taking another 5 second penalty here.

[Mina Galić (minagalic)]

Looks pretty okay so far, except for…

review: Needs Fixing

0291cf3... by =?utf-8?q?Igor_Gali=C4=87?= <email address hidden> on 2018-11-16

Remove private _connectivity_check()

we're using a public module function now

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:5dcdff256a4a6b87a5b5cb54ebcadb04864b1fbb
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~i.galic/cloud-init/+git/cloud-init/+merge/358876/+edit-commit-message

https://jenkins.ubuntu.com/server/job/cloud-init-ci/452/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/452/rebuild

review: Needs Fixing (continuous-integration)

[Mina Galić (minagalic)]

should we pass connectivity_url from EphemeralDhcp4Network to EphemeralIPv4Network?

review: Needs Information

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:a3b483d25a8e0972f53783696a20de8b305978d6
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~i.galic/cloud-init/+git/cloud-init/+merge/358876/+edit-commit-message

https://jenkins.ubuntu.com/server/job/cloud-init-ci/453/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/453/rebuild

review: Needs Fixing (continuous-integration)

[Chad Smith (chad.smith)]

Here is a patch with some unit test cleanup and unit test adds for the new function you added.

I also adding a bit of checking on the url param passed to has_url_connectivity function since we don't want callsites to stuff in the wrong type of content.

Unit test adds and functionality changes below
http://paste.ubuntu.com/p/PQkjG6pHG8/

[Chad Smith (chad.smith)]

The integration tests also voted failure on your branch because you didn't import UrlError in net/__init__.py. running tox locally on your system would have shown this if you have unit tests on the new function that was added.

f846655... by Chad Smith on 2018-11-21

unit test cleanup; add unit test for has_url_connectivity

Also added a bit of checking on the url param passed to has_url_connectivity
function since we don't want callsites to stuff in the wrong type of content.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:7cba05e77a39567c1d3967027a85e1e0f478a13f
https://jenkins.ubuntu.com/server/job/cloud-init-ci/455/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/455/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:2605bf0efab8dd64c9dde7c1c3b3f93123ce3b75
https://jenkins.ubuntu.com/server/job/cloud-init-ci/456/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/456/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:f8466558de72accfcb9aca923a147fb3bedb2a4e
https://jenkins.ubuntu.com/server/job/cloud-init-ci/457/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/457/rebuild

review: Approve (continuous-integration)

diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
index ad98a59..3642fb1 100644
--- a/cloudinit/net/__init__.py
+++ b/cloudinit/net/__init__.py
@@ -12,6 +12,7 @@ import re
 
 from cloudinit.net.network_state import mask_to_net_prefix
 from cloudinit import util
+from cloudinit.url_helper import UrlError, readurl
 
 LOG = logging.getLogger(__name__)
 SYS_CLASS_NET = "/sys/class/net/"
@@ -647,16 +648,36 @@ def get_ib_hwaddrs_by_interface():
     return ret
 
 
+def has_url_connectivity(url):
+    """Return true when the instance has access to the provided URL
+
+    Logs a warning if url is not the expected format.
+    """
+    if not any([url.startswith('http://'), url.startswith('https://')]):
+        LOG.warning(
+            "Ignoring connectivity check. Expected URL beginning with http*://"
+            " received '%s'", url)
+        return False
+    try:
+        readurl(url, timeout=5)
+    except UrlError:
+        return False
+    return True
+
+
 class EphemeralIPv4Network(object):
     """Context manager which sets up temporary static network configuration.
 
-    No operations are performed if the provided interface is already connected.
+    No operations are performed if the provided interface already has the
+    specified configuration.
+    This can be verified with the connectivity_url.
     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):
+    def __init__(self, interface, ip, prefix_or_mask, broadcast, router=None,
+                 connectivity_url=None):
         """Setup context manager and validate call signature.
 
         @param interface: Name of the network interface to bring up.
@@ -665,6 +686,8 @@ 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
+           connection already exists.
         """
         if not all([interface, ip, prefix_or_mask, broadcast]):
             raise ValueError(
@@ -675,6 +698,8 @@ class EphemeralIPv4Network(object):
         except ValueError as e:
             raise ValueError(
                 'Cannot setup network: {0}'.format(e))
+
+        self.connectivity_url = connectivity_url
         self.interface = interface
         self.ip = ip
         self.broadcast = broadcast
@@ -683,6 +708,13 @@ 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):
+                LOG.debug(
+                    'Skip ephemeral network setup, instance has connectivity'
+                    ' to %s', self.connectivity_url)
+                return
+
         self._bringup_device()
         if self.router:
             self._bringup_router()
diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py
index bdc5799..0db991d 100644
--- a/cloudinit/net/dhcp.py
+++ b/cloudinit/net/dhcp.py
@@ -11,7 +11,8 @@ import re
 import signal
 
 from cloudinit.net import (
-    EphemeralIPv4Network, find_fallback_nic, get_devicelist)
+    EphemeralIPv4Network, find_fallback_nic, get_devicelist,
+    has_url_connectivity)
 from cloudinit.net.network_state import mask_and_ipv4_to_bcast_addr as bcip
 from cloudinit import temp_utils
 from cloudinit import util
@@ -37,13 +38,21 @@ class NoDHCPLeaseError(Exception):
 
 
 class EphemeralDHCPv4(object):
-    def __init__(self, iface=None):
+    def __init__(self, iface=None, connectivity_url=None):
         self.iface = iface
         self._ephipv4 = None
         self.lease = None
+        self.connectivity_url = connectivity_url
 
     def __enter__(self):
-        """Setup sandboxed dhcp context."""
+        """Setup sandboxed dhcp context, unless connectivity_url can already be
+        reached."""
+        if self.connectivity_url:
+            if has_url_connectivity(self.connectivity_url):
+                LOG.debug(
+                    'Skip ephemeral DHCP setup, instance has connectivity'
+                    ' to %s', self.connectivity_url)
+                return
         return self.obtain_lease()
 
     def __exit__(self, excp_type, excp_value, excp_traceback):
@@ -86,6 +95,8 @@ class EphemeralDHCPv4(object):
         kwargs = dict([(k, self.lease.get(v)) for k, v in nmap.items()])
         if not kwargs['broadcast']:
             kwargs['broadcast'] = bcip(kwargs['prefix_or_mask'], kwargs['ip'])
+        if self.connectivity_url:
+            kwargs['connectivity_url'] = self.connectivity_url
         ephipv4 = EphemeralIPv4Network(**kwargs)
         ephipv4.__enter__()
         self._ephipv4 = ephipv4
diff --git a/cloudinit/net/tests/test_dhcp.py b/cloudinit/net/tests/test_dhcp.py
index db25b6f..cd3e732 100644
--- a/cloudinit/net/tests/test_dhcp.py
+++ b/cloudinit/net/tests/test_dhcp.py
@@ -1,15 +1,17 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
+import httpretty
 import os
 import signal
 from textwrap import dedent
 
+import cloudinit.net as net
 from cloudinit.net.dhcp import (
     InvalidDHCPLeaseFileError, maybe_perform_dhcp_discovery,
     parse_dhcp_lease_file, dhcp_discovery, networkd_load_leases)
 from cloudinit.util import ensure_file, write_file
 from cloudinit.tests.helpers import (
-    CiTestCase, mock, populate_dir, wrap_and_call)
+    CiTestCase, HttprettyTestCase, mock, populate_dir, wrap_and_call)
 
 
 class TestParseDHCPLeasesFile(CiTestCase):
@@ -321,3 +323,35 @@ class TestSystemdParseLeases(CiTestCase):
                                     '9': self.lxd_lease})
         self.assertEqual({'1': self.azure_parsed, '9': self.lxd_parsed},
                          networkd_load_leases(self.lease_d))
+
+
+class TestEphemeralDhcpNoNetworkSetup(HttprettyTestCase):
+
+    @mock.patch('cloudinit.net.dhcp.maybe_perform_dhcp_discovery')
+    def test_ephemeral_dhcp_no_network_if_url_connectivity(self, m_dhcp):
+        """No EphemeralDhcp4 network setup when connectivity_url succeeds."""
+        url = 'http://example.org/index.html'
+
+        httpretty.register_uri(httpretty.GET, url)
+        with net.dhcp.EphemeralDHCPv4(connectivity_url=url) as lease:
+            self.assertIsNone(lease)
+        # Ensure that no teardown happens:
+        m_dhcp.assert_not_called()
+
+    @mock.patch('cloudinit.net.dhcp.util.subp')
+    @mock.patch('cloudinit.net.dhcp.maybe_perform_dhcp_discovery')
+    def test_ephemeral_dhcp_setup_network_if_url_connectivity(
+            self, m_dhcp, m_subp):
+        """No EphemeralDhcp4 network setup when connectivity_url succeeds."""
+        url = 'http://example.org/index.html'
+        fake_lease = {
+            'interface': 'eth9', 'fixed-address': '192.168.2.2',
+            'subnet-mask': '255.255.0.0'}
+        m_dhcp.return_value = [fake_lease]
+        m_subp.return_value = ('', '')
+
+        httpretty.register_uri(httpretty.GET, url, body={}, status=404)
+        with net.dhcp.EphemeralDHCPv4(connectivity_url=url) as lease:
+            self.assertEqual(fake_lease, lease)
+        # Ensure that dhcp discovery occurs
+        m_dhcp.called_once_with()
diff --git a/cloudinit/net/tests/test_init.py b/cloudinit/net/tests/test_init.py
index 58e0a59..f55c31e 100644
--- a/cloudinit/net/tests/test_init.py
+++ b/cloudinit/net/tests/test_init.py
@@ -2,14 +2,16 @@
 
 import copy
 import errno
+import httpretty
 import mock
 import os
+import requests
 import textwrap
 import yaml
 
 import cloudinit.net as net
 from cloudinit.util import ensure_file, write_file, ProcessExecutionError
-from cloudinit.tests.helpers import CiTestCase
+from cloudinit.tests.helpers import CiTestCase, HttprettyTestCase
 
 
 class TestSysDevPath(CiTestCase):
@@ -458,6 +460,22 @@ class TestEphemeralIPV4Network(CiTestCase):
             self.assertEqual(expected_setup_calls, m_subp.call_args_list)
         m_subp.assert_has_calls(expected_teardown_calls)
 
+    @mock.patch('cloudinit.net.readurl')
+    def test_ephemeral_ipv4_no_network_if_url_connectivity(
+            self, m_readurl, m_subp):
+        """No network setup is performed if we can successfully connect to
+        connectivity_url."""
+        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'}
+
+        with net.EphemeralIPv4Network(**params):
+            self.assertEqual([mock.call('http://example.org/index.html',
+                                        timeout=5)], m_readurl.call_args_list)
+        # Ensure that no teardown happens:
+        m_subp.assert_has_calls([])
+
     def test_ephemeral_ipv4_network_noop_when_configured(self, m_subp):
         """EphemeralIPv4Network handles exception when address is setup.
 
@@ -619,3 +637,35 @@ class TestApplyNetworkCfgNames(CiTestCase):
     def test_apply_v2_renames_raises_runtime_error_on_unknown_version(self):
         with self.assertRaises(RuntimeError):
             net.apply_network_config_names(yaml.load("version: 3"))
+
+
+class TestHasURLConnectivity(HttprettyTestCase):
+
+    def setUp(self):
+        super(TestHasURLConnectivity, self).setUp()
+        self.url = 'http://fake/'
+        self.kwargs = {'allow_redirects': True, 'timeout': 5.0}
+
+    @mock.patch('cloudinit.net.readurl')
+    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')
+
+    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')
+
+    @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),
+            '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')