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

fix: allow multiple client tokens at startup #188

Merged
merged 1 commit into from
May 18, 2023
Merged

fix: allow multiple client tokens at startup #188

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
2 changes: 1 addition & 1 deletion server/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ async fn build_edge(args: &EdgeArgs) -> EdgeResult<EdgeInfo> {
.iter()
.filter(|candidate| candidate.value().token_type == Some(TokenType::Client))
{
let _ = feature_refresher
feature_refresher
.register_token_for_refresh(validated_token.clone(), None)
.await;
}
Expand Down
87 changes: 57 additions & 30 deletions server/src/http/feature_refresher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ impl FeatureRefresher {
}

async fn register_and_hydrate_token(&self, token: &EdgeToken) -> EdgeResult<ClientFeatures> {
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))
Expand Down Expand Up @@ -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
Copy link
Member Author

@sighphyre sighphyre May 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Patches to this function are just for sanity. The doc string got outdated here and we never updated it. This function could also never fail - there was no path to raise an error but the function signature suggested that it could, so now the return signature reflects that . You'll see a few places in this PR where

let _ = feature_refresher has been swapped for feature_refresher, which is married to the change to this function

pub async fn register_token_for_refresh(
&self,
token: EdgeToken,
etag: Option<EntityTag>,
) -> 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<EntityTag>) {
if !self.tokens_to_refresh.contains_key(&token.token) {
let _ = self
.unleash_client
Expand All @@ -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(())
}
}

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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);
Expand Down
48 changes: 45 additions & 3 deletions server/src/tokens.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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<TokenRefresh> {
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)
Expand All @@ -29,9 +31,28 @@ pub(crate) fn simplify(tokens: &[TokenRefresh]) -> Vec<&TokenRefresh> {
})
})
})
.cloned()
.collect()
}

fn filter_unique_tokens(tokens: &[TokenRefresh]) -> Vec<TokenRefresh> {
Copy link
Member Author

Choose a reason for hiding this comment

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

The TokenRefresh type already has a Hash impl, so we gotta go the long way around to make a unique list here

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
Expand Down Expand Up @@ -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<TokenRefresh> = 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<EdgeToken> = simplify(&tokens).iter().map(|x| x.token.clone()).collect();

assert_eq!(actual, expected);
}
}