hubcio commented on code in PR #3818:
URL: https://github.com/apache/iggy/pull/3818#discussion_r3719578728
##########
core/configs/src/server_ng_config/cluster.rs:
##########
@@ -933,6 +1121,84 @@ impl Validatable<ConfigurationError> for ClusterConfig {
}
}
+/// One advertised client endpoint and the client network it serves, pooled by
+/// [`ClusterConfig::validate`] so selectors and catch-all conflict-check
+/// against each other. `network: None` is the catch-all (`advertised_address`,
+/// or the roster `ip` as fallback): it matches every client, so it overlaps
+/// every network.
+struct AdvertisedEndpoint<'roster> {
+ node_name: &'roster str,
+ transport: &'static str,
+ network: Option<IpNet>,
+ host: AdvertisedHost<'roster>,
+ port: u16,
+}
+
+/// Advertised host in comparable form. `Raw` carries a roster `ip` that
+/// failed the strict [`AdvertisedAddress`] parse, so it can never equal a
+/// `Parsed` host.
+#[derive(PartialEq, Eq)]
+enum AdvertisedHost<'roster> {
+ Parsed(AdvertisedAddress),
+ Raw(&'roster str),
+}
+
+impl AdvertisedEndpoint<'_> {
+ /// True when some client would resolve both entries to one host:port on
+ /// two different nodes. Entries of one node never conflict: longest-prefix
+ /// match picks exactly one selector per transport, and the bind-endpoint
+ /// check already forces distinct ports across a node's transports.
+ fn conflicts_with(&self, other: &Self) -> bool {
+ if self.node_name == other.node_name || self.port != other.port ||
self.host != other.host {
+ return false;
+ }
+ match (self.network, other.network) {
+ (Some(network), Some(other_network)) => {
+ network.contains(&other_network) ||
other_network.contains(&network)
+ }
+ _ => true,
Review Comment:
the `_ => true` arm treats a catch-all as overlapping every network, but a
node's catch-all really serves only clients matching none of that node's own
selectors. this rejects a legal roster at boot:
- n1: selector `10.0.0.0/8` -> `192.0.2.10`, tcp 8090
- n2: `advertised_address = "192.0.2.10"` plus its own `10.0.0.0/8` selector
-> `192.0.2.20`, tcp 8090
no client resolves both nodes to `192.0.2.10:8090` (inside 10/8 both
selectors win LPM, outside 10/8 n1 serves its own catch-all), yet n2's
catch-all entry hits n1's pooled selector and boot dies - we reproduced this
against the transcribed pool build with real ipnet, and the controls behave (a
genuine conflict is still rejected, the disjoint-network reuse from
`validate_accepts_identical_selector_endpoint_across_different_cidrs` still
passes). that test blesses exactly this NAT pattern in the selector-vs-selector
spelling, so the validator accepts one spelling and refuses the other, and the
error message describes a client in the overlap that cannot exist. there is
also no workaround, since "everything except 10/8" is not expressible as a cidr.
sound minimal fix: give each catch-all entry its node's own selector nets,
and skip the selector-vs-catch-all conflict when the selector's net is
contained in one of them (every client in it resolves that node through the
longer match, never the catch-all). needs a small reorder: catch-all endpoints
are inserted before the node's selectors are parsed, so the shadow set has to
be collected in a per-node pre-pass first.
##########
core/configs/src/server_ng_config/cluster.rs:
##########
@@ -367,6 +368,12 @@ pub struct ClusterNodeConfig {
/// to use [`Self::ip`].
#[serde(default)]
pub advertised_address: Option<String>,
+ /// Client-network-scoped overrides of [`Self::advertised_address`],
+ /// resolved by longest-prefix match over the client's IP (see
+ /// [`AdvertisedAddressSelector`]). Empty by default, so existing configs
+ /// keep the single catch-all address.
+ #[serde(default)]
+ pub advertised_addresses: Vec<AdvertisedAddressSelector>,
Review Comment:
this field is the first Vec-of-struct nested inside another Vec-of-struct in
the config tree, and the `ConfigEnv` derive expands every nested vec over a
fixed `0..256` range without looking at the actual length. the two ceilings
multiply: `ClusterConfig::env_mappings()` grows from ~2.3k to ~133.4k entries
(131,072 of them, 98%, from this field alone), each entry `Box::leak`ing its
strings. measured on the prebuilt configs test binary via getrusage (baseline
test vs one forcing `env_mappings()`): ~44-50MB extra maxRSS and ~51ms, paid on
every server-ng boot even with zero selectors configured. it also feeds
`find_similar_vars`, which we measured going from ~5ms to ~177ms per unknown
`IGGY_` env var in release.
cheapest fix: a per-field cap attribute in the derive, e.g.
`#[config_env(max_elements = 16)]` on this field (the attribute doesn't exist
yet, ~10 lines in `configs_derive`). prefer 16 over 8: overshooting the cap
turns a legitimate `..._ADVERTISED_ADDRESSES_16_*` var into an unknown one, and
`typed_env_provider.rs` has a `debug_assert!(false)` on unknown `IGGY_` vars,
so a debug build would panic. `#[config_env(skip)]` is not an option - the new
e2e tests configure selectors exactly through these env vars. the derive
comment claiming `cluster.nodes` is the only nested-vec surface needs updating
too.
##########
core/configs/src/server_ng_config/cluster.rs:
##########
@@ -375,6 +382,132 @@ pub struct ClusterNodeConfig {
pub ports: TransportPorts,
}
+/// One client-network-scoped advertised address: clients whose IP falls
+/// inside `client_cidr` are told `address` instead of the node's catch-all
+/// [`ClusterNodeConfig::advertised_address`].
+///
+/// Typical split-network case: the roster `ip` is VPC-private and
+/// `advertised_address` is public; a selector with the VPC CIDR keeps
+/// in-VPC clients on the private address while everyone else stays on the
+/// public one. Selection is longest-prefix match across a node's selectors;
+/// a tie on prefix length resolves to the first declared selector. Selection
+/// sees the transport-level peer address, so clients arriving through a
+/// proxy or load balancer match the proxy's network, not their own.
+#[derive(Debug, Deserialize, Serialize, Clone, ConfigEnv)]
+#[serde(deny_unknown_fields)]
+pub struct AdvertisedAddressSelector {
+ /// Client network this selector matches, in CIDR notation
+ /// (`10.0.0.0/16`, `2001:db8::/32`). Must parse at boot; duplicate
+ /// networks within one node are rejected. A v4-mapped v6 network
+ /// (`::ffff:10.0.0.0/104`) canonicalizes to its v4 form (`10.0.0.0/8`),
+ /// matching how client IPs canonicalize before matching.
+ pub client_cidr: String,
+ /// Address advertised to matching clients: a literal IP or a DNS
+ /// hostname, validated as [`AdvertisedAddress`] at boot (no port; ports
+ /// come from [`ClusterNodeConfig::ports`]).
+ pub address: String,
+}
+
+/// A roster node with its advertised-address selectors parsed once, built
+/// wherever a roster is assembled for serving clients (listener/shard start).
+/// Per-request resolution must not re-parse selector strings: validation
+/// guarantees they parse only while the cluster is enabled, so on an
+/// unvalidated config a per-request parse would skip bad selectors silently
+/// on every call. Here they are dropped once, loudly, at build time.
+#[derive(Debug, Clone)]
+pub struct ResolvedClusterNode {
+ pub config: ClusterNodeConfig,
Review Comment:
`config` is pub while the derived `selectors` is private, which gives two
adjacent fields opposite mutation semantics: pushing to
`config.advertised_addresses` after the `From` conversion compiles and silently
does nothing (snapshotted), while mutating the neighbouring
`config.advertised_address` takes effect because `advertised_for` re-reads it
per call. nothing mutates one today (the roster lives behind `Rc` everywhere),
so this is a future-footgun - but if the catch-all ever gets cached at build
time (see the `selector_address` comment), the whole struct becomes a snapshot
and this should switch to a private field with a `config()` accessor at the
same time.
##########
core/configs/src/server_ng_config/cluster.rs:
##########
@@ -375,6 +382,132 @@ pub struct ClusterNodeConfig {
pub ports: TransportPorts,
}
+/// One client-network-scoped advertised address: clients whose IP falls
+/// inside `client_cidr` are told `address` instead of the node's catch-all
+/// [`ClusterNodeConfig::advertised_address`].
+///
+/// Typical split-network case: the roster `ip` is VPC-private and
+/// `advertised_address` is public; a selector with the VPC CIDR keeps
+/// in-VPC clients on the private address while everyone else stays on the
+/// public one. Selection is longest-prefix match across a node's selectors;
+/// a tie on prefix length resolves to the first declared selector. Selection
+/// sees the transport-level peer address, so clients arriving through a
+/// proxy or load balancer match the proxy's network, not their own.
+#[derive(Debug, Deserialize, Serialize, Clone, ConfigEnv)]
+#[serde(deny_unknown_fields)]
+pub struct AdvertisedAddressSelector {
+ /// Client network this selector matches, in CIDR notation
+ /// (`10.0.0.0/16`, `2001:db8::/32`). Must parse at boot; duplicate
+ /// networks within one node are rejected. A v4-mapped v6 network
+ /// (`::ffff:10.0.0.0/104`) canonicalizes to its v4 form (`10.0.0.0/8`),
+ /// matching how client IPs canonicalize before matching.
+ pub client_cidr: String,
+ /// Address advertised to matching clients: a literal IP or a DNS
+ /// hostname, validated as [`AdvertisedAddress`] at boot (no port; ports
+ /// come from [`ClusterNodeConfig::ports`]).
+ pub address: String,
+}
+
+/// A roster node with its advertised-address selectors parsed once, built
+/// wherever a roster is assembled for serving clients (listener/shard start).
+/// Per-request resolution must not re-parse selector strings: validation
+/// guarantees they parse only while the cluster is enabled, so on an
+/// unvalidated config a per-request parse would skip bad selectors silently
+/// on every call. Here they are dropped once, loudly, at build time.
+#[derive(Debug, Clone)]
+pub struct ResolvedClusterNode {
+ pub config: ClusterNodeConfig,
+ /// Truncated, canonicalized selector networks with their parsed
+ /// addresses, in declaration order (the longest-prefix tiebreak).
+ selectors: Vec<(IpNet, AdvertisedAddress)>,
+}
+
+impl From<ClusterNodeConfig> for ResolvedClusterNode {
+ fn from(config: ClusterNodeConfig) -> Self {
+ let selectors = config
+ .advertised_addresses
+ .iter()
+ .filter_map(|selector| {
+ let network = match selector.client_cidr.parse::<IpNet>() {
+ Ok(network) => canonical_ip_net(network.trunc()),
+ Err(error) => {
+ eprintln!(
+ "Ignoring advertised_addresses selector for node
'{}': client_cidr '{}': {error}",
Review Comment:
two issues with these drop branches. the doc above says selectors are
"dropped once, loudly, at build time", but the roster is built per shard plus
once for the HTTP listener, so one bad selector prints shards+1 identical
lines. more importantly the branch is only reachable when validation was
skipped, which only happens with `cluster.enabled = false` - exactly the state
where `cluster_metadata` synthesizes the self node and never reads `nodes`, so
the selectors are inert anyway. the eprintln pair, the two tests documenting
this unreachable state, and the doc paragraphs can all go. keep
`advertised_for`'s fail-closed `None` though - the `.ok()` costs nothing and
still guards rosters built without `validate`. (don't convert these to
`tracing::warn!` either - during config load the log sinks are still
placeholders, so a warn there is discarded on exactly the failing path.)
##########
core/configs/src/server_ng_config/cluster.rs:
##########
@@ -933,6 +1121,84 @@ impl Validatable<ConfigurationError> for ClusterConfig {
}
}
+/// One advertised client endpoint and the client network it serves, pooled by
+/// [`ClusterConfig::validate`] so selectors and catch-all conflict-check
+/// against each other. `network: None` is the catch-all (`advertised_address`,
+/// or the roster `ip` as fallback): it matches every client, so it overlaps
+/// every network.
+struct AdvertisedEndpoint<'roster> {
+ node_name: &'roster str,
+ transport: &'static str,
+ network: Option<IpNet>,
+ host: AdvertisedHost<'roster>,
+ port: u16,
+}
+
+/// Advertised host in comparable form. `Raw` carries a roster `ip` that
+/// failed the strict [`AdvertisedAddress`] parse, so it can never equal a
+/// `Parsed` host.
+#[derive(PartialEq, Eq)]
+enum AdvertisedHost<'roster> {
Review Comment:
`AdvertisedHost::Raw` can never produce a conflict: `Raw` only arises when
`advertised_address` is unset and `node.ip` fails the strict parse; two `Raw`s
conflicting would need the identical ip string and port on different nodes,
which the cluster-wide bind-endpoint check earlier in validate already rejects;
and `Raw` can never equal `Parsed` because a parsed value passed the exact
parse the raw ip failed. the pre-PR raw-endpoint set was dead for the same
reason. `host: AdvertisedAddress` plus skipping the catch-all insert when the
address doesn't parse removes the enum, its lifetime param, and the
`authority()` match.
##########
core/server-ng/config.toml:
##########
@@ -739,6 +739,29 @@ ca_file = ""
# (RFC 1123: ASCII letters, digits, '-' and '.'; no port, no trailing dot).
# When unset, clients receive 'ip'.
#
+# When different client networks need different addresses (a public
+# 'advertised_address' would route in-VPC clients out through the public
+# side), add per-network 'advertised_addresses' selectors: clients whose
+# peer IP falls inside 'client_cidr' are handed 'address' instead of the
+# catch-all. The longest matching prefix wins; a tie on prefix length goes
Review Comment:
two doc gaps here. the tie clause promises behavior no valid config can
produce (see the cluster.rs comment). and the family asymmetry is worth a
sentence: client IPs are canonicalized and matching is family-strict, so no
single selector matches both IP families - `0.0.0.0/0` matches no IPv6 client
and `::/0` matches no IPv4 client; anyone wanting "all v4 plus all v6" needs
one selector per family or the catch-all. (also `::ffff:0:0/95` and shorter
v4-mapped prefixes are silently dead selectors - only prefixes >= 96
canonicalize to v4.)
##########
core/integration/tests/server/cluster_metadata_vsr.rs:
##########
@@ -74,3 +85,182 @@ async fn
given_two_node_cluster_when_getting_cluster_metadata_should_return_full
"every other node must follow, got {metadata}"
);
}
+
+const SELECTOR_CLUSTER_NODES: usize = 2;
+
+/// Every test client in this file connects over loopback, so this CIDR is the
+/// one the server must match its peer address against.
+const LOOPBACK_CIDR: &str = "127.0.0.0/8";
+
+/// Selector marker: distinct from the harness roster ip (`127.0.0.1`), so it
+/// appears in metadata or a redirect only when the selector path ran - yet it
+/// still resolves, which matters because the leader-aware SDK client redials
+/// whatever address metadata advertises.
+const SELECTOR_ADDRESS: &str = "localhost";
+
+/// A 2-node cluster whose roster gives every node a loopback-CIDR selector.
+/// The roster (selectors included) must be identical on every node, so each
+/// server process gets the env vars for all nodes.
+fn selector_cluster() -> TestHarness {
+ let mut harness = TestHarness::builder()
+ .default_server()
+ .cluster_nodes(SELECTOR_CLUSTER_NODES)
+ .build()
+ .expect("build selector cluster harness");
+ for node in 0..SELECTOR_CLUSTER_NODES {
+ for roster_entry in 0..SELECTOR_CLUSTER_NODES {
+ harness.node_mut(node).add_env(
+
format!("IGGY_CLUSTER_NODES_{roster_entry}_ADVERTISED_ADDRESSES_0_CLIENT_CIDR"),
+ LOOPBACK_CIDR,
+ );
+ harness.node_mut(node).add_env(
+
format!("IGGY_CLUSTER_NODES_{roster_entry}_ADVERTISED_ADDRESSES_0_ADDRESS"),
+ SELECTOR_ADDRESS,
+ );
+ }
+ }
+ harness
+}
+
+/// Assert every roster node reports the selector address to this loopback
+/// client; without the selector the same roster publishes `127.0.0.1`.
+fn assert_selector_addresses(nodes: impl IntoIterator<Item = (String,
String)>) {
+ let mut seen = 0;
+ for (name, ip) in nodes {
+ assert_eq!(
+ ip, SELECTOR_ADDRESS,
+ "node '{name}' must advertise its selector address to a loopback
client"
+ );
+ seen += 1;
+ }
+ assert_eq!(seen, SELECTOR_CLUSTER_NODES, "full roster must be reported");
+}
+
+#[tokio::test]
+#[serial_test::parallel]
+async fn
given_matching_client_cidr_when_getting_binary_cluster_metadata_should_return_selector_addresses()
+ {
+ let mut harness = selector_cluster();
+ harness.start().await.expect("start selector cluster");
+
+ let client = harness
+ .node(0)
+ .tcp_client()
+ .expect("tcp client")
+ .with_root_login()
+ .connect()
+ .await
+ .expect("connect to node 0");
+ let metadata = client
+ .get_cluster_metadata()
+ .await
+ .expect("get cluster metadata");
+
+ assert_selector_addresses(
+ metadata
+ .nodes
+ .iter()
+ .map(|node| (node.name.clone(), node.ip.clone())),
+ );
+}
+
+#[tokio::test]
+#[serial_test::parallel]
+async fn
given_matching_client_cidr_when_getting_http_cluster_metadata_should_return_selector_addresses()
+ {
+ let mut harness = selector_cluster();
+ harness.start().await.expect("start selector cluster");
+
+ let http = HttpClient::login_root(&harness).await;
+ let response = http.get("/cluster/metadata").await;
+ assert_eq!(response.status(), StatusCode::OK);
+ let metadata: serde_json::Value = response.json().await.expect("decode
cluster metadata");
+
+ let nodes = metadata["nodes"]
+ .as_array()
+ .expect("metadata must carry a nodes array")
+ .iter()
+ .map(|node| {
+ (
+ node["name"].as_str().expect("node name").to_owned(),
+ node["ip"].as_str().expect("node ip").to_owned(),
+ )
+ })
+ .collect::<Vec<_>>();
+ assert_selector_addresses(nodes);
+}
+
+/// A linearizable read on the follower must 307 to the primary, and the
+/// `Location` host must be the primary's SELECTOR address: the redirected
+/// client is the same loopback peer, so pointing it at the catch-all (or the
+/// roster ip) would route it off its network.
+#[tokio::test]
+#[serial_test::parallel]
+async fn
given_matching_client_cidr_when_redirected_to_primary_should_target_selector_address()
{
+ const READ_PATH: &str = "/streams?consistency=linearizable";
+
+ let mut harness = selector_cluster();
+ harness.start().await.expect("start selector cluster");
+
+ // Redirects must surface, not be followed: the `Location` itself is what
+ // is under test, and the selector hostname resolves by design (see
+ // SELECTOR_ADDRESS), so a redirect-following client would chase it to the
+ // primary, get a 200, and hide a wrong Location host. Bearers are
+ // node-local in this keyless cluster, so each node gets its own login.
+ let deadline = Instant::now() + LOGIN_TIMEOUT;
+ let mut verdicts: Vec<(usize, StatusCode, Option<String>)>;
+ loop {
+ verdicts = Vec::new();
+ for node in 0..SELECTOR_CLUSTER_NODES {
+ let addr = harness.node(node).http_addr().expect("node http
address");
+ let session =
HttpClient::login_root_no_redirect(format!("http://{addr}";)).await;
Review Comment:
this builds a fresh `reqwest::Client` and runs the full root-login retry
loop per node per settle round. on server-ng every login mints a `Register`
through metadata consensus, so worst case this fires hundreds of committed ops
racing the very election the loop is waiting to settle. the inner login loop
also shares the same `LOGIN_TIMEOUT` budget as this outer loop, so login
retries eat the settle budget and a failure surfaces as "did not settle"
instead of "login failed". building one client per node before the loop and
retrying only the GET fixes both.
##########
core/configs/src/server_ng_config/cluster.rs:
##########
@@ -375,6 +382,132 @@ pub struct ClusterNodeConfig {
pub ports: TransportPorts,
}
+/// One client-network-scoped advertised address: clients whose IP falls
+/// inside `client_cidr` are told `address` instead of the node's catch-all
+/// [`ClusterNodeConfig::advertised_address`].
+///
+/// Typical split-network case: the roster `ip` is VPC-private and
+/// `advertised_address` is public; a selector with the VPC CIDR keeps
+/// in-VPC clients on the private address while everyone else stays on the
+/// public one. Selection is longest-prefix match across a node's selectors;
+/// a tie on prefix length resolves to the first declared selector. Selection
+/// sees the transport-level peer address, so clients arriving through a
+/// proxy or load balancer match the proxy's network, not their own.
+#[derive(Debug, Deserialize, Serialize, Clone, ConfigEnv)]
+#[serde(deny_unknown_fields)]
+pub struct AdvertisedAddressSelector {
+ /// Client network this selector matches, in CIDR notation
+ /// (`10.0.0.0/16`, `2001:db8::/32`). Must parse at boot; duplicate
+ /// networks within one node are rejected. A v4-mapped v6 network
+ /// (`::ffff:10.0.0.0/104`) canonicalizes to its v4 form (`10.0.0.0/8`),
+ /// matching how client IPs canonicalize before matching.
+ pub client_cidr: String,
+ /// Address advertised to matching clients: a literal IP or a DNS
+ /// hostname, validated as [`AdvertisedAddress`] at boot (no port; ports
+ /// come from [`ClusterNodeConfig::ports`]).
+ pub address: String,
+}
+
+/// A roster node with its advertised-address selectors parsed once, built
+/// wherever a roster is assembled for serving clients (listener/shard start).
+/// Per-request resolution must not re-parse selector strings: validation
+/// guarantees they parse only while the cluster is enabled, so on an
+/// unvalidated config a per-request parse would skip bad selectors silently
+/// on every call. Here they are dropped once, loudly, at build time.
+#[derive(Debug, Clone)]
+pub struct ResolvedClusterNode {
+ pub config: ClusterNodeConfig,
+ /// Truncated, canonicalized selector networks with their parsed
+ /// addresses, in declaration order (the longest-prefix tiebreak).
+ selectors: Vec<(IpNet, AdvertisedAddress)>,
+}
+
+impl From<ClusterNodeConfig> for ResolvedClusterNode {
+ fn from(config: ClusterNodeConfig) -> Self {
+ let selectors = config
+ .advertised_addresses
+ .iter()
+ .filter_map(|selector| {
+ let network = match selector.client_cidr.parse::<IpNet>() {
+ Ok(network) => canonical_ip_net(network.trunc()),
+ Err(error) => {
+ eprintln!(
+ "Ignoring advertised_addresses selector for node
'{}': client_cidr '{}': {error}",
+ config.name, selector.client_cidr
+ );
+ return None;
+ }
+ };
+ match selector.address.parse::<AdvertisedAddress>() {
+ Ok(address) => Some((network, address)),
+ Err(error) => {
+ eprintln!(
+ "Ignoring advertised_addresses selector for node
'{}': address '{}': {error}",
+ config.name, selector.address
+ );
+ None
+ }
+ }
+ })
+ .collect();
+ Self { config, selectors }
+ }
+}
+
+impl ResolvedClusterNode {
+ /// The client-facing address for a client connecting from `client_ip`:
+ /// longest-prefix match over the selector networks, then the catch-all
+ /// [`ClusterNodeConfig::advertised_address`], then the roster
+ /// [`ClusterNodeConfig::ip`]. A configured catch-all that does not parse
+ /// fails closed (`None`) instead of leaking the roster ip - unreachable
+ /// after validation, but validation is skipped while the cluster is
+ /// disabled.
+ #[must_use]
+ pub fn advertised_for(&self, client_ip: Option<IpAddr>) ->
Option<AdvertisedAddress> {
+ client_ip
+ .and_then(|client_ip| self.selector_address(client_ip))
+ .or_else(|| match self.config.advertised_address.as_deref() {
+ Some(advertised_address) => advertised_address.parse().ok(),
+ None => self.config.ip.parse().ok(),
+ })
+ }
+
+ /// Longest-prefix match over the boot-parsed selector networks; a tie on
+ /// prefix length resolves to the first declared selector. The client IP
+ /// is canonicalized first so a v4-mapped v6 peer (`::ffff:10.0.0.7`, the
+ /// shape a dual-stack listener reports) matches v4 networks.
+ fn selector_address(&self, client_ip: IpAddr) -> Option<AdvertisedAddress>
{
Review Comment:
two things here. the hand-rolled LPM can be a chain:
`self.selectors.iter().filter(|(n, _)| n.contains(&client_ip)).min_by_key(|(n,
_)| std::cmp::Reverse(n.prefix_len())).map(|(_, a)| a.clone())` - shorter,
computes `prefix_len()` once per element, and `min_by_key` returns the first of
equal elements so first-declared is preserved (careful in any later cleanup:
`max_by_key(prefix_len)` returns the last, which would invert it).
separately, `advertised_for` below re-parses the catch-all
`advertised_address` (or `ip`) on every call, which contradicts the
`cluster_meta.rs` doc that per-request resolution "never re-parses config
strings", and the matched `AdvertisedAddress` is cloned although both callers
only need a borrow. caching `catch_all: Option<AdvertisedAddress>` in the
`From` impl and returning `Option<&AdvertisedAddress>` fixes both - just land
it together with making `config` private (see above), since it widens the
snapshot surface.
##########
core/server-ng/src/http/tls.rs:
##########
@@ -124,6 +127,9 @@ pub fn spawn_accept_pump(
/// shutdown the pump drops its sender, this loop ends, and the in-flight
/// connection tasks drain via their own shutdown clone.
pub async fn serve(connections: Receiver<Handshaken>, router: Router,
shutdown: ShutdownToken) {
+ // `with_state(())` finalizes the routes eagerly, once for the whole
+ // listener; per-connection clones share the finalized internals.
+ let router: Router = router.with_state(());
Review Comment:
good hoist, but it creates an asymmetry: the plain listener never gets this
treatment. in `router()` the web-ui routes are merged after
`.with_state(state)`, so under the default `iggy-web` feature the `/ui`
endpoints stay `MethodEndpoint::BoxedHandler` and axum rebuilds the boxed
handler per request on plain HTTP, while HTTPS gets the finalized form here.
ending `router()` with `merge_web_ui(router, web_ui).with_state(())` fixes both
paths and this line (plus its comment) can go - `with_state` on
already-finalized routes is identity, so it's safe.
##########
core/server-ng/src/http/tls.rs:
##########
@@ -143,8 +149,16 @@ async fn serve_connection(
let io = HyperStream::new_tls(tls);
// `Router<()>` already maps the incoming body to axum's `Body` in its own
// `Service` impl, so it serves hyper's `Request<Incoming>` directly - no
- // `map_request` shim. `with_state(())` finalizes the routes eagerly.
- let service = TowerToHyperService::new(router.with_state(()));
+ // `map_request` shim.
+ //
+ // This hand-rolled loop bypasses axum's connect-info make-service (the
+ // plain listener's source of the peer address), so stamp the identical
+ // `ConnectInfo<ClientAddr>` extension on every request of this connection
+ // here - the extractors cannot tell the two paths apart. `AddExtension`
+ // wraps the shared router as one thin per-request insert; `Router::layer`
+ // would rebuild every route's boxed service on each connection.
+ let service =
+ TowerToHyperService::new(AddExtension::new(router,
ConnectInfo(ClientAddr(peer))));
Review Comment:
this stamp has zero reachable test coverage: the only ng HTTPS test hits
`/ping` and `/users/login`, neither of which constructs `Identity`, and
`Identity.client_ip` is an `Option` that silently degrades to the catch-all
address if the extension ever goes missing. the plain-path e2e can't cover for
it either - see the comment in the e2e file. the code reads correct today (we
traced both install paths), so this is purely a regression barrier: one HTTPS
`/cluster/metadata` assertion in the existing TLS test would close it.
##########
core/integration/tests/server/cluster_metadata_vsr.rs:
##########
@@ -74,3 +85,182 @@ async fn
given_two_node_cluster_when_getting_cluster_metadata_should_return_full
"every other node must follow, got {metadata}"
);
}
+
+const SELECTOR_CLUSTER_NODES: usize = 2;
+
+/// Every test client in this file connects over loopback, so this CIDR is the
+/// one the server must match its peer address against.
+const LOOPBACK_CIDR: &str = "127.0.0.0/8";
+
+/// Selector marker: distinct from the harness roster ip (`127.0.0.1`), so it
+/// appears in metadata or a redirect only when the selector path ran - yet it
+/// still resolves, which matters because the leader-aware SDK client redials
+/// whatever address metadata advertises.
+const SELECTOR_ADDRESS: &str = "localhost";
Review Comment:
`localhost` is the one advertised value that dodges the SDK path these tests
are meant to prove: the leader-aware client's `parse_address` special-cases
exactly `localhost` (rewriting it to `127.0.0.1`, which happens to equal the
dialed loopback address), so `is_same_address` matches. any other hostname
selector fails the socket-addr parse, falls to the raw string compare,
mismatches the dialed IP, and triggers a spurious redirect + reconnect +
re-login on every fresh connect - the suite stays green only because of this
specific pair. relatedly, matching `127.0.0.0/8` against a loopback peer means
the server's own bind address and the client peer are both `127.0.0.1`, so a
bug that matched the local address instead of the peer address would still pass
all three tests.
two cheap additions would pin both: a second selector on a non-loopback cidr
(e.g. `10.0.0.0/8` -> a distinct address) asserted NOT to be chosen, and one
case using a non-`localhost` hostname selector.
##########
core/server-ng/src/cluster_meta.rs:
##########
@@ -137,14 +147,23 @@ impl ClusterRoster {
/// Client-facing host in normalized form (lowercase hostname, canonical IP),
/// matching what boot validation compared and what redirect URLs render, so
-/// textual config variants of one address publish identical metadata. A
-/// roster `ip` that parses as neither (boot only requires it non-empty)
-/// passes through verbatim; a configured `advertised_address` always parses,
-/// validation rejects it otherwise.
-fn client_host(node: &ClusterNodeConfig) -> String {
- let host = node.advertised_address.as_deref().unwrap_or(&node.ip);
- host.parse::<AdvertisedAddress>()
- .map_or_else(|_| host.to_owned(), |address| address.to_string())
+/// textual config variants of one address publish identical metadata. The
+/// per-client-network selectors, the catch-all `advertised_address`, and the
+/// roster `ip` are consulted in that order
([`ResolvedClusterNode::advertised_for`]);
+/// a host that parses as neither IP nor hostname (only reachable when boot
+/// validation was skipped, or for the roster `ip`, which boot only requires
+/// non-empty) passes through verbatim.
+fn client_host(node: &ResolvedClusterNode, client_ip: Option<IpAddr>) ->
String {
Review Comment:
the fallback closure re-walks the `advertised_address`-else-`ip` ladder that
`advertised_for` already encodes, so the same chain lives in two crates and can
drift. if this gets single-sourced, the verbatim pass-through is load-bearing:
an unparsable roster ip (docker service name with an underscore) must still be
published verbatim, so any shared helper has to return the raw `&str`, not an
`Option<AdvertisedAddress>`.
##########
core/configs/src/server_ng_config/cluster.rs:
##########
@@ -375,6 +382,132 @@ pub struct ClusterNodeConfig {
pub ports: TransportPorts,
}
+/// One client-network-scoped advertised address: clients whose IP falls
+/// inside `client_cidr` are told `address` instead of the node's catch-all
+/// [`ClusterNodeConfig::advertised_address`].
+///
+/// Typical split-network case: the roster `ip` is VPC-private and
+/// `advertised_address` is public; a selector with the VPC CIDR keeps
+/// in-VPC clients on the private address while everyone else stays on the
+/// public one. Selection is longest-prefix match across a node's selectors;
+/// a tie on prefix length resolves to the first declared selector. Selection
Review Comment:
the tie-break rule documented here (and in config.toml, and on
`selector_address`) is unreachable on any validated config: two distinct
truncated networks of equal prefix length are disjoint, and duplicate truncated
cidrs within a node are rejected by validation. the
`prefix_length_tie_resolves_to_first_declared` test only passes because
`10.0.1.0/16` and `10.0.2.0/16` truncate to the same net - the exact input
`validate_rejects_duplicate_selector_cidr_within_a_node` rejects. suggest
dropping the operator-facing promise from the three doc sites and relabeling
the test, while keeping the code deterministic so a future validation
relaxation can't become order-dependent.
##########
core/configs/src/server_ng_config/cluster.rs:
##########
@@ -375,6 +382,132 @@ pub struct ClusterNodeConfig {
pub ports: TransportPorts,
}
+/// One client-network-scoped advertised address: clients whose IP falls
+/// inside `client_cidr` are told `address` instead of the node's catch-all
+/// [`ClusterNodeConfig::advertised_address`].
+///
+/// Typical split-network case: the roster `ip` is VPC-private and
+/// `advertised_address` is public; a selector with the VPC CIDR keeps
+/// in-VPC clients on the private address while everyone else stays on the
+/// public one. Selection is longest-prefix match across a node's selectors;
+/// a tie on prefix length resolves to the first declared selector. Selection
+/// sees the transport-level peer address, so clients arriving through a
+/// proxy or load balancer match the proxy's network, not their own.
+#[derive(Debug, Deserialize, Serialize, Clone, ConfigEnv)]
+#[serde(deny_unknown_fields)]
+pub struct AdvertisedAddressSelector {
+ /// Client network this selector matches, in CIDR notation
+ /// (`10.0.0.0/16`, `2001:db8::/32`). Must parse at boot; duplicate
+ /// networks within one node are rejected. A v4-mapped v6 network
+ /// (`::ffff:10.0.0.0/104`) canonicalizes to its v4 form (`10.0.0.0/8`),
+ /// matching how client IPs canonicalize before matching.
+ pub client_cidr: String,
+ /// Address advertised to matching clients: a literal IP or a DNS
+ /// hostname, validated as [`AdvertisedAddress`] at boot (no port; ports
+ /// come from [`ClusterNodeConfig::ports`]).
+ pub address: String,
+}
+
+/// A roster node with its advertised-address selectors parsed once, built
+/// wherever a roster is assembled for serving clients (listener/shard start).
+/// Per-request resolution must not re-parse selector strings: validation
+/// guarantees they parse only while the cluster is enabled, so on an
+/// unvalidated config a per-request parse would skip bad selectors silently
+/// on every call. Here they are dropped once, loudly, at build time.
+#[derive(Debug, Clone)]
+pub struct ResolvedClusterNode {
+ pub config: ClusterNodeConfig,
+ /// Truncated, canonicalized selector networks with their parsed
+ /// addresses, in declaration order (the longest-prefix tiebreak).
+ selectors: Vec<(IpNet, AdvertisedAddress)>,
+}
+
+impl From<ClusterNodeConfig> for ResolvedClusterNode {
+ fn from(config: ClusterNodeConfig) -> Self {
+ let selectors = config
+ .advertised_addresses
+ .iter()
+ .filter_map(|selector| {
+ let network = match selector.client_cidr.parse::<IpNet>() {
+ Ok(network) => canonical_ip_net(network.trunc()),
+ Err(error) => {
+ eprintln!(
+ "Ignoring advertised_addresses selector for node
'{}': client_cidr '{}': {error}",
+ config.name, selector.client_cidr
+ );
+ return None;
+ }
+ };
+ match selector.address.parse::<AdvertisedAddress>() {
+ Ok(address) => Some((network, address)),
+ Err(error) => {
+ eprintln!(
+ "Ignoring advertised_addresses selector for node
'{}': address '{}': {error}",
+ config.name, selector.address
+ );
+ None
+ }
+ }
+ })
+ .collect();
+ Self { config, selectors }
+ }
+}
+
+impl ResolvedClusterNode {
+ /// The client-facing address for a client connecting from `client_ip`:
+ /// longest-prefix match over the selector networks, then the catch-all
+ /// [`ClusterNodeConfig::advertised_address`], then the roster
+ /// [`ClusterNodeConfig::ip`]. A configured catch-all that does not parse
+ /// fails closed (`None`) instead of leaking the roster ip - unreachable
Review Comment:
the fail-closed promise is honored by only one of the two callers: the
redirect path fails to a 503, but `client_host` in `cluster_meta.rs` re-derives
the same fallback and publishes the unparsable string verbatim (deliberately -
see the docker-service-name rationale in the validate docs). the promise
belongs on `primary_advertised_http_authority`, with the verbatim contract
stated on the metadata side.
--
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]
