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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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..


> 
>>> +        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.