Re: [pdm-devel] [PATCH datacenter-manager v3 08/11] api: subscription status: add support for view parameter

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

On Wed Nov 12, 2025 at 9:19 AM CET, Shannon Sterz wrote:
> On Tue Nov 11, 2025 at 3:46 PM CET, Michael Köppl wrote:
>> 1 comment inline
>>
>> On Thu Nov 6, 2025 at 2:43 PM CET, Lukas Wagner wrote:
>>> A view allows one to get filtered subset of all resources, based on
>>> filter rules defined in a config file. Views integrate with the
>>> permission system - if a user has permissions on /view/{view-id}, then
>>> these privileges are transitively applied to all resources which are
>>> matched by the rules. All other permission checks are replaced if
>>> requesting data through a view.
>>>
>>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>>> ---
>>>
>>> Notes:
>>>     Changes since v2:
>>>       - make sure to not filter out a remote if it has been explicitly
>>>         included
>>>
>>>  server/src/api/resources.rs | 66 ++++++++++++++++++++++++++++++-------
>>>  1 file changed, 54 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs
>>> index 143335fe..2930da8b 100644
>>> --- a/server/src/api/resources.rs
>>> +++ b/server/src/api/resources.rs
>>> @@ -552,6 +552,10 @@ pub async fn get_status(
>>>                  default: false,
>>>                  description: "If true, includes subscription information per node (with enough privileges)",
>>>              },
>>> +            view: {
>>> +                schema: VIEW_ID_SCHEMA,
>>> +                optional: true,
>>> +            },
>>>          },
>>>      },
>>>      returns: {
>>> @@ -566,6 +570,7 @@ pub async fn get_status(
>>>  pub async fn get_subscription_status(
>>>      max_age: u64,
>>>      verbose: bool,
>>> +    view: Option<String>,
>>>      rpcenv: &mut dyn RpcEnvironment,
>>>  ) -> Result<Vec<RemoteSubscriptions>, Error> {
>>>      let (remotes_config, _) = pdm_config::remotes::config()?;
>>> @@ -574,9 +579,17 @@ pub async fn get_subscription_status(
>>>
>>>      let auth_id = rpcenv.get_auth_id().unwrap().parse()?;
>>>      let user_info = CachedUserInfo::new()?;
>>> -    let allow_all = user_info
>>> -        .check_privs(&auth_id, &["resource"], PRIV_RESOURCE_AUDIT, false)
>>> -        .is_ok();
>>> +
>>> +    let allow_all = if let Some(view) = &view {
>>> +        user_info.check_privs(&auth_id, &["view", view], PRIV_RESOURCE_AUDIT, false)?;
>>> +        false
>>> +    } else {
>>> +        user_info
>>> +            .check_privs(&auth_id, &["resource"], PRIV_RESOURCE_AUDIT, false)
>>> +            .is_ok()
>>> +    };
>>> +
>>> +    let view = views::get_optional_view(view.as_deref())?;
>>>
>>>      let check_priv = |remote_name: &str| -> bool {
>>>          user_info
>>> @@ -590,35 +603,64 @@ pub async fn get_subscription_status(
>>>      };
>>>
>>>      for (remote_name, remote) in remotes_config {
>>> -        if !allow_all && !check_priv(&remote_name) {
>>> +        if let Some(view) = &view {
>>> +            if view.can_skip_remote(&remote_name) {
>>> +                continue;
>>> +            }
>>> +        } else if !allow_all && !check_priv(&remote_name) {
>>>              continue;
>>>          }
>>>
>>> +        let view = view.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(view) = &view {
>>> +                                view.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 let Some(view) = view {
>>> +                    if error.is_some() && !view.is_remote_explicitly_included(&remote.id) {
>>
>> Can this ever be be true? From the code above, you can never have the
>> case that node_status and error are both Some(...) at the same time, so
>> error.is_some() would always evaluate to false here, unless I'm missing
>> something.
>>
>> I think you'd have to put the check outside of this block.
>>
>>     if let Some(view) = &view {
>>         if error.is_some() && !view.is_remote_explicitly_included(&remote.id) {
>>             return None;
>>         }
>>     }
>>
>>     let state = if let Some(node_status) = &node_status {
>>         ...
>>     }
>>
>
> i thikn this is right, at least i noted something similar in a previous
> version of this patch series already. thanks for spotting it again!
>
> https://lore.proxmox.com/all/DDVMQ0KGW0IP.50W15XZS8TQ@proxmox.com/
>

Ah, my bad. I remember reading your comment about this patch, but I
think it was overshadowed by some other changes, including the
`is_remote_explicitly_included` thing.

I moved the block as suggested.

Thanks :)

>>> +                        // Don't leak the existence of failed remotes unless they were explicitly
>>> +                        // pulled in by a `include remote:<id>` rule.
>>> +                        return None;
>>> +                    }
>>> +                }


_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel