all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Dietmar Maurer <dietmar@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
	Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>
Subject: [pbs-devel] applied: [PATCH proxmox-backup] correctly set apt proxy configuration
Date: Wed, 12 May 2021 12:21:09 +0200	[thread overview]
Message-ID: <ecb1a9a4-0da4-a4fd-3e06-c34bf59cb1f3@proxmox.com> (raw)
In-Reply-To: <4a78fa42-02de-664f-90f6-eaa16fe9ea31@proxmox.com>

applied with suggested changes


On 5/12/21 12:01 PM, Thomas Lamprecht wrote:
> looks mostly good, a few comments in line.
>
> On 11.05.21 08:54, Dietmar Maurer wrote:
>> ---
>>   src/api2/node/apt.rs    | 41 ++++++++++++++++++++++++++++++++---------
>>   src/api2/node/config.rs |  9 ++++++++-
>>   src/config/node.rs      |  4 +++-
>>   src/tools/http.rs       | 18 +++++++++++++++---
>>   4 files changed, 58 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/api2/node/apt.rs b/src/api2/node/apt.rs
>> index 1e57ea8d..83ccc460 100644
>> --- a/src/api2/node/apt.rs
>> +++ b/src/api2/node/apt.rs
>> @@ -5,11 +5,11 @@ use std::collections::HashMap;
>>   use proxmox::list_subdirs_api_method;
>>   use proxmox::api::{api, RpcEnvironment, RpcEnvironmentType, Permission};
>>   use proxmox::api::router::{Router, SubdirMap};
>> +use proxmox::tools::fs::{replace_file, CreateOptions};
>>   
>>   use crate::config::node;
>>   use crate::server::WorkerTask;
>> -use crate::tools::{apt, SimpleHttp, subscription};
>> -
>> +use crate::tools::{apt, SimpleHttp, http::ProxyConfig, subscription};
>>   use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_SYS_MODIFY};
>>   use crate::api2::types::{Authid, APTUpdateInfo, NODE_SCHEMA, UPID_SCHEMA};
>>   
>> @@ -47,10 +47,36 @@ fn apt_update_available(_param: Value) -> Result<Value, Error> {
>>       Ok(json!(cache.package_status))
>>   }
>>   
>> +pub fn update_apt_proxy_config(proxy_config: Option<&ProxyConfig>) -> Result<(), Error> {
>> +
>> +    const PROXY_CFG_FN: &str = "/etc/apt/apt.conf.d/76pveproxy"; // use same file as PVE
>> +
>> +    if let Some(proxy_config) = proxy_config {
>> +        let proxy = proxy_config.to_proxy_string()?;
>> +        let data = format!("Acquire::http::Proxy \"{}\";\n", proxy);
>> +        replace_file(PROXY_CFG_FN, data.as_bytes(), CreateOptions::new())?;
>> +    } else {
>> +        let _ = std::fs::remove_file(PROXY_CFG_FN);
>
> I do not like such results thrown away, I mean it is unlikely, but if the removal fails the
> proxy would still be configured for apt, which may come as a surprise for an user.
>
> Can we do something like:
>
> std::fs::remove_file(PROXY_CFG_FN).map_err(|err| {
>      format_err!("failed to remove proxy config '{}' - {}", PROXY_CFG_FN, err)
> })?;
>
> here?
>
>> +    }
>> +
>> +    Ok(())
>> +}
>> +
>> +fn read_and_update_proxy_config() -> Result<Option<ProxyConfig>, Error> {
>> +    let proxy_config = if let Ok((node_config, _digest)) = node::config() {
>> +        node_config.http_proxy()
>> +    } else {
>> +        None
>> +    };
>> +    update_apt_proxy_config(proxy_config.as_ref())?;
>> +
>> +    Ok(proxy_config)
>> +}
>> +
>>   fn do_apt_update(worker: &WorkerTask, quiet: bool) -> Result<(), Error> {
>>       if !quiet { worker.log("starting apt-get update") }
>>   
>> -    // TODO: set proxy /etc/apt/apt.conf.d/76pbsproxy like PVE
>> +    read_and_update_proxy_config()?;
>>   
>>       let mut command = std::process::Command::new("apt-get");
>>       command.arg("update");
>> @@ -153,6 +179,7 @@ pub fn apt_update_database(
>>   }
>>   
>>   #[api(
>> +    protected: true,
>>       input: {
>>           properties: {
>>               node: {
>> @@ -195,12 +222,7 @@ fn apt_get_changelog(
>>           bail!("Package '{}' not found", name);
>>       }
>>   
>> -    let proxy_config = if let Ok((node_config, _digest)) = node::config() {
>> -        node_config.http_proxy()
>> -    } else {
>> -        None
>> -    };
>> -
>> +    let proxy_config = read_and_update_proxy_config()?;
>>       let mut client = SimpleHttp::new(proxy_config);
>>   
>>       let changelog_url = &pkg_info[0].change_log_url;
>> @@ -235,6 +257,7 @@ fn apt_get_changelog(
>>           Ok(json!(changelog))
>>   
>>       } else {
>> +        eprintln!("TZEST2");
>
> please remove the left over debug print above
>
>
>>           let mut command = std::process::Command::new("apt-get");
>>           command.arg("changelog");
>>           command.arg("-qq"); // don't display download progress
>> diff --git a/src/api2/node/config.rs b/src/api2/node/config.rs
>> index 92c0100e..290e9284 100644
>> --- a/src/api2/node/config.rs
>> +++ b/src/api2/node/config.rs
>> @@ -4,6 +4,7 @@ use proxmox::api::schema::Updatable;
>>   use proxmox::api::{api, Permission, Router, RpcEnvironment};
>>   
>>   use crate::api2::types::NODE_SCHEMA;
>> +use crate::api2::node::apt::update_apt_proxy_config;
>>   use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_SYS_MODIFY};
>>   use crate::config::node::{NodeConfig, NodeConfigUpdater};
>>   
>> @@ -78,5 +79,11 @@ pub fn update_node_config(
>>   
>>       config.update_from(updater, &delete)?;
>>   
>> -    crate::config::node::save_config(&config)
>> +    let http_proxy = config.http_proxy();
>> +
>> +    crate::config::node::save_config(&config)?;
>> +
>> +    update_apt_proxy_config(http_proxy.as_ref())?;
> would be enough and IMO still readable if we'd just that in one line here:
>
> update_apt_proxy_config(config.http_proxy().as_ref())?;
>
> but no hard feelings here
>
>> +
>> +    Ok(())
>>   }
>> diff --git a/src/config/node.rs b/src/config/node.rs
>> index 31a2c5ab..33f3e2ad 100644
>> --- a/src/config/node.rs
>> +++ b/src/config/node.rs
>> @@ -151,6 +151,7 @@ impl NodeConfig {
>>           AcmeDomainIter::new(self)
>>       }
>>   
>> +    /// Returns the parsed ProxyConfig
>>       pub fn http_proxy(&self) -> Option<ProxyConfig> {
>>           if let Some(http_proxy) = &self.http_proxy {
>>               match ProxyConfig::parse_proxy_url(&http_proxy) {
>> @@ -162,7 +163,8 @@ impl NodeConfig {
>>           }
>>       }
>>   
>> -    pub fn set_proxy(&mut self, http_proxy: Option<String>) {
>> +    /// Sets the HTTP proxy configuration
>> +    pub fn set_http_proxy(&mut self, http_proxy: Option<String>) {
>>           self.http_proxy = http_proxy;
>>       }
>>   
>> diff --git a/src/tools/http.rs b/src/tools/http.rs
>> index 998e7647..0821992a 100644
>> --- a/src/tools/http.rs
>> +++ b/src/tools/http.rs
>> @@ -43,7 +43,7 @@ pub(crate) fn build_authority(host: &str, port: u16) -> Result<Authority, Error>
>>   pub struct ProxyConfig {
>>       pub host: String,
>>       pub port: u16,
>> -    pub authorization: Option<String>, // Proxy-Authorization header value
>> +    pub authorization: Option<String>, // user:pass
>>       pub force_connect: bool,
>>   }
>>   
>> @@ -94,7 +94,7 @@ impl ProxyConfig {
>>   
>>               let authority_vec: Vec<&str> = proxy_authority.as_str().rsplitn(2, '@').collect();
>>               let authorization = if authority_vec.len() == 2 {
>> -                Some(format!("Basic {}", base64::encode(authority_vec[1])))
>> +                Some(authority_vec[1].to_string())
> It seems like all changes to this file and config/node.rs could haven been its own patch,
> as they are not directly related to setting the apt proxy, but I'm fine with it as is too.
>
>>               } else {
>>                   None
>>               };
>> @@ -107,6 +107,15 @@ impl ProxyConfig {
>>               })
>>           }).map_err(|err| format_err!("parse_proxy_url failed: {}", err))
>>       }
>> +
>> +    /// Assemble canonical proxy string (including scheme and port)
>> +    pub fn to_proxy_string(&self) -> Result<String, Error> {
>> +        let authority = build_authority(&self.host, self.port)?;
>> +        Ok(match self.authorization {
>> +            None => format!("http://{}", authority),
>> +            Some(ref authorization) => format!("http://{}@{}", authorization, authority)
>> +        })
>> +    }
>>   }
>>   
>>   #[derive(Clone)]
>> @@ -241,7 +250,10 @@ impl hyper::service::Service<Uri> for HttpsConnector {
>>   
>>                       let mut connect_request = format!("CONNECT {0}:{1} HTTP/1.1\r\n", host, port);
>>                       if let Some(authorization) = authorization {
>> -                        connect_request.push_str(&format!("Proxy-Authorization: {}\r\n", authorization));
>> +                        connect_request.push_str(&format!(
>> +                            "Proxy-Authorization: Basic {}\r\n",
>> +                            base64::encode(authorization),
>> +                        ));
>>                       }
>>                       connect_request.push_str(&format!("Host: {0}:{1}\r\n\r\n", host, port));
>>   
>>




      reply	other threads:[~2021-05-12 10:21 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-11  6:54 [pbs-devel] " Dietmar Maurer
2021-05-12 10:01 ` Thomas Lamprecht
2021-05-12 10:21   ` Dietmar Maurer [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ecb1a9a4-0da4-a4fd-3e06-c34bf59cb1f3@proxmox.com \
    --to=dietmar@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=t.lamprecht@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal