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

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

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<String>,
>>      rpcenv: &mut dyn RpcEnvironment,
>>  ) -> Result<Vec<RemoteSubscriptions>, 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