6:28 a.m.
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 <mik(a)miknet.net>
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_...
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())
--
To view, visit http://gerrit.ovirt.org/24195
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Idda6f193c0095241bc1540a0241d49426c000fb3
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Michael Samuel <mik(a)miknet.net>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes