[PATCH pve-mamager v4 0/3] fix #8031: Write APT proxy config on http_proxy change

[PATCH pve-mamager v4 0/3] fix #8031: Write APT proxy config on http_proxy change

[Jonas Theisen]

I messed up the versions of the sent patches, sorry!
This was supposed to be v4 from the start and does not differ
from this v4:
https://lore.proxmox.com/pve-devel/20261012144603.288276-1-j.theisen@proxmox.com/
This was and is v3:
https://lore.proxmox.com/pve-devel/20260915091758.85522-1-j.theisen@proxmox.com/

Goal of this patch series is to update the http_proxy config
across the whole cluster immideately after a config change
and independently of the use of PVE tooling to trigger
a database update.

To allow reuse of the same code this patch series factors out
the relevant function inside the APT part of the API and reuses
the function on change of the http_proxy variable in the Cluster API.

This version also moves from trying to update the local file across
all nodes after a change to updating one file in the pmxcfs and
pointing all nodes to that file via a symlink.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=8031

Thanks for the reviews of v3 @e.huhsovitz and @m.sandoval

Changes from v3:
 * added symlink logic to update call
 * adapted update function to only write on change
 * moved update into datacenter.cfg lock phase

Jonas Theisen (3):
  api: apt: factor out writing the APT proxy config
  api: apt: move proxy config to pmxcfs and symlink to it
  fix #8031: api: cluster options: update APT proxy config on change

 PVE/API2/APT.pm     | 48 ++++++++++++++++++++++++++++++++++++---------
 PVE/API2/Cluster.pm | 12 ++++++++++++
 2 files changed, 51 insertions(+), 9 deletions(-)

-- 
2.47.3

[PATCH pve-mamager v4 1/3] api: apt: factor out writing the APT proxy config

[Jonas Theisen]

Move writing the APT proxy config out of the update_database
endpoint into its own function, so that it can also be called when
the datacenter http_proxy option is changed.

The function takes the already parsed datacenter config, so callers
holding the datacenter.cfg lock can pass their updated copy.

Signed-off-by: Jonas Theisen <j.theisen@proxmox.com>
---
 PVE/API2/APT.pm | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
index 93562b06..8e16b813 100644
--- a/PVE/API2/APT.pm
+++ b/PVE/API2/APT.pm
@@ -290,6 +290,19 @@ __PACKAGE__->register_method({
     },
 });
 
+sub update_apt_proxy_config {
+    my ($dcconf) = @_;
+
+    my $aptconf = "// no proxy configured\n";
+
+    if (my $http_proxy = $dcconf->{http_proxy}) {
+        my $http_proxy_uri = URI->new($http_proxy);
+        $aptconf = "Acquire::http::Proxy \"${http_proxy_uri}\";\n";
+    }
+    my $aptcfn = "/etc/apt/apt.conf.d/76pveproxy";
+    PVE::Tools::file_set_contents($aptcfn, $aptconf);
+}
+
 __PACKAGE__->register_method({
     name => 'update_database',
     path => 'update',
@@ -334,15 +347,7 @@ __PACKAGE__->register_method({
         my $realcmd = sub {
             my $upid = shift;
 
-            # setup proxy for apt
-
-            my $aptconf = "// no proxy configured\n";
-            if (my $http_proxy = $dcconf->{http_proxy}) {
-                my $http_proxy_uri = URI->new($http_proxy);
-                $aptconf = "Acquire::http::Proxy \"${http_proxy_uri}\";\n";
-            }
-            my $aptcfn = "/etc/apt/apt.conf.d/76pveproxy";
-            PVE::Tools::file_set_contents($aptcfn, $aptconf);
+            update_apt_proxy_config($dcconf);
 
             my $cmd = ['apt-get', 'update'];
 
-- 
2.47.3

[PATCH pve-mamager v4 2/3] api: apt: move proxy config to pmxcfs and symlink to it

[Jonas Theisen]

The APT proxy config was written to /etc/apt/apt.conf.d/76pveproxy
only if a database update was triggered through the PVE tooling.

Write the config to /etc/pve/apt-proxy.conf instead and replace the
local file with a symlink to it, so that changes are applied
cluster-wide.

The symlink is auto-generated on the next database update and checks
for existance of the target file before creating it.

The config is also only updated if it differs from the current
to avoid unncessarry cluster traffic.

Updateing the database without write access to /etc/pve
still works and only issues a warning.

Note that files in /etc/pve are not readable for users outside the
www-data group, so APT invoked by such users will fail to read the
proxy config.

Signed-off-by: Jonas Theisen <j.theisen@proxmox.com>
---
 PVE/API2/APT.pm | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
index 8e16b813..2359a5f5 100644
--- a/PVE/API2/APT.pm
+++ b/PVE/API2/APT.pm
@@ -290,6 +290,24 @@ __PACKAGE__->register_method({
     },
 });
 
+my $apt_proxy_cfn = "/etc/apt/apt.conf.d/76pveproxy";
+my $pve_apt_proxy_cfn = "/etc/pve/apt-proxy.conf";
+
+my $setup_apt_proxy_symlink = sub {
+    my $target = readlink($apt_proxy_cfn);
+    return if defined($target) && $target eq $pve_apt_proxy_cfn;
+
+    my $tmpname = "$apt_proxy_cfn.tmp.$$";
+    unlink($tmpname);
+    symlink($pve_apt_proxy_cfn, $tmpname)
+        || die "unable to create symlink '$tmpname' - $!\n";
+    if (!rename($tmpname, $apt_proxy_cfn)) {
+        my $err = $!;
+        unlink($tmpname);
+        die "unable to replace '$apt_proxy_cfn' - $err\n";
+    }
+};
+
 sub update_apt_proxy_config {
     my ($dcconf) = @_;
 
@@ -299,8 +317,14 @@ sub update_apt_proxy_config {
         my $http_proxy_uri = URI->new($http_proxy);
         $aptconf = "Acquire::http::Proxy \"${http_proxy_uri}\";\n";
     }
-    my $aptcfn = "/etc/apt/apt.conf.d/76pveproxy";
-    PVE::Tools::file_set_contents($aptcfn, $aptconf);
+
+    my $current;
+    $current = PVE::Tools::file_get_contents($pve_apt_proxy_cfn) if -f $pve_apt_proxy_cfn;
+    if (!defined($current) || $current ne $aptconf) {
+        PVE::Tools::file_set_contents($pve_apt_proxy_cfn, $aptconf);
+    }
+
+    $setup_apt_proxy_symlink->();
 }
 
 __PACKAGE__->register_method({
@@ -347,7 +371,8 @@ __PACKAGE__->register_method({
         my $realcmd = sub {
             my $upid = shift;
 
-            update_apt_proxy_config($dcconf);
+            eval { update_apt_proxy_config($dcconf) };
+            warn "unable to update APT proxy config - $@" if $@;
 
             my $cmd = ['apt-get', 'update'];
 
-- 
2.47.3

Re: [PATCH pve-mamager v4 2/3] api: apt: move proxy config to pmxcfs and symlink to it

[Thomas Lamprecht]

Am 12.10.26 um 16:57 schrieb Jonas Theisen:
> The APT proxy config was written to /etc/apt/apt.conf.d/76pveproxy
> only if a database update was triggered through the PVE tooling.

But your change here still needs such a refresh (or after the next
patch a change in proyy value) at least once per node, this is not
activated immediately automatically.

> Write the config to /etc/pve/apt-proxy.conf instead and replace the
> local file with a symlink to it, so that changes are applied
> cluster-wide.
> 
> The symlink is auto-generated on the next database update and checks
> for existance of the target file before creating it.

typo: s/existance/existence/

> 
> The config is also only updated if it differs from the current
> to avoid unncessarry cluster traffic.

typo: s/unncessarry/unnecessary/

> 
> Updateing the database without write access to /etc/pve

typo: s/Updateing/Updating/

> still works and only issues a warning.

But not if pmxcfs fails to start, which can make disaster recovery
harder. I mean, sure if pmxcfs fails to start completely it's gonna
be annoying either way, but still not a good excuse to add more
coupling to core system configs here.

> 
> Note that files in /etc/pve are not readable for users outside the
> www-data group, so APT invoked by such users will fail to read the
> proxy config.

But that's breaking existing unprivileged apt use cases then. Also
cases where some nodes use a different proxy than others, bit of an
odd edge case, but not nice to break, at least not as normal update
during a stable release series.

I'd rather skip this approach, at least in current form. A hook for
apt that checks configured and setup proxy and warns on mismatch
might be nicer here? If we really want to auto-apply it - which can
be probably OK, I just did not got around to think it fully through
- that hook could also do that and print a nice visible log line for
when something was changed, making the admin updating aware of it.

> 
> Signed-off-by: Jonas Theisen <j.theisen@proxmox.com>
> ---
>  PVE/API2/APT.pm | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
> index 8e16b813..2359a5f5 100644
> --- a/PVE/API2/APT.pm
> +++ b/PVE/API2/APT.pm
> @@ -290,6 +290,24 @@ __PACKAGE__->register_method({
>      },
>  });
>  
> +my $apt_proxy_cfn = "/etc/apt/apt.conf.d/76pveproxy";
> +my $pve_apt_proxy_cfn = "/etc/pve/apt-proxy.conf";
> +
> +my $setup_apt_proxy_symlink = sub {
> +    my $target = readlink($apt_proxy_cfn);
> +    return if defined($target) && $target eq $pve_apt_proxy_cfn;
> +
> +    my $tmpname = "$apt_proxy_cfn.tmp.$$";
> +    unlink($tmpname);
> +    symlink($pve_apt_proxy_cfn, $tmpname)
> +        || die "unable to create symlink '$tmpname' - $!\n";
> +    if (!rename($tmpname, $apt_proxy_cfn)) {
> +        my $err = $!;
> +        unlink($tmpname);
> +        die "unable to replace '$apt_proxy_cfn' - $err\n";
> +    }
> +};
> +
>  sub update_apt_proxy_config {
>      my ($dcconf) = @_;
>  
> @@ -299,8 +317,14 @@ sub update_apt_proxy_config {
>          my $http_proxy_uri = URI->new($http_proxy);
>          $aptconf = "Acquire::http::Proxy \"${http_proxy_uri}\";\n";
>      }
> -    my $aptcfn = "/etc/apt/apt.conf.d/76pveproxy";
> -    PVE::Tools::file_set_contents($aptcfn, $aptconf);
> +
> +    my $current;
> +    $current = PVE::Tools::file_get_contents($pve_apt_proxy_cfn) if -f $pve_apt_proxy_cfn;
> +    if (!defined($current) || $current ne $aptconf) {
> +        PVE::Tools::file_set_contents($pve_apt_proxy_cfn, $aptconf);
> +    }
> +
> +    $setup_apt_proxy_symlink->();
>  }
>  
>  __PACKAGE__->register_method({
> @@ -347,7 +371,8 @@ __PACKAGE__->register_method({
>          my $realcmd = sub {
>              my $upid = shift;
>  
> -            update_apt_proxy_config($dcconf);
> +            eval { update_apt_proxy_config($dcconf) };
> +            warn "unable to update APT proxy config - $@" if $@;
>  
>              my $cmd = ['apt-get', 'update'];
>  

[PATCH pve-mamager v4 3/3] fix #8031: api: cluster options: update APT proxy config on change

[Jonas Theisen]

Changing the http_proxy datacenter option did not update the APT
proxy config until the next package database update. Write it right
away, so APT on all nodes uses the new proxy immediately.

Do so while still holding the datacenter.cfg lock, so concurrent
changes cannot write the APT config in a different order than the
datacenter config.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=8031
Signed-off-by: Jonas Theisen <j.theisen@proxmox.com>
---
 PVE/API2/Cluster.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
index 4e5efbfd..29820c59 100644
--- a/PVE/API2/Cluster.pm
+++ b/PVE/API2/Cluster.pm
@@ -23,6 +23,7 @@ use PVE::Tools qw(extract_param);
 
 use PVE::API2::ACMEAccount;
 use PVE::API2::ACMEPlugin;
+use PVE::API2::APT;
 use PVE::API2::Backup;
 use PVE::API2::Cluster::BackupInfo;
 use PVE::API2::Cluster::BulkAction;
@@ -842,8 +843,17 @@ __PACKAGE__->register_method({
     code => sub {
         my ($param) = @_;
 
+        my $http_proxy_change = 0;
+
         my $delete = extract_param($param, 'delete');
 
+        if (
+            defined($param->{http_proxy})
+            || (defined($delete) && $delete =~ m/\bhttp_proxy\b/)
+        ) {
+            $http_proxy_change = 1;
+        }
+
         cfs_lock_file(
             'datacenter.cfg',
             undef,
@@ -855,6 +865,8 @@ __PACKAGE__->register_method({
                 delete $conf->{$_} for PVE::Tools::split_list($delete);
 
                 cfs_write_file('datacenter.cfg', $conf);
+
+                PVE::API2::APT::update_apt_proxy_config($conf) if $http_proxy_change;
             },
         );
         die $@ if $@;
-- 
2.47.3