[PATCH] Addition of custom mac address field for interfaces v3
From: Brent Baude <bbaude@redhat.com> Fixed json issue and included comments from pvital --- docs/API.md | 1 + src/kimchi/API.json | 6 ++++++ src/kimchi/control/vms.py | 2 +- src/kimchi/exception.py | 1 - src/kimchi/i18n.py | 4 ++++ src/kimchi/model/vmifaces.py | 38 +++++++++++++++++++++++++++++-------- tests/test_model.py | 12 ++++++++++++ ui/css/theme-default/guest-edit.css | 4 ++-- ui/js/src/kimchi.guest_edit_main.js | 18 ++++++++++++------ ui/pages/guest-edit.html.tmpl | 11 ++++++++--- 10 files changed, 76 insertions(+), 21 deletions(-) diff --git a/docs/API.md b/docs/API.md index b679ce7..4f92eaa 100644 --- a/docs/API.md +++ b/docs/API.md @@ -221,6 +221,7 @@ Represents all network interfaces attached to a Virtual Machine. interface type is network. * type: The type of VM network interface that libvirt supports. Now kimchi just supports 'network' type. + * custommac: A custom MAC address for the interface ### Sub-Resource: Virtual Machine Network Interface diff --git a/src/kimchi/API.json b/src/kimchi/API.json index b8604d2..e46aedc 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -334,6 +334,12 @@ "type": "string", "pattern": "^ne2k_pci|i82551|i82557b|i82559er|rtl8139|e1000|pcnet|virtio$", "error": "KCHVMIF0006E" + }, + "custommac": { + "description": "optional custom mac address for the network interface", + "type": "string", + "pattern": "^(?i)([0-9A-F]{2}[:-]){5}([0-9A-F]{2})$", + "error": "KCHVMIF0009E" } } }, diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py index 88d8a81..627f1cc 100644 --- a/src/kimchi/control/vms.py +++ b/src/kimchi/control/vms.py @@ -36,7 +36,7 @@ class VM(Resource): super(VM, self).__init__(model, ident) self.role_key = 'guests' self.update_params = ["name", "users", "groups", "cpus", "memory", - "graphics"] + "graphics", "custommac"] self.screenshot = VMScreenShot(model, ident) self.uri_fmt = '/vms/%s' for ident, node in sub_nodes.items(): diff --git a/src/kimchi/exception.py b/src/kimchi/exception.py index 039152a..7ab1878 100644 --- a/src/kimchi/exception.py +++ b/src/kimchi/exception.py @@ -46,7 +46,6 @@ class KimchiException(Exception): msg = self._get_translation() else: msg = _messages.get(code, code) - msg = unicode(msg, 'utf-8') % args pattern = "%s: %s" % (code, msg) Exception.__init__(self, pattern) diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 9e66c68..8690657 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -105,6 +105,10 @@ messages = { "KCHVMIF0006E": _("Invalid network model card specified for virtual machine interface"), "KCHVMIF0007E": _("Specify type and network to add a new virtual machine interface"), "KCHVMIF0008E": _("Specify type and network to update a virtual machine interface"), + "KCHVMIF0009E": _("Invalid MAC address format. Expected a 12 character hexadecimal value separated by colons(:)"), + "KCHVMIF0010E": _("The custom MAC address %(value) is already in use on this guest"), + "KCHVMIF0011E": _("Libvirt requires a unicast MAC address which %(value) is not."), + "KCHTMPL0001E": _("Template %(name)s already exists"), "KCHTMPL0002E": _("Template %(name)s does not exist"), diff --git a/src/kimchi/model/vmifaces.py b/src/kimchi/model/vmifaces.py index 66b3827..ef67d34 100644 --- a/src/kimchi/model/vmifaces.py +++ b/src/kimchi/model/vmifaces.py @@ -18,6 +18,7 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA import random +import re import libvirt from lxml import etree, objectify @@ -45,6 +46,20 @@ class VMIfacesModel(object): random.randint(0x00, 0xff)] return ':'.join(map(lambda x: "%02x" % x, mac)) + def checkduplicatemacs(vm, custommac): + macs = self.get_list(vm) + if custommac in macs: + raise InvalidParameter("KCHVMIF0010E", {'custommac': mac}) + + def validMac(mymac): + pattern = r'^(?i)([0-9A-F]{2}[:-]){5}([0-9A-F]{2})$' + if bool(re.match(pattern, mac)) is not True: + raise InvalidParameter("KCHVMIF0009E", {'custommac': mac}) + else: + # Checking for multicast + if (int(mymac.split(":")[0], 16) & 0x1) == 1: + raise InvalidParameter("KCHVMIF0011E", {'custommac': mac}) + conn = self.conn.get() networks = conn.listNetworks() + conn.listDefinedNetworks() @@ -56,14 +71,22 @@ class VMIfacesModel(object): if DOM_STATE_MAP[dom.info()[0]] != "shutoff": raise InvalidOperation("KCHVMIF0003E") - macs = (iface.mac.get('address') - for iface in self.get_vmifaces(vm, self.conn)) - - mac = randomMAC() - while True: - if mac not in macs: - break + if 'custommac' not in params: + macs = (iface.mac.get('address') + for iface in self.get_vmifaces(vm, self.conn)) mac = randomMAC() + while True: + if mac not in macs: + break + mac = randomMAC() + + else: + if len(params['custommac']) == 0: + mac = randomMAC() + else: + mac = params["custommac"] + validMac(mac) + checkduplicatemacs(vm, mac) children = [E.mac(address=mac)] ("network" in params.keys() and @@ -75,7 +98,6 @@ class VMIfacesModel(object): xml = etree.tostring(E.interface(*children, **attrib)) dom.attachDeviceFlags(xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT) - return mac @staticmethod diff --git a/tests/test_model.py b/tests/test_model.py index ceedc6f..59c3e50 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -219,6 +219,18 @@ class ModelTests(unittest.TestCase): self.assertEquals("default", iface['network']) self.assertEquals("e1000", iface["model"]) + # custom MAC address test + iface_args = {"type": "network", + "network": "test-network", + "model": "virtio", + "custommac": "52:54:00:17:cd:e7"} + mac = inst.vmifaces_create('kimchi-ifaces', iface_args) + iface = inst.vmiface_lookup('kimchi-ifaces', mac) + self.assertEquals("52:54:00:17:cd:e7", iface['mac']) + self.assertRaises(InvalidParameter, inst.vmifaces_create, + 'kimchi-ifaces', iface_args) + rollback.prependDefer(inst.vmiface_delete, 'kimchi-ifaces', mac) + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_vm_disk(self): disk_path = '/tmp/existent2.iso' diff --git a/ui/css/theme-default/guest-edit.css b/ui/css/theme-default/guest-edit.css index 74c2237..f0e1bb0 100644 --- a/ui/css/theme-default/guest-edit.css +++ b/ui/css/theme-default/guest-edit.css @@ -112,7 +112,7 @@ #form-guest-edit-storage .cell, .guest-edit-interface .cell { display: inline-block; - width: 250px; + width: 150px; } #form-guest-edit-storage .cell.dev { @@ -130,7 +130,7 @@ } .guest-edit-interface .body select { - width: 180px; + width: 150px; padding: 0px; } diff --git a/ui/js/src/kimchi.guest_edit_main.js b/ui/js/src/kimchi.guest_edit_main.js index c281289..7baf296 100644 --- a/ui/js/src/kimchi.guest_edit_main.js +++ b/ui/js/src/kimchi.guest_edit_main.js @@ -176,7 +176,10 @@ kimchi.guest_edit_main = function() { var toggleEdit = function(item, on){ $("label", item).toggleClass("hide", on); $("select", item).toggleClass("hide", !on); - $(".action-area", item).toggleClass("hide"); + $("input", item).toggleClass("hide", !on); + $(".action-area#editmode", item).toggleClass("hide", !on); + $(".action-area#viewmode", item).toggleClass("hide", on); + }; var addItem = function(data) { var itemNode = $.parseHTML(kimchi.substitute($('#interface-tmpl').html(),data)); @@ -215,20 +218,23 @@ kimchi.guest_edit_main = function() { var item = $(this).parent().parent(); var interface = { network: $("select", item).val(), - type: "network" + type: "network", + custommac: $("input",item).val().toLowerCase() }; - var postUpdate = function(){ + var postUpdate = function(mymac){ $("label", item).text(interface.network); - toggleEdit(item, false); + $("label#custommac", item).text(mymac); + toggleEdit(item.prop, false); }; if(item.prop("id")==""){ kimchi.createGuestInterface(kimchi.selectedGuest, interface, function(data){ item.prop("id", data.mac); - postUpdate(); + $("label#custommac", item).text(data.mac) + postUpdate(data.mac); }); }else{ kimchi.updateGuestInterface(kimchi.selectedGuest, item.prop("id"), interface, function(){ - postUpdate(); + postUpdate(data.mac); }); } }); diff --git a/ui/pages/guest-edit.html.tmpl b/ui/pages/guest-edit.html.tmpl index 0b7dad3..0aa9f3b 100644 --- a/ui/pages/guest-edit.html.tmpl +++ b/ui/pages/guest-edit.html.tmpl @@ -108,7 +108,8 @@ <div class="header"> <span class="cell">$_("Network")</span> <span class="cell">$_("Type")</span> - <button class="add action-area"></button> + <span class="cell">$_("MAC")</span> + <button id="interfaceadd" class="add action-area"></button> </div> <div class="body"></div> </form> @@ -192,10 +193,14 @@ <span class="cell"> <span>{type}</span> </span> - <span class="action-area {editMode}"> + <span class="cell"> + <label class="{viewMode}" id="custommac">{mac}</label> + <input type="text" id="insertmac" class="{editMode}"/> + </span> + <span class="action-area {editMode}" id="editmode"> <button class="save"></button><button class="cancel"></button> </span> - <span class="action-area {viewMode}"> + <span class="action-area {viewMode}" id="viewmode"> <button class="edit"></button><button class="delete"></button> </span> <div> -- 1.9.3
Hi Brent, We have a set of commands to check whether the code has the right syntax according to our standards (e.g. Python PEP8, etc.). You can run it by typing "make check-local". I tried it after applying your patch and I got the following error: bad i18n string formatting: KCHVMIF0010E: The custom MAC address %(value) is already in use on this guest You need to provide a formatting mask (e.g. "%(value)s") when using a variable inside an internationalized string. I also launched Kimchi after applying this patch and I tried to add a few new NICs with custom MAC addresses. I tried "52:54:00:41:65:a1" and it worked fine. However, when I tried "ab:cd:ef:ab:cd:ef", an unexpected exception was raised: [01/Oct/2014:16:14:47] HTTP Traceback (most recent call last): File "/usr/lib/python2.7/site-packages/cherrypy/_cprequest.py", line 656, in respond response.body = self.handler() File "/usr/lib/python2.7/site-packages/cherrypy/lib/encoding.py", line 188, in __call__ self.body = self.oldhandler(*args, **kwargs) File "/usr/lib/python2.7/site-packages/cherrypy/_cpdispatch.py", line 34, in __call__ return self.callable(*self.args, **self.kwargs) File "/home/vianac/LTC/kimchi/src/kimchi/control/base.py", line 310, in index return self.create(parse_request(), *args) File "/home/vianac/LTC/kimchi/src/kimchi/control/base.py", line 242, in create name = create(*args) File "/home/vianac/LTC/kimchi/src/kimchi/model/vmifaces.py", line 88, in create validMac(mac) File "/home/vianac/LTC/kimchi/src/kimchi/model/vmifaces.py", line 61, in validMac raise InvalidParameter("KCHVMIF0011E", {'custommac': mac}) File "/home/vianac/LTC/kimchi/src/kimchi/exception.py", line 49, in __init__ msg = unicode(msg, 'utf-8') % args KeyError: u'value' The exception above is also raised when we run our test suite (i.e. by typing "sudo make check"). And I have one last comment about the code, which is below: On Qua, 2014-10-01 at 10:03 -0500, bbaude@redhat.com wrote:
+ mac = inst.vmifaces_create('kimchi-ifaces', iface_args) + iface = inst.vmiface_lookup('kimchi-ifaces', mac) + self.assertEquals("52:54:00:17:cd:e7", iface['mac']) + self.assertRaises(InvalidParameter, inst.vmifaces_create, + 'kimchi-ifaces', iface_args) + rollback.prependDefer(inst.vmiface_delete, 'kimchi-ifaces', mac)
It's a good practice to add a delete rollback action as soon as the referred object is created. That means that the call to "prependDefer" would be better placed right after "vmifaces_create". Using the code above, however, if the call to "vmiface_lookup" somehow fails, the network interface won't be removed by the transaction.
Oh, and I forgot to add another thing. With this patch, the MAC address becomes required when the user adds a new NIC. If I don't type an address, I get the following error: Invalid MAC address format. Expected a 12 character hexadecimal value separated by colons(:) I think that's a bad thing because most of the times the user doesn't care about which MAC address is assigned to their NIC. So IMO if the user leaves that field blank, a random MAC should be generated, just like before. But if they type something, that must be validated indeed. I see that there's code dealing exactly with that, but it's not working with me. On Qua, 2014-10-01 at 10:03 -0500, bbaude@redhat.com wrote:
From: Brent Baude <bbaude@redhat.com>
Fixed json issue and included comments from pvital
I think that is because Aline and Royce wanted me to regex check the incoming MAC address with JSON. This is a direct result of that. I'll have to see if I can make it be blank which was allowed prior to that. On Wed, 2014-10-01 at 16:59 -0300, Crístian Viana wrote:
Oh, and I forgot to add another thing. With this patch, the MAC address becomes required when the user adds a new NIC. If I don't type an address, I get the following error:
Invalid MAC address format. Expected a 12 character hexadecimal value separated by colons(:)
I think that's a bad thing because most of the times the user doesn't care about which MAC address is assigned to their NIC. So IMO if the user leaves that field blank, a random MAC should be generated, just like before. But if they type something, that must be validated indeed.
I see that there's code dealing exactly with that, but it's not working with me.
On Qua, 2014-10-01 at 10:03 -0500, bbaude@redhat.com wrote:
From: Brent Baude <bbaude@redhat.com>
Fixed json issue and included comments from pvital
participants (3)
-
bbaude＠redhat.com -
Brent Baude
-
Crístian Viana