Change in vdsm[master]: Added private vlan before_device_create/before_nic_hotplug hook

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Added private vlan before_device_create/before_nic_hotplug hook
......................................................................

Patch Set 1: Code-Review-1

(12 comments)

Thanks Michael for your contribution. Would please respond to my comments? I believe that this patch requires some more work.

http://gerrit.ovirt.org/#/c/24195/1//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2014-02-07 21:48:20 +1100
Line 4: Commit:     Michael Samuel 
Line 5: CommitDate: 2014-02-07 21:48:20 +1100
Line 6: 
Line 7: Added private vlan before_device_create/before_nic_hotplug hook
It might seems repetitious, but please include here a short introduction to your hook, telling what it does and where it can become useful.
Line 8: 
Line 9: Change-Id: Idda6f193c0095241bc1540a0241d49426c000fb3

http://gerrit.ovirt.org/#/c/24195/1/vdsm.spec.in
File vdsm.spec.in:

Line 499: %description hook-pincpu
Line 500: pincpu is hook for VDSM.
Line 501: pincpu enable to pin virtual machine to a specific CPUs.
Line 502: 
Line 503: %package hook-privatevlan
similar changes should be done to debian (Cf. http://gerrit.ovirt.org/22124 )
Line 504: Summary:        Hook to only allow traffic to/from the gateway
Line 505: BuildArch:      noarch
Line 506: 
Line 507: %description hook-privatevlan

http://gerrit.ovirt.org/#/c/24195/1/vdsm_hooks/privatevlan/README
File vdsm_hooks/privatevlan/README:

Line 1: privatevlan vdsm hook (based off isolatedprivatevlan)
You hook is quite different (being a per-device hook). It would be better to credit isolatedprivatevlan in the commit message, and here.
Line 2: =====================================================
Line 3: limit VM traffic to a specific gateway by its mac address,
Line 4: hook prevents VM from spoofing its mac or ip address
Line 5: by using <filterref filter='clean-traffic'/> libvirt filter

Line 14: </interface>
Line 15: 
Line 16: syntax:
Line 17: privatevlan_gw_macs=aa:bb:cc:dd:ee:ff,ff:aa:bb:cc:dd:ee
Line 18:   The MAC address of the gateway(s), this should include
address->addresses
Line 19:   virtual MAC addresses for HSRP/VRRP if necessary.
Line 20: 
Line 21: privatevlan_guest_ip=1.2.3.4,2.3.4.5
Line 22:   The IP address/addresses of guests. If not specified,

Line 33:   will be allowed at all.
Line 34:   Note that if you don't specify the IP, the filter doesn't apply at all
Line 35:   until you send some traffic.  This is a libvirt limitation, so secure
Line 36:   installations should consider specifying a static IP.
Line 37:   This URL documents all the caveats, of which there are many, especially
This -> the following
Line 38:   with regards to migration!
Line 39:   http://libvirt.org/formatnwfilter.html#nwfelemsRulesAdvIPAddrDetection
Line 40: 
Line 41: Generally speaking, privatevlan_gw_macs and privatevlan_guest_learn would

Line 39:   http://libvirt.org/formatnwfilter.html#nwfelemsRulesAdvIPAddrDetection
Line 40: 
Line 41: Generally speaking, privatevlan_gw_macs and privatevlan_guest_learn would
Line 42: be supplied in the VM interface profile, while privatevlan_guest_ip needs
Line 43: to be specified per-guest.
why? I'd suppose that a gues with multiple interfaces would generally have different IP addresses for each interface (with the exception of in-guest bonds).
Line 44: 
Line 45: NOTE:
Line 46: the filter will be applied only after restart of libvirtd on the host system.
Line 47: # initctl restart libvirtd

Line 43: to be specified per-guest.
Line 44: 
Line 45: NOTE:
Line 46: the filter will be applied only after restart of libvirtd on the host system.
Line 47: # initctl restart libvirtd
on el6. on Fedora, use `systemctl restart libvirtd`
Line 48: 
Line 49: to see libvirt filters use:

http://gerrit.ovirt.org/#/c/24195/1/vdsm_hooks/privatevlan/before_device_cre...
File vdsm_hooks/privatevlan/before_device_create.py:

Line 1: #!/usr/bin/python
Could you add the GPL boiler plate, please? Lawyers love it.
Line 2: 
Line 3: import os
Line 4: import sys
Line 5: import hooking

Line 4: import sys
Line 5: import hooking
Line 6: import traceback
Line 7: 
Line 8: filtername = 'privatevlan-vdsm'
it's nicer to have CONSTANTS in all caps.
Line 9: 
Line 10: 
Line 11: def addInterfaceFilter(devicexml, interface, gateways, ipaddresses, learn):
Line 12:     """Add or replace filters in the interface description"""

Line 50: 
Line 51:     if gateways is None and ipaddresses is None and learn is None:
Line 52:         sys.exit(0)
Line 53: 
Line 54:     domxml = hooking.read_domxml()
actually, it is an interface xml, isn't it?
Line 55:     interfaces = domxml.getElementsByTagName('interface')
Line 56: 
Line 57:     # interfaces may be an empty list - this is fine
Line 58:     for interface in interfaces:

Line 53: 
Line 54:     domxml = hooking.read_domxml()
Line 55:     interfaces = domxml.getElementsByTagName('interface')
Line 56: 
Line 57:     # interfaces may be an empty list - this is fine
how is that fine? before_device_create should receive exactly one interface xml.
Line 58:     for interface in interfaces:
Line 59:         addInterfaceFilter(domxml, interface, gateways, ipaddresses, learn)
Line 60: 
Line 61:     hooking.write_domxml(domxml)

Line 59:         addInterfaceFilter(domxml, interface, gateways, ipaddresses, learn)
Line 60: 
Line 61:     hooking.write_domxml(domxml)
Line 62: 
Line 63: except Exception:
We try to make new hooks nicer (see extnet for an example):
* define main() and test() functions
* use hooking.exit_hook()

a test() is priceless, as it provides users and reviewers an understanding of what the hook is doing.
Line 64:     sys.stderr.write('privatevlan: [unexpected error]: %s\n' %
Line 65:                      traceback.format_exc())