[pdm-devel] [PATCH datacenter-manager 2/3] views: add 'include-all' param; change semantics when there are no includes
Dominik Csapak
d.csapak at proxmox.com
Wed Nov 26 15:25:45 CET 2025
wouldn't it be less work if this patch (and maybe the first one too)
would be ordered after the tests refactoring?
just saying this because you have to modify the tests here, which
promptly is irrelevant in the next patch
no hard feelings from my side though
additional comment inline
On 11/17/25 3:11 PM, Lukas Wagner wrote:
> Previously, having no 'include' rules would mean that *all* resources
> are included. Due view permissions being transitively applied to all
> included resources, this also has some security consequences.
> For instance, if there is only a single include rule which is then
> removed by accident, suddenly any AuthID having privileges on the view
> ACL object would then be granted these privileges on *all* resources.
>
> To somewhat mitigate this, the semantics are slightly changed. If there
> are no include rules, then no resources will be contained in the view.
> A new 'include-all' parameter is added, which aims to emphasize the
> intent of *really* including everything.
>
> Suggested-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> Signed-off-by: Lukas Wagner <l.wagner at proxmox.com>
> ---
> lib/pdm-api-types/src/views.rs | 4 +++
> server/src/views/mod.rs | 60 +++++++++++++++++++---------------
> server/src/views/tests.rs | 35 ++++++++++++++++++--
> 3 files changed, 69 insertions(+), 30 deletions(-)
>
> diff --git a/lib/pdm-api-types/src/views.rs b/lib/pdm-api-types/src/views.rs
> index 950a3a04..82ab8781 100644
> --- a/lib/pdm-api-types/src/views.rs
> +++ b/lib/pdm-api-types/src/views.rs
> @@ -58,6 +58,9 @@ pub struct ViewConfig {
> #[updater(skip)]
> pub id: String,
>
> + /// Include all resources by default.
> + pub include_all: Option<bool>,
this probably needs at least
#[updater(serde(skip_serializing_if = "Option::is_none"))]
?
maybe also a serde skip_serializing?
> +
> /// List of includes.
> #[serde(default, skip_serializing_if = "Vec::is_empty")]
> #[updater(serde(skip_serializing_if = "Option::is_none"))]
> @@ -261,6 +264,7 @@ mod test {
> fn config_smoke_test() {
> let config = "
> view: some-view
> + include-all true
> include exact:remote=someremote
> include remote=someremote
> include resource-type=qemu
> diff --git a/server/src/views/mod.rs b/server/src/views/mod.rs
> index 8b4c70a4..3669ace4 100644
> --- a/server/src/views/mod.rs
> +++ b/server/src/views/mod.rs
> @@ -64,31 +64,34 @@ impl View {
> /// When there are `include remote:<...>` or `exclude remote:<...>` rules, we can use these to
> /// check if a remote needs to be considered at all.
> pub fn can_skip_remote(&self, remote: &str) -> bool {
> - let mut has_any_include_remote = false;
> - let mut matches_any_include_remote = false;
> -
> - let mut any_other = false;
> -
> - for include in &self.config.include {
> - if let FilterRule::Remote(r) = include {
> - has_any_include_remote = true;
> - if r.matches(remote) {
> - matches_any_include_remote = true;
> - break;
> - }
> - } else {
> - any_other = true;
> - }
> - }
> -
> let matches_any_exclude_remote = self
> .config
> .exclude
> .iter()
> .any(|rule| Self::matches_remote_rule(remote, rule));
>
> - (has_any_include_remote && !matches_any_include_remote && !any_other)
> - || matches_any_exclude_remote
> + if matches_any_exclude_remote {
> + return true;
> + }
> +
> + if self.config.include_all.unwrap_or_default() {
> + return false;
> + }
> +
> + for include in &self.config.include {
> + if let FilterRule::Remote(r) = include {
> + if r.matches(remote) {
> + return false;
> + }
> + } else {
> + // If there is any other type of rule, we cannot safely infer whether we can skip
> + // the remote (e.g. for 'tag' matches, we have to check *all* remotes for resources
> + // with a given tag)
> + return false;
> + }
> + }
> +
> + true
> }
>
> /// Check if a remote is *explicitly* included (and not excluded).
> @@ -96,18 +99,22 @@ impl View {
> /// A subset of the resources of a remote might still be pulled in by other rules,
> /// but this function check if the remote as a whole is matched.
> pub fn is_remote_explicitly_included(&self, remote: &str) -> bool {
> - let matches_include_remote = self
> - .config
> - .include
> - .iter()
> - .any(|rule| Self::matches_remote_rule(remote, rule));
> + let included = if self.config.include_all.unwrap_or_default() {
> + true
> + } else {
> + self.config
> + .include
> + .iter()
> + .any(|rule| Self::matches_remote_rule(remote, rule))
> + };
> +
> let matches_exclude_remote = self
> .config
> .exclude
> .iter()
> .any(|rule| Self::matches_remote_rule(remote, rule));
>
> - matches_include_remote && !matches_exclude_remote
> + included && !matches_exclude_remote
> }
>
> /// Check if a node is matched by the filter rules.
> @@ -131,8 +138,7 @@ impl View {
> }
>
> fn check_if_included(&self, remote: &str, resource: &ResourceData) -> bool {
> - if self.config.include.is_empty() {
> - // If there are no include rules, any resource is included (unless excluded)
> + if self.config.include_all.unwrap_or_default() {
> return true;
> }
>
> diff --git a/server/src/views/tests.rs b/server/src/views/tests.rs
> index 14d94ac9..0d83ae70 100644
> --- a/server/src/views/tests.rs
> +++ b/server/src/views/tests.rs
> @@ -135,6 +135,7 @@ fn exclude_remotes() {
> FilterRule::Remote(StringMatcher::Exact("remote-a".into())),
> FilterRule::Remote(StringMatcher::Exact("remote-b".into())),
> ],
> + include_all: Some(true),
> ..Default::default()
> };
>
> @@ -184,6 +185,8 @@ fn include_exclude_remotes() {
> FilterRule::Remote(StringMatcher::Exact("remote-b".into())),
> FilterRule::Remote(StringMatcher::Exact("remote-c".into())),
> ],
> +
> + ..Default::default()
> };
> run_test(
> config.clone(),
> @@ -224,6 +227,7 @@ fn include_exclude_remotes() {
> fn empty_config() {
> let config = ViewConfig {
> id: "empty".into(),
> + include_all: Some(true),
> ..Default::default()
> };
> run_test(
> @@ -301,6 +305,7 @@ fn exclude_type() {
> FilterRule::ResourceType(EnumMatcher(ResourceType::PveStorage)),
> FilterRule::ResourceType(EnumMatcher(ResourceType::PveQemu)),
> ],
> + include_all: Some(true),
> ..Default::default()
> },
> &[
> @@ -329,6 +334,7 @@ fn include_exclude_type() {
> exclude: vec![FilterRule::ResourceType(EnumMatcher(
> ResourceType::PveStorage,
> ))],
> + ..Default::default()
> },
> &[
> (
> @@ -357,6 +363,7 @@ fn include_exclude_tags() {
> FilterRule::Tag(StringMatcher::Exact("tag2".to_string())),
> ],
> exclude: vec![FilterRule::Tag(StringMatcher::Exact("tag3".to_string()))],
> + ..Default::default()
> },
> &[
> (
> @@ -404,6 +411,7 @@ fn include_exclude_resource_pool() {
> exclude: vec![FilterRule::ResourcePool(StringMatcher::Exact(
> "pool2".to_string(),
> ))],
> + ..Default::default()
> },
> &[
> (
> @@ -459,6 +467,7 @@ fn include_exclude_resource_id() {
> "remote/{REMOTE}/storage/{NODE}/otherstorage"
> ))),
> ],
> + ..Default::default()
> },
> &[
> (
> @@ -509,6 +518,7 @@ fn node_included() {
> exclude: vec![FilterRule::Remote(StringMatcher::Exact(
> "remote-b".to_string(),
> ))],
> + ..Default::default()
> });
>
> assert!(view.is_node_included("remote-a", "somenode"));
> @@ -528,6 +538,7 @@ fn can_skip_remote_if_excluded() {
> exclude: vec![FilterRule::Remote(StringMatcher::Exact(
> "remote-b".to_string(),
> ))],
> + include_all: Some(true),
> });
>
> assert!(!view.can_skip_remote("remote-a"));
> @@ -542,6 +553,7 @@ fn can_skip_remote_if_included() {
> "remote-b".to_string(),
> ))],
> exclude: vec![],
> + ..Default::default()
> });
>
> assert!(!view.can_skip_remote("remote-b"));
> @@ -559,6 +571,7 @@ fn can_skip_remote_cannot_skip_if_any_other_include() {
> )),
> ],
> exclude: vec![],
> + ..Default::default()
> });
>
> assert!(!view.can_skip_remote("remote-b"));
> @@ -575,6 +588,7 @@ fn can_skip_remote_explicit_remote_exclude() {
> exclude: vec![FilterRule::Remote(StringMatcher::Exact(
> "remote-a".to_string(),
> ))],
> + ..Default::default()
> });
>
> assert!(view.can_skip_remote("remote-a"));
> @@ -584,8 +598,19 @@ fn can_skip_remote_explicit_remote_exclude() {
> fn can_skip_remote_with_empty_config() {
> let view = View::new(ViewConfig {
> id: "abc".into(),
> - include: vec![],
> - exclude: vec![],
> + ..Default::default()
> + });
> +
> + assert!(view.can_skip_remote("remote-a"));
> + assert!(view.can_skip_remote("remote-b"));
> +}
> +
> +#[test]
> +fn can_skip_remote_cannot_skip_if_all_included() {
> + let view = View::new(ViewConfig {
> + id: "abc".into(),
> + include_all: Some(true),
> + ..Default::default()
> });
>
> assert!(!view.can_skip_remote("remote-a"));
> @@ -600,6 +625,7 @@ fn can_skip_remote_with_no_remote_includes() {
> "resource/remote-a/guest/100".to_string(),
> ))],
> exclude: vec![],
> + ..Default::default()
> });
>
> assert!(!view.can_skip_remote("remote-a"));
> @@ -614,6 +640,7 @@ fn explicitly_included_remote() {
> "remote-b".to_string(),
> ))],
> exclude: vec![],
> + ..Default::default()
> });
>
> assert!(view.is_remote_explicitly_included("remote-b"));
> @@ -629,6 +656,7 @@ fn included_and_excluded_same_remote() {
> exclude: vec![FilterRule::Remote(StringMatcher::Exact(
> "remote-b".to_string(),
> ))],
> + ..Default::default()
> });
>
> assert!(!view.is_remote_explicitly_included("remote-b"));
> @@ -640,8 +668,9 @@ fn not_explicitly_included_remote() {
> id: "abc".into(),
> include: vec![],
> exclude: vec![],
> + include_all: Some(true),
> });
>
> // Assert that is not *explicitly* included
> - assert!(!view.is_remote_explicitly_included("remote-b"));
> + assert!(view.is_remote_explicitly_included("remote-b"));
> }
More information about the pdm-devel
mailing list