Re: [pbs-devel] [PATCH proxmox-backup] fix #3296: allow set subscription through proxy

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On March 19, 2021 4:32 pm, Thomas Lamprecht wrote:
> On 19.03.21 14:35, Dylan Whyte wrote:
>> when setting a subscription key, use http(s)_proxy as tunnel if
>> evironment variable is set.
> 
> first thanks for sending a patch for this important featire.
> 
> A few high level comments/issues I see:
> * this now uses proxies for all current and future uses of the tools::http::post
>   function, but not the other http request helpers from that tool, IMO weird and
>   possible unexpected
> 
> * In Proxmox VE and Proxmox Mail Gateway we have a datacenter/admin config for
>   the http(s) proxy, and do not rely on the environment variables - which required
>   a reload or restart of the PBS daemon(s) to get applied, also not sure how
>   systemd handles the http_one, as it may clear up env quite a bit and we do not set
>   an EnvironmentFile by default. Did you test this when running the daemons not
>   manually but under systemd supervision?
> 
> * In PVE and PMG we also use the proxy configuration for writing out an APT config
>   on the apt api upgrade path
> 
> So, what would be nice to have is:
> 
> * A config similar to PVE/PMG; we wanted to add a PBS wide node config anyway for
>   setting things like FQDN, email sender and now the proxy could fit in there too.
> 
> * Don't just auto-magically use some env variable in a single http request helper,
>   but make it more explicit, from top of my head that could be:
>   - add a Option<ProxyConnector> to get/post function which some value is used over
>     the static HTTP_CLIENT
>   - add a separate post_proxied method
> 
>   In any case, the "get the ProxyConnector" part may be nicer to live in its own
>   method (possibly with getting the node config and checking it for an http proxy)
> 
> * another patch handling the apt proxy auth, like we do in PVE/PMG; that can be
>   future stuff, but is required to make the proxy handling somewhat complete
> 
>> 
>> Signed-off-by: Dylan Whyte <d.whyte@proxmox.com>
>> ---
>> 
>> * required packages can be found in nasi/iso/packages/hyper-proxy
>> 
>> Note that proxy authorization/authentication is not implemented yet.
>> hyper-proxy implements it using the 'headers' crate, which we do
>> not have as a direct dependency. I figured i'd leave it for a
>> follow up patch, just in case we decide not to use hyper-proxy afterall.
>> 
>>  Cargo.toml        |  3 ++-
>>  src/tools/http.rs | 30 +++++++++++++++++++++++++++---
>>  2 files changed, 29 insertions(+), 4 deletions(-)
>> 
>> diff --git a/Cargo.toml b/Cargo.toml
>> index 9483831c..5a8bcc81 100644
>> --- a/Cargo.toml
>> +++ b/Cargo.toml
>> @@ -24,7 +24,7 @@ path = "src/lib.rs"
>>  
>>  [dependencies]
>>  apt-pkg-native = "0.3.2"
>> -base64 = "0.12"
>> +base64 = "0.13"
> 
> why does this get upgraded?

chiming in since I did the packaging..

hyper-proxy requires it (transitively). it's a drop-in update which 
we'll need to do at some point anyway, and seems easier to go in the 
upwards direction than patching stuff to use older deps. but 
alternatively, it should be possible to go down that route as well if 
prefered ;)