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 0AC721FF183 for ; Wed, 5 Nov 2025 15:28:19 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 63AAC274A9; Wed, 5 Nov 2025 15:28:59 +0100 (CET) Message-ID: Date: Wed, 5 Nov 2025 15:28:55 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Lukas Wagner , Proxmox Datacenter Manager development discussion References: <20251103123521.266258-1-l.wagner@proxmox.com> <20251103123521.266258-10-l.wagner@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762352917546 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.027 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: [pdm-devel] [PATCH datacenter-manager v2 09/12] 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 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" On 11/5/25 3:11 PM, Lukas Wagner wrote: > On Wed Nov 5, 2025 at 11:08 AM CET, Dominik Csapak wrote: >>> @@ -590,35 +605,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(); >> >> this could just be named 'view_filter' too, no need to postfix it with >> '_clone' > > Ack - will do for v3. > >> >>> + >>> 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() && view_filter_clone.is_some() { >>> + // Don't leak the existence of failed remotes, since we cannot apply >>> + // view-filters here. >> >> why not? we can check if the remote should be included, that does not >> requires any more info? >> >> > > The problem is that we cannot always reliably tell if a remote is > included or not without checking the remote's resources. There is > ViewFilter::can_skip_remote - but since that one only checks > `include/exclude remote:...` rules, it might return false (meaning, > the remote must be considered) even if at the end the data from this > remote might be filtered out due to the other rules (and if no resources > remain, the remote as a whole would then be filtered out from the > response - similar to what I did in patch 6 to which you replied). > > Example: > > Consider only having `include tag:sometag` in the config. From this > alone, we cannot exclude any remotes from being queried a priori and > must do *all* filtering when we already have the data. > > Similar thing here. If we cannot reach the remote, then we don't have a > list of nodes for this remote. Without the list of nodes, we cannot do > any checks with the node resource. This means if we do not filter out > the failed remote, we would leak its existence in a view which > potentially would not match on any of the node's resources. > > I hope you could follow this explanation. Do you maybe have an idea on > how to fix this? > > Only thing that comes to mind is making the include/exclude > remote:{remote} rules required, so that a view *always* needs to specify > which remotes to (not) consider. If we make these required, we probably > should make these a separate key in the config file again. > ok i understand the issue, but instead of excluding all remotes that have errors, we could at least include those that we know should be included by the 'remote' filter? IMHO a remote is already a resource, so if i include some remote with it's name (or by type, etc.), even if it does not contain any sub resources, i should get back an (empty) remote does that make sense? i get we can't include/exclude based on other filters though, that must remain hidden as you mentioned, but for explicitly included remotes we should include them also with errors. >>> + return None; >>> + } >>> >>> - 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