From 808db80fcc68606f540a0339acb8cd757934ecdb Mon Sep 17 00:00:00 2001 From: Simon Hornby Date: Thu, 18 May 2023 14:50:35 +0200 Subject: [PATCH] fix: allow multiple client tokens at startup (#188) --- server/src/builder.rs | 2 +- server/src/http/feature_refresher.rs | 87 ++++++++++++++++++---------- server/src/tokens.rs | 48 ++++++++++++++- 3 files changed, 103 insertions(+), 34 deletions(-) diff --git a/server/src/builder.rs b/server/src/builder.rs index c3838e1a..607eda61 100644 --- a/server/src/builder.rs +++ b/server/src/builder.rs @@ -173,7 +173,7 @@ async fn build_edge(args: &EdgeArgs) -> EdgeResult { .iter() .filter(|candidate| candidate.value().token_type == Some(TokenType::Client)) { - let _ = feature_refresher + feature_refresher .register_token_for_refresh(validated_token.clone(), None) .await; } diff --git a/server/src/http/feature_refresher.rs b/server/src/http/feature_refresher.rs index 8f153d35..e0ebe8ae 100644 --- a/server/src/http/feature_refresher.rs +++ b/server/src/http/feature_refresher.rs @@ -111,7 +111,7 @@ impl FeatureRefresher { } async fn register_and_hydrate_token(&self, token: &EdgeToken) -> EdgeResult { - self.register_token_for_refresh(token.clone(), None).await?; + self.register_token_for_refresh(token.clone(), None).await; self.hydrate_new_tokens().await; self.get_filtered_features(token) .ok_or(EdgeError::ClientFeaturesFetchError(FeatureError::Retriable)) @@ -145,12 +145,8 @@ impl FeatureRefresher { } /// - /// Registers a token for refresh, returns true if token did not exist and as such needs hydration before we can guarantee that we have data for it - pub async fn register_token_for_refresh( - &self, - token: EdgeToken, - etag: Option, - ) -> EdgeResult<()> { + /// Registers a token for refresh, the token will be discarded if it can be subsumed by another previously registered token + pub async fn register_token_for_refresh(&self, token: EdgeToken, etag: Option) { if !self.tokens_to_refresh.contains_key(&token.token) { let _ = self .unleash_client @@ -173,9 +169,6 @@ impl FeatureRefresher { .insert(refreshes.token.token.clone(), refreshes.clone()); } self.tokens_to_refresh.retain(|key, _| keys.contains(key)); - Ok(()) - } else { - Ok(()) } } @@ -350,13 +343,47 @@ mod tests { ); let token = EdgeToken::try_from("*:development.abcdefghijklmnopqrstuvwxyz".to_string()).unwrap(); - let _ = feature_refresher + feature_refresher .register_token_for_refresh(token, None) .await; assert_eq!(feature_refresher.tokens_to_refresh.len(), 1); } + #[tokio::test] + pub async fn registering_multiple_tokens_with_same_environment_reduces_tokens_to_valid_minimal_set( + ) { + let unleash_client = UnleashClient::from_url( + Url::parse("http://localhost:4242").unwrap(), + false, + None, + None, + ); + let features_cache = Arc::new(DashMap::default()); + let engines_cache = Arc::new(DashMap::default()); + + let duration = Duration::seconds(5); + let feature_refresher = FeatureRefresher::new( + Arc::new(unleash_client), + features_cache, + engines_cache, + duration, + None, + ); + let token1 = + EdgeToken::try_from("*:development.abcdefghijklmnopqrstuvwxyz".to_string()).unwrap(); + let token2 = + EdgeToken::try_from("*:development.zyxwvutsrqponmlkjihgfedcba".to_string()).unwrap(); + feature_refresher + .register_token_for_refresh(token1, None) + .await; + feature_refresher + .register_token_for_refresh(token2, None) + .await; + + assert_eq!(feature_refresher.tokens_to_refresh.len(), 1); + } + #[tokio::test] pub async fn registering_multiple_non_overlapping_tokens_will_keep_all() { let unleash_client = UnleashClient::from_url( @@ -384,13 +411,13 @@ mod tests { let project_c_token = EdgeToken::try_from("projectc:development.abcdefghijklmnopqrstuvwxyz".to_string()) .unwrap(); - let _ = feature_refresher + feature_refresher .register_token_for_refresh(project_a_token, None) .await; - let _ = feature_refresher + feature_refresher .register_token_for_refresh(project_b_token, None) .await; - let _ = feature_refresher + feature_refresher .register_token_for_refresh(project_c_token, None) .await; @@ -427,16 +454,16 @@ mod tests { let wildcard_token = EdgeToken::try_from("*:development.abcdefghijklmnopqrstuvwxyz".to_string()).unwrap(); - let _ = feature_refresher + feature_refresher .register_token_for_refresh(project_a_token, None) .await; - let _ = feature_refresher + feature_refresher .register_token_for_refresh(project_b_token, None) .await; - let _ = feature_refresher + feature_refresher .register_token_for_refresh(project_c_token, None) .await; - let _ = feature_refresher + feature_refresher .register_token_for_refresh(wildcard_token, None) .await; @@ -477,16 +504,16 @@ mod tests { EdgeToken::try_from("[]:development.abcdefghijklmnopqrstuvwxyz".to_string()).unwrap(); project_a_and_c_token.projects = vec!["projecta".into(), "projectc".into()]; - let _ = feature_refresher + feature_refresher .register_token_for_refresh(project_a_token, None) .await; - let _ = feature_refresher + feature_refresher .register_token_for_refresh(project_b_token, None) .await; - let _ = feature_refresher + feature_refresher .register_token_for_refresh(project_c_token, None) .await; - let _ = feature_refresher + feature_refresher .register_token_for_refresh(project_a_and_c_token, None) .await; @@ -524,10 +551,10 @@ mod tests { EdgeToken::try_from("projecta:development.abcdefghijklmnopqrstuvwxyz".to_string()) .unwrap(); - let _ = feature_refresher + feature_refresher .register_token_for_refresh(star_token, None) .await; - let _ = feature_refresher + feature_refresher .register_token_for_refresh(project_a_token, None) .await; @@ -561,10 +588,10 @@ mod tests { .unwrap(); let production_wildcard_token = EdgeToken::try_from("*:production.abcdefghijklmnopqrstuvwxyz".to_string()).unwrap(); - let _ = feature_refresher + feature_refresher .register_token_for_refresh(project_a_token, None) .await; - let _ = feature_refresher + feature_refresher .register_token_for_refresh(production_wildcard_token, None) .await; assert_eq!(feature_refresher.tokens_to_refresh.len(), 2); @@ -681,7 +708,7 @@ mod tests { let mut token = EdgeToken::try_from("*:development.secret123".to_string()).unwrap(); token.status = Validated; token.token_type = Some(TokenType::Client); - let _ = feature_refresher + feature_refresher .register_token_for_refresh(token, None) .await; assert!(!feature_refresher.tokens_to_refresh.is_empty()); @@ -717,7 +744,7 @@ mod tests { let unleash_client = UnleashClient::new(server.url("/").as_str(), None).unwrap(); let mut feature_refresher = FeatureRefresher::with_client(Arc::new(unleash_client)); feature_refresher.refresh_interval = Duration::seconds(0); - let _ = feature_refresher + feature_refresher .register_token_for_refresh(valid_token.clone(), None) .await; assert!(!feature_refresher.tokens_to_refresh.is_empty()); @@ -762,10 +789,10 @@ mod tests { let unleash_client = UnleashClient::new(server.url("/").as_str(), None).unwrap(); let mut feature_refresher = FeatureRefresher::with_client(Arc::new(unleash_client)); feature_refresher.refresh_interval = Duration::seconds(0); - let _ = feature_refresher + feature_refresher .register_token_for_refresh(dx_token.clone(), None) .await; - let _ = feature_refresher + feature_refresher .register_token_for_refresh(eg_token.clone(), None) .await; assert_eq!(feature_refresher.tokens_to_refresh.len(), 2); diff --git a/server/src/tokens.rs b/server/src/tokens.rs index b6ef959f..0d8cd5fb 100644 --- a/server/src/tokens.rs +++ b/server/src/tokens.rs @@ -3,6 +3,7 @@ use actix_web::http::header::HeaderValue; use actix_web::web::Data; use actix_web::FromRequest; use actix_web::HttpRequest; +use std::collections::HashSet; use std::future::{ready, Ready}; use std::str::FromStr; @@ -13,11 +14,12 @@ use crate::types::EdgeToken; use crate::types::TokenRefresh; use crate::types::TokenValidationStatus; -pub(crate) fn simplify(tokens: &[TokenRefresh]) -> Vec<&TokenRefresh> { - tokens +pub(crate) fn simplify(tokens: &[TokenRefresh]) -> Vec { + let uniques = filter_unique_tokens(tokens); + uniques .iter() .filter_map(|token| { - tokens.iter().fold(Some(token), |acc, current| { + uniques.iter().fold(Some(token), |acc, current| { acc.and_then(|lead| { if current.token.token != lead.token.token && current.token.subsumes(&lead.token) @@ -29,9 +31,28 @@ pub(crate) fn simplify(tokens: &[TokenRefresh]) -> Vec<&TokenRefresh> { }) }) }) + .cloned() .collect() } +fn filter_unique_tokens(tokens: &[TokenRefresh]) -> Vec { + let mut unique_tokens = Vec::new(); + let mut unique_keys = HashSet::new(); + + for token in tokens { + let key = ( + token.token.projects.clone(), + token.token.environment.clone(), + ); + if !unique_keys.contains(&key) { + unique_tokens.push(token.clone()); + unique_keys.insert(key); + } + } + + unique_tokens +} + pub(crate) fn cache_key(token: &EdgeToken) -> String { token .environment @@ -259,4 +280,25 @@ mod tests { assert_eq!(actual, expected); } + + #[test] + fn test_case_5_when_two_tokens_share_environments_and_products_we_return_only_the_first() { + let tokens: Vec = vec![ + test_token(Some("abcdefghijklmnopqrst"), Some("development"), vec!["*"]), + test_token(Some("tsrqponmlkjihgfedcba"), Some("development"), vec!["*"]), + ] + .into_iter() + .map(|t| TokenRefresh::new(t, None)) + .collect(); + + let expected = vec![test_token( + Some("abcdefghijklmnopqrst"), + Some("development"), + vec!["*"], + )]; + + let actual: Vec = simplify(&tokens).iter().map(|x| x.token.clone()).collect(); + + assert_eq!(actual, expected); + } }