[pve-devel] [PATCH v3 container 0/2] fix #3443: unique machine-id for containers

[pve-devel] [PATCH v3 container 0/2] fix #3443: unique machine-id for containers

[Oguz Bektas]

v2->v3:
* clear machine-id at the end of clone task worker

Oguz Bektas (2):
  setup: clear /etc/machine-id for newly created containers
  clear machine-id also after container clone

 src/PVE/API2/LXC.pm       |  6 ++++++
 src/PVE/LXC/Setup.pm      | 10 ++++++++++
 src/PVE/LXC/Setup/Base.pm | 25 +++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

-- 
2.20.1

[pve-devel] [PATCH v3 container 1/2] setup: clear /etc/machine-id for newly created containers

[Oguz Bektas]

this way when new containers are created they will have a unique
/etc/machine-id

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v3:
no changes


 src/PVE/LXC/Setup.pm      | 10 ++++++++++
 src/PVE/LXC/Setup/Base.pm | 25 +++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
index 8b8fee9..c31a164 100644
--- a/src/PVE/LXC/Setup.pm
+++ b/src/PVE/LXC/Setup.pm
@@ -352,6 +352,16 @@ sub pre_start_hook {
     $self->protected_call($code);
 }
 
+sub clear_machine_id {
+    my ($self, $conf, $clone) = @_;
+
+    my $code = sub {
+	$self->{plugin}->clear_machine_id($self->{conf}, $clone);
+    };
+    $self->protected_call($code);
+
+}
+
 sub post_create_hook {
     my ($self, $root_password, $ssh_keys) = @_;
 
diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
index d73335b..21074b7 100644
--- a/src/PVE/LXC/Setup/Base.pm
+++ b/src/PVE/LXC/Setup/Base.pm
@@ -476,6 +476,30 @@ sub set_timezone {
     }
 }
 
+sub clear_machine_id {
+    my ($self, $conf, $clone) = @_;
+
+    my $uses_systemd = $self->ct_is_executable("/lib/systemd/systemd")
+	|| $self->ct_is_executable("/usr/lib/systemd/systemd");
+
+    my $dbus_machine_id_path = "/var/lib/dbus/machine-id";
+    my $machine_id_path = "/etc/machine-id";
+    if (
+	$self->ct_file_exists($dbus_machine_id_path)
+	&& !$self->ct_is_symlink($dbus_machine_id_path)
+	&& $uses_systemd
+    ) {
+        $self->ct_unlink($dbus_machine_id_path);
+    }
+
+    # don't remove file if container is being cloned
+    if ($clone) {
+	$self->ct_file_set_contents($machine_id_path, "\n");
+    } else {
+	$self->ct_unlink($machine_id_path);
+    }
+}
+
 sub pre_start_hook {
     my ($self, $conf) = @_;
 
@@ -491,6 +515,7 @@ sub pre_start_hook {
 sub post_create_hook {
     my ($self, $conf, $root_password, $ssh_keys) = @_;
 
+    $self->clear_machine_id($conf);
     $self->template_fixup($conf);
 
     &$randomize_crontab($self, $conf);
-- 
2.20.1

[pve-devel] [PATCH v3 container 2/2] clear machine-id also after container clone

[Oguz Bektas]

pass $clone=1 to avoid removing the file. instead we truncate it to an
empty file

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v2->v3:
* clear machine-id at the end of clone task worker

 src/PVE/API2/LXC.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index a9ea3a6..d5c12dc 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1590,6 +1590,12 @@ __PACKAGE__->register_method({
 		die "clone failed: $err";
 	    }
 
+	    my $lastconf = PVE::LXC::Config->load_config($newid);
+	    my $rootdir = PVE::LXC::mount_all($newid, $storecfg, $lastconf, 1);
+	    my $lxc_setup = PVE::LXC::Setup->new($lastconf, $rootdir);
+	    $lxc_setup->clear_machine_id($lastconf, 1);
+	    PVE::LXC::umount_all($newid, $storecfg, $lastconf, 1);
+
 	    return;
 	};
 
-- 
2.20.1

Re: [pve-devel] [PATCH v3 container 1/2] setup: clear /etc/machine-id for newly created containers

[Fabian Grünbichler]

On May 27, 2021 11:26 am, Oguz Bektas wrote:
> this way when new containers are created they will have a unique
> /etc/machine-id
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> v3:
> no changes
> 
> 
>  src/PVE/LXC/Setup.pm      | 10 ++++++++++
>  src/PVE/LXC/Setup/Base.pm | 25 +++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
> index 8b8fee9..c31a164 100644
> --- a/src/PVE/LXC/Setup.pm
> +++ b/src/PVE/LXC/Setup.pm
> @@ -352,6 +352,16 @@ sub pre_start_hook {
>      $self->protected_call($code);
>  }
>  
> +sub clear_machine_id {
> +    my ($self, $conf, $clone) = @_;
> +
> +    my $code = sub {
> +	$self->{plugin}->clear_machine_id($self->{conf}, $clone);
> +    };
> +    $self->protected_call($code);
> +
> +}

maybe it would make more sense to call this "post_clone_hook", so it is 
re-usable for other, similar changes (like optionally regenerating SSH 
keys, or ...) in the future without polluting the entry-point namespace 
too much?

> +
>  sub post_create_hook {
>      my ($self, $root_password, $ssh_keys) = @_;
>  
> diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
> index d73335b..21074b7 100644
> --- a/src/PVE/LXC/Setup/Base.pm
> +++ b/src/PVE/LXC/Setup/Base.pm
> @@ -476,6 +476,30 @@ sub set_timezone {
>      }
>  }
>  
> +sub clear_machine_id {
> +    my ($self, $conf, $clone) = @_;
> +
> +    my $uses_systemd = $self->ct_is_executable("/lib/systemd/systemd")
> +	|| $self->ct_is_executable("/usr/lib/systemd/systemd");
> +
> +    my $dbus_machine_id_path = "/var/lib/dbus/machine-id";
> +    my $machine_id_path = "/etc/machine-id";
> +    if (
> +	$self->ct_file_exists($dbus_machine_id_path)
> +	&& !$self->ct_is_symlink($dbus_machine_id_path)
> +	&& $uses_systemd
> +    ) {
> +        $self->ct_unlink($dbus_machine_id_path);
> +    }
> +
> +    # don't remove file if container is being cloned
> +    if ($clone) {
> +	$self->ct_file_set_contents($machine_id_path, "\n");
> +    } else {
> +	$self->ct_unlink($machine_id_path);
> +    }
> +}
> +
>  sub pre_start_hook {
>      my ($self, $conf) = @_;
>  
> @@ -491,6 +515,7 @@ sub pre_start_hook {
>  sub post_create_hook {
>      my ($self, $conf, $root_password, $ssh_keys) = @_;
>  
> +    $self->clear_machine_id($conf);
>      $self->template_fixup($conf);
>  
>      &$randomize_crontab($self, $conf);
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v3 container 2/2] clear machine-id also after container clone

[Fabian Grünbichler]

On May 27, 2021 11:26 am, Oguz Bektas wrote:
> pass $clone=1 to avoid removing the file. instead we truncate it to an
> empty file
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> v2->v3:
> * clear machine-id at the end of clone task worker
> 
>  src/PVE/API2/LXC.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index a9ea3a6..d5c12dc 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -1590,6 +1590,12 @@ __PACKAGE__->register_method({
>  		die "clone failed: $err";
>  	    }
>  
> +	    my $lastconf = PVE::LXC::Config->load_config($newid);
> +	    my $rootdir = PVE::LXC::mount_all($newid, $storecfg, $lastconf, 1);
> +	    my $lxc_setup = PVE::LXC::Setup->new($lastconf, $rootdir);
> +	    $lxc_setup->clear_machine_id($lastconf, 1);
> +	    PVE::LXC::umount_all($newid, $storecfg, $lastconf, 1);

this is dangerous - the config is not locked anymore at this point, and 
we don't hold any other lock either!

another unrelated thing I just noticed - in case the clone fails, we 
don't remove a potentially cloned firewall config..

> +
>  	    return;
>  	};
>  
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v3 container 1/2] setup: clear /etc/machine-id for newly created containers

[Oguz Bektas]

hi,

> +sub clear_machine_id {
> > +    my ($self, $conf, $clone) = @_;
> > +
> > +    my $code = sub {
> > +	$self->{plugin}->clear_machine_id($self->{conf}, $clone);
> > +    };
> > +    $self->protected_call($code);
> > +
> > +}
> 
> maybe it would make more sense to call this "post_clone_hook", so it is 
> re-usable for other, similar changes (like optionally regenerating SSH 
> keys, or ...) in the future without polluting the entry-point namespace 
> too much?

yes i was also thinking why we don't have a post_clone_hook :D

i'll change that accordingly :)

> 
> > +
> >  sub post_create_hook {
> >      my ($self, $root_password, $ssh_keys) = @_;
> >  
> > diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
> > index d73335b..21074b7 100644
> > --- a/src/PVE/LXC/Setup/Base.pm
> > +++ b/src/PVE/LXC/Setup/Base.pm
> > @@ -476,6 +476,30 @@ sub set_timezone {
> >      }
> >  }