Re: [pdm-devel] [PATCH datacenter-manager v2 09/12] api: subscription status: add support for view-filter parameter

["Lukas Wagner" <l.wagner@proxmox.com>]

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.

>> +                    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