[pve-devel] [PATCH container] fix #3367: skip bind mounts when converting to template

[pve-devel] [PATCH container] fix #3367: skip bind mounts when converting to template

[Fabian Ebner]

Restoring a backup of a template with a bind mount point failed, because the
volume_has_feature check would fail.

Fix this by skipping bind mounts on template creation. There is no check against
adding a bind mount with 'pct set' *after* converting to a template, so it
shouldn't be an issue if we also allow conversion if bind mounts are present *at*
conversion time.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/LXC.pm | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 6395d12..5a9f76d 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1186,6 +1186,8 @@ sub template_create {
     PVE::LXC::Config->foreach_volume($conf, sub {
 	my ($ms, $mountpoint) = @_;
 
+	return if $mountpoint->{type} eq 'bind';
+
 	my $volid = $mountpoint->{volume};
 
 	die "Template feature is not available for '$volid'\n"
@@ -1195,6 +1197,8 @@ sub template_create {
     PVE::LXC::Config->foreach_volume($conf, sub {
 	my ($ms, $mountpoint) = @_;
 
+	return if $mountpoint->{type} eq 'bind';
+
 	my $volid = $mountpoint->{volume};
 
 	PVE::Storage::activate_volumes($storecfg, [$volid]);
-- 
2.20.1

Re: [pve-devel] [PATCH container] fix #3367: skip bind mounts when converting to template

[Fabian Ebner]

It turns out that we do not yet allow cloning from container templates 
with bind mounts. So in a sense container templates with bind mounts are 
currently misconfigured, and this patch would make it easier to get there...

Should I send a v2 with a patch making cloning from such templates 
possible, or were there some concerns to not allow it in the first 
place? There is a
     # TODO: allow bind mounts?
comment in the clone API call.

Am 02.04.21 um 14:36 schrieb Fabian Ebner:
> Restoring a backup of a template with a bind mount point failed, because the
> volume_has_feature check would fail.
> 
> Fix this by skipping bind mounts on template creation. There is no check against
> adding a bind mount with 'pct set' *after* converting to a template, so it
> shouldn't be an issue if we also allow conversion if bind mounts are present *at*
> conversion time.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>   src/PVE/LXC.pm | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 6395d12..5a9f76d 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1186,6 +1186,8 @@ sub template_create {
>       PVE::LXC::Config->foreach_volume($conf, sub {
>   	my ($ms, $mountpoint) = @_;
>   
> +	return if $mountpoint->{type} eq 'bind';
> +
>   	my $volid = $mountpoint->{volume};
>   
>   	die "Template feature is not available for '$volid'\n"
> @@ -1195,6 +1197,8 @@ sub template_create {
>       PVE::LXC::Config->foreach_volume($conf, sub {
>   	my ($ms, $mountpoint) = @_;
>   
> +	return if $mountpoint->{type} eq 'bind';
> +
>   	my $volid = $mountpoint->{volume};
>   
>   	PVE::Storage::activate_volumes($storecfg, [$volid]);
> 

Re: [pve-devel] [PATCH container] fix #3367: skip bind mounts when converting to template

[Wolfgang Bumiller]

On Tue, Apr 06, 2021 at 08:26:50AM +0200, Fabian Ebner wrote:
> It turns out that we do not yet allow cloning from container templates with
> bind mounts. So in a sense container templates with bind mounts are
> currently misconfigured, and this patch would make it easier to get there...
> 
> Should I send a v2 with a patch making cloning from such templates possible,
> or were there some concerns to not allow it in the first place? There is a
>     # TODO: allow bind mounts?
> comment in the clone API call.

It's mostly that bind mounts are generally root-only.

Silently dropping them when converting to a template seems awkward, I'd
rather have this throw an error, too.

(Also remember that containers don't need to be templates to be cloned.)

Re: [pve-devel] [PATCH container] fix #3367: skip bind mounts when converting to template

[Fabian Grünbichler]

On June 21, 2021 10:41 am, Wolfgang Bumiller wrote:
> On Tue, Apr 06, 2021 at 08:26:50AM +0200, Fabian Ebner wrote:
>> It turns out that we do not yet allow cloning from container templates with
>> bind mounts. So in a sense container templates with bind mounts are
>> currently misconfigured, and this patch would make it easier to get there...
>> 
>> Should I send a v2 with a patch making cloning from such templates possible,
>> or were there some concerns to not allow it in the first place? There is a
>>     # TODO: allow bind mounts?
>> comment in the clone API call.
> 
> It's mostly that bind mounts are generally root-only.

maybe we should re-visit the idea of "admin-defined bind mounts" (or 
rather, "admin-defined bind mount SOURCES") that have ACLs, so that we 
can make them more accessible to regular users..

> Silently dropping them when converting to a template seems awkward, I'd
> rather have this throw an error, too.
> 
> (Also remember that containers don't need to be templates to be cloned.)