docs/dev/libvirt: update libvirt/firewalld setup instructions #3677

travier · 2020-05-28T10:41:35Z

Update libvirt setup instructions to clearly separate the systemd activated version from the classic one.
Update firewalld setup to take into account recent libvirt integration.
Also include some minor Markdown styling fixes.

openshift-ci-robot · 2020-05-28T10:41:50Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

cfergeau

A couple of comments, but overall nice cleanup!
/lgtm

docs/dev/libvirt/README.md

cfergeau · 2020-05-28T10:59:24Z

docs/dev/libvirt/README.md

-various zones.
+If using `firewalld`, the specifics will depend on how your distribution setup
+the various zones. The following instructions should work as is for Fedora,
+CentOS, RHEL and Arch Linux.


CentOS8/RHEL8 ? Or even EL7 distros?

I think the "libvirt version less than 5.1" covers EL7. Maybe the phrasing here is misleading?

Ah yes, you differentiate between the misc distro versions through libvirt version, which makes sense.

docs/dev/libvirt/README.md

cfergeau · 2020-05-28T11:06:40Z

docs/dev/libvirt/README.md

+sudo firewall-cmd --zone=dmz --change-interface=tt0
+sudo firewall-cmd --zone=dmz --add-service=libvirt
+sudo firewall-cmd --zone=dmz --add-service=dns
+sudo firewall-cmd --zone=dmz --add-service=dhcp


Why the change from the previous rules?

sudo firewall-cmd --zone=dmz --change-interface=virbr0 sudo firewall-cmd --zone=dmz --change-interface=tt0 sudo firewall-cmd --zone=dmz --add-service=libvirt

I assume the rules in this PR are enough to allow connections from the cluster VMs to qemu+tcp://192.168.122.1/system?

I have not re-checked this but by default the dmz zone does not allow DNS/DHCP access and the nodes in the cluster need to get DHCP & DNS from the host dnsmasq daemon started by libvirt.

The virbr0 change is never required as far as I understand as nodes traffic will get routed through the tt0 bridge.

I have not re-checked this but by default the dmz zone does not allow DNS/DHCP access and the nodes in the cluster need to get DHCP & DNS from the host dnsmasq daemon started by libvirt.

Yep, makes sense, dunno why I did not need that when I tried this :)

The virbr0 change is never required as far as I understand as nodes traffic will get routed through the tt0 bridge.

I ran some tests, and seems to be fine indeed. Not quite sure why the VMs on the tt0 bridge can reach virbr0 (we need to be able to reach qemu+tcp://192.168.122.1/system from the guest OS).

cfergeau · 2020-05-28T14:48:57Z

/lgtm
/approve

praveenkumar · 2020-05-29T03:15:42Z

/ok-to-test

travier · 2020-06-05T16:32:05Z

Updated to take into account the changes from: #3638.

travier · 2020-06-09T08:46:12Z

Updated to not make the TCP socket unit enabled at boot by default to reduce security issue (i.e. a reboot will default to a safe state on modern systems). I removed the draft state.

cfergeau

In the last commit shortlog, there's a 'socket' typo.

cfergeau · 2020-06-09T11:07:51Z

docs/dev/libvirt/README.md

-running a telnet server with no root password set. It is critical
-to follow the steps below to **configure the firewall to prevent
-access to libvirt from other hosts on the LAN/WAN**.
+To make this change persistent accross reboots you can optionnaly enable it:


optionnaly->optionally

cfergeau · 2020-06-09T11:09:56Z

docs/dev/libvirt/README.md

 sudo systemctl start libvirtd-tcp.socket
 ```

-after which you need to restart libvirtd.


I believe this still applies?

cfergeau · 2020-06-09T11:10:41Z

docs/dev/libvirt/README.md

-activation via the `libvirt-tcp.socket` systemd unit, the `--listen`
-argument should not be added and thus this step can be skipped.
+This applies only if the libvirt daemon is configured to start and listen on
+UNIX/TCP sockets by itself and not through system activation.


s/system/socket?
After your changes, it is sometimes not clear whether you are talking about libvirtd.service started through systemd (when configuring through /etc/sysconfig/libvirt would still apply), and about libvirtd systemd socket activation, where libvirtd.socket is the only systemd unit running, and this unit starts libvirtd.service upon connections on this socket.

cfergeau · 2020-06-09T11:12:47Z

docs/dev/libvirt/README.md


 ```
 libvirtd_opts="--listen"
 ```

-Next, restart libvirt: `systemctl restart libvirtd`


In this #### Configure the service runner to pass --listen to libvirtd section, I think the previous wording was better.

cfergeau · 2020-06-09T11:14:20Z

docs/dev/libvirt/README.md

-Next, restart libvirt: `systemctl restart libvirtd`
+Then to enable TCP access to libvirtd, modify `/etc/libvirt/libvirtd.conf` and
+set the following:
+


Why is this coming so late? I think this is always needed, and must be done in all cases, even in the socket activated case. From looking at this diff, it seems it's only in the "#### Configure the service runner to pass --listen to libvirtd" section

I think I would keep most of the previous order and not move most of the text. Splitting the libvirt-tcp.socket discussion from its paragraph could be useful indeed, and being more explicit about what enable does is good too.

Ah ok, most of the libvirtd.conf changes are not needed with tcp socket activation.auth_tcp = none is needed:

$ LC_ALL=C virsh -c qemu+tcp://localhost/system error: failed to connect to the hypervisor error: l'authentification a échoué : échec de l'authentification

travier · 2020-06-09T17:40:09Z

Updated to take into account comments. As this reorders lots of sections of text in the README, I find it more practical to directly read the final version than to make sense of the diff: https://github.com/travier/installer/blob/master-docs-dev-libvirt-update/docs/dev/libvirt/README.md

cfergeau · 2020-06-11T10:08:20Z

docs/dev/libvirt/README.md

+listen_tcp = 1
+auth_tcp = "none"
+tcp_port = "16509"
+```


I'm still a bit dubious with the splitting of the libvirtd.conf instructions, even if they are ignored, it's not harmful to set them in socket-activation setups

It's not harmful but it's also unnecessary and might trigger questions. I think we need to have two clearly separated cases to avoid confusion and keeping each case to the point helps with that.

cfergeau · 2020-06-11T10:09:41Z

docs/dev/libvirt/README.md

-activation via the `libvirt-tcp.socket` systemd unit, the `--listen`
-argument should not be added and thus this step can be skipped.
+This applies only if the libvirt daemon is configured to start and listen on
+UNIX/TCP sockets by itself and not through socket activation.


This reads to me as if this section is when starting libvirtd 'by hand', while it really is about systems using libvirtd.service without socket activation (ie without using libvirtd.socket)
Maybe This applies only if the libvirt daemon is started through libvirtd.service without making use of socket activation (through libvirtd.socket and similar systemd units)

cfergeau

/lgtm
/approve

openshift-ci-robot · 2020-06-16T14:05:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~docs/dev/libvirt/OWNERS~~ [cfergeau]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 28, 2020

openshift-ci-robot requested review from cfergeau and praveenkumar May 28, 2020 10:42

cfergeau reviewed May 28, 2020

View reviewed changes

openshift-ci-robot assigned cfergeau May 28, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 28, 2020

travier force-pushed the master-docs-dev-libvirt-update branch from 8e73108 to 3fcc846 Compare May 28, 2020 14:12

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 28, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 28, 2020

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 29, 2020

travier force-pushed the master-docs-dev-libvirt-update branch from 3fcc846 to 2d62b38 Compare June 5, 2020 16:26

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 5, 2020

travier marked this pull request as ready for review June 5, 2020 16:31

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 5, 2020

cfergeau mentioned this pull request Jun 8, 2020

Libvirtd setup instructions unclear #3604

Closed

travier force-pushed the master-docs-dev-libvirt-update branch from 2d62b38 to dad757c Compare June 9, 2020 08:42

travier force-pushed the master-docs-dev-libvirt-update branch from dad757c to 12744f6 Compare June 9, 2020 09:22

cfergeau reviewed Jun 9, 2020

View reviewed changes

travier force-pushed the master-docs-dev-libvirt-update branch from 12744f6 to 051d578 Compare June 9, 2020 17:37

cfergeau reviewed Jun 11, 2020

View reviewed changes

docs/dev/libvirt: minor Markdown styling fixes

c91e2b1

travier added 2 commits June 15, 2020 11:20

docs/dev/libvirt: update firewalld setup instructions

6426073

docs/dev/libvirt: update libvirt TCP socket setup instructions

5d7e32f

travier force-pushed the master-docs-dev-libvirt-update branch from 051d578 to 5d7e32f Compare June 15, 2020 09:20

cfergeau reviewed Jun 16, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2020

openshift-merge-robot merged commit 4e46d0a into openshift:master Jun 16, 2020

travier deleted the master-docs-dev-libvirt-update branch June 16, 2020 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs/dev/libvirt: update libvirt/firewalld setup instructions #3677

docs/dev/libvirt: update libvirt/firewalld setup instructions #3677

travier commented May 28, 2020

openshift-ci-robot commented May 28, 2020

cfergeau left a comment

cfergeau May 28, 2020

travier May 28, 2020

cfergeau May 28, 2020

cfergeau May 28, 2020

travier May 28, 2020

travier May 28, 2020

cfergeau May 28, 2020

cfergeau commented May 28, 2020

praveenkumar commented May 29, 2020

travier commented Jun 5, 2020

travier commented Jun 9, 2020

cfergeau left a comment

cfergeau Jun 9, 2020

cfergeau Jun 9, 2020

cfergeau Jun 9, 2020

cfergeau Jun 9, 2020

cfergeau Jun 9, 2020

cfergeau Jun 9, 2020

cfergeau Jun 9, 2020 •

edited

travier commented Jun 9, 2020 •

edited

cfergeau Jun 11, 2020

travier Jun 15, 2020

cfergeau Jun 11, 2020 •

edited

travier Jun 15, 2020

cfergeau left a comment

openshift-ci-robot commented Jun 16, 2020

docs/dev/libvirt: update libvirt/firewalld setup instructions #3677

docs/dev/libvirt: update libvirt/firewalld setup instructions #3677

Conversation

travier commented May 28, 2020

openshift-ci-robot commented May 28, 2020

cfergeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfergeau commented May 28, 2020

praveenkumar commented May 29, 2020

travier commented Jun 5, 2020

travier commented Jun 9, 2020

cfergeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfergeau Jun 9, 2020 • edited

Choose a reason for hiding this comment

travier commented Jun 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfergeau Jun 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfergeau left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jun 16, 2020

cfergeau Jun 9, 2020 •

edited

travier commented Jun 9, 2020 •

edited

cfergeau Jun 11, 2020 •

edited