Re: [PATCH datacenter-manager v3 2/6] lib/api: add 'location-info' api call with cached information

["Shannon Sterz" <s.sterz@proxmox.com>]

On Fri May 22, 2026 at 10:34 AM CEST, Dominik Csapak wrote:
> PVE and PBS remotes can have a location configured, and we want to query
> that, so add a cache for this location info and an API call to query it.
>
> Modeled after the other resource api calls (status/subscription) but
> lets the location info be optional since this is a relatively new
> feature.
>
> The biggest difference is that we try to get a location at all costs, so
> if there is no new enough location in the cache we try to update it.
>
> If that fails (e.g. because the remote is offline) we retry the cache
> with unlimited 'max-age'. Otherwise there could be a situation where the
> location info is so old that offline remotes won't be shown.
>
> Refactor some permission checks from the subscription api call so we can
> reuse that.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  lib/pdm-api-types/src/lib.rs |  34 ++++++++
>  server/src/api/resources.rs  | 120 +++++++++++++++++++++-----
>  server/src/lib.rs            |   1 +
>  server/src/location_cache.rs | 160 +++++++++++++++++++++++++++++++++++
>  4 files changed, 294 insertions(+), 21 deletions(-)
>  create mode 100644 server/src/location_cache.rs

-->8 snip 8<--

> diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs
> index 1a6a23d9..9ec1ce89 100644
> --- a/server/src/api/resources.rs
> +++ b/server/src/api/resources.rs
> @@ -18,7 +18,7 @@ use pdm_api_types::resource::{
>  use pdm_api_types::subscription::{
>      NodeSubscriptionInfo, RemoteSubscriptionState, RemoteSubscriptions, SubscriptionLevel,
>  };
> -use pdm_api_types::{Authid, PRIV_RESOURCE_AUDIT, VIEW_ID_SCHEMA};
> +use pdm_api_types::{Authid, CachedLocationInfo, PRIV_RESOURCE_AUDIT, VIEW_ID_SCHEMA};
>  use pdm_search::{Search, SearchTerm};
>  use proxmox_access_control::CachedUserInfo;
>  use proxmox_router::{
> @@ -41,6 +41,10 @@ pub const ROUTER: Router = Router::new()
>  #[sortable]
>  const SUBDIRS: SubdirMap = &sorted!([
>      ("list", &Router::new().get(&API_METHOD_GET_RESOURCES)),
> +    (
> +        "location-info",
> +        &Router::new().get(&API_METHOD_GET_LOCATION_INFO)
> +    ),
>      ("status", &Router::new().get(&API_METHOD_GET_STATUS)),
>      (
>          "top-entities",
> @@ -214,6 +218,29 @@ impl From<RemoteWithResources> for RemoteResources {
>      }
>  }
>
> +fn check_remote_priv(user_info: &CachedUserInfo, auth_id: &Authid, remote: &str) -> bool {
> +    user_info
> +        .check_privs(auth_id, &["resource", remote], PRIV_RESOURCE_AUDIT, false)
> +        .is_ok()
> +}
> +
> +/// When returning true, all remotes are allowed and no per-remote permission check should be
> +/// necessary
> +fn check_all_remotes_allowed(
> +    user_info: &CachedUserInfo,
> +    auth_id: &Authid,
> +    view: Option<&str>,
> +) -> Result<bool, Error> {
> +    Ok(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()
> +    })
> +}
> +

i wonder if these two sould eventually also take a `privilege` parameter
and maybe be moved to a more central place, but nothing that needs to be
done with this series.

maybe even a helper that combines the `if` statements below as well, as
they are almost identical.

>  #[api(
>      // FIXME:: see list-like API calls in resource routers, we probably want more fine-grained
>      // checks..
> @@ -641,34 +668,16 @@ pub async fn get_subscription_status(
>          .parse()?;
>      let user_info = CachedUserInfo::new()?;
>
> -    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 allow_all = check_all_remotes_allowed(&user_info, &auth_id, view.as_deref())?;
>
>      let view = views::get_optional_view(view.as_deref())?;
>
> -    let check_priv = |remote_name: &str| -> bool {
> -        user_info
> -            .check_privs(
> -                &auth_id,
> -                &["resource", remote_name],
> -                PRIV_RESOURCE_AUDIT,
> -                false,
> -            )
> -            .is_ok()
> -    };
> -
>      for (remote_name, remote) in remotes_config {
>          if let Some(view) = &view {
>              if view.can_skip_remote(&remote_name) {
>                  continue;
>              }
> -        } else if !allow_all && !check_priv(&remote_name) {
> +        } else if !allow_all && !check_remote_priv(&user_info, &auth_id, &remote_name) {
>              continue;
>          }

^ these if statements here

-->8 snip 8<--