Merge ~raphael-glon/cloud-init:master into cloud-init:master

Proposed by raphael.glon on 2019-03-05
Status: Merged
Approved by: Dan Watkins on 2019-03-21
Approved revision: 1af82aaadd9639ea6e32884a2dfcaa4cb205a5fd
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~raphael-glon/cloud-init:master
Merge into: cloud-init:master
Diff against target: 233 lines (+143/-20)
2 files modified
cloudinit/net/eni.py (+11/-5)
tests/unittests/test_net.py (+132/-15)
Reviewer Review Type Date Requested Status
Ryan Harper 2019-03-06 Approve on 2019-03-15
Server Team CI bot continuous-integration Approve on 2019-03-06
Dan Watkins 2019-03-05 Abstain on 2019-03-06
Review via email: mp+363970@code.launchpad.net

Commit message

net: Fix ipv6 static routes when using eni renderer

When rendering ipv6 static routes in eni format the
post-up/pre down commands were not correct for ipv6.

LP: #1818669

To post a comment you must log in.
Dan Watkins (daniel-thewatkins) wrote :

Hi Raphael,

Thank you for contributing to cloud-init.

To contribute, you must sign the Canonical Contributor License Agreement
(CLA) [1].

If you have already signed it as an individual, your Launchpad user will
be listed in the contributor-agreement-canonical launchpad group [2].
Unfortunately there is no easy way to check if an organization or company
you are doing work for has signed. If you are unsure or have questions,
email <email address hidden> or ping powersj in #cloud-init channel
via freenode.

For information on how to sign, please see the HACKING document [3].

Thanks again, and please feel free to reach out with any questions.


[1] http://www.canonical.com/contributors
[2] https://launchpad.net/~contributor-agreement-canonical/+members
[3] http://cloudinit.readthedocs.io/en/latest/topics/hacking.html

review: Needs Information
~raphael-glon/cloud-init:master updated on 2019-03-05
1af82aa... by raphael.glon on 2019-03-05

Unittests: add bond rendering test for eni config

Ryan Harper (raharper) wrote :

I've taken a look and I can confirm that for v6 routes, route needs the network with prefix.
This is information is present in the internal network_state of the route so a little juggling is needed to always use network/prefix when rendering route and ensure that on v6 routes we use -A inet6.

I've added the test-case from the bug and a few others.

This passes tox and does not regress anything.

Please consider applying this instead

http://paste.ubuntu.com/p/s3tYjGv3Jh/

raphael.glon (raphael-glon) wrote :

Ok for the considered patch
-> but there is still one problem with it:
-net is mandatory for route -4 but unsupported for ipv6
so we need to remove the -net param for ipv6 only

~raphael-glon/cloud-init:master updated on 2019-03-06
aa64677... by raphael.glon on 2019-03-06

Fix net/eni static routes for ipv6

this fixes:
    https://bugs.launchpad.net/cloud-init/+bug/1818669
    eni: post-up/pre down commands were not correct for ipv6

unittests: additional test to check the
rendered static routes for eni config

Dan Watkins (daniel-thewatkins) wrote :

CLA has been signed; thanks Raphael!

(I'm going to abstain as Ryan has started working with you on this.)

review: Abstain
Ryan Harper (raharper) wrote :

Yes; that's a good catch on the -net | -A inet6.

Ryan Harper (raharper) wrote :

I've pointed CI at this branch. If that comes out clean, I'll mark approve and land.

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

review: Approve (continuous-integration)
raphael.glon (raphael-glon) wrote :

Hello Ryan,

Not sure what I should do next about this merge request. Is there something more needed for approval ?

Regards

Ryan Harper (raharper) wrote :

Sorry for not replying. The branch is good to go. I just need to mark it for landing.

Ryan Harper (raharper) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/cloudinit/net/eni.py b/cloudinit/net/eni.py
index 6423632..b129bb6 100644
--- a/cloudinit/net/eni.py
+++ b/cloudinit/net/eni.py
@@ -366,8 +366,6 @@ class Renderer(renderer.Renderer):
366 down = indent + "pre-down route del"366 down = indent + "pre-down route del"
367 or_true = " || true"367 or_true = " || true"
368 mapping = {368 mapping = {
369 'network': '-net',
370 'netmask': 'netmask',
371 'gateway': 'gw',369 'gateway': 'gw',
372 'metric': 'metric',370 'metric': 'metric',
373 }371 }
@@ -379,13 +377,21 @@ class Renderer(renderer.Renderer):
379 default_gw = ' -A inet6 default'377 default_gw = ' -A inet6 default'
380378
381 route_line = ''379 route_line = ''
382 for k in ['network', 'netmask', 'gateway', 'metric']:380 for k in ['network', 'gateway', 'metric']:
383 if default_gw and k in ['network', 'netmask']:381 if default_gw and k == 'network':
384 continue382 continue
385 if k == 'gateway':383 if k == 'gateway':
386 route_line += '%s %s %s' % (default_gw, mapping[k], route[k])384 route_line += '%s %s %s' % (default_gw, mapping[k], route[k])
387 elif k in route:385 elif k in route:
388 route_line += ' %s %s' % (mapping[k], route[k])386 if k == 'network':
387 if ':' in route[k]:
388 route_line += ' -A inet6'
389 else:
390 route_line += ' -net'
391 if 'prefix' in route:
392 route_line += ' %s/%s' % (route[k], route['prefix'])
393 else:
394 route_line += ' %s %s' % (mapping[k], route[k])
389 content.append(up + route_line + or_true)395 content.append(up + route_line + or_true)
390 content.append(down + route_line + or_true)396 content.append(down + route_line + or_true)
391 return content397 return content
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index e3b9e02..f725a4b 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -1113,8 +1113,8 @@ iface eth0.101 inet static
1113iface eth0.101 inet static1113iface eth0.101 inet static
1114 address 192.168.2.10/241114 address 192.168.2.10/24
11151115
1116post-up route add -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true1116post-up route add -net 10.0.0.0/8 gw 11.0.0.1 metric 3 || true
1117pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true1117pre-down route del -net 10.0.0.0/8 gw 11.0.0.1 metric 3 || true
1118"""),1118"""),
1119 'expected_netplan': textwrap.dedent("""1119 'expected_netplan': textwrap.dedent("""
1120 network:1120 network:
@@ -1505,17 +1505,18 @@ pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
1505 - gateway: 192.168.0.31505 - gateway: 192.168.0.3
1506 netmask: 255.255.255.01506 netmask: 255.255.255.0
1507 network: 10.1.3.01507 network: 10.1.3.0
1508 - gateway: 2001:67c:1562:1
1509 network: 2001:67c:1
1510 netmask: ffff:ffff:0
1511 - gateway: 3001:67c:1562:1
1512 network: 3001:67c:1
1513 netmask: ffff:ffff:0
1514 metric: 10000
1515 - type: static1508 - type: static
1516 address: 192.168.1.2/241509 address: 192.168.1.2/24
1517 - type: static1510 - type: static
1518 address: 2001:1::1/921511 address: 2001:1::1/92
1512 routes:
1513 - gateway: 2001:67c:1562:1
1514 network: 2001:67c:1
1515 netmask: ffff:ffff:0
1516 - gateway: 3001:67c:1562:1
1517 network: 3001:67c:1
1518 netmask: ffff:ffff:0
1519 metric: 10000
1519 """),1520 """),
1520 'expected_netplan': textwrap.dedent("""1521 'expected_netplan': textwrap.dedent("""
1521 network:1522 network:
@@ -1554,6 +1555,51 @@ pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
1554 to: 3001:67c:1/321555 to: 3001:67c:1/32
1555 via: 3001:67c:1562:11556 via: 3001:67c:1562:1
1556 """),1557 """),
1558 'expected_eni': textwrap.dedent("""\
1559auto lo
1560iface lo inet loopback
1561
1562auto bond0s0
1563iface bond0s0 inet manual
1564 bond-master bond0
1565 bond-mode active-backup
1566 bond-xmit-hash-policy layer3+4
1567 bond_miimon 100
1568
1569auto bond0s1
1570iface bond0s1 inet manual
1571 bond-master bond0
1572 bond-mode active-backup
1573 bond-xmit-hash-policy layer3+4
1574 bond_miimon 100
1575
1576auto bond0
1577iface bond0 inet static
1578 address 192.168.0.2/24
1579 gateway 192.168.0.1
1580 bond-mode active-backup
1581 bond-slaves none
1582 bond-xmit-hash-policy layer3+4
1583 bond_miimon 100
1584 hwaddress aa:bb:cc:dd:e8:ff
1585 mtu 9000
1586 post-up route add -net 10.1.3.0/24 gw 192.168.0.3 || true
1587 pre-down route del -net 10.1.3.0/24 gw 192.168.0.3 || true
1588
1589# control-alias bond0
1590iface bond0 inet static
1591 address 192.168.1.2/24
1592
1593# control-alias bond0
1594iface bond0 inet6 static
1595 address 2001:1::1/92
1596 post-up route add -A inet6 2001:67c:1/32 gw 2001:67c:1562:1 || true
1597 pre-down route del -A inet6 2001:67c:1/32 gw 2001:67c:1562:1 || true
1598 post-up route add -A inet6 3001:67c:1/32 gw 3001:67c:1562:1 metric 10000 \
1599|| true
1600 pre-down route del -A inet6 3001:67c:1/32 gw 3001:67c:1562:1 metric 10000 \
1601|| true
1602 """),
1557 'yaml-v2': textwrap.dedent("""1603 'yaml-v2': textwrap.dedent("""
1558 version: 21604 version: 2
1559 ethernets:1605 ethernets:
@@ -3570,17 +3616,17 @@ class TestEniRoundTrip(CiTestCase):
3570 'iface eth0 inet static',3616 'iface eth0 inet static',
3571 ' address 172.23.31.42/26',3617 ' address 172.23.31.42/26',
3572 ' gateway 172.23.31.2',3618 ' gateway 172.23.31.2',
3573 ('post-up route add -net 10.0.0.0 netmask 255.240.0.0 gw '3619 ('post-up route add -net 10.0.0.0/12 gw '
3574 '172.23.31.1 metric 0 || true'),3620 '172.23.31.1 metric 0 || true'),
3575 ('pre-down route del -net 10.0.0.0 netmask 255.240.0.0 gw '3621 ('pre-down route del -net 10.0.0.0/12 gw '
3576 '172.23.31.1 metric 0 || true'),3622 '172.23.31.1 metric 0 || true'),
3577 ('post-up route add -net 192.168.2.0 netmask 255.255.0.0 gw '3623 ('post-up route add -net 192.168.2.0/16 gw '
3578 '172.23.31.1 metric 0 || true'),3624 '172.23.31.1 metric 0 || true'),
3579 ('pre-down route del -net 192.168.2.0 netmask 255.255.0.0 gw '3625 ('pre-down route del -net 192.168.2.0/16 gw '
3580 '172.23.31.1 metric 0 || true'),3626 '172.23.31.1 metric 0 || true'),
3581 ('post-up route add -net 10.0.200.0 netmask 255.255.0.0 gw '3627 ('post-up route add -net 10.0.200.0/16 gw '
3582 '172.23.31.1 metric 1 || true'),3628 '172.23.31.1 metric 1 || true'),
3583 ('pre-down route del -net 10.0.200.0 netmask 255.255.0.0 gw '3629 ('pre-down route del -net 10.0.200.0/16 gw '
3584 '172.23.31.1 metric 1 || true'),3630 '172.23.31.1 metric 1 || true'),
3585 ]3631 ]
3586 found = files['/etc/network/interfaces'].splitlines()3632 found = files['/etc/network/interfaces'].splitlines()
@@ -3588,6 +3634,77 @@ class TestEniRoundTrip(CiTestCase):
3588 self.assertEqual(3634 self.assertEqual(
3589 expected, [line for line in found if line])3635 expected, [line for line in found if line])
35903636
3637 def test_ipv6_static_routes(self):
3638 # as reported in bug 1818669
3639 conf = [
3640 {'name': 'eno3', 'type': 'physical',
3641 'subnets': [{
3642 'address': 'fd00::12/64',
3643 'dns_nameservers': ['fd00:2::15'],
3644 'gateway': 'fd00::1',
3645 'ipv6': True,
3646 'type': 'static',
3647 'routes': [{'netmask': '32',
3648 'network': 'fd00:12::',
3649 'gateway': 'fd00::2'},
3650 {'network': 'fd00:14::',
3651 'gateway': 'fd00::3'},
3652 {'destination': 'fe00:14::/48',
3653 'gateway': 'fe00::4',
3654 'metric': 500},
3655 {'gateway': '192.168.23.1',
3656 'metric': 999,
3657 'netmask': 24,
3658 'network': '192.168.23.0'},
3659 {'destination': '10.23.23.0/24',
3660 'gateway': '10.23.23.2',
3661 'metric': 300}]}]},
3662 ]
3663
3664 files = self._render_and_read(
3665 network_config={'config': conf, 'version': 1})
3666 expected = [
3667 'auto lo',
3668 'iface lo inet loopback',
3669 'auto eno3',
3670 'iface eno3 inet6 static',
3671 ' address fd00::12/64',
3672 ' dns-nameservers fd00:2::15',
3673 ' gateway fd00::1',
3674 (' post-up route add -A inet6 fd00:12::/32 gw '
3675 'fd00::2 || true'),
3676 (' pre-down route del -A inet6 fd00:12::/32 gw '
3677 'fd00::2 || true'),
3678 (' post-up route add -A inet6 fd00:14::/64 gw '
3679 'fd00::3 || true'),
3680 (' pre-down route del -A inet6 fd00:14::/64 gw '
3681 'fd00::3 || true'),
3682 (' post-up route add -A inet6 fe00:14::/48 gw '
3683 'fe00::4 metric 500 || true'),
3684 (' pre-down route del -A inet6 fe00:14::/48 gw '
3685 'fe00::4 metric 500 || true'),
3686 (' post-up route add -net 192.168.23.0/24 gw '
3687 '192.168.23.1 metric 999 || true'),
3688 (' pre-down route del -net 192.168.23.0/24 gw '
3689 '192.168.23.1 metric 999 || true'),
3690 (' post-up route add -net 10.23.23.0/24 gw '
3691 '10.23.23.2 metric 300 || true'),
3692 (' pre-down route del -net 10.23.23.0/24 gw '
3693 '10.23.23.2 metric 300 || true'),
3694
3695 ]
3696 found = files['/etc/network/interfaces'].splitlines()
3697
3698 self.assertEqual(
3699 expected, [line for line in found if line])
3700
3701 def testsimple_render_bond(self):
3702 entry = NETWORK_CONFIGS['bond']
3703 files = self._render_and_read(network_config=yaml.load(entry['yaml']))
3704 self.assertEqual(
3705 entry['expected_eni'].splitlines(),
3706 files['/etc/network/interfaces'].splitlines())
3707
35913708
3592class TestNetRenderers(CiTestCase):3709class TestNetRenderers(CiTestCase):
3593 @mock.patch("cloudinit.net.renderers.sysconfig.available")3710 @mock.patch("cloudinit.net.renderers.sysconfig.available")

Subscribers

People subscribed via source and target branches