From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 86D5B1FF15C for ; Fri, 31 Oct 2025 11:38:54 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 300B0EDAC; Fri, 31 Oct 2025 11:39:28 +0100 (CET) Mime-Version: 1.0 Date: Fri, 31 Oct 2025 11:38:54 +0100 Message-Id: From: "Lukas Wagner" To: "Shannon Sterz" , "Lukas Wagner" X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20251029144902.446852-1-l.wagner@proxmox.com> <20251029144902.446852-11-l.wagner@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1761907119898 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.028 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pdm-devel] [PATCH datacenter-manager 10/13] api: subscription status: add support for view-filter parameter X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Cc: Proxmox Datacenter Manager development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" On Thu Oct 30, 2025 at 12:44 PM CET, Shannon Sterz wrote: >> @@ -560,6 +564,7 @@ pub async fn get_status( >> pub async fn get_subscription_status( >> max_age: u64, >> verbose: bool, >> + view_filter: Option, >> rpcenv: &mut dyn RpcEnvironment, >> ) -> Result, Error> { >> let (remotes_config, _) = pdm_config::remotes::config()?; >> @@ -572,6 +577,14 @@ pub async fn get_subscription_status( >> .check_privs(&auth_id, &["resources"], PRIV_RESOURCE_AUDIT, false) >> .is_ok(); >> >> + if let Some(view_filter) = &view_filter { >> + user_info.check_privs(&auth_id, &["view", view_filter], PRIV_RESOURCE_AUDIT, false)?; >> + } > > this is minor, but maybe consider moving the view check before the > `allow_all` check and skipping the `allow_all` check (by setting it to > false) if a view was found. that should safe us an unecessary traversal > of the acl tree here. > > i think wolfgang already noted that tho. > ack, will be fixed for the next iteration. >> + >> + let view_filter = view_filter >> + .map(|filter_name| views::view_filter::get_view_filter(&filter_name)) >> + .transpose()?; >> + >> let check_priv = |remote_name: &str| -> bool { >> user_info >> .check_privs( >> @@ -584,35 +597,62 @@ pub async fn get_subscription_status( >> }; >> >> for (remote_name, remote) in remotes_config { >> - if !allow_all && !check_priv(&remote_name) { >> + if let Some(filter) = &view_filter { >> + if filter.can_skip_remote(&remote_name) { >> + continue; >> + } >> + } else if !allow_all && !check_priv(&remote_name) { >> continue; >> } >> >> + let view_filter_clone = view_filter.clone(); >> + >> let future = async move { >> let (node_status, error) = >> match get_subscription_info_for_remote(&remote, max_age).await { >> - Ok(node_status) => (Some(node_status), None), >> + Ok(mut node_status) => { >> + node_status.retain(|node, _| { >> + if let Some(filter) = &view_filter_clone { >> + filter.is_node_included(&remote.id, node) >> + } else { >> + true >> + } >> + }); >> + (Some(node_status), None) >> + } >> Err(error) => (None, Some(error.to_string())), >> }; >> >> - let mut state = RemoteSubscriptionState::Unknown; >> + let state = if let Some(node_status) = &node_status { >> + if error.is_some() { >> + // Don't leak the existence of failed remotes, since we cannot apply >> + // view-filters here. >> + return None; > > shouldn't this be gated by checking if view_filters was defined? > otherwise we now return nothing every time we get an error? > > also correct me if i'm wrong, but isn't this `return None;` unreachable? > error is assigned `Some` iff the `get_subscription_status` call above > returns an `Err`, but in that case `node_status` is set to `None`, so > the `if let Some(node_status) = &node_status` above should be false? > Excellent catch! You are absolutely right, I think I can just drop the if-clause completely. Thanks a lot! >> + } >> >> - if let Some(node_status) = &node_status { >> - state = map_node_subscription_list_to_state(node_status); >> - } >> + if node_status.is_empty() { >> + return None; >> + } >> >> - RemoteSubscriptions { >> + map_node_subscription_list_to_state(node_status) >> + } else { >> + RemoteSubscriptionState::Unknown >> + }; >> + >> + Some(RemoteSubscriptions { >> remote: remote_name, >> error, >> state, >> node_status: if verbose { node_status } else { None }, >> - } >> + }) >> }; >> >> futures.push(future); >> } >> >> - Ok(join_all(futures).await) >> + let status = join_all(futures).await.into_iter().flatten().collect(); >> + >> + Ok(status) >> } >> >> // FIXME: make timeframe and count parameters? _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel