-
Notifications
You must be signed in to change notification settings - Fork 93
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.
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(server): Use global config from file if provided #2458
Changes from 12 commits
dbc71f3
a320a33
dc8fe7a
d631912
c125ec5
aed5e6e
153c9f1
9367f89
9068be4
1a43645
3776ec6
2d56213
761198e
c3221b7
8fa4521
387c246
0fadcea
749306a
5281b67
68b0bbf
383d850
67d3f73
43e4b4c
e9b7272
5e94b76
9e0f3e3
4f87968
da63e18
a5e9c03
a7030d4
a5c0224
a6efd91
99f2acd
20a593e
17eba98
a6ada9f
a69eb2f
a4f5a14
c0e767d
c9f0ef9
c39f12c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ use std::borrow::Cow; | |
use std::sync::Arc; | ||
|
||
use relay_config::Config; | ||
use relay_config::RelayMode; | ||
use relay_dynamic_config::GlobalConfig; | ||
use relay_statsd::metric; | ||
use relay_system::{Addr, AsyncResponse, Controller, FromMessage, Interface, Service}; | ||
|
@@ -148,8 +149,39 @@ pub struct GlobalConfigService { | |
impl GlobalConfigService { | ||
/// Creates a new [`GlobalConfigService`]. | ||
pub fn new(config: Arc<Config>, upstream: Addr<UpstreamRelay>) -> Self { | ||
let (global_config_watch, _) = watch::channel(Arc::default()); | ||
let (internal_tx, internal_rx) = mpsc::channel(1); | ||
|
||
let (global_config_watch, _) = match ( | ||
config.relay_mode(), | ||
config.has_credentials(), | ||
config.global_config(), | ||
) { | ||
(RelayMode::Proxy | RelayMode::Static, true, None) | (RelayMode::Managed, true, _) => { | ||
relay_log::info!("global config service starting"); | ||
// This request will trigger the request intervals when internal_rx receives the | ||
// result from upstream. | ||
Self::request_global_config(upstream.clone(), internal_tx.clone()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should not make requests during initialization, as this can block relay startup. We should only do requests once the service is actually started, see my comment below in the |
||
watch::channel(Arc::new(GlobalConfig::default())) | ||
} | ||
(RelayMode::Proxy | RelayMode::Static, false, None) | ||
| (RelayMode::Managed, false, _) => { | ||
// NOTE(iker): not making a request results in the sleep handler | ||
// not being reset, so no new requests are made. | ||
relay_log::info!("global config service starting with fetching disabled: no credentials configured"); | ||
watch::channel(Arc::new(GlobalConfig::default())) | ||
} | ||
(RelayMode::Proxy | RelayMode::Static | RelayMode::Capture, _, Some(global_config)) => { | ||
relay_log::info!("global config service starting with fetching disabled: using static global config"); | ||
watch::channel(global_config.clone()) | ||
} | ||
(RelayMode::Capture, _, None) => { | ||
relay_log::info!( | ||
"global config service starting with fetching disabled: using default config" | ||
); | ||
watch::channel(Arc::new(GlobalConfig::default())) | ||
} | ||
}; | ||
TBS1996 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Self { | ||
config, | ||
global_config_watch, | ||
|
@@ -185,12 +217,10 @@ impl GlobalConfigService { | |
/// | ||
/// We check if we have credentials before sending, | ||
/// otherwise we would log an [`UpstreamRequestError::NoCredentials`] error. | ||
fn update_global_config(&mut self) { | ||
self.fetch_handle.reset(); | ||
|
||
let upstream_relay = self.upstream.clone(); | ||
let internal_tx = self.internal_tx.clone(); | ||
|
||
fn request_global_config( | ||
upstream_relay: Addr<UpstreamRelay>, | ||
internal_tx: mpsc::Sender<UpstreamQueryResult>, | ||
) { | ||
TBS1996 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
tokio::spawn(async move { | ||
metric!(timer(RelayTimers::GlobalConfigRequestDuration), { | ||
let query = GetGlobalConfig::new(); | ||
|
@@ -209,7 +239,7 @@ impl GlobalConfigService { | |
/// 1. Whether the request to the upstream was successful. | ||
/// 2. If the request was successful, it then checks whether the returned | ||
/// global config is valid and contains the expected data. | ||
fn handle_result(&mut self, result: UpstreamQueryResult) { | ||
fn handle_upstream_result(&mut self, result: UpstreamQueryResult) { | ||
match result { | ||
Ok(Ok(config)) => { | ||
let mut success = false; | ||
|
@@ -253,25 +283,15 @@ impl Service for GlobalConfigService { | |
tokio::spawn(async move { | ||
let mut shutdown_handle = Controller::shutdown_handle(); | ||
|
||
relay_log::info!("global config service starting"); | ||
|
||
if self.config.has_credentials() { | ||
iker-barriocanal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// NOTE(iker): if this first request fails it's possible the default | ||
// global config is forwarded. This is not ideal, but we accept it | ||
// for now. | ||
self.update_global_config(); | ||
} else { | ||
// NOTE(iker): not making a request results in the sleep handler | ||
// not being reset, so no new requests are made. | ||
relay_log::info!("fetching global configs disabled: no credentials configured"); | ||
} | ||
|
||
loop { | ||
tokio::select! { | ||
biased; | ||
|
||
() = &mut self.fetch_handle => self.update_global_config(), | ||
Some(result) = self.internal_rx.recv() => self.handle_result(result), | ||
() = &mut self.fetch_handle => { | ||
Self::request_global_config(self.upstream.clone(), self.internal_tx.clone()); | ||
// Disable new requests interval until we receive the result from upstream. | ||
self.fetch_handle.reset(); | ||
} | ||
Some(result) = self.internal_rx.recv() => self.handle_upstream_result(result), | ||
Some(message) = rx.recv() => self.handle_message(message), | ||
_ = shutdown_handle.notified() => self.handle_shutdown(), | ||
|
||
|
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.
wasn't 100% sure if i should put it here or not. My reasoning was that if we fail to parse the file, it's better to fail early than later, and it just seems idiomatic within the codebase that we don't do these kind of risky IO-stuff in the services.
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.
We have a precedent where we defer the deserialization of a static config file to a service:
relay/relay-config/src/config.rs
Lines 1916 to 1919 in 498401a
That service reloads the config every 10 seconds though, which I think is overkill. In short, I think your way of doing it is better because it fails early. I'll leave final review to @jan-auer.
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.
I concur with this: We can load the global configs directly from within the actual service like we do it for project configs. That keeps the responsibility of the
relay-config
crate lower. The overall implementation in this PR does look good, so it would just have to be moved.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.
alright, I moved it now to the spawn handler, where I'll log an error if the file exists yet it fails to load it.