From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 3EC3C1FF16B for ; Fri, 7 Nov 2025 12:20:03 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8BDE3EC0C; Fri, 7 Nov 2025 12:20:44 +0100 (CET) Message-ID: <7a20645f-6a03-4484-9968-186bc556f4d9@proxmox.com> Date: Fri, 7 Nov 2025 12:20:40 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox Backup Server development discussion , Hannes Laimer References: <20250909085245.91641-1-h.laimer@proxmox.com> <20250909085245.91641-6-h.laimer@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20250909085245.91641-6-h.laimer@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762514421042 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.046 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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 2/3] traffic-control: handle users specified in a rule correctly 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: , Reply-To: Proxmox Backup Server development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On 9/9/25 10:53 AM, Hannes Laimer wrote: > Signed-off-by: Hannes Laimer > --- > src/bin/proxmox-backup-proxy.rs | 7 ++- > src/traffic_control_cache.rs | 100 +++++++++++++++++++++++++++----- > 2 files changed, 93 insertions(+), 14 deletions(-) > > diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs > index cfd93f92..c39e4587 100644 > --- a/src/bin/proxmox-backup-proxy.rs > +++ b/src/bin/proxmox-backup-proxy.rs > @@ -955,6 +955,7 @@ async fn run_traffic_control_updater() { > > fn lookup_rate_limiter( > peer: std::net::SocketAddr, > + user: Option, > ) -> (Option, Option) { > let mut cache = TRAFFIC_CONTROL_CACHE.lock().unwrap(); > > @@ -962,7 +963,11 @@ fn lookup_rate_limiter( > > cache.reload(now); > > - let (_rule_name, read_limiter, write_limiter) = cache.lookup_rate_limiter(peer, now); > + let authid = user.and_then(|s| s.parse::().ok()); nit: can be shortened by dropping pbs_api_types prefix, it is already imported above. > + let user_parsed = authid.as_ref().map(|auth_id| auth_id.user()); > + > + let (_rule_name, read_limiter, write_limiter) = > + cache.lookup_rate_limiter(peer, now, user_parsed); > > (read_limiter, write_limiter) > } > diff --git a/src/traffic_control_cache.rs b/src/traffic_control_cache.rs > index 830a8c04..0c2718d5 100644 > --- a/src/traffic_control_cache.rs > +++ b/src/traffic_control_cache.rs > @@ -13,7 +13,7 @@ use proxmox_section_config::SectionConfigData; > > use proxmox_time::{parse_daily_duration, DailyDuration, TmEditor}; > > -use pbs_api_types::TrafficControlRule; > +use pbs_api_types::{TrafficControlRule, Userid}; > > use pbs_config::ConfigVersionCache; > > @@ -322,6 +322,7 @@ impl TrafficControlCache { > /// > /// - Rules where timeframe does not match are skipped. > /// - Rules with smaller network size have higher priority. > + /// - Rules with users are prioritized over IP-only rules, if multiple match > /// > /// Behavior is undefined if more than one rule matches after > /// above selection. > @@ -329,10 +330,15 @@ impl TrafficControlCache { > &self, > peer: SocketAddr, > now: i64, > + user: Option<&Userid>, > ) -> (&str, Option, Option) { > let peer_ip = canonical_ip(peer.ip()); > > - log::debug!("lookup_rate_limiter: {:?}", peer_ip); > + log::debug!( > + "lookup_rate_limiter: {:?} - {}", > + peer_ip, > + user.map_or("(no user)", |u| u.as_str()) > + ); > > let now = match TmEditor::with_epoch(now, self.use_utc) { > Ok(now) => now, > @@ -342,19 +348,33 @@ impl TrafficControlCache { > } > }; > > - let mut last_rule_match = None; > + let mut last_rule_match: Option<(&ParsedTcRule, u8, bool)> = None; // (rule, netlen, is_user) this could get its own `type` definition, similar to e.g. what is done for the `RateLimiterCallback` and the corresponding docstring there. > > for rule in self.rules.iter() { > if !timeframe_match(&rule.timeframe, &now) { > continue; > } > > + if let Some(ref rule_users) = rule.config.users { > + if let Some(cur_user) = user { > + if !rule_users.iter().any(|u| u == cur_user) { > + continue; > + } > + } else { > + continue; > + } > + } > + > if let Some(match_len) = network_match_len(&rule.networks, &peer_ip) { > + let is_user_rule = rule.config.users.is_some(); > match last_rule_match { > - None => last_rule_match = Some((rule, match_len)), > - Some((_, last_len)) => { > - if match_len > last_len { > - last_rule_match = Some((rule, match_len)); > + None => last_rule_match = Some((rule, match_len, is_user_rule)), > + Some((_, last_len, last_is_user)) => { > + // Prefer rules with users over IP-only rules; for same class use longest prefix > + if (is_user_rule && !last_is_user) > + || (is_user_rule == last_is_user && match_len > last_len) > + { > + last_rule_match = Some((rule, match_len, is_user_rule)); > } > } > } > @@ -362,7 +382,7 @@ impl TrafficControlCache { > } > > match last_rule_match { > - Some((rule, _)) => { > + Some((rule, _, _)) => { > match self.limiter_map.get(&rule.config.name) { > Some((read_limiter, write_limiter)) => ( > &rule.config.name, > @@ -454,34 +474,88 @@ rule: somewhere > let somewhere = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(1, 2, 3, 4)), 1234); > > let (rule, read_limiter, write_limiter) = > - cache.lookup_rate_limiter(somewhere, THURSDAY_80_00); > + cache.lookup_rate_limiter(somewhere, THURSDAY_80_00, None); > assert_eq!(rule, "somewhere"); > assert!(read_limiter.is_some()); > assert!(write_limiter.is_some()); > > - let (rule, read_limiter, write_limiter) = cache.lookup_rate_limiter(local, THURSDAY_19_00); > + let (rule, read_limiter, write_limiter) = > + cache.lookup_rate_limiter(local, THURSDAY_19_00, None); > assert_eq!(rule, "rule2"); > assert!(read_limiter.is_some()); > assert!(write_limiter.is_some()); > > let (rule, read_limiter, write_limiter) = > - cache.lookup_rate_limiter(gateway, THURSDAY_15_00); > + cache.lookup_rate_limiter(gateway, THURSDAY_15_00, None); > assert_eq!(rule, "rule1"); > assert!(read_limiter.is_some()); > assert!(write_limiter.is_some()); > > let (rule, read_limiter, write_limiter) = > - cache.lookup_rate_limiter(gateway, THURSDAY_19_00); > + cache.lookup_rate_limiter(gateway, THURSDAY_19_00, None); > assert_eq!(rule, "somewhere"); > assert!(read_limiter.is_some()); > assert!(write_limiter.is_some()); > > let (rule, read_limiter, write_limiter) = > - cache.lookup_rate_limiter(private, THURSDAY_19_00); > + cache.lookup_rate_limiter(private, THURSDAY_19_00, None); > assert_eq!(rule, "rule2"); > assert!(read_limiter.is_some()); > assert!(write_limiter.is_some()); > > Ok(()) > } > + > + #[test] > + fn test_user_based_rule_match_and_precedence() -> Result<(), Error> { > + // rule user1: user-specific for alice@pam on 192.168.2.0/24 > + // rule ip1: ip-only broader network also matches, but user rule should win > + // rule user2: user-specific for bob@pam but different network shouldn't match > + let config_data = " > +rule: user1 > + comment user rule for alice > + network 192.168.2.0/24 > + rate-in 50000000 > + rate-out 50000000 > + users alice@pam > + > +rule: ip1 > + network 192.168.2.0/24 > + rate-in 100000000 > + rate-out 100000000 > + > +rule: user2 > + network 10.0.0.0/8 > + rate-in 75000000 > + rate-out 75000000 > + users bob@pam > +"; > + > + let config = pbs_config::traffic_control::CONFIG.parse("testconfig", config_data)?; > + > + let mut cache = TrafficControlCache::new(); > + cache.use_utc = true; > + cache.use_shared_memory = false; // avoid permission problems in test environment > + > + cache.update_config(&config)?; > + > + const NOW: i64 = make_test_time(0, 12, 0); > + let peer = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(192, 168, 2, 55)), 1234); > + > + // No user -> should match ip1 > + let (rule, _, _) = cache.lookup_rate_limiter(peer, NOW, None); > + assert_eq!(rule, "ip1"); > + > + // alice@pam -> should match user1 and take precedence over ip1 > + let alice = Userid::try_from("alice@pam".to_string())?; > + let (rule, _, _) = cache.lookup_rate_limiter(peer, NOW, Some(&alice)); > + assert_eq!(rule, "user1"); > + > + // bob@pam on same peer/network -> user2 is different network, should fall back to ip1 > + let bob = Userid::try_from("bob@pam".to_string())?; > + let (rule, _, _) = cache.lookup_rate_limiter(peer, NOW, Some(&bob)); > + assert_eq!(rule, "ip1"); > + > + Ok(()) > + } > } _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel