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

[Thomas Lamprecht]

On 22.03.21 09:39, Fabian Grünbichler wrote:
> 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 at 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 ;)
> 

no it can be fine, but some mentioning of the new requirement would be nice ;-)
For what I can know this could come from some direct use of crates.io or what not,
so noting it specifically helps to understand that this is actually required, and
not an accident.