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 AADF369174 for ; Mon, 22 Mar 2021 09:39:48 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A0DC920E59 for ; Mon, 22 Mar 2021 09:39:48 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (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 id 0556920E4F for ; Mon, 22 Mar 2021 09:39:48 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id BB2D34283F for ; Mon, 22 Mar 2021 09:39:47 +0100 (CET) Date: Mon, 22 Mar 2021 09:39:39 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Dylan Whyte , Proxmox Backup Server development discussion References: <20210319133503.5398-1-d.whyte@proxmox.com> In-Reply-To: MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1616402168.dbhrg0pqgc.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.027 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust 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 #3296: allow set subscription through proxy 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: Mon, 22 Mar 2021 08:39:48 -0000 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. >=20 > first thanks for sending a patch for this important featire. >=20 > A few high level comments/issues I see: > * this now uses proxies for all current and future uses of the tools::htt= p::post > function, but not the other http request helpers from that tool, IMO we= ird and > possible unexpected >=20 > * In Proxmox VE and Proxmox Mail Gateway we have a datacenter/admin confi= g 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 daemo= ns not > manually but under systemd supervision? >=20 > * In PVE and PMG we also use the proxy configuration for writing out an A= PT config > on the apt api upgrade path >=20 > So, what would be nice to have is: >=20 > * A config similar to PVE/PMG; we wanted to add a PBS wide node config an= yway for > setting things like FQDN, email sender and now the proxy could fit in t= here too. >=20 > * Don't just auto-magically use some env variable in a single http reques= t helper, > but make it more explicit, from top of my head that could be: > - add a Option to get/post function which some value is= used over > the static HTTP_CLIENT > - add a separate post_proxied method >=20 > 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 ht= tp proxy) >=20 > * 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 compl= ete >=20 >>=20 >> Signed-off-by: Dylan Whyte >> --- >>=20 >> * required packages can be found in nasi/iso/packages/hyper-proxy >>=20 >> 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. >>=20 >> Cargo.toml | 3 ++- >> src/tools/http.rs | 30 +++++++++++++++++++++++++++--- >> 2 files changed, 29 insertions(+), 4 deletions(-) >>=20 >> diff --git a/Cargo.toml b/Cargo.toml >> index 9483831c..5a8bcc81 100644 >> --- a/Cargo.toml >> +++ b/Cargo.toml >> @@ -24,7 +24,7 @@ path =3D "src/lib.rs" >> =20 >> [dependencies] >> apt-pkg-native =3D "0.3.2" >> -base64 =3D "0.12" >> +base64 =3D "0.13" >=20 > why does this get upgraded? chiming in since I did the packaging.. hyper-proxy requires it (transitively). it's a drop-in update which=20 we'll need to do at some point anyway, and seems easier to go in the=20 upwards direction than patching stuff to use older deps. but=20 alternatively, it should be possible to go down that route as well if=20 prefered ;) =