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

source.remote_address well known attribute #111

Merged
merged 11 commits into from
Oct 18, 2024
Merged

source.remote_address well known attribute #111

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
496ae82
remote_address: step 1 proxy all hostcalls::get_property
eguzki Oct 15, 2024
7cdd953
remote_address: fix tests
eguzki Oct 16, 2024
85beb1c
remote_address: refactor Path to its own source file
eguzki Oct 16, 2024
c55d814
remote_address: step 2 implement source.remote_address attribute
eguzki Oct 16, 2024
26408a2
remote_address: step 2 simplified alternative implementation
eguzki Oct 16, 2024
358acbb
remote_address: step 3 new integration environment
eguzki Oct 17, 2024
b4b68aa
remote_address: step 4 e2e integration test
eguzki Oct 17, 2024
2b6335f
pattern expressions path() back
eguzki Oct 17, 2024
05fdecd
remote_address: step 5 doc
eguzki Oct 17, 2024
8def009
fix doc link
eguzki Oct 17, 2024
3e256ed
remote_address: step 6 integration test
eguzki Oct 18, 2024
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
42 changes: 21 additions & 21 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ curl -H "Host: test.a.auth.com" -H "Authorization: APIKEY IAMALICE" http://127.0
# HTTP/1.1 200 OK
```

And three rate limit policies defined for e2e testing:
And some rate limit policies defined for e2e testing:

* `rlp-a`: Only one data item. Data selector should not generate return any value. Thus, descriptor should be empty and
rate limiting service should **not** be called.
Expand All @@ -187,45 +187,45 @@ curl -H "Host: test.a.rlp.com" http://127.0.0.1:8000/get -i
curl -H "Host: test.b.rlp.com" http://127.0.0.1:8000/get -i
```

* `rlp-c`: Four descriptors from multiple rules should be generated. Hence, rate limiting service should be called.
* `rlp-c`: Descriptor entries from multiple data items should be generated. Hence, rate limiting service should be called.

```sh
curl -H "Host: test.c.rlp.com" -H "x-forwarded-for: 127.0.0.1" -H "My-Custom-Header-01: my-custom-header-value-01" -H "x-dyn-user-id: bob" http://127.0.0.1:8000/get -i
curl -H "Host: test.c.rlp.com" -H "x-forwarded-for: 50.0.0.1" -H "My-Custom-Header-01: my-custom-header-value-01" -H "x-dyn-user-id: bob" http://127.0.0.1:8000/get -i
```

* `multi-a` which defines two actions for authenticated ratelimiting.
The expected descriptor entries:

```sh
curl -H "Host: test.a.multi.com" http://127.0.0.1:8000/get -i
# HTTP/1.1 401 Unauthorized
```

Alice has 5 requests per 10 seconds:
```sh
while :; do curl --write-out '%{http_code}\n' --silent --output /dev/null -H "Authorization: APIKEY IAMALICE" -H "Host: test.a.multi.com" http://127.0.0.1:8000/get | grep -E --color "\b(429)\b|$"; sleep 1; done
Entry { key: "limit_to_be_activated", value: "1" }
```

Bob has 2 requests per 10 seconds:
```sh
while :; do curl --write-out '%{http_code}\n' --silent --output /dev/null -H "Authorization: APIKEY IAMBOB" -H "Host: test.a.multi.com" http://127.0.0.1:8000/get | grep -E --color "\b(429)\b|$"; sleep 1; done
```

The expected descriptors:
Entry { key: "source.address", value: "50.0.0.1:0" }
```

```
RateLimitDescriptor { entries: [Entry { key: "limit_to_be_activated", value: "1" }], limit: None }
Entry { key: "request.headers.My-Custom-Header-01", value: "my-custom-header-value-01" }
```

```
RateLimitDescriptor { entries: [Entry { key: "source.address", value: "127.0.0.1:0" }], limit: None }
Entry { key: "user_id", value: "bob" }
```

```
RateLimitDescriptor { entries: [Entry { key: "request.headers.My-Custom-Header-01", value: "my-custom-header-value-01" }], limit: None }
* `multi-a` which defines two actions for authenticated ratelimiting.

```sh
curl -H "Host: test.a.multi.com" http://127.0.0.1:8000/get -i
# HTTP/1.1 401 Unauthorized
```

Alice has 5 requests per 10 seconds:
```sh
while :; do curl --write-out '%{http_code}\n' --silent --output /dev/null -H "Authorization: APIKEY IAMALICE" -H "Host: test.a.multi.com" http://127.0.0.1:8000/get | grep -E --color "\b(429)\b|$"; sleep 1; done
```
RateLimitDescriptor { entries: [Entry { key: "user_id", value: "bob" }], limit: None }

Bob has 2 requests per 10 seconds:
```sh
while :; do curl --write-out '%{http_code}\n' --silent --output /dev/null -H "Authorization: APIKEY IAMBOB" -H "Host: test.a.multi.com" http://127.0.0.1:8000/get | grep -E --color "\b(429)\b|$"; sleep 1; done
```

To rebuild and deploy to the cluster:
Expand Down
4 changes: 2 additions & 2 deletions src/attribute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::configuration::Path;
use crate::property_path::Path;
use chrono::{DateTime, FixedOffset};
use log::{debug, error};
use protobuf::well_known_types::Struct;
Expand Down Expand Up @@ -111,7 +111,7 @@ pub fn get_attribute<T>(attr: &str) -> Result<T, String>
where
T: Attribute,
{
match hostcalls::get_property(Path::from(attr).tokens()) {
match crate::property::get_property(attr) {
Ok(Some(attribute_bytes)) => T::parse(attribute_bytes),
Ok(None) => Err(format!("get_attribute: not found or null: {attr}")),
Err(e) => Err(format!("get_attribute: error: {e:?}")),
Expand Down
153 changes: 7 additions & 146 deletions src/configuration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
use std::cell::OnceCell;
use std::collections::HashMap;
use std::fmt::{Debug, Display, Formatter};
use std::fmt::{Debug, Formatter};
use std::rc::Rc;
use std::sync::Arc;

use crate::attribute::Attribute;
use crate::envoy::{RateLimitDescriptor, RateLimitDescriptor_Entry};
use crate::policy::Policy;
use crate::policy_index::PolicyIndex;
use crate::property_path::Path;
use crate::service::GrpcService;
use cel_interpreter::functions::duration;
use cel_interpreter::objects::ValueType;
use cel_interpreter::{Context, Expression, Value};
use cel_parser::{Atom, RelationOp};
use log::debug;
use protobuf::RepeatedField;
use proxy_wasm::hostcalls;
use serde::de::{Error, Visitor};
use serde::{Deserialize, Deserializer};
use std::time::Duration;
Expand All @@ -34,73 +34,6 @@ pub struct SelectorItem {
// If not set and the selector is not found in the context, then no data is generated.
#[serde(default)]
pub default: Option<String>,

#[serde(skip_deserializing)]
path: OnceCell<Path>,
}

impl SelectorItem {
pub fn compile(&self) -> Result<(), String> {
self.path
.set(self.selector.as_str().into())
.map_err(|p| format!("Err on {p:?}"))
}

pub fn path(&self) -> &Path {
self.path
.get()
.expect("SelectorItem wasn't previously compiled!")
}
}

#[derive(Debug, Clone)]
pub struct Path {
tokens: Vec<String>,
}

impl Display for Path {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
write!(
f,
"{}",
self.tokens
.iter()
.map(|t| t.replace('.', "\\."))
.collect::<Vec<String>>()
.join(".")
)
}
}

impl From<&str> for Path {
fn from(value: &str) -> Self {
let mut token = String::new();
let mut tokens: Vec<String> = Vec::new();
let mut chars = value.chars();
while let Some(ch) = chars.next() {
match ch {
'.' => {
tokens.push(token);
token = String::new();
}
'\\' => {
if let Some(next) = chars.next() {
token.push(next);
}
}
_ => token.push(ch),
}
}
tokens.push(token);

Self { tokens }
}
}

impl Path {
pub fn tokens(&self) -> Vec<&str> {
self.tokens.iter().map(String::as_str).collect()
}
}

#[derive(Deserialize, Debug, Clone)]
Expand All @@ -117,15 +50,6 @@ pub enum DataType {
Selector(SelectorItem),
}

impl DataType {
pub fn compile(&self) -> Result<(), String> {
match self {
DataType::Static(_) => Ok(()),
DataType::Selector(selector) => selector.compile(),
}
}
}

#[derive(Deserialize, Debug, Clone)]
#[serde(deny_unknown_fields)]
pub struct DataItem {
Expand Down Expand Up @@ -154,27 +78,16 @@ pub struct PatternExpression {
pub operator: WhenConditionOperator,
pub value: String,

#[serde(skip_deserializing)]
path: OnceCell<Path>,
#[serde(skip_deserializing)]
compiled: OnceCell<CelExpression>,
}

impl PatternExpression {
pub fn compile(&self) -> Result<(), String> {
self.path
.set(self.selector.as_str().into())
.map_err(|_| "Duh!")?;
self.compiled
.set(self.try_into()?)
.map_err(|_| "Ooops".to_string())
}
pub fn path(&self) -> Vec<&str> {
self.path
.get()
.expect("PatternExpression wasn't previously compiled!")
.tokens()
}

pub fn eval(&self, raw_attribute: Vec<u8>) -> Result<bool, String> {
let cel_type = &self.compiled.get().unwrap().cel_type;
Expand Down Expand Up @@ -476,14 +389,6 @@ impl TryFrom<PluginConfiguration> for FilterConfig {
}
}
}
for action in &rule.actions {
for datum in &action.data {
let result = datum.item.compile();
if result.is_err() {
return Err(result.err().unwrap());
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Actually one thing that's standing out to me is that we no longer keep the path as well as compiled version for PatternExpressions and don't pre-compile data items. I'm not 100%, does this conflict somewhat with the intention here cc @alexsnaps

Copy link
Member

Choose a reason for hiding this comment

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

I need to look into those changes properly, but that would also mean that we'd err out at evaluation time instead of at config parse time... that seems... not desirable 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed because I did not see any point in early compilation. The "compilation" is just about building the path out of the selector string, which can never fail. Hence, I removed.

Can you confirm my analysis?

Anyway, I am happy to add that back.

Copy link
Member

Choose a reason for hiding this comment

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

Doing it once, instead of on all invocations? Now we need to parse on every Policy::pattern_expression_applies, right? And it will possibly fail when we move these to CEL expression as @adam-cattermole pointed out. But arguably that's my problem.

In any case, I'm a big proponent of doing things as early as possible, represent them in the proper way from there on in the system and do the least amount of work. This code was early parsing the "path" and keeping it in proper form to avoid paying the computation time on the data plane's path. These are my guiding principle in general, but more so here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for confirming.

Bringing that back

}

for hostname in policy.hostnames.iter() {
Expand Down Expand Up @@ -615,17 +520,15 @@ impl Action {
entries.push(descriptor_entry);
}
DataType::Selector(selector_item) => {
let attribute_path = Path::from(selector_item.selector.as_str());
let descriptor_key = match &selector_item.key {
None => selector_item.path().to_string(),
None => attribute_path.to_string(),
Some(key) => key.to_owned(),
};

let attribute_path = selector_item.path();
debug!(
"get_property: selector: {} path: {:?}",
selector_item.selector, attribute_path
);
let value = match hostcalls::get_property(attribute_path.tokens()).unwrap() {
let value = match crate::property::get_property(selector_item.selector.as_str())
.unwrap()
{
//TODO(didierofrivia): Replace hostcalls by DI
None => {
debug!(
Expand Down Expand Up @@ -1177,39 +1080,6 @@ mod test {
assert!(rlp_option.is_none());
}

#[test]
fn path_tokenizes_with_escaping_basic() {
let path: Path = r"one\.two..three\\\\.four\\\.\five.".into();
assert_eq!(
path.tokens(),
vec!["one.two", "", r"three\\", r"four\.five", ""]
);
}

#[test]
fn path_tokenizes_with_escaping_ends_with_separator() {
let path: Path = r"one.".into();
assert_eq!(path.tokens(), vec!["one", ""]);
}

#[test]
fn path_tokenizes_with_escaping_ends_with_escape() {
let path: Path = r"one\".into();
assert_eq!(path.tokens(), vec!["one"]);
}

#[test]
fn path_tokenizes_with_escaping_starts_with_separator() {
let path: Path = r".one".into();
assert_eq!(path.tokens(), vec!["", "one"]);
}

#[test]
fn path_tokenizes_with_escaping_starts_with_escape() {
let path: Path = r"\one".into();
assert_eq!(path.tokens(), vec!["one"]);
}

mod pattern_expressions {
use crate::configuration::{PatternExpression, WhenConditionOperator};

Expand All @@ -1219,7 +1089,6 @@ mod test {
selector: "request.id".to_string(),
operator: WhenConditionOperator::Equal,
value: "request_id".to_string(),
path: Default::default(),
compiled: Default::default(),
};
p.compile().expect("Should compile fine!");
Expand All @@ -1237,7 +1106,6 @@ mod test {
selector: "request.id".to_string(),
operator: WhenConditionOperator::Equal,
value: "\"request_id\"".to_string(),
path: Default::default(),
compiled: Default::default(),
};
p.compile().expect("Should compile fine!");
Expand All @@ -1255,7 +1123,6 @@ mod test {
selector: "request.id".to_string(),
operator: WhenConditionOperator::Equal,
value: "123".to_string(),
path: Default::default(),
compiled: Default::default(),
};
p.compile().expect("Should compile fine!");
Expand All @@ -1273,7 +1140,6 @@ mod test {
selector: "foobar".to_string(),
operator: WhenConditionOperator::Equal,
value: "\"123\"".to_string(),
path: Default::default(),
compiled: Default::default(),
};
p.compile().expect("Should compile fine!");
Expand All @@ -1291,7 +1157,6 @@ mod test {
selector: "destination.port".to_string(),
operator: WhenConditionOperator::Equal,
value: "8080".to_string(),
path: Default::default(),
compiled: Default::default(),
};
p.compile().expect("Should compile fine!");
Expand All @@ -1309,7 +1174,6 @@ mod test {
selector: "foobar".to_string(),
operator: WhenConditionOperator::Equal,
value: "8080".to_string(),
path: Default::default(),
compiled: Default::default(),
};
p.compile().expect("Should compile fine!");
Expand All @@ -1327,7 +1191,6 @@ mod test {
selector: "foobar".to_string(),
operator: WhenConditionOperator::Equal,
value: "1.0".to_string(),
path: Default::default(),
compiled: Default::default(),
};
p.compile().expect("Should compile fine!");
Expand All @@ -1345,7 +1208,6 @@ mod test {
selector: "connection.mtls".to_string(),
operator: WhenConditionOperator::Equal,
value: "true".to_string(),
path: Default::default(),
compiled: Default::default(),
};
p.compile().expect("Should compile fine!");
Expand All @@ -1363,7 +1225,6 @@ mod test {
selector: "request.time".to_string(),
operator: WhenConditionOperator::Equal,
value: "2023-05-28T00:00:00+00:00".to_string(),
path: Default::default(),
compiled: Default::default(),
};
p.compile().expect("Should compile fine!");
Expand Down
2 changes: 2 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ mod glob;
mod operation_dispatcher;
mod policy;
mod policy_index;
mod property;
mod property_path;
mod service;

#[cfg(test)]
Expand Down
Loading
Loading