Merge ~raharper/cloud-init:fix/ephemeral-dhcp-static-routes into cloud-init:master
- Git
- lp:~raharper/cloud-init
- fix/ephemeral-dhcp-static-routes
- Merge into master
Status: | Merged |
---|---|
Approved by: | Dan Watkins |
Approved revision: | 24e1632715344f3cdb615f153e2fae30dce0f4bc |
Merge reported by: | Server Team CI bot |
Merged at revision: | not available |
Proposed branch: | ~raharper/cloud-init:fix/ephemeral-dhcp-static-routes |
Merge into: | cloud-init:master |
Diff against target: |
407 lines (+286/-6) 6 files modified
cloudinit/net/__init__.py (+32/-2) cloudinit/net/dhcp.py (+90/-0) cloudinit/net/tests/test_dhcp.py (+119/-1) cloudinit/net/tests/test_init.py (+39/-0) tests/unittests/test_datasource/test_azure.py (+4/-2) tests/unittests/test_datasource/test_ec2.py (+2/-1) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Dan Watkins | Approve | ||
Server Team CI bot | continuous-integration | Approve | |
Review via email: mp+368553@code.launchpad.net |
Commit message
net: add rfc3442 (classless static routes) to EphemeralDHCP
The EphemeralDHCP context manager did not parse or handle
rfc3442 classless static routes which prevented reading
datasource metadata in some clouds. This branch adds support
for extracting the field from the leases output, parsing the
format and then adding the required iproute2 ip commands to
apply (and teardown) the static routes.
LP: #1821102
Description of the change
Ryan Harper (raharper) wrote : | # |
+1 on expanded testing of the rfc3442 parsing.
Ryan Harper (raharper) : | # |
Dan Watkins (oddbloke) : | # |
- 20187bf... by Ryan Harper
-
address feedback on parsing rfc3442
dhcp.py/
parse_static_ routes:
- Fix comments to match code and correct values
- Always return a list type
- Reduce parsing of the result but returning a list of tuples
containing the network address and the gateway.
- fix unittests - 7ff9772... by Ryan Harper
-
RFC3442: add additional unittests and error logging
Adding additional unittests for parsing of RFC3442 format.
- Fix missing net_length for class a, b, c routes
- Removed hard-coded net_length for default routes
- Log an error with details on what failed during parsing - b5a4708... by Ryan Harper
-
rfc3442: update docstring to indicate inclusion of net length
Server Team CI bot (server-team-bot) wrote : | # |
FAILED: Continuous integration, rev:b5a47081cfd
https:/
Executed test runs:
SUCCESS: Checkout
FAILED: Unit & Style Tests
Click here to trigger a rebuild:
https:/
Server Team CI bot (server-team-bot) wrote : | # |
FAILED: Continuous integration, rev:b5a47081cfd
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
FAILED: Ubuntu LTS: Build
Click here to trigger a rebuild:
https:/
Server Team CI bot (server-team-bot) wrote : | # |
FAILED: Continuous integration, rev:b5a47081cfd
https:/
Executed test runs:
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:b5a47081cfd
https:/
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:/
Dan Watkins (oddbloke) wrote : | # |
Incomplete review at EOD.
Scott Moser (smoser) wrote : | # |
You've done a good job here. I didn't read the implementation completely, but the tests are good.
Ryan Harper (raharper) wrote : | # |
Thanks, I'll update
- 3a9ec1a... by Ryan Harper
-
Log and error if netlength is invalid
Ryan Harper (raharper) wrote : | # |
@Scott,
I left the comment where it is so to explain when cloud-init will take one path or the other.
Server Team CI bot (server-team-bot) wrote : | # |
FAILED: Continuous integration, rev:3a9ec1a0ef6
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
FAILED: Ubuntu LTS: Build
Click here to trigger a rebuild:
https:/
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:3a9ec1a0ef6
https:/
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:/
Dan Watkins (oddbloke) wrote : | # |
Looking really good overall; a couple more inline questions, but this is v. close.
Ryan Harper (raharper) wrote : | # |
Thanks, I've replied inline, looking for your thoughts before I refactor.
Dan Watkins (oddbloke) : | # |
Dan Watkins (oddbloke) : | # |
- 24e1632... by Ryan Harper
-
parse_static_
routes: on parsing error return parsed static routes like dhclient
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:24e16327153
https:/
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:/
Dan Watkins (oddbloke) wrote : | # |
Thanks for working through all this, Ryan, looks good to me!
Preview Diff
1 | diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py | |||
2 | index e758006..624c9b4 100644 | |||
3 | --- a/cloudinit/net/__init__.py | |||
4 | +++ b/cloudinit/net/__init__.py | |||
5 | @@ -679,7 +679,7 @@ class EphemeralIPv4Network(object): | |||
6 | 679 | """ | 679 | """ |
7 | 680 | 680 | ||
8 | 681 | def __init__(self, interface, ip, prefix_or_mask, broadcast, router=None, | 681 | def __init__(self, interface, ip, prefix_or_mask, broadcast, router=None, |
10 | 682 | connectivity_url=None): | 682 | connectivity_url=None, static_routes=None): |
11 | 683 | """Setup context manager and validate call signature. | 683 | """Setup context manager and validate call signature. |
12 | 684 | 684 | ||
13 | 685 | @param interface: Name of the network interface to bring up. | 685 | @param interface: Name of the network interface to bring up. |
14 | @@ -690,6 +690,7 @@ class EphemeralIPv4Network(object): | |||
15 | 690 | @param router: Optionally the default gateway IP. | 690 | @param router: Optionally the default gateway IP. |
16 | 691 | @param connectivity_url: Optionally, a URL to verify if a usable | 691 | @param connectivity_url: Optionally, a URL to verify if a usable |
17 | 692 | connection already exists. | 692 | connection already exists. |
18 | 693 | @param static_routes: Optionally a list of static routes from DHCP | ||
19 | 693 | """ | 694 | """ |
20 | 694 | if not all([interface, ip, prefix_or_mask, broadcast]): | 695 | if not all([interface, ip, prefix_or_mask, broadcast]): |
21 | 695 | raise ValueError( | 696 | raise ValueError( |
22 | @@ -706,6 +707,7 @@ class EphemeralIPv4Network(object): | |||
23 | 706 | self.ip = ip | 707 | self.ip = ip |
24 | 707 | self.broadcast = broadcast | 708 | self.broadcast = broadcast |
25 | 708 | self.router = router | 709 | self.router = router |
26 | 710 | self.static_routes = static_routes | ||
27 | 709 | self.cleanup_cmds = [] # List of commands to run to cleanup state. | 711 | self.cleanup_cmds = [] # List of commands to run to cleanup state. |
28 | 710 | 712 | ||
29 | 711 | def __enter__(self): | 713 | def __enter__(self): |
30 | @@ -718,7 +720,21 @@ class EphemeralIPv4Network(object): | |||
31 | 718 | return | 720 | return |
32 | 719 | 721 | ||
33 | 720 | self._bringup_device() | 722 | self._bringup_device() |
35 | 721 | if self.router: | 723 | |
36 | 724 | # rfc3442 requires us to ignore the router config *if* classless static | ||
37 | 725 | # routes are provided. | ||
38 | 726 | # | ||
39 | 727 | # https://tools.ietf.org/html/rfc3442 | ||
40 | 728 | # | ||
41 | 729 | # If the DHCP server returns both a Classless Static Routes option and | ||
42 | 730 | # a Router option, the DHCP client MUST ignore the Router option. | ||
43 | 731 | # | ||
44 | 732 | # Similarly, if the DHCP server returns both a Classless Static Routes | ||
45 | 733 | # option and a Static Routes option, the DHCP client MUST ignore the | ||
46 | 734 | # Static Routes option. | ||
47 | 735 | if self.static_routes: | ||
48 | 736 | self._bringup_static_routes() | ||
49 | 737 | elif self.router: | ||
50 | 722 | self._bringup_router() | 738 | self._bringup_router() |
51 | 723 | 739 | ||
52 | 724 | def __exit__(self, excp_type, excp_value, excp_traceback): | 740 | def __exit__(self, excp_type, excp_value, excp_traceback): |
53 | @@ -762,6 +778,20 @@ class EphemeralIPv4Network(object): | |||
54 | 762 | ['ip', '-family', 'inet', 'addr', 'del', cidr, 'dev', | 778 | ['ip', '-family', 'inet', 'addr', 'del', cidr, 'dev', |
55 | 763 | self.interface]) | 779 | self.interface]) |
56 | 764 | 780 | ||
57 | 781 | def _bringup_static_routes(self): | ||
58 | 782 | # static_routes = [("169.254.169.254/32", "130.56.248.255"), | ||
59 | 783 | # ("0.0.0.0/0", "130.56.240.1")] | ||
60 | 784 | for net_address, gateway in self.static_routes: | ||
61 | 785 | via_arg = [] | ||
62 | 786 | if gateway != "0.0.0.0/0": | ||
63 | 787 | via_arg = ['via', gateway] | ||
64 | 788 | util.subp( | ||
65 | 789 | ['ip', '-4', 'route', 'add', net_address] + via_arg + | ||
66 | 790 | ['dev', self.interface], capture=True) | ||
67 | 791 | self.cleanup_cmds.insert( | ||
68 | 792 | 0, ['ip', '-4', 'route', 'del', net_address] + via_arg + | ||
69 | 793 | ['dev', self.interface]) | ||
70 | 794 | |||
71 | 765 | def _bringup_router(self): | 795 | def _bringup_router(self): |
72 | 766 | """Perform the ip commands to fully setup the router if needed.""" | 796 | """Perform the ip commands to fully setup the router if needed.""" |
73 | 767 | # Check if a default route exists and exit if it does | 797 | # Check if a default route exists and exit if it does |
74 | diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py | |||
75 | index c98a97c..1737991 100644 | |||
76 | --- a/cloudinit/net/dhcp.py | |||
77 | +++ b/cloudinit/net/dhcp.py | |||
78 | @@ -92,10 +92,14 @@ class EphemeralDHCPv4(object): | |||
79 | 92 | nmap = {'interface': 'interface', 'ip': 'fixed-address', | 92 | nmap = {'interface': 'interface', 'ip': 'fixed-address', |
80 | 93 | 'prefix_or_mask': 'subnet-mask', | 93 | 'prefix_or_mask': 'subnet-mask', |
81 | 94 | 'broadcast': 'broadcast-address', | 94 | 'broadcast': 'broadcast-address', |
82 | 95 | 'static_routes': 'rfc3442-classless-static-routes', | ||
83 | 95 | 'router': 'routers'} | 96 | 'router': 'routers'} |
84 | 96 | kwargs = dict([(k, self.lease.get(v)) for k, v in nmap.items()]) | 97 | kwargs = dict([(k, self.lease.get(v)) for k, v in nmap.items()]) |
85 | 97 | if not kwargs['broadcast']: | 98 | if not kwargs['broadcast']: |
86 | 98 | kwargs['broadcast'] = bcip(kwargs['prefix_or_mask'], kwargs['ip']) | 99 | kwargs['broadcast'] = bcip(kwargs['prefix_or_mask'], kwargs['ip']) |
87 | 100 | if kwargs['static_routes']: | ||
88 | 101 | kwargs['static_routes'] = ( | ||
89 | 102 | parse_static_routes(kwargs['static_routes'])) | ||
90 | 99 | if self.connectivity_url: | 103 | if self.connectivity_url: |
91 | 100 | kwargs['connectivity_url'] = self.connectivity_url | 104 | kwargs['connectivity_url'] = self.connectivity_url |
92 | 101 | ephipv4 = EphemeralIPv4Network(**kwargs) | 105 | ephipv4 = EphemeralIPv4Network(**kwargs) |
93 | @@ -272,4 +276,90 @@ def networkd_get_option_from_leases(keyname, leases_d=None): | |||
94 | 272 | return data[keyname] | 276 | return data[keyname] |
95 | 273 | return None | 277 | return None |
96 | 274 | 278 | ||
97 | 279 | |||
98 | 280 | def parse_static_routes(rfc3442): | ||
99 | 281 | """ parse rfc3442 format and return a list containing tuple of strings. | ||
100 | 282 | |||
101 | 283 | The tuple is composed of the network_address (including net length) and | ||
102 | 284 | gateway for a parsed static route. | ||
103 | 285 | |||
104 | 286 | @param rfc3442: string in rfc3442 format | ||
105 | 287 | @returns: list of tuple(str, str) for all valid parsed routes until the | ||
106 | 288 | first parsing error. | ||
107 | 289 | |||
108 | 290 | E.g. | ||
109 | 291 | sr = parse_state_routes("32,169,254,169,254,130,56,248,255,0,130,56,240,1") | ||
110 | 292 | sr = [ | ||
111 | 293 | ("169.254.169.254/32", "130.56.248.255"), ("0.0.0.0/0", "130.56.240.1") | ||
112 | 294 | ] | ||
113 | 295 | |||
114 | 296 | Python version of isc-dhclient's hooks: | ||
115 | 297 | /etc/dhcp/dhclient-exit-hooks.d/rfc3442-classless-routes | ||
116 | 298 | """ | ||
117 | 299 | # raw strings from dhcp lease may end in semi-colon | ||
118 | 300 | rfc3442 = rfc3442.rstrip(";") | ||
119 | 301 | tokens = rfc3442.split(',') | ||
120 | 302 | static_routes = [] | ||
121 | 303 | |||
122 | 304 | def _trunc_error(cidr, required, remain): | ||
123 | 305 | msg = ("RFC3442 string malformed. Current route has CIDR of %s " | ||
124 | 306 | "and requires %s significant octets, but only %s remain. " | ||
125 | 307 | "Verify DHCP rfc3442-classless-static-routes value: %s" | ||
126 | 308 | % (cidr, required, remain, rfc3442)) | ||
127 | 309 | LOG.error(msg) | ||
128 | 310 | |||
129 | 311 | current_idx = 0 | ||
130 | 312 | for idx, tok in enumerate(tokens): | ||
131 | 313 | if idx < current_idx: | ||
132 | 314 | continue | ||
133 | 315 | net_length = int(tok) | ||
134 | 316 | if net_length in range(25, 33): | ||
135 | 317 | req_toks = 9 | ||
136 | 318 | if len(tokens[idx:]) < req_toks: | ||
137 | 319 | _trunc_error(net_length, req_toks, len(tokens[idx:])) | ||
138 | 320 | return static_routes | ||
139 | 321 | net_address = ".".join(tokens[idx+1:idx+5]) | ||
140 | 322 | gateway = ".".join(tokens[idx+5:idx+req_toks]) | ||
141 | 323 | current_idx = idx + req_toks | ||
142 | 324 | elif net_length in range(17, 25): | ||
143 | 325 | req_toks = 8 | ||
144 | 326 | if len(tokens[idx:]) < req_toks: | ||
145 | 327 | _trunc_error(net_length, req_toks, len(tokens[idx:])) | ||
146 | 328 | return static_routes | ||
147 | 329 | net_address = ".".join(tokens[idx+1:idx+4] + ["0"]) | ||
148 | 330 | gateway = ".".join(tokens[idx+4:idx+req_toks]) | ||
149 | 331 | current_idx = idx + req_toks | ||
150 | 332 | elif net_length in range(9, 17): | ||
151 | 333 | req_toks = 7 | ||
152 | 334 | if len(tokens[idx:]) < req_toks: | ||
153 | 335 | _trunc_error(net_length, req_toks, len(tokens[idx:])) | ||
154 | 336 | return static_routes | ||
155 | 337 | net_address = ".".join(tokens[idx+1:idx+3] + ["0", "0"]) | ||
156 | 338 | gateway = ".".join(tokens[idx+3:idx+req_toks]) | ||
157 | 339 | current_idx = idx + req_toks | ||
158 | 340 | elif net_length in range(1, 9): | ||
159 | 341 | req_toks = 6 | ||
160 | 342 | if len(tokens[idx:]) < req_toks: | ||
161 | 343 | _trunc_error(net_length, req_toks, len(tokens[idx:])) | ||
162 | 344 | return static_routes | ||
163 | 345 | net_address = ".".join(tokens[idx+1:idx+2] + ["0", "0", "0"]) | ||
164 | 346 | gateway = ".".join(tokens[idx+2:idx+req_toks]) | ||
165 | 347 | current_idx = idx + req_toks | ||
166 | 348 | elif net_length == 0: | ||
167 | 349 | req_toks = 5 | ||
168 | 350 | if len(tokens[idx:]) < req_toks: | ||
169 | 351 | _trunc_error(net_length, req_toks, len(tokens[idx:])) | ||
170 | 352 | return static_routes | ||
171 | 353 | net_address = "0.0.0.0" | ||
172 | 354 | gateway = ".".join(tokens[idx+1:idx+req_toks]) | ||
173 | 355 | current_idx = idx + req_toks | ||
174 | 356 | else: | ||
175 | 357 | LOG.error('Parsed invalid net length "%s". Verify DHCP ' | ||
176 | 358 | 'rfc3442-classless-static-routes value.', net_length) | ||
177 | 359 | return static_routes | ||
178 | 360 | |||
179 | 361 | static_routes.append(("%s/%s" % (net_address, net_length), gateway)) | ||
180 | 362 | |||
181 | 363 | return static_routes | ||
182 | 364 | |||
183 | 275 | # vi: ts=4 expandtab | 365 | # vi: ts=4 expandtab |
184 | diff --git a/cloudinit/net/tests/test_dhcp.py b/cloudinit/net/tests/test_dhcp.py | |||
185 | index 5139024..91f503c 100644 | |||
186 | --- a/cloudinit/net/tests/test_dhcp.py | |||
187 | +++ b/cloudinit/net/tests/test_dhcp.py | |||
188 | @@ -8,7 +8,8 @@ from textwrap import dedent | |||
189 | 8 | import cloudinit.net as net | 8 | import cloudinit.net as net |
190 | 9 | from cloudinit.net.dhcp import ( | 9 | from cloudinit.net.dhcp import ( |
191 | 10 | InvalidDHCPLeaseFileError, maybe_perform_dhcp_discovery, | 10 | InvalidDHCPLeaseFileError, maybe_perform_dhcp_discovery, |
193 | 11 | parse_dhcp_lease_file, dhcp_discovery, networkd_load_leases) | 11 | parse_dhcp_lease_file, dhcp_discovery, networkd_load_leases, |
194 | 12 | parse_static_routes) | ||
195 | 12 | from cloudinit.util import ensure_file, write_file | 13 | from cloudinit.util import ensure_file, write_file |
196 | 13 | from cloudinit.tests.helpers import ( | 14 | from cloudinit.tests.helpers import ( |
197 | 14 | CiTestCase, HttprettyTestCase, mock, populate_dir, wrap_and_call) | 15 | CiTestCase, HttprettyTestCase, mock, populate_dir, wrap_and_call) |
198 | @@ -64,6 +65,123 @@ class TestParseDHCPLeasesFile(CiTestCase): | |||
199 | 64 | self.assertItemsEqual(expected, parse_dhcp_lease_file(lease_file)) | 65 | self.assertItemsEqual(expected, parse_dhcp_lease_file(lease_file)) |
200 | 65 | 66 | ||
201 | 66 | 67 | ||
202 | 68 | class TestDHCPRFC3442(CiTestCase): | ||
203 | 69 | |||
204 | 70 | def test_parse_lease_finds_rfc3442_classless_static_routes(self): | ||
205 | 71 | """parse_dhcp_lease_file returns rfc3442-classless-static-routes.""" | ||
206 | 72 | lease_file = self.tmp_path('leases') | ||
207 | 73 | content = dedent(""" | ||
208 | 74 | lease { | ||
209 | 75 | interface "wlp3s0"; | ||
210 | 76 | fixed-address 192.168.2.74; | ||
211 | 77 | option subnet-mask 255.255.255.0; | ||
212 | 78 | option routers 192.168.2.1; | ||
213 | 79 | option rfc3442-classless-static-routes 0,130,56,240,1; | ||
214 | 80 | renew 4 2017/07/27 18:02:30; | ||
215 | 81 | expire 5 2017/07/28 07:08:15; | ||
216 | 82 | } | ||
217 | 83 | """) | ||
218 | 84 | expected = [ | ||
219 | 85 | {'interface': 'wlp3s0', 'fixed-address': '192.168.2.74', | ||
220 | 86 | 'subnet-mask': '255.255.255.0', 'routers': '192.168.2.1', | ||
221 | 87 | 'rfc3442-classless-static-routes': '0,130,56,240,1', | ||
222 | 88 | 'renew': '4 2017/07/27 18:02:30', | ||
223 | 89 | 'expire': '5 2017/07/28 07:08:15'}] | ||
224 | 90 | write_file(lease_file, content) | ||
225 | 91 | self.assertItemsEqual(expected, parse_dhcp_lease_file(lease_file)) | ||
226 | 92 | |||
227 | 93 | @mock.patch('cloudinit.net.dhcp.EphemeralIPv4Network') | ||
228 | 94 | @mock.patch('cloudinit.net.dhcp.maybe_perform_dhcp_discovery') | ||
229 | 95 | def test_obtain_lease_parses_static_routes(self, m_maybe, m_ipv4): | ||
230 | 96 | """EphemeralDHPCv4 parses rfc3442 routes for EphemeralIPv4Network""" | ||
231 | 97 | lease = [ | ||
232 | 98 | {'interface': 'wlp3s0', 'fixed-address': '192.168.2.74', | ||
233 | 99 | 'subnet-mask': '255.255.255.0', 'routers': '192.168.2.1', | ||
234 | 100 | 'rfc3442-classless-static-routes': '0,130,56,240,1', | ||
235 | 101 | 'renew': '4 2017/07/27 18:02:30', | ||
236 | 102 | 'expire': '5 2017/07/28 07:08:15'}] | ||
237 | 103 | m_maybe.return_value = lease | ||
238 | 104 | eph = net.dhcp.EphemeralDHCPv4() | ||
239 | 105 | eph.obtain_lease() | ||
240 | 106 | expected_kwargs = { | ||
241 | 107 | 'interface': 'wlp3s0', | ||
242 | 108 | 'ip': '192.168.2.74', | ||
243 | 109 | 'prefix_or_mask': '255.255.255.0', | ||
244 | 110 | 'broadcast': '192.168.2.255', | ||
245 | 111 | 'static_routes': [('0.0.0.0/0', '130.56.240.1')], | ||
246 | 112 | 'router': '192.168.2.1'} | ||
247 | 113 | m_ipv4.assert_called_with(**expected_kwargs) | ||
248 | 114 | |||
249 | 115 | |||
250 | 116 | class TestDHCPParseStaticRoutes(CiTestCase): | ||
251 | 117 | |||
252 | 118 | with_logs = True | ||
253 | 119 | |||
254 | 120 | def parse_static_routes_empty_string(self): | ||
255 | 121 | self.assertEqual([], parse_static_routes("")) | ||
256 | 122 | |||
257 | 123 | def test_parse_static_routes_invalid_input_returns_empty_list(self): | ||
258 | 124 | rfc3442 = "32,169,254,169,254,130,56,248" | ||
259 | 125 | self.assertEqual([], parse_static_routes(rfc3442)) | ||
260 | 126 | |||
261 | 127 | def test_parse_static_routes_bogus_width_returns_empty_list(self): | ||
262 | 128 | rfc3442 = "33,169,254,169,254,130,56,248" | ||
263 | 129 | self.assertEqual([], parse_static_routes(rfc3442)) | ||
264 | 130 | |||
265 | 131 | def test_parse_static_routes_single_ip(self): | ||
266 | 132 | rfc3442 = "32,169,254,169,254,130,56,248,255" | ||
267 | 133 | self.assertEqual([('169.254.169.254/32', '130.56.248.255')], | ||
268 | 134 | parse_static_routes(rfc3442)) | ||
269 | 135 | |||
270 | 136 | def test_parse_static_routes_single_ip_handles_trailing_semicolon(self): | ||
271 | 137 | rfc3442 = "32,169,254,169,254,130,56,248,255;" | ||
272 | 138 | self.assertEqual([('169.254.169.254/32', '130.56.248.255')], | ||
273 | 139 | parse_static_routes(rfc3442)) | ||
274 | 140 | |||
275 | 141 | def test_parse_static_routes_default_route(self): | ||
276 | 142 | rfc3442 = "0,130,56,240,1" | ||
277 | 143 | self.assertEqual([('0.0.0.0/0', '130.56.240.1')], | ||
278 | 144 | parse_static_routes(rfc3442)) | ||
279 | 145 | |||
280 | 146 | def test_parse_static_routes_class_c_b_a(self): | ||
281 | 147 | class_c = "24,192,168,74,192,168,0,4" | ||
282 | 148 | class_b = "16,172,16,172,16,0,4" | ||
283 | 149 | class_a = "8,10,10,0,0,4" | ||
284 | 150 | rfc3442 = ",".join([class_c, class_b, class_a]) | ||
285 | 151 | self.assertEqual(sorted([ | ||
286 | 152 | ("192.168.74.0/24", "192.168.0.4"), | ||
287 | 153 | ("172.16.0.0/16", "172.16.0.4"), | ||
288 | 154 | ("10.0.0.0/8", "10.0.0.4") | ||
289 | 155 | ]), sorted(parse_static_routes(rfc3442))) | ||
290 | 156 | |||
291 | 157 | def test_parse_static_routes_logs_error_truncated(self): | ||
292 | 158 | bad_rfc3442 = { | ||
293 | 159 | "class_c": "24,169,254,169,10", | ||
294 | 160 | "class_b": "16,172,16,10", | ||
295 | 161 | "class_a": "8,10,10", | ||
296 | 162 | "gateway": "0,0", | ||
297 | 163 | "netlen": "33,0", | ||
298 | 164 | } | ||
299 | 165 | for rfc3442 in bad_rfc3442.values(): | ||
300 | 166 | self.assertEqual([], parse_static_routes(rfc3442)) | ||
301 | 167 | |||
302 | 168 | logs = self.logs.getvalue() | ||
303 | 169 | self.assertEqual(len(bad_rfc3442.keys()), len(logs.splitlines())) | ||
304 | 170 | |||
305 | 171 | def test_parse_static_routes_returns_valid_routes_until_parse_err(self): | ||
306 | 172 | class_c = "24,192,168,74,192,168,0,4" | ||
307 | 173 | class_b = "16,172,16,172,16,0,4" | ||
308 | 174 | class_a_error = "8,10,10,0,0" | ||
309 | 175 | rfc3442 = ",".join([class_c, class_b, class_a_error]) | ||
310 | 176 | self.assertEqual(sorted([ | ||
311 | 177 | ("192.168.74.0/24", "192.168.0.4"), | ||
312 | 178 | ("172.16.0.0/16", "172.16.0.4"), | ||
313 | 179 | ]), sorted(parse_static_routes(rfc3442))) | ||
314 | 180 | |||
315 | 181 | logs = self.logs.getvalue() | ||
316 | 182 | self.assertIn(rfc3442, logs.splitlines()[0]) | ||
317 | 183 | |||
318 | 184 | |||
319 | 67 | class TestDHCPDiscoveryClean(CiTestCase): | 185 | class TestDHCPDiscoveryClean(CiTestCase): |
320 | 68 | with_logs = True | 186 | with_logs = True |
321 | 69 | 187 | ||
322 | diff --git a/cloudinit/net/tests/test_init.py b/cloudinit/net/tests/test_init.py | |||
323 | index 6d2affe..d393e6a 100644 | |||
324 | --- a/cloudinit/net/tests/test_init.py | |||
325 | +++ b/cloudinit/net/tests/test_init.py | |||
326 | @@ -549,6 +549,45 @@ class TestEphemeralIPV4Network(CiTestCase): | |||
327 | 549 | self.assertEqual(expected_setup_calls, m_subp.call_args_list) | 549 | self.assertEqual(expected_setup_calls, m_subp.call_args_list) |
328 | 550 | m_subp.assert_has_calls(expected_teardown_calls) | 550 | m_subp.assert_has_calls(expected_teardown_calls) |
329 | 551 | 551 | ||
330 | 552 | def test_ephemeral_ipv4_network_with_rfc3442_static_routes(self, m_subp): | ||
331 | 553 | params = { | ||
332 | 554 | 'interface': 'eth0', 'ip': '192.168.2.2', | ||
333 | 555 | 'prefix_or_mask': '255.255.255.0', 'broadcast': '192.168.2.255', | ||
334 | 556 | 'static_routes': [('169.254.169.254/32', '192.168.2.1'), | ||
335 | 557 | ('0.0.0.0/0', '192.168.2.1')], | ||
336 | 558 | 'router': '192.168.2.1'} | ||
337 | 559 | expected_setup_calls = [ | ||
338 | 560 | mock.call( | ||
339 | 561 | ['ip', '-family', 'inet', 'addr', 'add', '192.168.2.2/24', | ||
340 | 562 | 'broadcast', '192.168.2.255', 'dev', 'eth0'], | ||
341 | 563 | capture=True, update_env={'LANG': 'C'}), | ||
342 | 564 | mock.call( | ||
343 | 565 | ['ip', '-family', 'inet', 'link', 'set', 'dev', 'eth0', 'up'], | ||
344 | 566 | capture=True), | ||
345 | 567 | mock.call( | ||
346 | 568 | ['ip', '-4', 'route', 'add', '169.254.169.254/32', | ||
347 | 569 | 'via', '192.168.2.1', 'dev', 'eth0'], capture=True), | ||
348 | 570 | mock.call( | ||
349 | 571 | ['ip', '-4', 'route', 'add', '0.0.0.0/0', | ||
350 | 572 | 'via', '192.168.2.1', 'dev', 'eth0'], capture=True)] | ||
351 | 573 | expected_teardown_calls = [ | ||
352 | 574 | mock.call( | ||
353 | 575 | ['ip', '-4', 'route', 'del', '0.0.0.0/0', | ||
354 | 576 | 'via', '192.168.2.1', 'dev', 'eth0'], capture=True), | ||
355 | 577 | mock.call( | ||
356 | 578 | ['ip', '-4', 'route', 'del', '169.254.169.254/32', | ||
357 | 579 | 'via', '192.168.2.1', 'dev', 'eth0'], capture=True), | ||
358 | 580 | mock.call( | ||
359 | 581 | ['ip', '-family', 'inet', 'link', 'set', 'dev', | ||
360 | 582 | 'eth0', 'down'], capture=True), | ||
361 | 583 | mock.call( | ||
362 | 584 | ['ip', '-family', 'inet', 'addr', 'del', | ||
363 | 585 | '192.168.2.2/24', 'dev', 'eth0'], capture=True) | ||
364 | 586 | ] | ||
365 | 587 | with net.EphemeralIPv4Network(**params): | ||
366 | 588 | self.assertEqual(expected_setup_calls, m_subp.call_args_list) | ||
367 | 589 | m_subp.assert_has_calls(expected_setup_calls + expected_teardown_calls) | ||
368 | 590 | |||
369 | 552 | 591 | ||
370 | 553 | class TestApplyNetworkCfgNames(CiTestCase): | 592 | class TestApplyNetworkCfgNames(CiTestCase): |
371 | 554 | V1_CONFIG = textwrap.dedent("""\ | 593 | V1_CONFIG = textwrap.dedent("""\ |
372 | diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py | |||
373 | index f27ef21..2de2aea 100644 | |||
374 | --- a/tests/unittests/test_datasource/test_azure.py | |||
375 | +++ b/tests/unittests/test_datasource/test_azure.py | |||
376 | @@ -1807,7 +1807,8 @@ class TestAzureDataSourcePreprovisioning(CiTestCase): | |||
377 | 1807 | self.assertEqual(m_dhcp.call_count, 2) | 1807 | self.assertEqual(m_dhcp.call_count, 2) |
378 | 1808 | m_net.assert_any_call( | 1808 | m_net.assert_any_call( |
379 | 1809 | broadcast='192.168.2.255', interface='eth9', ip='192.168.2.9', | 1809 | broadcast='192.168.2.255', interface='eth9', ip='192.168.2.9', |
381 | 1810 | prefix_or_mask='255.255.255.0', router='192.168.2.1') | 1810 | prefix_or_mask='255.255.255.0', router='192.168.2.1', |
382 | 1811 | static_routes=None) | ||
383 | 1811 | self.assertEqual(m_net.call_count, 2) | 1812 | self.assertEqual(m_net.call_count, 2) |
384 | 1812 | 1813 | ||
385 | 1813 | def test__reprovision_calls__poll_imds(self, fake_resp, | 1814 | def test__reprovision_calls__poll_imds(self, fake_resp, |
386 | @@ -1845,7 +1846,8 @@ class TestAzureDataSourcePreprovisioning(CiTestCase): | |||
387 | 1845 | self.assertEqual(m_dhcp.call_count, 2) | 1846 | self.assertEqual(m_dhcp.call_count, 2) |
388 | 1846 | m_net.assert_any_call( | 1847 | m_net.assert_any_call( |
389 | 1847 | broadcast='192.168.2.255', interface='eth9', ip='192.168.2.9', | 1848 | broadcast='192.168.2.255', interface='eth9', ip='192.168.2.9', |
391 | 1848 | prefix_or_mask='255.255.255.0', router='192.168.2.1') | 1849 | prefix_or_mask='255.255.255.0', router='192.168.2.1', |
392 | 1850 | static_routes=None) | ||
393 | 1849 | self.assertEqual(m_net.call_count, 2) | 1851 | self.assertEqual(m_net.call_count, 2) |
394 | 1850 | 1852 | ||
395 | 1851 | 1853 | ||
396 | diff --git a/tests/unittests/test_datasource/test_ec2.py b/tests/unittests/test_datasource/test_ec2.py | |||
397 | index 20d59bf..1ec8e00 100644 | |||
398 | --- a/tests/unittests/test_datasource/test_ec2.py | |||
399 | +++ b/tests/unittests/test_datasource/test_ec2.py | |||
400 | @@ -538,7 +538,8 @@ class TestEc2(test_helpers.HttprettyTestCase): | |||
401 | 538 | m_dhcp.assert_called_once_with('eth9') | 538 | m_dhcp.assert_called_once_with('eth9') |
402 | 539 | m_net.assert_called_once_with( | 539 | m_net.assert_called_once_with( |
403 | 540 | broadcast='192.168.2.255', interface='eth9', ip='192.168.2.9', | 540 | broadcast='192.168.2.255', interface='eth9', ip='192.168.2.9', |
405 | 541 | prefix_or_mask='255.255.255.0', router='192.168.2.1') | 541 | prefix_or_mask='255.255.255.0', router='192.168.2.1', |
406 | 542 | static_routes=None) | ||
407 | 542 | self.assertIn('Crawl of metadata service took', self.logs.getvalue()) | 543 | self.assertIn('Crawl of metadata service took', self.logs.getvalue()) |
408 | 543 | 544 | ||
409 | 544 | 545 |
Wow, what an annoying format to have to parse. Overall, I think this looks good; I have a couple of inline comments. I'd also like to see us expand the unit testing of the parsing code so that we're at least covering every branch in there (and ideally doing at least one test with a bunch combined, to make sure that our skip lengths are accurate).