Merge ~akaris/cloud-init:bug1684349 into cloud-init:master

Proposed by Andreas Karis on 2017-04-22
Status: Merged
Merged at revision: 16a7302f6acb69adb0aee75eaf12392fa3688853
Proposed branch: ~akaris/cloud-init:bug1684349
Merge into: cloud-init:master
Diff against target: 16 lines (+2/-2)
1 file modified
cloudinit/net/network_state.py (+2/-2)
Reviewer Review Type Date Requested Status
Chad Smith Needs Fixing on 2017-05-24
Server Team CI bot continuous-integration Approve on 2017-04-24
cloud-init commiters 2017-04-22 Pending
Review via email: mp+322992@code.launchpad.net

Commit message

Fix mask2cidr error with integer value

mask2cidr error with integer value - argument of type 'int' is not iterable. Method mask2cidr is not type safe. It tries to take into account that this can be a prefix (so it does not contain ':' nor '.') and then simly return mask without action. The problem is that if mask is an integer, then this throws an error, so this patch casts mask to type string in the if/elif.

LP: #1684349

To post a comment you must log in.
Lars Kellogg-Stedman (larsks) wrote :

If code is actually calling this with an integer argument, this looks like a fine temporary fix.

I say "temporary" because I'm a little confused by the return value of this function, too, which looks like it may be an int (when it calls ipv4mask2cidr) or a string (when it calls ipv6mask2cidr) or whatever "mask" happens to me if either of the previous conditionals fail. I will try to ping smoser about it tomorrow.

Dimitri John Ledkov (xnox) wrote :

It is called with whatever the config source provided.

It may be called with cidr, it could be an ipv6 netmask, or an ipv4 netmask.

Always return cidr.

I guess it would be better named any2cidr()

Chad Smith (chad.smith) wrote :

Generally, if we are changing functionality, I'd love to see a unit test exercising this functionality so we don't have to test it with integration tests.

It appears that this changeset will only pass this type problem down into ipv4mask2cidr and ipv6mask2cidr which will fall over with the same traceback as they attempt the same string search in mask(which is still an int).

Scott mentioned in IRC today that he's working a branch cleaning up network_state so I'm wondering if we just wait on the cleanup there to see if it addresses this issue.

review: Needs Fixing
Scott Moser (smoser) wrote :

I've marked this 'merged'
the change was present at
 https://code.launchpad.net/~xnox/cloud-init/+git/cloud-init/+merge/324010

i have a lot of further improvements to that general area, fixing Andreas's issues with the type safety of that function. Theres more to do, but you can see at https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
2index db3c357..9e9c05a 100644
3--- a/cloudinit/net/network_state.py
4+++ b/cloudinit/net/network_state.py
5@@ -734,9 +734,9 @@ def ipv6mask2cidr(mask):
6
7
8 def mask2cidr(mask):
9- if ':' in mask:
10+ if ':' in str(mask):
11 return ipv6mask2cidr(mask)
12- elif '.' in mask:
13+ elif '.' in str(mask):
14 return ipv4mask2cidr(mask)
15 else:
16 return mask

Subscribers

People subscribed via source and target branches