Skip to content
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

Limits max_value and name field play no role in their identity… #87

Merged
merged 3 commits into from
Jul 8, 2022
Merged

Limits max_value and name field play no role in their identity… #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9adf279
Only namespace, seconds, conditions and variable make for a Limit's i…
alexsnaps Jun 23, 2022
3cd00cb
Can't ignore the max_value and name from counter's limit, as we deser…
alexsnaps Jun 23, 2022
0e72581
Reset the data need in deserialized Limits
alexsnaps Jun 27, 2022
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
4 changes: 3 additions & 1 deletion limitador-server/src/envoy_rls/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,9 @@ mod tests {
Limit::new(namespace, 0, 60, vec!["x == 1", "y == 2"], vec!["z"]),
]
.into_iter()
.for_each(|limit| limiter.add_limit(limit));
.for_each(|limit| {
limiter.add_limit(limit);
});

let rate_limiter = MyRateLimiter::new(Arc::new(Limiter::Blocking(limiter)));

Expand Down
2 changes: 1 addition & 1 deletion limitador-server/src/http_api/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ mod tests {
match &limiter {
Limiter::Blocking(limiter) => limiter.add_limit(limit.clone()),
Limiter::Async(limiter) => limiter.add_limit(limit.clone()),
}
};
limit
}
}
11 changes: 11 additions & 0 deletions limitador/src/counter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,17 @@ impl Counter {
self.limit.max_value()
}

pub fn update_to_limit(&mut self, limit: &Limit) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it makes sense to get rid of the limit included in the counter. This update_to_limit is solely to keep in sync limit objects representing the same. And it is the source of panic's because limits are not equal and should be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah… there are few places where these Limits and/or Counters are juggled around with for … no real reason in the end. I do believe that the model should be more Namespace is composed of Limits, which in turn may have Counters associated with it… where some of that depends on the Limit itself (i.e. does it have variables to qualify Counters).
There are few way this could be addressed tho and, as discussed in the sync this morning, I'd like to us all to decide on a design forward, one that accounts for the possible futures, e.g. multi-location Limitadors sharing counters.

if limit == &self.limit {
self.limit.set_max_value(limit.max_value());
if let Some(name) = limit.name() {
self.limit.set_name(name.to_string());
}
return true;
}
false
}

pub fn seconds(&self) -> u64 {
self.limit.seconds()
}
Expand Down
5 changes: 3 additions & 2 deletions limitador/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ use crate::storage::{AsyncCounterStorage, AsyncStorage, Authorization, CounterSt

#[macro_use]
extern crate lazy_static;
extern crate core;

pub mod counter;
pub mod errors;
Expand Down Expand Up @@ -312,7 +313,7 @@ impl RateLimiter {
self.storage.get_namespaces()
}

pub fn add_limit(&self, limit: Limit) {
pub fn add_limit(&self, limit: Limit) -> bool {
self.storage.add_limit(limit)
}

Expand Down Expand Up @@ -483,7 +484,7 @@ impl AsyncRateLimiter {
self.storage.get_namespaces()
}

pub fn add_limit(&self, limit: Limit) {
pub fn add_limit(&self, limit: Limit) -> bool {
self.storage.add_limit(limit)
}

Expand Down
10 changes: 6 additions & 4 deletions limitador/src/limit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@ impl From<String> for Namespace {
#[derive(Eq, Debug, Clone, Serialize, Deserialize)]
pub struct Limit {
namespace: Namespace,
#[serde(skip)]
max_value: i64,
seconds: u64,
#[serde(skip)]
name: Option<String>,

// Need to sort to generate the same object when using the JSON as a key or
Expand Down Expand Up @@ -88,6 +90,10 @@ impl Limit {
self.name = Some(name)
}

pub fn set_max_value(&mut self, value: i64) {
self.max_value = value;
}

pub fn conditions(&self) -> HashSet<String> {
self.conditions.iter().map(|cond| cond.into()).collect()
}
Expand Down Expand Up @@ -129,9 +135,7 @@ impl Limit {
impl Hash for Limit {
fn hash<H: Hasher>(&self, state: &mut H) {
self.namespace.hash(state);
self.max_value.hash(state);
self.seconds.hash(state);
self.name.hash(state);

let mut encoded_conditions = self
.conditions
Expand All @@ -156,9 +160,7 @@ impl Hash for Limit {
impl PartialEq for Limit {
fn eq(&self, other: &Self) -> bool {
self.namespace == other.namespace
&& self.max_value == other.max_value
&& self.seconds == other.seconds
&& self.name == other.name
&& self.conditions == other.conditions
&& self.variables == other.variables
}
Expand Down
2 changes: 1 addition & 1 deletion limitador/src/storage/infinispan/infinispan_storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ impl AsyncCounterStorage for InfinispanStorage {
// unnecessarily.

if let Some(val) = counter_val {
let mut counter: Counter = counter_from_counter_key(&counter_key);
let mut counter: Counter = counter_from_counter_key(&counter_key, &limit);
let ttl = 0; // TODO: calculate TTL from response headers.
counter.set_remaining(val);
counter.set_expires_in(Duration::from_secs(ttl));
Expand Down
34 changes: 32 additions & 2 deletions limitador/src/storage/keys.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,39 @@ pub fn key_for_counters_of_limit(limit: &Limit) -> String {
)
}

pub fn counter_from_counter_key(key: &str) -> Counter {
pub fn counter_from_counter_key(key: &str, limit: &Limit) -> Counter {
let counter_prefix = "counter:";
let start_pos_counter = key.find(counter_prefix).unwrap() + counter_prefix.len();

serde_json::from_str(&key[start_pos_counter..]).unwrap()
let mut counter: Counter = serde_json::from_str(&key[start_pos_counter..]).unwrap();
if !counter.update_to_limit(limit) {
// this means some kind of data corruption _or_ most probably
// an out of sync `impl PartialEq for Limit` vs `pub fn key_for_counter(counter: &Counter) -> String`
panic!(
"Failed to rebuild Counter's Limit from the provided Limit: {:?} vs {:?}",
counter.limit(),
limit
)
}
counter
}

#[cfg(test)]
mod tests {
use crate::storage::keys::key_for_counters_of_limit;
use crate::Limit;

#[test]
fn key_for_limit_format() {
let limit = Limit::new(
"example.com",
10,
60,
vec!["req.method == GET"],
vec!["app_id"],
);
assert_eq!(
"namespace:{example.com},counters_of_limit:{\"namespace\":\"example.com\",\"seconds\":60,\"conditions\":[\"req.method == GET\"],\"variables\":[\"app_id\"]}",
key_for_counters_of_limit(&limit))
}
}
11 changes: 5 additions & 6 deletions limitador/src/storage/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,14 @@ impl Storage {
self.limits.read().unwrap().keys().cloned().collect()
}

pub fn add_limit(&self, limit: Limit) {
pub fn add_limit(&self, limit: Limit) -> bool {
let namespace = limit.namespace().clone();
self.limits
.write()
.unwrap()
.entry(namespace)
.or_default()
.insert(limit);
.insert(limit)
}

pub fn get_limits(&self, namespace: &Namespace) -> HashSet<Limit> {
Expand Down Expand Up @@ -140,19 +140,18 @@ impl AsyncStorage {
self.limits.read().unwrap().keys().cloned().collect()
}

pub fn add_limit(&self, limit: Limit) {
pub fn add_limit(&self, limit: Limit) -> bool {
Copy link
Member Author

Choose a reason for hiding this comment

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

So this change is interesting… what does it mean when now a Limit isn't added. Like when (re)loading the limit file?

It probably indicates an invalid config file, so in the case of:

  • Fresh load at start up:
    • we fail starting up completely?
    • Use the first Limit in the config file? Tho what is first in the case of compiling these from RateLimit CRs?
  • On reload of the config file:
    • Don't apply any changes? (tho with the code as is, it already started applying changes… shouldn't be much of probably to load all Limit in a Set first tho!)
    • Apply some kind of equivalent logic as on graceful startup? i.e. only add the first, and report on the ones "ignored"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, it interesting to account for what it'd mean when RateLimitPolicy are compiled down to the single Limitador CR… what can we already see there? what should make it to the ConfigMap to eventually be picked up by the Limitador server and possibly reflect it thru some (REST?) API to the individual RateLimitPolicy CRs…

Copy link
Contributor

@eguzki eguzki Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

First, let's confirm that I understand by invalid config file. a config file with a list of limits and at least two of them are equal with the new identity method (for example, only different max_value value).

Fresh load at start up: we fail starting up completely?

I would say so. A CLI tool to validate config files would be nice too.

On reload of the config file: Don't apply any changes?

I would say, do not. I would prefer being fully rejected than accepted only some part. As payback for that constraint, the service should make it very clear if the changes have been accepted or not.

Those are my options without taking into account implementation efforts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it interesting to account for what it'd mean when RateLimitPolicy are compiled down to the single Limitador CR… what can we already see there? what should make it to the ConfigMap to eventually be picked up by the Limitador server and possibly reflect it thru some (REST?) API to the individual RateLimitPolicy CRs…

I am not sure I follow. You mean add some info in the status of the RLP? FYI, check the schema of the RLP new design,

 limits:
        - conditions: ["admin == yes"]
          max_value: 500
          seconds: 30
          variables: []

no namespace (aka domain) is defined. It is up to the control plane to assign namespace to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

What if two people define two "equivalent" RateLimitPolicy CRs (logically speaking, so say they have different max_value, but possibly the same, it doesn't really matter I think), when compiling the Limitador CR with all the Limits from the different RateLimitPolicy CRs, we might have a conflict now…

What does that mean? Who (the operator, Limitador, ...?) makes what call? How is it then reflected on the individual CRs' status? How does that affect further changes (valid new RateLimitPolicy CRs being created, or existing changed while the conflicting ones are still there)? If limitador rejects a complete new Limit file, these question matter.

Ideally, the operator would be able to "ignore" these CRs and compile to an always valid limitador limit file. But is this even possible? Do we need to duplicate logic? Expose some service to test a config before applying it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good questions here.

The kuadrant control plane, the owner of the content in the Limitador CR in the kuadrant context, should do the validations. And if any conflict is found, reject the RLP adding the reason in the status field.

The limitador operator can also run some validation checks to avoid such invalid limit definitions.

I will think about that and add it to the proposal doc. This information should end up in the kuadrant's doc about the behavior of the control plane.

let namespace = limit.namespace().clone();

let mut limits_for_namespace = self.limits.write().unwrap();

match limits_for_namespace.get_mut(&namespace) {
Some(limits) => {
limits.insert(limit);
}
Some(limits) => limits.insert(limit),
None => {
let mut limits = HashSet::new();
limits.insert(limit);
limits_for_namespace.insert(namespace, limits);
true
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion limitador/src/storage/redis/redis_async.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ impl AsyncCounterStorage for AsyncRedisStorage {
.await?;

for counter_key in counter_keys {
let mut counter: Counter = counter_from_counter_key(&counter_key);
let mut counter: Counter = counter_from_counter_key(&counter_key, &limit);

// If the key does not exist, it means that the counter expired,
// so we don't have to return it.
Expand Down
2 changes: 1 addition & 1 deletion limitador/src/storage/redis/redis_sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ impl CounterStorage for RedisStorage {
con.smembers::<String, HashSet<String>>(key_for_counters_of_limit(limit))?;

for counter_key in counter_keys {
let mut counter: Counter = counter_from_counter_key(&counter_key);
let mut counter: Counter = counter_from_counter_key(&counter_key, limit);

// If the key does not exist, it means that the counter expired,
// so we don't have to return it.
Expand Down
2 changes: 1 addition & 1 deletion limitador/tests/helpers/tests_limiter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ impl TestsLimiter {
}
}

pub async fn add_limit(&self, limit: &Limit) {
pub async fn add_limit(&self, limit: &Limit) -> bool {
match &self.limiter_impl {
LimiterImpl::Blocking(limiter) => limiter.add_limit(limit.clone()),
LimiterImpl::Async(limiter) => limiter.add_limit(limit.clone()),
Expand Down
27 changes: 25 additions & 2 deletions limitador/tests/integration_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ mod test {
test_with_all_storage_impls!(configure_with_creates_the_given_limits);
test_with_all_storage_impls!(configure_with_keeps_the_given_limits_and_counters_if_they_exist);
test_with_all_storage_impls!(configure_with_deletes_all_except_the_limits_given);
test_with_all_storage_impls!(add_limit_only_adds_if_not_present);

// All these functions need to use async/await. That's needed to support
// both the sync and the async implementations of the rate limiter.
Expand Down Expand Up @@ -328,7 +329,7 @@ mod test {
];

for limit in limits.iter() {
rate_limiter.add_limit(limit).await
rate_limiter.add_limit(limit).await;
}

rate_limiter.delete_limits(namespace).await.unwrap();
Expand Down Expand Up @@ -830,7 +831,7 @@ mod test {
let namespace = "test_namespace";

let limit_to_be_kept =
Limit::new(namespace, 10, 60, vec!["req.method == GET"], vec!["app_id"]);
Limit::new(namespace, 10, 1, vec!["req.method == GET"], vec!["app_id"]);

let limit_to_be_deleted =
Limit::new(namespace, 20, 60, vec!["req.method == GET"], vec!["app_id"]);
Expand All @@ -849,4 +850,26 @@ mod test {
assert!(limits.contains(&limit_to_be_kept));
assert!(!limits.contains(&limit_to_be_deleted));
}

async fn add_limit_only_adds_if_not_present(rate_limiter: &mut TestsLimiter) {
let namespace = "test_namespace";

let limit_1 = Limit::new(namespace, 10, 60, vec!["req.method == GET"], vec!["app_id"]);

let limit_2 = Limit::new(namespace, 20, 60, vec!["req.method == GET"], vec!["app_id"]);

let mut limit_3 = Limit::new(namespace, 20, 60, vec!["req.method == GET"], vec!["app_id"]);
limit_3.set_name("Name is irrelevant too".to_owned());

assert!(rate_limiter.add_limit(&limit_1).await);
assert!(!rate_limiter.add_limit(&limit_2).await);
assert!(!rate_limiter.add_limit(&limit_3).await);

let limits = rate_limiter.get_limits(namespace).await;

assert_eq!(limits.len(), 1);
let known_limit = limits.iter().next().unwrap();
assert_eq!(known_limit.max_value(), 10);
assert_eq!(known_limit.name(), None);
}
}