[PATCH 0/4] rpm: Some ssh-proxy improvements
CI pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/1292193155 Andrea Bolognani (4): rpm: Drop weak dependency on ssh-proxy from client rpm: Only Recommend ssh-proxy rpm: Move dependency on ssh-proxy to QEMU driver rpm: Drop with_ssh_proxy define libvirt.spec.in | 23 ++--------------------- 1 file changed, 2 insertions(+), 21 deletions(-) -- 2.45.0
The ssh-proxy feature works independently of the clients, just like the NSS plugin does. Moreover, ssh-proxy only works for local VMs, while clients are routinely used to manage remote hypervisors. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 3 --- 1 file changed, 3 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index f7c128d809..5cb19fa433 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1025,9 +1025,6 @@ Summary: Client side utilities of the libvirt library Requires: libvirt-libs = %{version}-%{release} # Needed by virt-pki-validate script. Requires: gnutls-utils - %if %{with_ssh_proxy} -Recommends: libvirt-ssh-proxy = %{version}-%{release} - %endif # Ensure smooth upgrades Obsoletes: libvirt-bash-completion < 7.3.0 -- 2.45.0
On Thu, May 16, 2024 at 10:24:19AM +0200, Andrea Bolognani wrote:
The ssh-proxy feature works independently of the clients, just like the NSS plugin does.
Moreover, ssh-proxy only works for local VMs, while clients are routinely used to manage remote hypervisors.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 3 --- 1 file changed, 3 deletions(-)
Yes, I guess this is ultimately pointless, as if the users picked the libvirt-daemon-{qemu,kvm} convenience RPMs, they'll have the SSH proxy installed regardless. If they didn't pick that and selected individual RPMs, without libvirt-ssh-proxy, then we should not be pulling it in anyway with the client. Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
diff --git a/libvirt.spec.in b/libvirt.spec.in index f7c128d809..5cb19fa433 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1025,9 +1025,6 @@ Summary: Client side utilities of the libvirt library Requires: libvirt-libs = %{version}-%{release} # Needed by virt-pki-validate script. Requires: gnutls-utils - %if %{with_ssh_proxy} -Recommends: libvirt-ssh-proxy = %{version}-%{release} - %endif
# Ensure smooth upgrades Obsoletes: libvirt-bash-completion < 7.3.0 -- 2.45.0
With 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 :|
The way things are implemented, installing the package not only makes the feature available but also enables it. Some admins might not want that to happen, so let's make the dependency a weak one to offer them a way out. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 5cb19fa433..329b923e8f 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -905,7 +905,7 @@ Requires: libvirt-daemon-driver-nwfilter = %{version}-%{release} Requires: libvirt-daemon-driver-secret = %{version}-%{release} Requires: libvirt-daemon-driver-storage = %{version}-%{release} %if %{with_ssh_proxy} -Requires: libvirt-ssh-proxy = %{version}-%{release} +Recommends: libvirt-ssh-proxy = %{version}-%{release} %endif Requires: qemu @@ -936,7 +936,7 @@ Requires: libvirt-daemon-driver-nwfilter = %{version}-%{release} Requires: libvirt-daemon-driver-secret = %{version}-%{release} Requires: libvirt-daemon-driver-storage = %{version}-%{release} %if %{with_ssh_proxy} -Requires: libvirt-ssh-proxy = %{version}-%{release} +Recommends: libvirt-ssh-proxy = %{version}-%{release} %endif Requires: qemu-kvm -- 2.45.0
On Thu, May 16, 2024 at 10:24:20AM +0200, Andrea Bolognani wrote:
The way things are implemented, installing the package not only makes the feature available but also enables it.
Some admins might not want that to happen, so let's make the dependency a weak one to offer them a way out.
The whole reason for existance of the 'libvirt-daemon-{kvm,qemu}' packages is to install the *entire* feature set relevant to QEMU/ KVM. If admins want to select individual components, then pick the actual sub-RPMs desire, and ignore libvirt-daemon{kvm,qemu} meta packages.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 5cb19fa433..329b923e8f 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -905,7 +905,7 @@ Requires: libvirt-daemon-driver-nwfilter = %{version}-%{release} Requires: libvirt-daemon-driver-secret = %{version}-%{release} Requires: libvirt-daemon-driver-storage = %{version}-%{release} %if %{with_ssh_proxy} -Requires: libvirt-ssh-proxy = %{version}-%{release} +Recommends: libvirt-ssh-proxy = %{version}-%{release} %endif Requires: qemu
@@ -936,7 +936,7 @@ Requires: libvirt-daemon-driver-nwfilter = %{version}-%{release} Requires: libvirt-daemon-driver-secret = %{version}-%{release} Requires: libvirt-daemon-driver-storage = %{version}-%{release} %if %{with_ssh_proxy} -Requires: libvirt-ssh-proxy = %{version}-%{release} +Recommends: libvirt-ssh-proxy = %{version}-%{release} %endif Requires: qemu-kvm
-- 2.45.0
With 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 :|
This way we can avoid repeating it twice. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 329b923e8f..0d6f15460d 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -810,6 +810,9 @@ Summary: QEMU driver plugin for the libvirtd daemon Requires: libvirt-daemon-common = %{version}-%{release} Requires: libvirt-daemon-log = %{version}-%{release} Requires: libvirt-libs = %{version}-%{release} + %if %{with_ssh_proxy} +Recommends: libvirt-ssh-proxy = %{version}-%{release} + %endif Requires: /usr/bin/qemu-img # For image compression Requires: gzip @@ -904,9 +907,6 @@ Requires: libvirt-daemon-driver-nodedev = %{version}-%{release} Requires: libvirt-daemon-driver-nwfilter = %{version}-%{release} Requires: libvirt-daemon-driver-secret = %{version}-%{release} Requires: libvirt-daemon-driver-storage = %{version}-%{release} - %if %{with_ssh_proxy} -Recommends: libvirt-ssh-proxy = %{version}-%{release} - %endif Requires: qemu %description daemon-qemu @@ -935,9 +935,6 @@ Requires: libvirt-daemon-driver-nodedev = %{version}-%{release} Requires: libvirt-daemon-driver-nwfilter = %{version}-%{release} Requires: libvirt-daemon-driver-secret = %{version}-%{release} Requires: libvirt-daemon-driver-storage = %{version}-%{release} - %if %{with_ssh_proxy} -Recommends: libvirt-ssh-proxy = %{version}-%{release} - %endif Requires: qemu-kvm %description daemon-kvm -- 2.45.0
On Thu, May 16, 2024 at 10:24:21AM +0200, Andrea Bolognani wrote:
This way we can avoid repeating it twice.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
This shouldn't be done. It will pull in the SSH proxy on every install of the 'libvirt-ademon-driver-qemu' RPM which is not desirable - this RPM is supposed to be providing the bare minimum install with no optional extras. With 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 :|
As a general rule, we use defines for features that can only be enabled on a subset of the platforms that we target, and we don't offer fine-grained control over every single possible meson configuration knob at the RPM level. In the case of ssh-proxy, we are enabling it everywhere already, so having a define for it is unnecessary. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 0d6f15460d..b6f9bf86f3 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -91,7 +91,6 @@ # Other optional features %define with_numactl 0%{!?_without_numactl:1} %define with_userfaultfd_sysctl 0%{!?_without_userfaultfd_sysctl:1} -%define with_ssh_proxy 0%{!?_without_ssh_proxy:1} # A few optional bits off by default, we enable later %define with_fuse 0 @@ -810,9 +809,7 @@ Summary: QEMU driver plugin for the libvirtd daemon Requires: libvirt-daemon-common = %{version}-%{release} Requires: libvirt-daemon-log = %{version}-%{release} Requires: libvirt-libs = %{version}-%{release} - %if %{with_ssh_proxy} Recommends: libvirt-ssh-proxy = %{version}-%{release} - %endif Requires: /usr/bin/qemu-img # For image compression Requires: gzip @@ -1104,14 +1101,12 @@ Requires: libvirt-daemon-driver-network = %{version}-%{release} Libvirt plugin for NSS for translating domain names into IP addresses. %endif -%if %{with_ssh_proxy} %package ssh-proxy Summary: Libvirt SSH proxy Requires: libvirt-libs = %{version}-%{release} %description ssh-proxy Allows SSH into domains via VSOCK without need for network. -%endif %if %{with_mingw32} %package -n mingw32-libvirt @@ -1304,12 +1299,6 @@ exit 1 %define arg_userfaultfd_sysctl -Duserfaultfd_sysctl=disabled %endif -%if %{with_ssh_proxy} - %define arg_ssh_proxy -Dssh_proxy=enabled -%else - %define arg_ssh_proxy -Dssh_proxy=disabled -%endif - %define when %(date +"%%F-%%T") %define where %(hostname) %define who %{?packager}%{!?packager:Unknown} @@ -1391,7 +1380,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/libvirt.spec) -Dtls_priority=%{tls_priority} \ -Dsysctl_config=enabled \ %{?arg_userfaultfd_sysctl} \ - %{?arg_ssh_proxy} \ + -Dssh_proxy=enabled \ %{?enable_werror} \ -Dexpensive_tests=enabled \ -Dinit_script=systemd \ @@ -2447,11 +2436,9 @@ exit 0 %{_libdir}/libnss_libvirt.so.2 %{_libdir}/libnss_libvirt_guest.so.2 - %if %{with_ssh_proxy} %files ssh-proxy %config(noreplace) %{_sysconfdir}/ssh/ssh_config.d/30-libvirt-ssh-proxy.conf %{_libexecdir}/libvirt-ssh-proxy - %endif %if %{with_lxc} %files login-shell -- 2.45.0
On Thu, May 16, 2024 at 10:24:22AM +0200, Andrea Bolognani wrote:
As a general rule, we use defines for features that can only be enabled on a subset of the platforms that we target, and we don't offer fine-grained control over every single possible meson configuration knob at the RPM level.
In the case of ssh-proxy, we are enabling it everywhere already, so having a define for it is unnecessary.
The only reason for a conditional would be if some older distro lacks support for this SSH proxy'ing feature. Assuming RHEL-9 / Ubuntu 22.04 have it, then this is indeed redundant
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With 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 :|
On Thu, May 16, 2024 at 10:03:02AM GMT, Daniel P. Berrangé wrote:
On Thu, May 16, 2024 at 10:24:22AM +0200, Andrea Bolognani wrote:
As a general rule, we use defines for features that can only be enabled on a subset of the platforms that we target, and we don't offer fine-grained control over every single possible meson configuration knob at the RPM level.
In the case of ssh-proxy, we are enabling it everywhere already, so having a define for it is unnecessary.
The only reason for a conditional would be if some older distro lacks support for this SSH proxy'ing feature. Assuming RHEL-9 / Ubuntu 22.04 have it, then this is indeed redundant
We don't need to worry about Ubuntu in the spec file ;) IIUC requirements are mostly on the guest OS side, and on the host OS side we just need the ssh ProxyCommand feature which would have been available since forever. Michal, can you please confirm that this is accurate? -- Andrea Bolognani / Red Hat / Virtualization
On Thu, May 16, 2024 at 02:23:13AM -0700, Andrea Bolognani wrote:
On Thu, May 16, 2024 at 10:03:02AM GMT, Daniel P. Berrangé wrote:
On Thu, May 16, 2024 at 10:24:22AM +0200, Andrea Bolognani wrote:
As a general rule, we use defines for features that can only be enabled on a subset of the platforms that we target, and we don't offer fine-grained control over every single possible meson configuration knob at the RPM level.
In the case of ssh-proxy, we are enabling it everywhere already, so having a define for it is unnecessary.
The only reason for a conditional would be if some older distro lacks support for this SSH proxy'ing feature. Assuming RHEL-9 / Ubuntu 22.04 have it, then this is indeed redundant
We don't need to worry about Ubuntu in the spec file ;)
Heh. Face palm.
IIUC requirements are mostly on the guest OS side, and on the host OS side we just need the ssh ProxyCommand feature which would have been available since forever. Michal, can you please confirm that this is accurate?
We need ProxyUseFdpass too, and I wasn't sure how far back that one went. With 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 :|
On Thu, May 16, 2024 at 10:26:28AM GMT, Daniel P. Berrangé wrote:
On Thu, May 16, 2024 at 02:23:13AM -0700, Andrea Bolognani wrote:
IIUC requirements are mostly on the guest OS side, and on the host OS side we just need the ssh ProxyCommand feature which would have been available since forever. Michal, can you please confirm that this is accurate?
We need ProxyUseFdpass too, and I wasn't sure how far back that one went.
According to [1] it was introduced in OpenSSH 6.5 (2014), so we should be good. [1] https://www.openssh.com/releasenotes.html -- Andrea Bolognani / Red Hat / Virtualization
On 5/16/24 11:23, Andrea Bolognani wrote:
On Thu, May 16, 2024 at 10:03:02AM GMT, Daniel P. Berrangé wrote:
On Thu, May 16, 2024 at 10:24:22AM +0200, Andrea Bolognani wrote:
As a general rule, we use defines for features that can only be enabled on a subset of the platforms that we target, and we don't offer fine-grained control over every single possible meson configuration knob at the RPM level.
In the case of ssh-proxy, we are enabling it everywhere already, so having a define for it is unnecessary.
The only reason for a conditional would be if some older distro lacks support for this SSH proxy'ing feature. Assuming RHEL-9 / Ubuntu 22.04 have it, then this is indeed redundant
We don't need to worry about Ubuntu in the spec file ;)
IIUC requirements are mostly on the guest OS side, and on the host OS side we just need the ssh ProxyCommand feature which would have been available since forever. Michal, can you please confirm that this is accurate?
Indeed. I just wanted to give distro maintainers ability to fine tune what features they enable. But I guess it makes sens to have this always enabled. Michal
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Prívozník