Re: [libvirt] [PATCH 19/41] remote: introduce virtproxyd daemon to handle IP connectivity
On Mon, 2019-07-29 at 13:30 +0100, Daniel P. Berrangé wrote: > On Sun, Jul 28, 2019 at 04:42:52PM +0200, Andrea Bolognani wrote: > > On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > > > +[Install] > > > +WantedBy=multi-user.target > > > +Also=virtproxyd.socket > > > +Also=virtproxyd-ro.socket > > > > Kind of a side note since it's pre-existing, but don't we want to > > list virtproxyd-admin.socket here too? > > It is redundant - the deps force virtproxyd-admin.socket to become > enabled regardless. Hm, yeah: we either want this to be socket activated, in which case we'd have to enable the various sockets but not the service, or we want it to start at boot, in which case enabling the sockets is unnecessary. Can you please drop the Also= lines for this as well as all other virt*d.service files then? -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 19/41] remote: introduce virtproxyd daemon to handle IP connectivity
On 7/23/19 10:02 AM, Daniel P. Berrangé wrote: > The libvirtd daemon provides the traditional libvirt experience where > all the drivers are in a single daemon, and is accessible over both > local UNIX sockets and remote IP sockets. > > In the new world we're having a set of per-driver daemons which will > primarily be accessed locally via their own UNIX sockets. > > We still, however, need to allow for case of applications which will > connect to libvirt remotely. These remote connections can be done as > TCP/TLS sockets, or by SSH tunnelling to the UNIX socket. > > In the later case, the old libvirt.so clients will only know about > the path to the old libvirtd socket /var/run/libvirt/libvirt-sock, > and not the new driver sockets /var/run/libvirt/virtqemud-sock. > > It is also not desirable to expose the main driver specific daemons > over IP directly to minimize their attack service. > > Thus the virtproxyd daemon steps into place, to provide TCP/TLS sockets, > and back compat for the old libvirtd UNIX socket path(s). It will then > forward all RPC calls made to the appropriate driver specific daemon. > > Essentially it is equivalent to the old libvirtd with absolutely no > drivers registered except for the remote driver (and other stateless > drivers in libvirt.so). > > We could have modified libvirtd so none of the drivers are registed > to get the same end result. We could even add a libvirtd.conf parameter > to control whether the drivers are loaded to enable users to switch back > to the old world if we discover bugs in the split-daemon model. Using a > new daemon though has some advantages > > - We can make virtproxyd and the virtXXXd per-driver daemons all > have "Conflicts: libvirtd.service" in their systemd unit files. > This will guarantee that libvirtd is never started at the same > time, as this would result in two daemons running the same driver. > Fortunately drivers use locking to protect themselves, but it is > better to avoid starting a daemon we know will conflict. > > - It allows us to break CLI compat to remove the --listen parameter. > Both listen_tcp and listen_tls parameters in /etc/libvirtd/virtd.conf > will default to zero. Either TLS or TCP can be enabled exclusively > though virtd.conf without requiring the extra step of adding --listen. > > - It allows us to set a strict SELinux policy over virtproxyd. For > back compat the libvirtd policy must continue to allow all drivers > to run. We can't easily give a second policy to libvirtd which > locks it down. By introducing a new virtproxyd we can set a strict > policy for that daemon only. Reading this paragraph reminds me that the apparmor profiles will need adjusting too. Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 19/41] remote: introduce virtproxyd daemon to handle IP connectivity
On Mon, Jul 29, 2019 at 01:50:22PM +0100, Daniel P. Berrangé wrote: > On Mon, Jul 29, 2019 at 01:30:42PM +0100, Daniel P. Berrangé wrote: > > On Sun, Jul 28, 2019 at 04:42:52PM +0200, Andrea Bolognani wrote: > > > On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > > > [...] > > > > - We can make virtproxyd and the virtXXXd per-driver daemons all > > > >have "Conflicts: libvirtd.service" in their systemd unit files. > > > >This will guarantee that libvirtd is never started at the same > > > >time, as this would result in two daemons running the same driver. > > > >Fortunately drivers use locking to protect themselves, but it is > > > >better to avoid starting a daemon we know will conflict. > > > > > > I feel like this will need to be tested extensively to make sure > > > we're always doing the right thing, including on non-systemd hosts. > > > > Testing is quite easy - just try to start the two units and make > > sure only one ends up running. Similarly for non-systemd hosts, > > start both daemons & see that only one succeeds - the others > > fail with lock conflict. > > > > > > > > +++ b/src/remote/virtproxyd.service.in > > > > @@ -0,0 +1,24 @@ > > > > +[Unit] > > > > +Description=Virtualization daemon > > > > +Conflicts=libvirtd.service > > > > +Requires=virtproxyd.socket > > > > +Requires=virtproxyd-ro.socket > > > > +Requires=virtproxyd-admin.socket > > > > +After=network.target > > > > +After=dbus.service > > > > +After=apparmor.service > > > > +After=local-fs.target > > > > +After=remote-fs.target > > > > +Documentation=man:libvirtd(8) > > > > +Documentation=https://libvirt.org > > > > > > There are a few non-obvious changes between libvirtd.service.in and > > > this file: > > > > > > -Requires=virtlogd.socket > > > -Requires=virtlockd.socket > > > -Wants=systemd-machined.service > > > -Before=libvirt-guests.service > > > -After=iscsid.service > > > -After=systemd-logind.service > > > -After=systemd-machined.service > > > > > > I can see why we'd move the relationships with iscsid and virtlockd > > > to virtstoraged, except looking ahead to patch 23 I see you haven't > > > actually done that; either way, I'm not so convinced about the > > > remaining changes. Care to explain the rationale behind them? > > > > virtproxdy contains no drivers, so it doesn't need to depend > > on any of these services. > > > > virtdstoraged/qemud/lxcd should have gained some of these though. > > I should have killed dbus.service and remote-fs.service too. Doh, not. dbus must always be present for polkit to work. remote-fs.service is ok as that's only needed for the hypervisor services (to access disk images). Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 19/41] remote: introduce virtproxyd daemon to handle IP connectivity
On Mon, Jul 29, 2019 at 01:30:42PM +0100, Daniel P. Berrangé wrote: > On Sun, Jul 28, 2019 at 04:42:52PM +0200, Andrea Bolognani wrote: > > On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > > [...] > > > - We can make virtproxyd and the virtXXXd per-driver daemons all > > >have "Conflicts: libvirtd.service" in their systemd unit files. > > >This will guarantee that libvirtd is never started at the same > > >time, as this would result in two daemons running the same driver. > > >Fortunately drivers use locking to protect themselves, but it is > > >better to avoid starting a daemon we know will conflict. > > > > I feel like this will need to be tested extensively to make sure > > we're always doing the right thing, including on non-systemd hosts. > > Testing is quite easy - just try to start the two units and make > sure only one ends up running. Similarly for non-systemd hosts, > start both daemons & see that only one succeeds - the others > fail with lock conflict. > > > > > +++ b/src/remote/virtproxyd.service.in > > > @@ -0,0 +1,24 @@ > > > +[Unit] > > > +Description=Virtualization daemon > > > +Conflicts=libvirtd.service > > > +Requires=virtproxyd.socket > > > +Requires=virtproxyd-ro.socket > > > +Requires=virtproxyd-admin.socket > > > +After=network.target > > > +After=dbus.service > > > +After=apparmor.service > > > +After=local-fs.target > > > +After=remote-fs.target > > > +Documentation=man:libvirtd(8) > > > +Documentation=https://libvirt.org > > > > There are a few non-obvious changes between libvirtd.service.in and > > this file: > > > > -Requires=virtlogd.socket > > -Requires=virtlockd.socket > > -Wants=systemd-machined.service > > -Before=libvirt-guests.service > > -After=iscsid.service > > -After=systemd-logind.service > > -After=systemd-machined.service > > > > I can see why we'd move the relationships with iscsid and virtlockd > > to virtstoraged, except looking ahead to patch 23 I see you haven't > > actually done that; either way, I'm not so convinced about the > > remaining changes. Care to explain the rationale behind them? > > virtproxdy contains no drivers, so it doesn't need to depend > on any of these services. > > virtdstoraged/qemud/lxcd should have gained some of these though. I should have killed dbus.service and remote-fs.service too. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 19/41] remote: introduce virtproxyd daemon to handle IP connectivity
On Sun, Jul 28, 2019 at 04:42:52PM +0200, Andrea Bolognani wrote: > On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > [...] > > - We can make virtproxyd and the virtXXXd per-driver daemons all > >have "Conflicts: libvirtd.service" in their systemd unit files. > >This will guarantee that libvirtd is never started at the same > >time, as this would result in two daemons running the same driver. > >Fortunately drivers use locking to protect themselves, but it is > >better to avoid starting a daemon we know will conflict. > > I feel like this will need to be tested extensively to make sure > we're always doing the right thing, including on non-systemd hosts. Testing is quite easy - just try to start the two units and make sure only one ends up running. Similarly for non-systemd hosts, start both daemons & see that only one succeeds - the others fail with lock conflict. > > +++ b/src/remote/virtproxyd.service.in > > @@ -0,0 +1,24 @@ > > +[Unit] > > +Description=Virtualization daemon > > +Conflicts=libvirtd.service > > +Requires=virtproxyd.socket > > +Requires=virtproxyd-ro.socket > > +Requires=virtproxyd-admin.socket > > +After=network.target > > +After=dbus.service > > +After=apparmor.service > > +After=local-fs.target > > +After=remote-fs.target > > +Documentation=man:libvirtd(8) > > +Documentation=https://libvirt.org > > There are a few non-obvious changes between libvirtd.service.in and > this file: > > -Requires=virtlogd.socket > -Requires=virtlockd.socket > -Wants=systemd-machined.service > -Before=libvirt-guests.service > -After=iscsid.service > -After=systemd-logind.service > -After=systemd-machined.service > > I can see why we'd move the relationships with iscsid and virtlockd > to virtstoraged, except looking ahead to patch 23 I see you haven't > actually done that; either way, I'm not so convinced about the > remaining changes. Care to explain the rationale behind them? virtproxdy contains no drivers, so it doesn't need to depend on any of these services. virtdstoraged/qemud/lxcd should have gained some of these though. > > > +[Service] > > +Type=notify > > +ExecStart=@sbindir@/virtproxyd --timeout 120 > > +ExecReload=/bin/kill -HUP $MAINPID > > +Restart=on-failure > > More changes in this section: > > -EnvironmentFile=-@sysconfdir@/sysconfig/libvirtd > -KillMode=process > -LimitNOFILE=8192 > -TasksMax=32768 > > EnvironmentFile is clearly no longer needed, while both LimitNOFILE > and TasksMax probably belong to the hypervisor-specific daemons, but > I'm unclear on why KillMode was changed. The systemd default is fine as we don't need any other processes to survive shutdown. > > +[Install] > > +WantedBy=multi-user.target > > +Also=virtproxyd.socket > > +Also=virtproxyd-ro.socket > > Kind of a side note since it's pre-existing, but don't we want to > list virtproxyd-admin.socket here too? It is redundant - the deps force virtproxyd-admin.socket to become enabled regardless. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 19/41] remote: introduce virtproxyd daemon to handle IP connectivity
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: [...] > - We can make virtproxyd and the virtXXXd per-driver daemons all >have "Conflicts: libvirtd.service" in their systemd unit files. >This will guarantee that libvirtd is never started at the same >time, as this would result in two daemons running the same driver. >Fortunately drivers use locking to protect themselves, but it is >better to avoid starting a daemon we know will conflict. I feel like this will need to be tested extensively to make sure we're always doing the right thing, including on non-systemd hosts. [...] > After some time we can deprecate use of libvirtd and after some more > time delete it entirely, leaving us in a pretty world filled with > prancing unicorns. Awww! > The main downside with introducing a new daemon, and with the > per-driver daemons in general, is figuring out the correct upgrade > path. > > The conservative option is to leave libvirtd running if it was > an existing installation. Only use the new daemons & virtproxyd > on completely new installs. > > The aggressive option is to disable libvirtd if already running > and activate all the new daemons. I vote for the conservative option :) As an aside, the above basically a master class in how to write a good commit message. Well done! [...] > +++ b/src/remote/Makefile.inc.am [...] > +VIRTD_UNIT_VARS = \ > + $(COMMON_UNIT_VARS) \ > + -e 's|[@]deps[@]|Conflicts=$(LIBVIRTD_SOCKET_UNIT_FILES)|g' \ > + $(NULL) Considering that we only use LIBVIRTD_SOCKET_UNIT_FILES here, I'd move its definition to this general area. [...] > +++ b/src/remote/remote_daemon.c > @@ -303,11 +303,19 @@ static int daemonErrorLogFilter(virErrorPtr err, int > priority) > > static int daemonInitialize(void) > { > -#ifdef MODULE_NAME > +#ifndef LIBVIRTD > +# ifdef MODULE_NAME > +/* This a dedicated per-driver daemon build */ > if (virDriverLoadModule(MODULE_NAME, MODULE_NAME "Register", true) < 0) > return -1; > +# else > +/* This is virtproxyd which merely proxies to the per-driver > + * daemons for back compat, and also allows IP connectivity. > + */ > +# endif > #else > -/* > +/* This is the legacy monolithic libvirtd built with all drivers > + * This is exactly the kind of comment I suggested you add in patch 9, so I guess just move the first and third one to that patch. [...] > @@ -893,9 +901,9 @@ daemonUsage(const char *argv0, bool privileged) > { "-h | --help", N_("Display program help") }, > { "-v | --verbose", N_("Verbose messages") }, > { "-d | --daemon", N_("Run as a daemon & write PID file") }, > -#ifdef ENABLE_IP > +#if defined(ENABLE_IP) && defined (LIBVIRTD) Extra whitespace in "defined (LIBVIRTD)". [...] > +++ b/src/remote/virtproxyd.service.in > @@ -0,0 +1,24 @@ > +[Unit] > +Description=Virtualization daemon > +Conflicts=libvirtd.service > +Requires=virtproxyd.socket > +Requires=virtproxyd-ro.socket > +Requires=virtproxyd-admin.socket > +After=network.target > +After=dbus.service > +After=apparmor.service > +After=local-fs.target > +After=remote-fs.target > +Documentation=man:libvirtd(8) > +Documentation=https://libvirt.org There are a few non-obvious changes between libvirtd.service.in and this file: -Requires=virtlogd.socket -Requires=virtlockd.socket -Wants=systemd-machined.service -Before=libvirt-guests.service -After=iscsid.service -After=systemd-logind.service -After=systemd-machined.service I can see why we'd move the relationships with iscsid and virtlockd to virtstoraged, except looking ahead to patch 23 I see you haven't actually done that; either way, I'm not so convinced about the remaining changes. Care to explain the rationale behind them? > +[Service] > +Type=notify > +ExecStart=@sbindir@/virtproxyd --timeout 120 > +ExecReload=/bin/kill -HUP $MAINPID > +Restart=on-failure More changes in this section: -EnvironmentFile=-@sysconfdir@/sysconfig/libvirtd -KillMode=process -LimitNOFILE=8192 -TasksMax=32768 EnvironmentFile is clearly no longer needed, while both LimitNOFILE and TasksMax probably belong to the hypervisor-specific daemons, but I'm unclear on why KillMode was changed. > +[Install] > +WantedBy=multi-user.target > +Also=virtproxyd.socket > +Also=virtproxyd-ro.socket Kind of a side note since it's pre-existing, but don't we want to list virtproxyd-admin.socket here too? Overall, the changes look good. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 19/41] remote: introduce virtproxyd daemon to handle IP connectivity
The libvirtd daemon provides the traditional libvirt experience where all the drivers are in a single daemon, and is accessible over both local UNIX sockets and remote IP sockets. In the new world we're having a set of per-driver daemons which will primarily be accessed locally via their own UNIX sockets. We still, however, need to allow for case of applications which will connect to libvirt remotely. These remote connections can be done as TCP/TLS sockets, or by SSH tunnelling to the UNIX socket. In the later case, the old libvirt.so clients will only know about the path to the old libvirtd socket /var/run/libvirt/libvirt-sock, and not the new driver sockets /var/run/libvirt/virtqemud-sock. It is also not desirable to expose the main driver specific daemons over IP directly to minimize their attack service. Thus the virtproxyd daemon steps into place, to provide TCP/TLS sockets, and back compat for the old libvirtd UNIX socket path(s). It will then forward all RPC calls made to the appropriate driver specific daemon. Essentially it is equivalent to the old libvirtd with absolutely no drivers registered except for the remote driver (and other stateless drivers in libvirt.so). We could have modified libvirtd so none of the drivers are registed to get the same end result. We could even add a libvirtd.conf parameter to control whether the drivers are loaded to enable users to switch back to the old world if we discover bugs in the split-daemon model. Using a new daemon though has some advantages - We can make virtproxyd and the virtXXXd per-driver daemons all have "Conflicts: libvirtd.service" in their systemd unit files. This will guarantee that libvirtd is never started at the same time, as this would result in two daemons running the same driver. Fortunately drivers use locking to protect themselves, but it is better to avoid starting a daemon we know will conflict. - It allows us to break CLI compat to remove the --listen parameter. Both listen_tcp and listen_tls parameters in /etc/libvirtd/virtd.conf will default to zero. Either TLS or TCP can be enabled exclusively though virtd.conf without requiring the extra step of adding --listen. - It allows us to set a strict SELinux policy over virtproxyd. For back compat the libvirtd policy must continue to allow all drivers to run. We can't easily give a second policy to libvirtd which locks it down. By introducing a new virtproxyd we can set a strict policy for that daemon only. - It gets rid of the wierd naming of having a daemon with "lib" in its name. Now all normal daemons libvirt ships will have "virt" as their prefix not "libvirt". - Distros can more easily choose their upgrade path. They can ship both sets of daemons in their packages, and choose to either enable libvirtd, or enable the per-driver daemons and virtproxyd out of the box. Users can easily override this if desired by just tweaking which systemd units are active. After some time we can deprecate use of libvirtd and after some more time delete it entirely, leaving us in a pretty world filled with prancing unicorns. The main downside with introducing a new daemon, and with the per-driver daemons in general, is figuring out the correct upgrade path. The conservative option is to leave libvirtd running if it was an existing installation. Only use the new daemons & virtproxyd on completely new installs. The aggressive option is to disable libvirtd if already running and activate all the new daemons. Signed-off-by: Daniel P. Berrangé --- .gitignore| 4 ++ libvirt.spec.in | 10 +++ src/remote/Makefile.inc.am| 112 +++--- src/remote/remote_daemon.c| 32 ++--- src/remote/remote_daemon_config.c | 6 +- src/remote/virtproxyd.service.in | 24 +++ 6 files changed, 167 insertions(+), 21 deletions(-) create mode 100644 src/remote/virtproxyd.service.in diff --git a/.gitignore b/.gitignore index 4463660c85..05bc166860 100644 --- a/.gitignore +++ b/.gitignore @@ -161,6 +161,9 @@ /src/remote/libvirtd.aug /src/remote/libvirtd.conf /src/remote/test_libvirtd.aug +/src/remote/test_virtproxyd.aug +/src/remote/virtproxyd.aug +/src/remote/virtproxyd.conf /src/rpc/virkeepaliveprotocol.[ch] /src/rpc/virnetprotocol.[ch] /src/util/virkeycodetable*.h @@ -168,6 +171,7 @@ /src/virt-aa-helper /src/virtlockd /src/virtlogd +/src/virtproxyd /src/virt-guest-shutdown.target /tests/*.log /tests/*.pid diff --git a/libvirt.spec.in b/libvirt.spec.in index b13b863928..2f64dcabe3 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1527,6 +1527,12 @@ exit 0 %{_unitdir}/libvirtd-admin.socket %{_unitdir}/libvirtd-tcp.socket %{_unitdir}/libvirtd-tls.socket +%{_unitdir}/virtproxyd.service +%{_unitdir}/virtproxyd.socket +%{_unitdir}/virtproxyd-ro.socket +%{_unitdir}/virtproxyd-admin.socket +%{_unitdir}/virtproxyd-tcp.socket