Endpoint metadata support from static config #160
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,209 @@ | ||
| use serde_json::map::Map as JsonMap; | ||
| use serde_json::value::Value as JSONValue; | ||
| use serde_json::Number as JSONNumber; | ||
| use serde_yaml::Value as YamlValue; | ||
| use std::collections::HashSet; | ||
|
|
||
| /// METADATA_KEY is the key under which quilkin specific configuration is | ||
| /// placed in an endpoint metadata. | ||
| pub(crate) const METADATA_KEY: &str = "quilkin.dev"; | ||
|
|
||
| /// ENDPOINT_METADATA_KEY_TOKENS is the key under which tokens for an endpoint | ||
| /// exist in an endpoint metadata. | ||
| pub const ENDPOINT_METADATA_TOKENS: &str = "tokens"; | ||
|
|
||
| // Returns an empty map if no tokens exist. | ||
| pub fn extract_endpoint_tokens( | ||
markmandel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| metadata: &mut JsonMap<String, JSONValue>, | ||
| ) -> Result<HashSet<Vec<u8>>, String> { | ||
| let tokens = metadata.remove(METADATA_KEY) | ||
| .map(|raw_value| { | ||
| match raw_value { | ||
| JSONValue::Object(mut object) => { | ||
| match object.remove(ENDPOINT_METADATA_TOKENS) { | ||
| Some(JSONValue::Array(raw_tokens)) => { | ||
| raw_tokens.into_iter().fold(Ok(HashSet::new()), |acc, val| { | ||
| let mut tokens = acc?; | ||
|
|
||
| let token = match val { | ||
| JSONValue::String(token) => | ||
| base64::decode(token) | ||
| .map_err(|err| format!( | ||
| "key {}.{}: failed to decode token as a base64 string:{}", | ||
| METADATA_KEY, | ||
| ENDPOINT_METADATA_TOKENS, | ||
| err | ||
| )), | ||
| _ => Err(format!( | ||
| "invalid value in token list for key `{}`: value must a base64 string", | ||
| ENDPOINT_METADATA_TOKENS | ||
| )) | ||
| }; | ||
|
|
||
| tokens.insert(token?); | ||
| Ok(tokens) | ||
| }) | ||
| }, | ||
| Some(_) => Err(format!( | ||
| "invalid data type for key `{}.{}`: value must be a list of base64 strings", | ||
| METADATA_KEY, | ||
| ENDPOINT_METADATA_TOKENS | ||
| )), | ||
| None => Ok(Default::default()), | ||
| } | ||
| } | ||
| _ => Err(format!("invalid data type for key `{}`: value must be an object", METADATA_KEY)) | ||
| } | ||
| }) | ||
| .transpose()?; | ||
|
|
||
| Ok(tokens.unwrap_or_default()) | ||
| } | ||
|
|
||
| pub fn parse_endpoint_metadata_from_yaml( | ||
| yaml: YamlValue, | ||
| ) -> Result<(JSONValue, HashSet<Vec<u8>>), String> { | ||
| let mapping = if let YamlValue::Mapping(mapping) = yaml { | ||
| mapping | ||
| } else { | ||
| return Err("invalid endpoint metadata: value must be a yaml object".into()); | ||
| }; | ||
|
|
||
| let mut map = JsonMap::new(); | ||
| for (yaml_key, yaml_value) in mapping { | ||
| let key = parse_yaml_key(yaml_key)?; | ||
| let value = yaml_to_json_value(key.as_str(), yaml_value)?; | ||
| map.insert(key, value); | ||
| } | ||
|
|
||
| let tokens = extract_endpoint_tokens(&mut map)?; | ||
|
|
||
| Ok((JSONValue::Object(map), tokens)) | ||
| } | ||
|
|
||
| fn yaml_to_json_value(key: &str, yaml: YamlValue) -> Result<JSONValue, String> { | ||
| let json_value = match yaml { | ||
| YamlValue::Null => JSONValue::Null, | ||
| YamlValue::Bool(v) => JSONValue::Bool(v), | ||
| YamlValue::Number(v) => match v.as_f64() { | ||
| Some(v) => JSONValue::Number( | ||
| JSONNumber::from_f64(v) | ||
| .ok_or_else(|| format!("invalid f64 `{:?}` provided for key `{}`", v, key))?, | ||
| ), | ||
| None => return Err(format!("failed to parse key `{}` as f64 number", key)), | ||
| }, | ||
| YamlValue::String(v) => JSONValue::String(v), | ||
| YamlValue::Sequence(v) => { | ||
| let mut array = vec![]; | ||
| for yaml_value in v { | ||
| array.push(yaml_to_json_value(key, yaml_value)?); | ||
| } | ||
| JSONValue::Array(array) | ||
| } | ||
| YamlValue::Mapping(v) => { | ||
| let mut map = JsonMap::new(); | ||
|
|
||
| for (yaml_key, yaml_value) in v { | ||
| let nested_key = parse_yaml_key(yaml_key)?; | ||
| let value = | ||
| yaml_to_json_value(format!("{}.{}", key, nested_key).as_str(), yaml_value)?; | ||
| map.insert(nested_key, value); | ||
| } | ||
|
|
||
| JSONValue::Object(map) | ||
| } | ||
| }; | ||
|
|
||
| Ok(json_value) | ||
| } | ||
|
|
||
| fn parse_yaml_key(yaml: YamlValue) -> Result<String, String> { | ||
| match yaml { | ||
| YamlValue::String(v) => Ok(v), | ||
| v => Err(format!( | ||
| "invalid key `{:?}`: only string keys are allowed", | ||
| v | ||
| )), | ||
| } | ||
| } | ||
| #[cfg(test)] | ||
| mod tests { | ||
| use crate::config::metadata::{parse_endpoint_metadata_from_yaml, yaml_to_json_value}; | ||
| use std::collections::HashSet; | ||
|
|
||
| #[test] | ||
| fn yaml_data_types() { | ||
| let yaml = " | ||
| one: two | ||
| three: | ||
| four: | ||
| - five | ||
| - 6 | ||
| seven: | ||
| eight: true | ||
| "; | ||
| let yaml_value = serde_yaml::from_str(yaml).unwrap(); | ||
| let expected_json = serde_json::json!({ | ||
| "one": "two", | ||
| "three": { | ||
| "four": ["five", 6.0], | ||
| "seven": { | ||
| "eight": true | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| assert_eq!(yaml_to_json_value("k", yaml_value).unwrap(), expected_json); | ||
| } | ||
|
|
||
| #[test] | ||
| fn yaml_parse_endpoint_metadata() { | ||
| let yaml = " | ||
| user: | ||
| key1: value1 | ||
| quilkin.dev: | ||
| tokens: | ||
| - MXg3aWp5Ng== #1x7ijy6 | ||
| - OGdqM3YyaQ== #8gj3v2i | ||
| "; | ||
| let yaml_value = serde_yaml::from_str(yaml).unwrap(); | ||
| let expected_user_metadata = serde_json::json!({ | ||
| "user": { | ||
| "key1": "value1" | ||
| } | ||
| }); | ||
|
|
||
| let (user_metadata, tokens) = parse_endpoint_metadata_from_yaml(yaml_value).unwrap(); | ||
| assert_eq!(user_metadata, expected_user_metadata); | ||
| assert_eq!( | ||
| tokens, | ||
| vec!["1x7ijy6".into(), "8gj3v2i".into()] | ||
| .into_iter() | ||
| .collect::<HashSet<_>>() | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn yaml_parse_invalid_endpoint_metadata() { | ||
| let not_a_list = " | ||
| quilkin.dev: | ||
| tokens: OGdqM3YyaQ== | ||
| "; | ||
| let not_a_string_value = " | ||
| quilkin.dev: | ||
| tokens: | ||
| - OGdqM3YyaQ== #8gj3v2i | ||
| - 300 | ||
| "; | ||
| let not_a_base64_string = " | ||
| quilkin.dev: | ||
| tokens: | ||
| - OGdqM3YyaQ== #8gj3v2i | ||
| - 1x7ijy6 | ||
| "; | ||
| for yaml in &[not_a_list, not_a_string_value, not_a_base64_string] { | ||
| let yaml_value = serde_yaml::from_str(yaml).unwrap(); | ||
| assert!(parse_endpoint_metadata_from_yaml(yaml_value).is_err()); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be capturing tokens at all here, since this seems like it's all handled by the Filters?
If you can define metadata on an Endpoint in the static section - is that not enough?
Especially since the key for the token lookup or storage is configurable in(Edit: got this confused with dynamic metadata)TokenRouter?Should the Filter define which metadata value has all the tokens in it. so it is configurable? Rather than have it be hard-coded? (with a standard default as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason was to pre-process it so that tokens are simple and efficient to query. If we treat it as other metadata then the token router or any other filter that needs the info would need to traverse the json structure and scan a list each time.
Re pulling it out of the metadata, we can consider these as special cases since its primarily quilkin that needs them, compared to other metadata that the control plane might pass to a user's custom filter for example.
I'm not following the advantage of having it configurable? if e.g a user filter needs the tokens accessible on a particular field they can still put them under the desired key in the metadata without affecting any quilkin use case but I can't see why one would want it to be configurable if its already present in the endpoint itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense - it's a performance concern. We will need to document how we use xDS really well, and include this 👍
That's a fair point. I am wondering if at some point someone might want it to be configurable at the config.yaml file level - basically as a system wide configuration parameter -- but we could also cross that bridge when we come to it.