Re: [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars

[Stefan Hanreich <s.hanreich@proxmox.com>]

On 9/16/22 10:09, Thomas Lamprecht wrote:
> Am 16/09/2022 um 09:08 schrieb Stefan Hanreich:
>> On 9/16/22 08:58, Thomas Lamprecht wrote:
>>> Am 15/09/2022 um 16:08 schrieb Stefan Hanreich:
>>>> In order to be able to use a proxy with the proxmox-backup-client, use
>>>> ProxyConfig for parsing proxy server config from the environment. Also
>>>> added a section in the documentation that describes how to configure the
>>>> environment if a proxy server should be used.
>>>
>>> Proxy config was more intended for the server, not the client side(s), and IMO
>>> proxy's never have any use outside of central surveillance of (tls) traffic, but
>>> well, we already got it and some user may want it, so can be fine, IMO close
>>> to a breaking change though, would at least require entries in all product's
>>> "noteable changes" section of their next point release.
>>
>> Yes, it seemed like some users desperately wanted it, judging from the bugzilla issue as well as the forum.
> 
> Sounds still like a x-y problem to me, e.g., the reporter doesn't want
> a proxy, they want a way to bridge isolated networks to certain services
> (apt, pbs, ...), but anyway, it's to small of a change to not do compared
> to debating that with users semi-frequently ;-)
> 
> 
>>> so, if I get this right this test requires the following in the build env:
>>> * needs a proxy to run on 8080
>>> * needs PBS running
>>> * needs to build/tetst as root
>>>
>>> All three are a complete no-go, makes bootstrapping harder and is just a flaky
>>> test all together, I'd just drop it completely... Anyway, NAK until fixed.
>>>
>>
>> I don't think any of them are required actually, since it is just checking whether it can instantiate the HttpClient. It never makes any request to the outside. I ran this test as non-root on my local machine without PBS nor proxy runnning and it runs just fine.
> 
> But the HttpClient::new code clearly spawns a login future due to the auth not being a API
> token, so your test just seems brittle and you may got lucky with timing (i.e., future
> didn't schedule until test exited already), seems a recipe for flaky tests to me...
> 
> Also, note that you do not test the client's proxy capabilities, but only what ProxyConfig
> accepts, and that should go into the ProxyConfig implementation, if we really want to have
> that. Besides belonging there it can be also much simpler/less complex to test..

Yes, I was also not very happy with how I had to test it. If i really 
wanted to test it properly I'd add tests to the ProxyConfig as well as 
the HttpsConnector, but that seemed a bit out of scope for this patch 
series, which is why I resorted to this (seemingly) basic test.

It was more of a basic sanity check to see if I didn't break some stuff.
Do you think it would make sense to create a separate patch series that 
unit tests ProxyConfig with some (im)possible proxy settings?

I guess I'll just remove the tests for now.

> 
>>
>>>> +        env::set_var("ALL_PROXY", "https://localhost:8080");
>>>> +
>>>> +        let mut http_client = HttpClient::new(
> 
> fyi, it would be more idiomatic rust to avoid the `mut` here and
> just always do a `let http_client = ...` for subsequent declarations
> too.
> 

I'll keep it in mind.