Merge ~amzn-cmiller/cloud-init:ec2-metadatasrv-route-eexist into cloud-init:master

[Chad]

Allow route already to exist for disable-ec2-metadata

Try CHANGE route to metadata service, not just ADD route.

Linux doesn't allow two routes with the same match characteristics.
If an explicit route already exists, then ip-route "add" will fail.
Now, "change" first, and only "add" if it failed.

[Scott Moser (smoser)]

review: Approve

[Andrew Jorgensen (ajorgens)]

review: Approve

[Chad (amzn-cmiller)]

ping?

[Ryan Harper (raharper)]

I've pointed our CI at this branch.

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

FAILED: Continuous integration, rev:2f26e6e278ceb7ec564857c2a2fdeec882868714
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/~amzn-cmiller/cloud-init/+git/cloud-init/+merge/360082/+edit-commit-message

https://jenkins.ubuntu.com/server/job/cloud-init-ci/666/
Executed test runs:
    FAILED: Checkout

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

review: Needs Fixing (continuous-integration)

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

FAILED: Continuous integration, rev:2f26e6e278ceb7ec564857c2a2fdeec882868714
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/~amzn-cmiller/cloud-init/+git/cloud-init/+merge/360082/+edit-commit-message

https://jenkins.ubuntu.com/server/job/cloud-init-ci/667/
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/667/rebuild

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:2f26e6e278ceb7ec564857c2a2fdeec882868714
https://jenkins.ubuntu.com/server/job/cloud-init-ci/688/
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/688/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Was there an upstream bug or just cleaning things up?

A few inline comment typo/adjustments requested.

review: Needs Fixing

Unmerged commits

305804e... by Chad on 2019-07-15

Fix grammar in comment about ip-route assumptions

2f26e6e... by Chad on 2018-11-05

Allow route already to exist for disable-ec2-metadata

Try CHANGE route to metadata service, not just ADD route.

Linux doesn't allow two routes with the same match characteristics. If an
explicit route already exists, then ip-route "add" will fail. Now, "change"
first, and only "add" if it failed.

diff --git a/cloudinit/config/cc_disable_ec2_metadata.py b/cloudinit/config/cc_disable_ec2_metadata.py
index 885b313..5db76f0 100644
--- a/cloudinit/config/cc_disable_ec2_metadata.py
+++ b/cloudinit/config/cc_disable_ec2_metadata.py
@@ -33,22 +33,26 @@ from cloudinit.settings import PER_ALWAYS
 frequency = PER_ALWAYS
 
 REJECT_CMD_IF = ['route', 'add', '-host', '169.254.169.254', 'reject']
-REJECT_CMD_IP = ['ip', 'route', 'add', 'prohibit', '169.254.169.254']
+REJECT_CMD_IPRTADD = ['ip', 'route', 'add', 'prohibit', '169.254.169.254']
+REJECT_CMD_IPRTCHG = ['ip', 'route', 'change', 'prohibit', '169.254.169.254']
 
 
 def handle(name, cfg, _cloud, log, _args):
     disabled = util.get_cfg_option_bool(cfg, "disable_ec2_metadata", False)
     if disabled:
-        reject_cmd = None
         if util.which('ip'):
-            reject_cmd = REJECT_CMD_IP
+            # ip-route-add requires that no route already exist and
+            # ip-route-change requires that a route already exists. EAFP.
+            try:
+                util.subp(REJECT_CMD_IPRTCHG, capture=False)
+            except util.ProcessExecutionError:
+                util.subp(REJECT_CMD_IPRTADD, capture=False)
         elif util.which('ifconfig'):
-            reject_cmd = REJECT_CMD_IF
+            util.subp(REJECT_CMD_IF, capture=False)
         else:
             log.error(('Neither "route" nor "ip" command found, unable to '
                        'manipulate routing table'))
             return
-        util.subp(reject_cmd, capture=False)
     else:
         log.debug(("Skipping module named %s,"
                    " disabling the ec2 route not enabled"), name)
diff --git a/cloudinit/config/tests/test_disable_ec2_metadata.py b/cloudinit/config/tests/test_disable_ec2_metadata.py
index 67646b0..a6421dd 100644
--- a/cloudinit/config/tests/test_disable_ec2_metadata.py
+++ b/cloudinit/config/tests/test_disable_ec2_metadata.py
@@ -29,9 +29,16 @@ class TestEC2MetadataRoute(CiTestCase):
 
     @mock.patch('cloudinit.config.cc_disable_ec2_metadata.util.which')
     @mock.patch('cloudinit.config.cc_disable_ec2_metadata.util.subp')
-    def test_disable_ip(self, m_subp, m_which):
+    def test_disable_ip_route_is_new(self, m_subp, m_which):
         """Set the route if ip command is available"""
         m_which.side_effect = lambda x: x if x == 'ip' else None
+
+        def subp(cmd, **kwargs):
+            if cmd[2] == 'change':
+                raise ec2_meta.util.ProcessExecutionError("Oh noes")
+            return None, None
+        m_subp.side_effect = subp
+
         ec2_meta.handle('foo', DISABLE_CFG, None, LOG, None)
         m_subp.assert_called_with(
             ['ip', 'route', 'add', 'prohibit', '169.254.169.254'],
@@ -39,6 +46,16 @@ class TestEC2MetadataRoute(CiTestCase):
 
     @mock.patch('cloudinit.config.cc_disable_ec2_metadata.util.which')
     @mock.patch('cloudinit.config.cc_disable_ec2_metadata.util.subp')
+    def test_disable_ip_route_already_exists(self, m_subp, m_which):
+        """Set the route if ip command is available"""
+        m_which.side_effect = lambda x: x if x == 'ip' else None
+        ec2_meta.handle('foo', DISABLE_CFG, None, LOG, None)
+        m_subp.assert_called_with(
+            ['ip', 'route', 'change', 'prohibit', '169.254.169.254'],
+            capture=False)
+
+    @mock.patch('cloudinit.config.cc_disable_ec2_metadata.util.which')
+    @mock.patch('cloudinit.config.cc_disable_ec2_metadata.util.subp')
     def test_disable_no_tool(self, m_subp, m_which):
         """Log error when neither route nor ip commands are available"""
         m_which.return_value = None  # Find neither ifconfig nor ip