From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 9CF7793327 for ; Fri, 16 Sep 2022 10:17:08 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9406D21550 for ; Fri, 16 Sep 2022 10:17:08 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 16 Sep 2022 10:17:07 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id BCC5244474 for ; Fri, 16 Sep 2022 10:17:07 +0200 (CEST) Message-ID: <1d9bffa3-950b-360a-e472-8633fad2b49e@proxmox.com> Date: Fri, 16 Sep 2022 10:17:06 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0 Content-Language: en-US To: Thomas Lamprecht , Proxmox Backup Server development discussion References: <20220915140857.1041222-1-s.hanreich@proxmox.com> <6d281db9-90e8-9164-6979-4b73b04cc627@proxmox.com> <7ecef7a9-afda-f0fd-55d1-924d4819745b@proxmox.com> <5c890689-bd43-1057-6ecf-62c8e615a176@proxmox.com> From: Stefan Hanreich In-Reply-To: <5c890689-bd43-1057-6ecf-62c8e615a176@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.637 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -1.816 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup] fix #4095: make http client read proxy config from envvars X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Sep 2022 08:17:08 -0000 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.