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

feat: start using instance_id argument for instance_id header #616

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: start using instance_id argument for instance_id header #616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2c421e
feat: start using instance_id argument for instance_id header in http…
chriswk Dec 20, 2024
eec2fea
feat: Make ClientMetaInformation to avoid problems with ordering of a…
chriswk Dec 20, 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
33 changes: 26 additions & 7 deletions server/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use unleash_yggdrasil::EngineState;
use crate::cli::RedisMode;
use crate::feature_cache::FeatureCache;
use crate::http::feature_refresher::{FeatureRefreshConfig, FeatureRefresherMode};
use crate::http::unleash_client::new_reqwest_client;
use crate::http::unleash_client::{new_reqwest_client, ClientMetaInformation};
use crate::offline::offline_hotload::{load_bootstrap, load_offline_engine_cache};
use crate::persistence::file::FilePersister;
use crate::persistence::redis::RedisPersister;
Expand Down Expand Up @@ -218,7 +218,10 @@ async fn get_data_source(args: &EdgeArgs) -> Option<Arc<dyn EdgePersistence>> {
None
}

async fn build_edge(args: &EdgeArgs, app_name: &str) -> EdgeResult<EdgeInfo> {
async fn build_edge(
args: &EdgeArgs,
client_meta_information: ClientMetaInformation,
) -> EdgeResult<EdgeInfo> {
if !args.strict {
if !args.dynamic {
error!("You should explicitly opt into either strict or dynamic behavior. Edge has defaulted to dynamic to preserve legacy behavior, however we recommend using strict from now on. Not explicitly opting into a behavior will return an error on startup in a future release");
Expand All @@ -237,13 +240,12 @@ async fn build_edge(args: &EdgeArgs, app_name: &str) -> EdgeResult<EdgeInfo> {
let persistence = get_data_source(args).await;

let http_client = new_reqwest_client(
"unleash_edge".into(),
args.skip_ssl_verification,
args.client_identity.clone(),
args.upstream_certificate_file.clone(),
Duration::seconds(args.upstream_request_timeout),
Duration::seconds(args.upstream_socket_timeout),
app_name.into(),
client_meta_information.clone(),
)?;

let unleash_client = Url::parse(&args.upstream_url.clone())
Expand All @@ -267,7 +269,7 @@ async fn build_edge(args: &EdgeArgs, app_name: &str) -> EdgeResult<EdgeInfo> {
let feature_config = FeatureRefreshConfig::new(
Duration::seconds(args.features_refresh_interval_seconds as i64),
refresher_mode,
app_name.to_string(),
client_meta_information,
);
let feature_refresher = Arc::new(FeatureRefresher::new(
unleash_client,
Expand Down Expand Up @@ -315,7 +317,16 @@ pub async fn build_caches_and_refreshers(args: CliArgs) -> EdgeResult<EdgeInfo>
EdgeMode::Offline(offline_args) => {
build_offline(offline_args).map(|cache| (cache, None, None, None))
}
EdgeMode::Edge(edge_args) => build_edge(&edge_args, &args.app_name).await,
EdgeMode::Edge(edge_args) => {
build_edge(
&edge_args,
ClientMetaInformation {
app_name: args.app_name,
instance_id: args.instance_id,
},
)
.await
}
_ => unreachable!(),
}
}
Expand All @@ -325,6 +336,7 @@ mod tests {
use crate::{
builder::{build_edge, build_offline},
cli::{EdgeArgs, OfflineArgs, TokenHeader},
http::unleash_client::ClientMetaInformation,
};

#[test]
Expand Down Expand Up @@ -375,7 +387,14 @@ mod tests {
streaming: false,
};

let result = build_edge(&args, "test-app").await;
let result = build_edge(
&args,
ClientMetaInformation {
app_name: "test-app".into(),
instance_id: "test-instance-id".into(),
},
)
.await;
assert!(result.is_err());
assert_eq!(
result.err().unwrap().to_string(),
Expand Down
2 changes: 1 addition & 1 deletion server/src/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ pub struct CliArgs {
pub mode: EdgeMode,

/// Instance id. Used for metrics reporting.
#[clap(long, env, default_value_t = ulid::Ulid::new().to_string())]
#[clap(long, env, default_value_t = format!("unleash-edge@{}", ulid::Ulid::new()))]
pub instance_id: String,

/// App name. Used for metrics reporting.
Expand Down
4 changes: 2 additions & 2 deletions server/src/client_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ mod tests {

use crate::auth::token_validator::TokenValidator;
use crate::cli::{OfflineArgs, TokenHeader};
use crate::http::unleash_client::UnleashClient;
use crate::http::unleash_client::{UnleashClient, ClientMetaInformation};
use crate::middleware;
use crate::tests::{features_from_disk, upstream_server};
use actix_http::{Request, StatusCode};
Expand Down Expand Up @@ -1012,7 +1012,7 @@ mod tests {
persistence: None,
strict: false,
streaming: false,
app_name: "test-app".into(),
client_meta_information: ClientMetaInformation::test_config(),
});
let token_validator = Arc::new(TokenValidator {
unleash_client: unleash_client.clone(),
Expand Down
30 changes: 15 additions & 15 deletions server/src/http/feature_refresher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use crate::{
types::{ClientFeaturesRequest, ClientFeaturesResponse, EdgeToken, TokenRefresh},
};

use super::unleash_client::UnleashClient;
use super::unleash_client::{ClientMetaInformation, UnleashClient};

fn frontend_token_is_covered_by_tokens(
frontend_token: &EdgeToken,
Expand All @@ -48,7 +48,7 @@ pub struct FeatureRefresher {
pub persistence: Option<Arc<dyn EdgePersistence>>,
pub strict: bool,
pub streaming: bool,
pub app_name: String,
pub client_meta_information: ClientMetaInformation,
}

impl Default for FeatureRefresher {
Expand All @@ -62,21 +62,21 @@ impl Default for FeatureRefresher {
persistence: None,
strict: true,
streaming: false,
app_name: "unleash_edge".into(),
client_meta_information: Default::default(),
}
}
}

fn client_application_from_token_and_name(
token: EdgeToken,
refresh_interval: i64,
app_name: &str,
client_meta_information: ClientMetaInformation,
) -> ClientApplication {
ClientApplication {
app_name: app_name.into(),
app_name: client_meta_information.app_name,
connect_via: None,
environment: token.environment,
instance_id: None,
instance_id: Some(client_meta_information.instance_id),
interval: refresh_interval as u32,
started: Utc::now(),
strategies: vec![],
Expand All @@ -99,19 +99,19 @@ pub enum FeatureRefresherMode {
pub struct FeatureRefreshConfig {
features_refresh_interval: chrono::Duration,
mode: FeatureRefresherMode,
app_name: String,
client_meta_information: ClientMetaInformation,
}

impl FeatureRefreshConfig {
pub fn new(
features_refresh_interval: chrono::Duration,
mode: FeatureRefresherMode,
app_name: String,
client_meta_information: ClientMetaInformation,
) -> Self {
Self {
features_refresh_interval,
mode,
app_name,
client_meta_information,
}
}
}
Expand All @@ -133,7 +133,7 @@ impl FeatureRefresher {
persistence,
strict: config.mode != FeatureRefresherMode::Dynamic,
streaming: config.mode == FeatureRefresherMode::Streaming,
app_name: config.app_name,
client_meta_information: config.client_meta_information,
}
}

Expand Down Expand Up @@ -254,7 +254,7 @@ impl FeatureRefresher {
client_application_from_token_and_name(
token.clone(),
self.refresh_interval.num_seconds(),
&self.app_name,
self.client_meta_information.clone(),
),
)
.await
Expand All @@ -277,6 +277,7 @@ impl FeatureRefresher {
pub async fn start_streaming_features_background_task(
&self,
app_name: String,
instance_id: String,
custom_headers: Vec<(String, String)>,
Comment on lines 279 to 281
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh, this is a little sticky again. Having two very similar arguments with very similar values of the same type next to each other is a little dodgy. It'll be easy to flip them around. Can we chuck 'em in a config struct or use the type system to differentiate them?

) -> anyhow::Result<()> {
use anyhow::Context;
Expand All @@ -290,7 +291,7 @@ impl FeatureRefresher {
.context("Failed to create EventSource client for streaming")?
.header("Authorization", &token.token)?
.header(UNLEASH_APPNAME_HEADER, &app_name)?
.header(UNLEASH_INSTANCE_ID_HEADER, "unleash_edge")?
.header(UNLEASH_INSTANCE_ID_HEADER, &instance_id)?
.header(
UNLEASH_CLIENT_SPEC_HEADER,
unleash_yggdrasil::SUPPORTED_SPEC_VERSION,
Expand Down Expand Up @@ -557,7 +558,7 @@ mod tests {

use crate::feature_cache::{update_projects_from_feature_update, FeatureCache};
use crate::filters::{project_filter, FeatureFilterSet};
use crate::http::unleash_client::new_reqwest_client;
use crate::http::unleash_client::{new_reqwest_client, ClientMetaInformation};
use crate::tests::features_from_disk;
use crate::tokens::cache_key;
use crate::types::TokenValidationStatus::Validated;
Expand All @@ -580,13 +581,12 @@ mod tests {

fn create_test_client() -> UnleashClient {
let http_client = new_reqwest_client(
"unleash_edge".into(),
false,
None,
None,
Duration::seconds(5),
Duration::seconds(5),
"test-client".into(),
ClientMetaInformation::test_config(),
)
.expect("Failed to create client");

Expand Down
Loading
Loading