[libvirt] [libvirt-tck PATCH] TCK.pm: Define libvirt VMs with an RNG device
The nwfilter 220-no-ip-spoofing.t test relies on an SSH connection to the test VM. However, because the domain definition passed to libvirt lacks an RNG device, the SSH server isn't started inside the guest (even though that is the default on virt-builder images) and therefore: "ssh: connect to host 192.168.122.227 port 22: Connection refused" is returned which in turn makes a bunch of sub tests fail because the very basic premise - a working SSH connection - is false. This patch makes sure a virtio RNG is contained in the XML definition, thus causing the SSH server to be started successfully, thus allowing all the subtests to pass. --- lib/Sys/Virt/TCK.pm | 4 ++++ lib/Sys/Virt/TCK/DomainBuilder.pm | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm index 389d5cc..3ea06cc 100644 --- a/lib/Sys/Virt/TCK.pm +++ b/lib/Sys/Virt/TCK.pm @@ -807,6 +807,8 @@ sub generic_machine_domain { $b->disk(src => $config{root}, dst => $config{dev}, type => "file"); + $b->rng(backend_model => "random", + backend => "/dev/urandom"); if ($config{firstboot}) { print "# Running the first boot\n"; @@ -865,6 +867,8 @@ sub generic_machine_domain { dst => $config{dev}, type => "file", shareable => $shareddisk); + $b->rng(backend_model => "random", + backend => "/dev/urandom"); return $b; } } diff --git a/lib/Sys/Virt/TCK/DomainBuilder.pm b/lib/Sys/Virt/TCK/DomainBuilder.pm index 45336b5..be8708f 100644 --- a/lib/Sys/Virt/TCK/DomainBuilder.pm +++ b/lib/Sys/Virt/TCK/DomainBuilder.pm @@ -49,6 +49,7 @@ sub new { graphics => [], hostdevs => [], seclabel => {}, + rng => {}, }; bless $self, $class; @@ -328,6 +329,19 @@ sub seclabel { return $self; } +sub rng { + my $self = shift; + my %params = @_; + + die "backend model parameter is required" unless $params{backend_model}; + die "backend parameter is required" unless $params{backend}; + + $self->{rng} = \%params; + $self->{rng}->{model} = "virtio" unless defined $self->{rng}->{model}; + + return $self; +} + sub as_xml { my $self = shift; @@ -504,6 +518,13 @@ sub as_xml { $w->endTag("graphics"); } $w->emptyTag("console", type => "pty"); + + $w->startTag("rng", + model => $self->{rng}->{model}); + $w->dataElement("backend", $self->{rng}->{backend}, + model => $self->{rng}->{backend_model}); + $w->endTag("rng"); + $w->endTag("devices"); if ($self->{seclabel}->{model}) { $w->startTag("seclabel", -- 2.20.1
On 9/23/19 1:27 PM, Erik Skultety wrote:
The nwfilter 220-no-ip-spoofing.t test relies on an SSH connection to the test VM. However, because the domain definition passed to libvirt lacks an RNG device, the SSH server isn't started inside the guest (even though that is the default on virt-builder images) and therefore:
"ssh: connect to host 192.168.122.227 port 22: Connection refused"
Strange that this has never happened to me. Is it perhaps because I'm using a very old cached image from virt-builder, and had started it up manually at some time in the past (thus giving it a long enough time to generate the keys, which are now stored away for posterity)?
is returned which in turn makes a bunch of sub tests fail because the very basic premise - a working SSH connection - is false. This patch makes sure a virtio RNG is contained in the XML definition, thus causing the SSH server to be started successfully, thus allowing all the subtests to pass. --- lib/Sys/Virt/TCK.pm | 4 ++++ lib/Sys/Virt/TCK/DomainBuilder.pm | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+)
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm index 389d5cc..3ea06cc 100644 --- a/lib/Sys/Virt/TCK.pm +++ b/lib/Sys/Virt/TCK.pm @@ -807,6 +807,8 @@ sub generic_machine_domain { $b->disk(src => $config{root}, dst => $config{dev}, type => "file"); + $b->rng(backend_model => "random", + backend => "/dev/urandom");
if ($config{firstboot}) { print "# Running the first boot\n"; @@ -865,6 +867,8 @@ sub generic_machine_domain { dst => $config{dev}, type => "file", shareable => $shareddisk); + $b->rng(backend_model => "random", + backend => "/dev/urandom"); return $b; } } diff --git a/lib/Sys/Virt/TCK/DomainBuilder.pm b/lib/Sys/Virt/TCK/DomainBuilder.pm index 45336b5..be8708f 100644 --- a/lib/Sys/Virt/TCK/DomainBuilder.pm +++ b/lib/Sys/Virt/TCK/DomainBuilder.pm @@ -49,6 +49,7 @@ sub new { graphics => [], hostdevs => [], seclabel => {}, + rng => {}, };
bless $self, $class; @@ -328,6 +329,19 @@ sub seclabel { return $self; }
+sub rng { + my $self = shift; + my %params = @_; + + die "backend model parameter is required" unless $params{backend_model}; + die "backend parameter is required" unless $params{backend}; + + $self->{rng} = \%params; + $self->{rng}->{model} = "virtio" unless defined $self->{rng}->{model}; + + return $self; +} + sub as_xml { my $self = shift;
@@ -504,6 +518,13 @@ sub as_xml { $w->endTag("graphics"); } $w->emptyTag("console", type => "pty"); + + $w->startTag("rng", + model => $self->{rng}->{model}); + $w->dataElement("backend", $self->{rng}->{backend}, + model => $self->{rng}->{backend_model}); + $w->endTag("rng"); +
Because the <rng> element is added unconditionally, t/070-domain-builder.t fails when you run "./prepare-release.sh" I fixed this locally by making that bit of code conditional on having backen_model set (see the diff at the end). You could also modify the failing test to explicitly add an <rng> element, but I think we still should put it in conditionally, in case someone needs to *not* have it (e.g., there are xen tests run with libvirt-tck, and they might not like it if the domain had an <rng> element; I can't say that for sure, because I'm not setup to test Xen, and am too lazy to even look at the code and try to figure it out by hand :-)). From my POV, if you apply the diff at the end of this message, then: Reviewed-by: Laine Stump <laine@laine.org> You may or may not choose to add an rng to the domain-builder test (and may or may not get a complaint the next time someone runs a Xen test :-) diff --git a/lib/Sys/Virt/TCK/DomainBuilder.pm b/lib/Sys/Virt/TCK/DomainBuilder.pm index be8708f..9e0c49c 100644 --- a/lib/Sys/Virt/TCK/DomainBuilder.pm +++ b/lib/Sys/Virt/TCK/DomainBuilder.pm @@ -519,11 +519,13 @@ sub as_xml { } $w->emptyTag("console", type => "pty"); - $w->startTag("rng", - model => $self->{rng}->{model}); - $w->dataElement("backend", $self->{rng}->{backend}, - model => $self->{rng}->{backend_model}); - $w->endTag("rng"); + if ($self->{rng}->{backend_model}) { + $w->startTag("rng", + model => $self->{rng}->{model}); + $w->dataElement("backend", $self->{rng}->{backend}, + model => $self->{rng}->{backend_model}); + $w->endTag("rng"); + } $w->endTag("devices"); if ($self->{seclabel}->{model}) {
On Mon, Sep 23, 2019 at 04:47:06PM -0400, Laine Stump wrote:
On 9/23/19 1:27 PM, Erik Skultety wrote:
The nwfilter 220-no-ip-spoofing.t test relies on an SSH connection to the test VM. However, because the domain definition passed to libvirt lacks an RNG device, the SSH server isn't started inside the guest (even though that is the default on virt-builder images) and therefore:
"ssh: connect to host 192.168.122.227 port 22: Connection refused"
Strange that this has never happened to me. Is it perhaps because I'm using a very old cached image from virt-builder, and had started it up manually at some time in the past (thus giving it a long enough time to generate the keys, which are now stored away for posterity)?
is returned which in turn makes a bunch of sub tests fail because the very basic premise - a working SSH connection - is false. This patch makes sure a virtio RNG is contained in the XML definition, thus causing the SSH server to be started successfully, thus allowing all the subtests to pass. --- lib/Sys/Virt/TCK.pm | 4 ++++ lib/Sys/Virt/TCK/DomainBuilder.pm | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+)
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm index 389d5cc..3ea06cc 100644 --- a/lib/Sys/Virt/TCK.pm +++ b/lib/Sys/Virt/TCK.pm @@ -807,6 +807,8 @@ sub generic_machine_domain { $b->disk(src => $config{root}, dst => $config{dev}, type => "file"); + $b->rng(backend_model => "random", + backend => "/dev/urandom"); if ($config{firstboot}) { print "# Running the first boot\n"; @@ -865,6 +867,8 @@ sub generic_machine_domain { dst => $config{dev}, type => "file", shareable => $shareddisk); + $b->rng(backend_model => "random", + backend => "/dev/urandom"); return $b; } } diff --git a/lib/Sys/Virt/TCK/DomainBuilder.pm b/lib/Sys/Virt/TCK/DomainBuilder.pm index 45336b5..be8708f 100644 --- a/lib/Sys/Virt/TCK/DomainBuilder.pm +++ b/lib/Sys/Virt/TCK/DomainBuilder.pm @@ -49,6 +49,7 @@ sub new { graphics => [], hostdevs => [], seclabel => {}, + rng => {}, }; bless $self, $class; @@ -328,6 +329,19 @@ sub seclabel { return $self; } +sub rng { + my $self = shift; + my %params = @_; + + die "backend model parameter is required" unless $params{backend_model}; + die "backend parameter is required" unless $params{backend}; + + $self->{rng} = \%params; + $self->{rng}->{model} = "virtio" unless defined $self->{rng}->{model}; + + return $self; +} + sub as_xml { my $self = shift; @@ -504,6 +518,13 @@ sub as_xml { $w->endTag("graphics"); } $w->emptyTag("console", type => "pty"); + + $w->startTag("rng", + model => $self->{rng}->{model}); + $w->dataElement("backend", $self->{rng}->{backend}, + model => $self->{rng}->{backend_model}); + $w->endTag("rng"); +
Because the <rng> element is added unconditionally, t/070-domain-builder.t fails when you run "./prepare-release.sh"
Ah, missed that one, good catch :).
I fixed this locally by making that bit of code conditional on having backen_model set (see the diff at the end). You could also modify the failing test to explicitly add an <rng> element, but I think we still should put it in conditionally, in case someone needs to *not* have it (e.g., there are xen tests run with libvirt-tck, and they might not like it if the domain had an <rng> element; I can't say that for sure, because I'm not setup to test Xen, and am too lazy to even look at the code and try to figure it out by hand :-)).
From my POV, if you apply the diff at the end of this message, then:
Reviewed-by: Laine Stump <laine@laine.org>
You may or may not choose to add an rng to the domain-builder test (and may or may not get a complaint the next time someone runs a Xen test :-)
diff --git a/lib/Sys/Virt/TCK/DomainBuilder.pm b/lib/Sys/Virt/TCK/DomainBuilder.pm index be8708f..9e0c49c 100644 --- a/lib/Sys/Virt/TCK/DomainBuilder.pm +++ b/lib/Sys/Virt/TCK/DomainBuilder.pm @@ -519,11 +519,13 @@ sub as_xml { } $w->emptyTag("console", type => "pty");
- $w->startTag("rng", - model => $self->{rng}->{model}); - $w->dataElement("backend", $self->{rng}->{backend}, - model => $self->{rng}->{backend_model}); - $w->endTag("rng"); + if ($self->{rng}->{backend_model}) {
Hmm, wouldn't it be actually better to test for an empty hash instead? IOW if (%{$self->{rng}}) { ... sounds a bit more generic to me rather than test presence of a specific attribute within the hash and it seems to be working in context of both the nwfilter test and the domain builder test with an Xen XML in prepare_release.sh If you're okay with that kind of adjustment instead, I'll proceed with merging the patch. Thanks for review, Erik
+ $w->startTag("rng", + model => $self->{rng}->{model}); + $w->dataElement("backend", $self->{rng}->{backend}, + model => $self->{rng}->{backend_model}); + $w->endTag("rng"); + }
$w->endTag("devices"); if ($self->{seclabel}->{model}) {
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Sep 24, 2019, 2:25 AM Erik Skultety <eskultet@redhat.com> wrote:
On Mon, Sep 23, 2019 at 04:47:06PM -0400, Laine Stump wrote:
On 9/23/19 1:27 PM, Erik Skultety wrote:
From my POV, if you apply the diff at the end of this message, then:
Reviewed-by: Laine Stump <laine@laine.org>
You may or may not choose to add an rng to the domain-builder test (and may or may not get a complaint the next time someone runs a Xen test :-)
diff --git a/lib/Sys/Virt/TCK/DomainBuilder.pm b/lib/Sys/Virt/TCK/DomainBuilder.pm index be8708f..9e0c49c 100644 --- a/lib/Sys/Virt/TCK/DomainBuilder.pm +++ b/lib/Sys/Virt/TCK/DomainBuilder.pm @@ -519,11 +519,13 @@ sub as_xml { } $w->emptyTag("console", type => "pty");
- $w->startTag("rng", - model => $self->{rng}->{model}); - $w->dataElement("backend", $self->{rng}->{backend}, - model => $self->{rng}->{backend_model}); - $w->endTag("rng"); + if ($self->{rng}->{backend_model}) {
Hmm, wouldn't it be actually better to test for an empty hash instead? IOW if (%{$self->{rng}}) { ...
sounds a bit more generic to me rather than test presence of a specific attribute within the hash and it seems to be working in context of both the nwfilter test and the domain builder test with an Xen XML in prepare_release.sh If you're okay with that kind of adjustment instead, I'll proceed with merging the patch.
Sure, that's fine with me (assuming that you've run prepare-release and the test passes :-)). I was just blindly copying what was done in other code around it.
On Tue, Sep 24, 2019 at 05:41:30AM -0400, Laine Stump wrote:
On Tue, Sep 24, 2019, 2:25 AM Erik Skultety <eskultet@redhat.com> wrote:
On Mon, Sep 23, 2019 at 04:47:06PM -0400, Laine Stump wrote:
On 9/23/19 1:27 PM, Erik Skultety wrote:
From my POV, if you apply the diff at the end of this message, then:
Reviewed-by: Laine Stump <laine@laine.org>
You may or may not choose to add an rng to the domain-builder test (and may or may not get a complaint the next time someone runs a Xen test :-)
diff --git a/lib/Sys/Virt/TCK/DomainBuilder.pm b/lib/Sys/Virt/TCK/DomainBuilder.pm index be8708f..9e0c49c 100644 --- a/lib/Sys/Virt/TCK/DomainBuilder.pm +++ b/lib/Sys/Virt/TCK/DomainBuilder.pm @@ -519,11 +519,13 @@ sub as_xml { } $w->emptyTag("console", type => "pty");
- $w->startTag("rng", - model => $self->{rng}->{model}); - $w->dataElement("backend", $self->{rng}->{backend}, - model => $self->{rng}->{backend_model}); - $w->endTag("rng"); + if ($self->{rng}->{backend_model}) {
Hmm, wouldn't it be actually better to test for an empty hash instead? IOW if (%{$self->{rng}}) { ...
sounds a bit more generic to me rather than test presence of a specific attribute within the hash and it seems to be working in context of both the nwfilter test and the domain builder test with an Xen XML in prepare_release.sh If you're okay with that kind of adjustment instead, I'll proceed with merging the patch.
Sure, that's fine with me (assuming that you've run prepare-release and the test passes :-)). I was just blindly copying what was done in other code around it.
Fixed and pushed, thanks. Erik
On Mon, Sep 23, 2019 at 04:47:06PM -0400, Laine Stump wrote:
On 9/23/19 1:27 PM, Erik Skultety wrote:
The nwfilter 220-no-ip-spoofing.t test relies on an SSH connection to the test VM. However, because the domain definition passed to libvirt lacks an RNG device, the SSH server isn't started inside the guest (even though that is the default on virt-builder images) and therefore:
"ssh: connect to host 192.168.122.227 port 22: Connection refused"
Strange that this has never happened to me. Is it perhaps because I'm using a very old cached image from virt-builder, and had started it up manually at some time in the past (thus giving it a long enough time to generate the keys, which are now stored away for posterity)?
Btw I always thought that the keys are generated during the package installation rather than first execution of the daemon, clearly I was wrong. Erik
On Tue, 2019-09-24 at 08:27 +0200, Erik Skultety wrote:
On Mon, Sep 23, 2019 at 04:47:06PM -0400, Laine Stump wrote:
On 9/23/19 1:27 PM, Erik Skultety wrote:
The nwfilter 220-no-ip-spoofing.t test relies on an SSH connection to the test VM. However, because the domain definition passed to libvirt lacks an RNG device, the SSH server isn't started inside the guest (even though that is the default on virt-builder images) and therefore:
"ssh: connect to host 192.168.122.227 port 22: Connection refused"
Strange that this has never happened to me. Is it perhaps because I'm using a very old cached image from virt-builder, and had started it up manually at some time in the past (thus giving it a long enough time to generate the keys, which are now stored away for posterity)?
Btw I always thought that the keys are generated during the package installation rather than first execution of the daemon, clearly I was wrong.
I'm going to go out on a limb and assume virt-builder templates get their keys ripped out explicitly as part of the building process, because of course you wouldn't want all guests created from the same virt-builder template to share a single set of SSH keys, now would you? :) -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Sep 24, 2019 at 12:02:44PM +0200, Andrea Bolognani wrote:
On Tue, 2019-09-24 at 08:27 +0200, Erik Skultety wrote:
On Mon, Sep 23, 2019 at 04:47:06PM -0400, Laine Stump wrote:
On 9/23/19 1:27 PM, Erik Skultety wrote:
The nwfilter 220-no-ip-spoofing.t test relies on an SSH connection to the test VM. However, because the domain definition passed to libvirt lacks an RNG device, the SSH server isn't started inside the guest (even though that is the default on virt-builder images) and therefore:
"ssh: connect to host 192.168.122.227 port 22: Connection refused"
Strange that this has never happened to me. Is it perhaps because I'm using a very old cached image from virt-builder, and had started it up manually at some time in the past (thus giving it a long enough time to generate the keys, which are now stored away for posterity)?
Btw I always thought that the keys are generated during the package installation rather than first execution of the daemon, clearly I was wrong.
I'm going to go out on a limb and assume virt-builder templates get their keys ripped out explicitly as part of the building process, because of course you wouldn't want all guests created from the same virt-builder template to share a single set of SSH keys, now would you? :)
That makes a lot of sense, they do sanitize the images indeed. (btw I read somewhere that under some circumstances you'd want to share the server keys in a cluster environment, unfortunately the author of the article didn't bother explaining, so I'm taking that information with a grain of salt) Thanks, Erik
On Tue, Sep 24, 2019, 6:02 AM Andrea Bolognani <abologna@redhat.com> wrote:
On Mon, Sep 23, 2019 at 04:47:06PM -0400, Laine Stump wrote:
On 9/23/19 1:27 PM, Erik Skultety wrote:
The nwfilter 220-no-ip-spoofing.t test relies on an SSH connection to the test VM. However, because the domain definition passed to libvirt lacks an RNG device, the SSH server isn't started inside the guest (even though that is the default on virt-builder images) and
"ssh: connect to host 192.168.122.227 port 22: Connection refused"
Strange that this has never happened to me. Is it perhaps because I'm using a very old cached image from virt-builder, and had started it up manually at some time in the past (thus giving it a long enough time to generate
On Tue, 2019-09-24 at 08:27 +0200, Erik Skultety wrote: therefore: the
keys, which are now stored away for posterity)?
Btw I always thought that the keys are generated during the package installation rather than first execution of the daemon, clearly I was wrong.
I'm going to go out on a limb and assume virt-builder templates get their keys ripped out explicitly as part of the building process, because of course you wouldn't want all guests created from the same virt-builder template to share a single set of SSH keys, now would you? :)
My recollection about sshd was that it did this the first time the service was started, but of course that me.ory is at least 10 years old, and so is suspect from the start! This has started me thinking again about the problem we have with the creation of the default network at package installation time (I don't have access to bugzilla right now to reference the BZ, but want to write this down before I forget - I'm talking about the deal where we decide on the subnet to use during a postinstall script, and that potentially creates a conflict if libvirtd is later run in a different network environment than when it was installed). Anyway, maybe we should try mimicking what sshd does for it's initial run (or maybe not, since we need to do our stuff after the host networking has completely started and settled, but sshd has no such dependency. On the other hand, sshd does manage to properly delay setting up it's keys until it is running on the "final destination" system rather than doing it e.g. when a booting a live image (which will then be used to do a "copy everything en masse" install to the final destination).
participants (3)
-
Andrea Bolognani -
Erik Skultety -
Laine Stump