Conversation
Changed network definition with more fitting defaults Compined ipv4_subnet and ipv4 into ipv4_cidr to be more consistent with other uses of ipv4 updated test.py and range.py for new network setup renamed network.rm() to be consistent with network.destroy_by_name()
sn0ja
left a comment
sn0ja
left a comment
There was a problem hiding this comment.
Thank you for your contribution; changing the API to simplify handling of IPv4 addresses is a great idea. Also thank you for going through the hassle of writing comments and type hints.
There was a problem hiding this comment.
I think it would be good to also change how ipv4 is used with Interfaces so the api is consistent in that matter. So that Interfaces work with the cidr representation as well instead of prefix_length and ipv4
There was a problem hiding this comment.
It would probably be wise also do this for ipv6 in general as well right?
There was a problem hiding this comment.
yeah but ipv6 is not even fully implemented yet, i open an issue for that #2
cave/src/networks/network/network.py
Outdated
| import ipaddress | ||
| import jinja2 | ||
| import re | ||
| import socket | ||
| import struct |
There was a problem hiding this comment.
Do we actually need socket and struct for the IP parsing, maybe ipaddress offers the functionality you need?
added ipv6 cidr changed ipv4 splitting logic cleanup
sn0ja
left a comment
sn0ja
left a comment
There was a problem hiding this comment.
Some small change requests.
Thank you for your support :) 🐈
| self.ipv6_prefix = ipv6_tuple[1] | ||
| else: | ||
| self.ipv6 = "" | ||
| self.ipv6_subnet = "" |
There was a problem hiding this comment.
Did you mean self.ipv6_prefix = "" instead of self.ipv6_subnet = "" ?
There was a problem hiding this comment.
Other components of cave (e.g. this line) use the ipv4 and prefix_length properties, so we cannot remove them.
Maybe change Interface to a normal class and do the calculation of ipv4 and prefix_length from ipv4_cidr in the constructor like done in network.py?
There was a problem hiding this comment.
We could then also include sanity checks in the constructor e.g. assert "/" in ipv4_cidr, this is not a clean solution but we are just building a prototype here for now.
There was a problem hiding this comment.
Would it make sense to introduce a utility module to ease the introduction of functionalities, which are reused throughout cave? This would also ease the introduction of argument validation etc. in the future.
There was a problem hiding this comment.
I think for now it is not really necessary
| network=mngt, | ||
| ipv4="10.10.0.3", | ||
| prefix_length=24, | ||
| ipv4_cidr="10.10.0.3", |
2 participants
Changed network definition with more fitting defaults
Combined ipv4_subnet and ipv4 into ipv4_cidr to be more consistent with other uses of ipv4
updated test.py and range.py for new network setup
renamed network.rm() to be consistent with network.destroy_by_name()