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
1diff --git a/cloudinit/net/eni.py b/cloudinit/net/eni.py
2index 6423632..b129bb6 100644
3--- a/cloudinit/net/eni.py
4+++ b/cloudinit/net/eni.py
5@@ -366,8 +366,6 @@ class Renderer(renderer.Renderer):
6 down = indent + "pre-down route del"
7 or_true = " || true"
8 mapping = {
9- 'network': '-net',
10- 'netmask': 'netmask',
11 'gateway': 'gw',
12 'metric': 'metric',
13 }
14@@ -379,13 +377,21 @@ class Renderer(renderer.Renderer):
15 default_gw = ' -A inet6 default'
16
17 route_line = ''
18- for k in ['network', 'netmask', 'gateway', 'metric']:
19- if default_gw and k in ['network', 'netmask']:
20+ for k in ['network', 'gateway', 'metric']:
21+ if default_gw and k == 'network':
22 continue
23 if k == 'gateway':
24 route_line += '%s %s %s' % (default_gw, mapping[k], route[k])
25 elif k in route:
26- route_line += ' %s %s' % (mapping[k], route[k])
27+ if k == 'network':
28+ if ':' in route[k]:
29+ route_line += ' -A inet6'
30+ else:
31+ route_line += ' -net'
32+ if 'prefix' in route:
33+ route_line += ' %s/%s' % (route[k], route['prefix'])
34+ else:
35+ route_line += ' %s %s' % (mapping[k], route[k])
36 content.append(up + route_line + or_true)
37 content.append(down + route_line + or_true)
38 return content
39diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
40index e3b9e02..f725a4b 100644
41--- a/tests/unittests/test_net.py
42+++ b/tests/unittests/test_net.py
43@@ -1113,8 +1113,8 @@ iface eth0.101 inet static
44 iface eth0.101 inet static
45 address 192.168.2.10/24
46
47-post-up route add -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
48-pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true
49+post-up route add -net 10.0.0.0/8 gw 11.0.0.1 metric 3 || true
50+pre-down route del -net 10.0.0.0/8 gw 11.0.0.1 metric 3 || true
51 """),
52 'expected_netplan': textwrap.dedent("""
53 network:
54@@ -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
55 - gateway: 192.168.0.3
56 netmask: 255.255.255.0
57 network: 10.1.3.0
58- - gateway: 2001:67c:1562:1
59- network: 2001:67c:1
60- netmask: ffff:ffff:0
61- - gateway: 3001:67c:1562:1
62- network: 3001:67c:1
63- netmask: ffff:ffff:0
64- metric: 10000
65 - type: static
66 address: 192.168.1.2/24
67 - type: static
68 address: 2001:1::1/92
69+ routes:
70+ - gateway: 2001:67c:1562:1
71+ network: 2001:67c:1
72+ netmask: ffff:ffff:0
73+ - gateway: 3001:67c:1562:1
74+ network: 3001:67c:1
75+ netmask: ffff:ffff:0
76+ metric: 10000
77 """),
78 'expected_netplan': textwrap.dedent("""
79 network:
80@@ -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
81 to: 3001:67c:1/32
82 via: 3001:67c:1562:1
83 """),
84+ 'expected_eni': textwrap.dedent("""\
85+auto lo
86+iface lo inet loopback
87+
88+auto bond0s0
89+iface bond0s0 inet manual
90+ bond-master bond0
91+ bond-mode active-backup
92+ bond-xmit-hash-policy layer3+4
93+ bond_miimon 100
94+
95+auto bond0s1
96+iface bond0s1 inet manual
97+ bond-master bond0
98+ bond-mode active-backup
99+ bond-xmit-hash-policy layer3+4
100+ bond_miimon 100
101+
102+auto bond0
103+iface bond0 inet static
104+ address 192.168.0.2/24
105+ gateway 192.168.0.1
106+ bond-mode active-backup
107+ bond-slaves none
108+ bond-xmit-hash-policy layer3+4
109+ bond_miimon 100
110+ hwaddress aa:bb:cc:dd:e8:ff
111+ mtu 9000
112+ post-up route add -net 10.1.3.0/24 gw 192.168.0.3 || true
113+ pre-down route del -net 10.1.3.0/24 gw 192.168.0.3 || true
114+
115+# control-alias bond0
116+iface bond0 inet static
117+ address 192.168.1.2/24
118+
119+# control-alias bond0
120+iface bond0 inet6 static
121+ address 2001:1::1/92
122+ post-up route add -A inet6 2001:67c:1/32 gw 2001:67c:1562:1 || true
123+ pre-down route del -A inet6 2001:67c:1/32 gw 2001:67c:1562:1 || true
124+ post-up route add -A inet6 3001:67c:1/32 gw 3001:67c:1562:1 metric 10000 \
125+|| true
126+ pre-down route del -A inet6 3001:67c:1/32 gw 3001:67c:1562:1 metric 10000 \
127+|| true
128+ """),
129 'yaml-v2': textwrap.dedent("""
130 version: 2
131 ethernets:
132@@ -3570,17 +3616,17 @@ class TestEniRoundTrip(CiTestCase):
133 'iface eth0 inet static',
134 ' address 172.23.31.42/26',
135 ' gateway 172.23.31.2',
136- ('post-up route add -net 10.0.0.0 netmask 255.240.0.0 gw '
137+ ('post-up route add -net 10.0.0.0/12 gw '
138 '172.23.31.1 metric 0 || true'),
139- ('pre-down route del -net 10.0.0.0 netmask 255.240.0.0 gw '
140+ ('pre-down route del -net 10.0.0.0/12 gw '
141 '172.23.31.1 metric 0 || true'),
142- ('post-up route add -net 192.168.2.0 netmask 255.255.0.0 gw '
143+ ('post-up route add -net 192.168.2.0/16 gw '
144 '172.23.31.1 metric 0 || true'),
145- ('pre-down route del -net 192.168.2.0 netmask 255.255.0.0 gw '
146+ ('pre-down route del -net 192.168.2.0/16 gw '
147 '172.23.31.1 metric 0 || true'),
148- ('post-up route add -net 10.0.200.0 netmask 255.255.0.0 gw '
149+ ('post-up route add -net 10.0.200.0/16 gw '
150 '172.23.31.1 metric 1 || true'),
151- ('pre-down route del -net 10.0.200.0 netmask 255.255.0.0 gw '
152+ ('pre-down route del -net 10.0.200.0/16 gw '
153 '172.23.31.1 metric 1 || true'),
154 ]
155 found = files['/etc/network/interfaces'].splitlines()
156@@ -3588,6 +3634,77 @@ class TestEniRoundTrip(CiTestCase):
157 self.assertEqual(
158 expected, [line for line in found if line])
159
160+ def test_ipv6_static_routes(self):
161+ # as reported in bug 1818669
162+ conf = [
163+ {'name': 'eno3', 'type': 'physical',
164+ 'subnets': [{
165+ 'address': 'fd00::12/64',
166+ 'dns_nameservers': ['fd00:2::15'],
167+ 'gateway': 'fd00::1',
168+ 'ipv6': True,
169+ 'type': 'static',
170+ 'routes': [{'netmask': '32',
171+ 'network': 'fd00:12::',
172+ 'gateway': 'fd00::2'},
173+ {'network': 'fd00:14::',
174+ 'gateway': 'fd00::3'},
175+ {'destination': 'fe00:14::/48',
176+ 'gateway': 'fe00::4',
177+ 'metric': 500},
178+ {'gateway': '192.168.23.1',
179+ 'metric': 999,
180+ 'netmask': 24,
181+ 'network': '192.168.23.0'},
182+ {'destination': '10.23.23.0/24',
183+ 'gateway': '10.23.23.2',
184+ 'metric': 300}]}]},
185+ ]
186+
187+ files = self._render_and_read(
188+ network_config={'config': conf, 'version': 1})
189+ expected = [
190+ 'auto lo',
191+ 'iface lo inet loopback',
192+ 'auto eno3',
193+ 'iface eno3 inet6 static',
194+ ' address fd00::12/64',
195+ ' dns-nameservers fd00:2::15',
196+ ' gateway fd00::1',
197+ (' post-up route add -A inet6 fd00:12::/32 gw '
198+ 'fd00::2 || true'),
199+ (' pre-down route del -A inet6 fd00:12::/32 gw '
200+ 'fd00::2 || true'),
201+ (' post-up route add -A inet6 fd00:14::/64 gw '
202+ 'fd00::3 || true'),
203+ (' pre-down route del -A inet6 fd00:14::/64 gw '
204+ 'fd00::3 || true'),
205+ (' post-up route add -A inet6 fe00:14::/48 gw '
206+ 'fe00::4 metric 500 || true'),
207+ (' pre-down route del -A inet6 fe00:14::/48 gw '
208+ 'fe00::4 metric 500 || true'),
209+ (' post-up route add -net 192.168.23.0/24 gw '
210+ '192.168.23.1 metric 999 || true'),
211+ (' pre-down route del -net 192.168.23.0/24 gw '
212+ '192.168.23.1 metric 999 || true'),
213+ (' post-up route add -net 10.23.23.0/24 gw '
214+ '10.23.23.2 metric 300 || true'),
215+ (' pre-down route del -net 10.23.23.0/24 gw '
216+ '10.23.23.2 metric 300 || true'),
217+
218+ ]
219+ found = files['/etc/network/interfaces'].splitlines()
220+
221+ self.assertEqual(
222+ expected, [line for line in found if line])
223+
224+ def testsimple_render_bond(self):
225+ entry = NETWORK_CONFIGS['bond']
226+ files = self._render_and_read(network_config=yaml.load(entry['yaml']))
227+ self.assertEqual(
228+ entry['expected_eni'].splitlines(),
229+ files['/etc/network/interfaces'].splitlines())
230+
231
232 class TestNetRenderers(CiTestCase):
233 @mock.patch("cloudinit.net.renderers.sysconfig.available")

Subscribers

People subscribed via source and target branches