hubcio commented on code in PR #4218:
URL: https://github.com/apache/iggy/pull/4218#discussion_r4045535152
##########
core/configs/src/server_config/validators.rs:
##########
@@ -407,29 +410,115 @@ impl ServerConfig {
/// The listener the client-facing address is derived from must not bind a
/// wildcard unless that address is declared outright.
+ ///
+ /// When running inside a container, a loopback listener means the server
+ /// is unreachable from outside the container, which is warned.
fn validate_client_facing_address(&self) -> Result<(), ConfigurationError>
{
- if self.cluster.enabled || self.node.advertised_address.is_some() {
- return Ok(());
+ self.validate_client_facing_address_in_env(is_container())?;
Review Comment:
nit: `is_container()` runs on every boot, even when the first check returns.
move the probe into the loopback branch.
##########
core/configs/src/server_config/validators.rs:
##########
@@ -407,29 +410,115 @@ impl ServerConfig {
/// The listener the client-facing address is derived from must not bind a
/// wildcard unless that address is declared outright.
+ ///
+ /// When running inside a container, a loopback listener means the server
+ /// is unreachable from outside the container, which is warned.
fn validate_client_facing_address(&self) -> Result<(), ConfigurationError>
{
- if self.cluster.enabled || self.node.advertised_address.is_some() {
- return Ok(());
+ self.validate_client_facing_address_in_env(is_container())?;
+ Ok(())
+ }
+
+ fn validate_client_facing_address_in_env(
+ &self,
+ is_container: bool,
+ ) -> Result<Option<String>, ConfigurationError> {
+ if self.cluster.enabled {
+ return Ok(None);
}
// No client-facing listener runs, so no client dials this node and
// there is no address to demand.
let Some(listener) = self.derived_address_listener() else {
- return Ok(());
+ return Ok(None);
};
let bind = parse_bind_address(listener.key, listener.address)?;
Review Comment:
nit: this drops the `advertised_address` shortcut, so a malformed listener
address now fails validation instead of boot. both paths refuse, so a commit
body note is enough.
##########
core/configs/src/server_config/validators.rs:
##########
@@ -534,6 +623,116 @@ mod tests {
assert!(config.validate().is_ok());
}
+ #[test]
+ fn given_loopback_bind_in_container_when_validating_should_warn_and_pass()
{
+ let config = config_with_override(
+ "[tcp]\naddress = \"127.0.0.1:8090\"\n[cluster]\nenabled =
false\n",
+ );
+ let warning = config
+ .validate_client_facing_address_in_env(true)
+ .expect("validation should pass");
+ assert!(
+ warning.is_some(),
+ "loopback inside container must produce a warning"
+ );
+ let message = warning.unwrap();
+ assert!(message.contains("IGGY_TCP_ADDRESS=0.0.0.0:8090"));
+ assert!(message.contains("together with
IGGY_NODE_ADVERTISED_ADDRESS"));
+ }
+
+ #[test]
+ fn
given_loopback_bind_outside_container_when_validating_should_pass_without_warning()
{
+ let config = config_with_override(
+ "[tcp]\naddress = \"127.0.0.1:8090\"\n[cluster]\nenabled =
false\n",
+ );
+ let warning = config
+ .validate_client_facing_address_in_env(false)
+ .expect("validation should pass");
+ assert!(
+ warning.is_none(),
+ "loopback outside container must not produce a warning"
+ );
+ }
+
+ #[test]
+ fn
given_loopback_bind_in_container_with_advertised_address_when_validating_should_warn_and_pass()
+ {
+ let config = config_with_override(
+ "[tcp]\naddress = \"127.0.0.1:8090\"\n[cluster]\nenabled = false\n\
+ [node]\nadvertised_address = \"broker-1.example.com\"\n",
+ );
+ let warning = config
+ .validate_client_facing_address_in_env(true)
+ .expect("validation should pass");
+ assert!(
+ warning.is_some(),
+ "loopback inside container must produce a warning"
+ );
+ let message = warning.unwrap();
+ assert!(message.contains("IGGY_TCP_ADDRESS=0.0.0.0:8090"));
+ assert!(!message.contains("together with
IGGY_NODE_ADVERTISED_ADDRESS"));
+ }
+
+ #[test]
+ fn given_dockerenv_file_when_checking_container_should_return_true() {
+ let temp_dir = std::env::temp_dir();
+ let marker = temp_dir.join(format!("test_dockerenv_{}",
std::process::id()));
+ std::fs::write(&marker, "").unwrap();
+ let non_existent = temp_dir.join("non_existent_indicator");
+ let result =
+ is_container_indicators(&marker, &non_existent, None, None,
"/non/existent/cgroup");
+ let _ = std::fs::remove_file(&marker);
+ assert!(result);
+ }
+
+ #[test]
+ fn given_containerenv_file_when_checking_container_should_return_true() {
+ let temp_dir = std::env::temp_dir();
+ let marker = temp_dir.join(format!("test_containerenv_{}",
std::process::id()));
+ std::fs::write(&marker, "").unwrap();
+ let non_existent = temp_dir.join("non_existent_indicator");
+ let result =
+ is_container_indicators(&non_existent, &marker, None, None,
"/non/existent/cgroup");
+ let _ = std::fs::remove_file(&marker);
+ assert!(result);
+ }
Review Comment:
simplification: both tests write marker files into the shared temp dir and
ignore the cleanup error. use one temp dir for the pair.
##########
core/configs/src/server_config/validators.rs:
##########
@@ -407,29 +410,115 @@ impl ServerConfig {
/// The listener the client-facing address is derived from must not bind a
/// wildcard unless that address is declared outright.
+ ///
+ /// When running inside a container, a loopback listener means the server
+ /// is unreachable from outside the container, which is warned.
fn validate_client_facing_address(&self) -> Result<(), ConfigurationError>
{
- if self.cluster.enabled || self.node.advertised_address.is_some() {
- return Ok(());
+ self.validate_client_facing_address_in_env(is_container())?;
+ Ok(())
+ }
+
+ fn validate_client_facing_address_in_env(
+ &self,
+ is_container: bool,
+ ) -> Result<Option<String>, ConfigurationError> {
+ if self.cluster.enabled {
+ return Ok(None);
}
// No client-facing listener runs, so no client dials this node and
// there is no address to demand.
let Some(listener) = self.derived_address_listener() else {
Review Comment:
warning: this checks the derived listener only, so once the operator follows
the advice and sets `IGGY_TCP_ADDRESS=0.0.0.0:8090` with
`IGGY_NODE_ADVERTISED_ADDRESS`, the other three loopback binds go silent.
`core/server/src/boot/listeners.rs:118` already loops all four for the same
reason.
##########
core/configs/src/server_config/validators.rs:
##########
@@ -407,29 +410,115 @@ impl ServerConfig {
/// The listener the client-facing address is derived from must not bind a
/// wildcard unless that address is declared outright.
+ ///
+ /// When running inside a container, a loopback listener means the server
+ /// is unreachable from outside the container, which is warned.
fn validate_client_facing_address(&self) -> Result<(), ConfigurationError>
{
- if self.cluster.enabled || self.node.advertised_address.is_some() {
- return Ok(());
+ self.validate_client_facing_address_in_env(is_container())?;
+ Ok(())
+ }
+
+ fn validate_client_facing_address_in_env(
+ &self,
+ is_container: bool,
+ ) -> Result<Option<String>, ConfigurationError> {
+ if self.cluster.enabled {
+ return Ok(None);
}
// No client-facing listener runs, so no client dials this node and
// there is no address to demand.
let Some(listener) = self.derived_address_listener() else {
- return Ok(());
+ return Ok(None);
};
let bind = parse_bind_address(listener.key, listener.address)?;
- if !bind.ip().to_canonical().is_unspecified() {
- return Ok(());
+ let ip = bind.ip().to_canonical();
+ if ip.is_unspecified() {
+ if self.node.advertised_address.is_none() {
+ eprintln!(
+ "{COMPONENT} - {} binds the wildcard {bind}, which says
which interfaces this node \
+ accepts on rather than where a client reaches it, so
cluster metadata would carry no \
+ address for this node. Set node.advertised_address to the
address clients dial, or \
+ bind a concrete address.",
+ listener.key
+ );
+ return Err(ConfigurationError::InvalidConfigurationValue);
+ }
+ return Ok(None);
}
- eprintln!(
- "{COMPONENT} - {} binds the wildcard {bind}, which says which
interfaces this node \
- accepts on rather than where a client reaches it, so cluster
metadata would carry no \
- address for this node. Set node.advertised_address to the address
clients dial, or \
- bind a concrete address.",
- listener.key
- );
- Err(ConfigurationError::InvalidConfigurationValue)
+ if ip.is_loopback() && is_container {
Review Comment:
warning: the message asserts a reachability outcome the check cannot know -
`--network host`, a pod with `hostNetwork: true` and a client sidecar all reach
`127.0.0.1` while `is_container()` stays true. say `outside this network
namespace` instead, and drop the `0.0.0.0` hint in that mode, where it widens
the port on every host interface.
##########
core/configs/src/server_config/validators.rs:
##########
@@ -407,29 +410,115 @@ impl ServerConfig {
/// The listener the client-facing address is derived from must not bind a
/// wildcard unless that address is declared outright.
+ ///
+ /// When running inside a container, a loopback listener means the server
+ /// is unreachable from outside the container, which is warned.
fn validate_client_facing_address(&self) -> Result<(), ConfigurationError>
{
- if self.cluster.enabled || self.node.advertised_address.is_some() {
- return Ok(());
+ self.validate_client_facing_address_in_env(is_container())?;
+ Ok(())
+ }
+
+ fn validate_client_facing_address_in_env(
+ &self,
+ is_container: bool,
+ ) -> Result<Option<String>, ConfigurationError> {
+ if self.cluster.enabled {
+ return Ok(None);
}
// No client-facing listener runs, so no client dials this node and
// there is no address to demand.
let Some(listener) = self.derived_address_listener() else {
- return Ok(());
+ return Ok(None);
};
let bind = parse_bind_address(listener.key, listener.address)?;
- if !bind.ip().to_canonical().is_unspecified() {
- return Ok(());
+ let ip = bind.ip().to_canonical();
+ if ip.is_unspecified() {
+ if self.node.advertised_address.is_none() {
+ eprintln!(
+ "{COMPONENT} - {} binds the wildcard {bind}, which says
which interfaces this node \
+ accepts on rather than where a client reaches it, so
cluster metadata would carry no \
+ address for this node. Set node.advertised_address to the
address clients dial, or \
+ bind a concrete address.",
+ listener.key
+ );
+ return Err(ConfigurationError::InvalidConfigurationValue);
+ }
+ return Ok(None);
}
- eprintln!(
- "{COMPONENT} - {} binds the wildcard {bind}, which says which
interfaces this node \
- accepts on rather than where a client reaches it, so cluster
metadata would carry no \
- address for this node. Set node.advertised_address to the address
clients dial, or \
- bind a concrete address.",
- listener.key
- );
- Err(ConfigurationError::InvalidConfigurationValue)
+ if ip.is_loopback() && is_container {
+ let env_var = format!("IGGY_{}", listener.key.replace('.',
"_").to_uppercase());
Review Comment:
nit: the name is rebuilt by hand, so it stays correct only while no field
overrides `config_env(name)`. read it from
`ConfigEnvMappings::find_by_config_path()` instead.
##########
core/configs/src/server_config/validators.rs:
##########
@@ -407,29 +410,115 @@ impl ServerConfig {
/// The listener the client-facing address is derived from must not bind a
/// wildcard unless that address is declared outright.
+ ///
+ /// When running inside a container, a loopback listener means the server
+ /// is unreachable from outside the container, which is warned.
fn validate_client_facing_address(&self) -> Result<(), ConfigurationError>
{
- if self.cluster.enabled || self.node.advertised_address.is_some() {
- return Ok(());
+ self.validate_client_facing_address_in_env(is_container())?;
+ Ok(())
+ }
+
+ fn validate_client_facing_address_in_env(
+ &self,
+ is_container: bool,
+ ) -> Result<Option<String>, ConfigurationError> {
+ if self.cluster.enabled {
+ return Ok(None);
}
// No client-facing listener runs, so no client dials this node and
// there is no address to demand.
let Some(listener) = self.derived_address_listener() else {
- return Ok(());
+ return Ok(None);
};
let bind = parse_bind_address(listener.key, listener.address)?;
- if !bind.ip().to_canonical().is_unspecified() {
- return Ok(());
+ let ip = bind.ip().to_canonical();
+ if ip.is_unspecified() {
+ if self.node.advertised_address.is_none() {
+ eprintln!(
+ "{COMPONENT} - {} binds the wildcard {bind}, which says
which interfaces this node \
+ accepts on rather than where a client reaches it, so
cluster metadata would carry no \
+ address for this node. Set node.advertised_address to the
address clients dial, or \
+ bind a concrete address.",
+ listener.key
+ );
+ return Err(ConfigurationError::InvalidConfigurationValue);
+ }
+ return Ok(None);
}
- eprintln!(
- "{COMPONENT} - {} binds the wildcard {bind}, which says which
interfaces this node \
- accepts on rather than where a client reaches it, so cluster
metadata would carry no \
- address for this node. Set node.advertised_address to the address
clients dial, or \
- bind a concrete address.",
- listener.key
- );
- Err(ConfigurationError::InvalidConfigurationValue)
+ if ip.is_loopback() && is_container {
+ let env_var = format!("IGGY_{}", listener.key.replace('.',
"_").to_uppercase());
+ let port = bind.port();
+ let msg = if self.node.advertised_address.is_none() {
Review Comment:
simplification: the two `format!` arms repeat the same text and differ in
the `together with IGGY_NODE_ADVERTISED_ADDRESS` clause. build the hint once,
then format once.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
