Skip to content

rbac: add counters to config dump #1536

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

rbac: add counters to config dump #1536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions benches/throughput.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ fn create_test_policies() -> Vec<Authorization> {
scope: ztunnel::rbac::RbacScope::Global,
namespace: "default".into(),
rules: rules.clone(),
stats: Default::default(),
});
}

Expand Down
25 changes: 24 additions & 1 deletion src/rbac.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ use ipnet::IpNet;
use std::fmt;
use std::fmt::{Display, Formatter};
use std::net::SocketAddr;
use std::sync::Arc;
use std::sync::atomic::AtomicU64;
use std::sync::atomic::Ordering::Relaxed;
use tracing::{instrument, trace};
use xds::istio::security::Address as XdsAddress;
use xds::istio::security::Authorization as XdsRbac;
Expand All @@ -31,14 +34,32 @@ use crate::state::workload::{WorkloadError, byte_to_ip};
use crate::strng::Strng;
use crate::{strng, xds};

#[derive(Debug, Hash, Eq, PartialEq, Clone, serde::Serialize, serde::Deserialize)]
#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct Authorization {
pub name: Strng,
pub namespace: Strng,
pub scope: RbacScope,
pub action: RbacAction,
pub rules: Vec<Vec<Vec<RbacMatch>>>,
#[serde(default)]
pub stats: Arc<AuthzCounter>,
}

#[derive(Debug, Default, serde::Serialize, serde::Deserialize)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct AuthzCounter {
hits: AtomicU64,
misses: AtomicU64,
}
Comment on lines +51 to +54
Copy link
Contributor

@ilrudie ilrudie Apr 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hit and miss need to be understood relative to the kind of policy to make sense. hit a deny == denied. hit an allow == allowed. Would users be better served with "allows" and "denies" which inform them of a concrete result taken in association with this policy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry an allow policy with deny: 123 looks alarming. Its not really a deny, its more like it failed to allow

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if a policy doesn't match, miss/deny will be incremented, even if another policy later on allow or denies it?

Copy link
Contributor

@Stevenjin8 Stevenjin8 Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wonder if the order of operations might cause the stats to be confusing. For example, if there are multiple deny policies, but only one of them matches, then the order in which the deny policies are evaluated will affect the stats (assuming there is some short circuiting), which seems weird.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better say of saying this is, why do we need miss when we can just do total_requests - hit

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no total request counter for a policy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean total request counter across for ztunnel in general

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't exist is the first issue. We could add it but it doesn't help the use case.
Miss means "policy applies to the request but does not match"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok then only my second comment then: This doesn't seem to take into account short circuiting. Just because a policy could apply, short circuiting might mean that a counter doesn't get incremented, which might be the intended behavior

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, will need to fix that


impl AuthzCounter {
pub fn hit(&self) {
self.hits.fetch_add(1, Relaxed);
}
pub fn miss(&self) {
self.misses.fetch_add(1, Relaxed);
}
}

#[derive(Debug, Clone, Eq, Hash, Ord, PartialEq, PartialOrd, serde::Serialize)]
Expand Down Expand Up @@ -360,6 +381,7 @@ impl TryFrom<XdsRbac> for Authorization {
scope: RbacScope::from(xds::istio::security::Scope::try_from(resource.scope)?),
action: RbacAction::from(xds::istio::security::Action::try_from(resource.action)?),
rules,
stats: Default::default(),
})
}
}
Expand Down Expand Up @@ -483,6 +505,7 @@ mod tests {
scope: RbacScope::Global,
action: RbacAction::Allow,
rules,
stats: Default::default(),
}
}

Expand Down
6 changes: 6 additions & 0 deletions src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,12 +545,14 @@ impl DemandProxyState {
// "If there are any DENY policies that match the request, deny the request."
for pol in deny.iter() {
if pol.matches(conn) {
pol.stats.hit();
debug!(policy = pol.to_key().as_str(), "deny policy match");
return Err(proxy::AuthorizationRejectionError::ExplicitlyDenied(
pol.namespace.to_owned(),
pol.name.to_owned(),
));
} else {
pol.stats.miss();
trace!(policy = pol.to_key().as_str(), "deny policy does not match");
}
}
Expand All @@ -562,9 +564,11 @@ impl DemandProxyState {
// "If any of the ALLOW policies match the request, allow the request."
for pol in allow.iter() {
if pol.matches(conn) {
pol.stats.hit();
debug!(policy = pol.to_key().as_str(), "allow policy match");
return Ok(());
} else {
pol.stats.miss();
trace!(
policy = pol.to_key().as_str(),
"allow policy does not match"
Expand Down Expand Up @@ -1446,6 +1450,7 @@ mod tests {
],
],
scope: rbac::RbacScope::Namespace,
stats: Default::default(),
},
);
state.policies.insert(
Expand All @@ -1467,6 +1472,7 @@ mod tests {
],
],
scope: rbac::RbacScope::Namespace,
stats: Default::default(),
},
);

Expand Down
22 changes: 14 additions & 8 deletions src/state/policy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,14 @@ impl PolicyStore {
.collect()
}

pub fn insert(&mut self, xds_name: Strng, rbac: Authorization) {
self.remove(xds_name.clone());
pub fn insert(&mut self, xds_name: Strng, mut rbac: Authorization) {
if let Some(old) = self.remove(xds_name.clone()) {
// Copy over old stats. There could be some motivation to reset the stats if the policy
// changes, but right now this is getting triggered even for NO-OP changes possibly, so
// resets would be confusing.
rbac.stats = old.stats;
}

match rbac.scope {
RbacScope::Global => {
self.by_namespace
Expand All @@ -76,13 +82,11 @@ impl PolicyStore {
self.by_key.insert(xds_name.clone(), rbac);
}

pub fn remove(&mut self, xds_name: Strng) {
let Some(rbac) = self.by_key.remove(&xds_name) else {
return;
};
if let Some(key) = match rbac.scope {
pub fn remove(&mut self, xds_name: Strng) -> Option<Authorization> {
let rbac = self.by_key.remove(&xds_name)?;
if let Some(key) = match &rbac.scope {
RbacScope::Global => Some(strng::EMPTY),
RbacScope::Namespace => Some(rbac.namespace),
RbacScope::Namespace => Some(rbac.namespace.clone()),
RbacScope::WorkloadSelector => None,
} {
if let Some(pl) = self.by_namespace.get_mut(&key) {
Expand All @@ -92,6 +96,7 @@ impl PolicyStore {
}
}
}
Some(rbac)
}
pub fn subscribe(&self) -> watch::Receiver<()> {
self.notifier.sender.subscribe()
Expand Down Expand Up @@ -128,6 +133,7 @@ mod tests {
namespaces: vec![StringMatch::Exact("whatever".into())],
..Default::default()
}]]],
stats: Default::default(),
};
let policy_key = policy.to_key();
// insert this namespace-scoped policy into policystore then assert it is
Expand Down
2 changes: 1 addition & 1 deletion src/xds.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ pub struct LocalWorkload {
pub services: HashMap<String, HashMap<u16, u16>>,
}

#[derive(Default, Debug, Eq, PartialEq, Clone, serde::Serialize, serde::Deserialize)]
#[derive(Default, Debug, Clone, serde::Serialize, serde::Deserialize)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct LocalConfig {
#[serde(default)]
Expand Down
1 change: 1 addition & 0 deletions tests/namespaced.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1001,6 +1001,7 @@ mod namespaced {
)],
..Default::default()
}]]],
stats: Default::default(),
})
.await?;
let _ = manager
Expand Down