mmodzelewski commented on code in PR #3923:
URL: https://github.com/apache/iggy/pull/3923#discussion_r3836595341
##########
core/configs/src/server_config/server.rs:
##########
@@ -111,6 +112,7 @@ pub struct ServerConfig {
pub consumer_group: ConsumerGroupConfig,
pub data_maintenance: DataMaintenanceConfig,
#[serde(default)]
+ pub node: NodeConfig,
Review Comment:
This insertion lands between the `#[serde(default)]` attribute and
`personal_access_token`, so the attribute now applies to `node` and
`personal_access_token` has lost its default. A bare deserialize of a config
without a `[personal_access_token]` section now fails with "missing field".
It's masked in the server binary because the embedded default config.toml is
always merged first (`file_provider.rs:162`), but it's a silent contract change
on a public struct. Suggest keeping `#[serde(default)]` on both fields.
##########
core/server/src/bootstrap.rs:
##########
@@ -3030,6 +3040,12 @@ async fn start_tcp_runtime(
// reactor, so it binds independently. Shard-0 gating comes from the sole
// caller of this function.
if let Some(http_addr) = topology.http_listen_addr {
+ // One host for all four transports, resolved from the client-facing
+ // TCP bind so both listeners publish the same node address.
+ let self_advertised = self_advertised_address(
Review Comment:
This changes the source of the HTTP metadata self address: before this PR,
http.rs published the HTTP listener's own bound address (`self_ip:
bound_addr.ip().to_string()`); now it derives from the TCP bind. With the
shipped default `tcp.address = 127.0.0.1:8090` and HTTP bound to a concrete
external interface, the published address silently flips from the external IP
to `127.0.0.1` - no boot refusal, no warning. Consider warning on (or
validating) the interface mismatch between the TCP and HTTP binds.
##########
core/configs/src/server_config/cluster.rs:
##########
@@ -473,49 +474,80 @@ pub struct AdvertisedAddressSelector {
/// once, built wherever a roster is assembled for serving clients
/// (listener/shard start). Per-request resolution never re-parses config
/// strings: everything is snapshotted here, so mutating the source config
-/// after conversion has no effect on what clients are told. Entries that do
-/// not parse are dropped at build time; validation already rejects them
-/// whenever the cluster is enabled, and a disabled cluster never consults
-/// the roster.
+/// after conversion has no effect on what clients are told.
#[derive(Debug, Clone)]
pub struct ResolvedClusterNode {
config: ClusterNodeConfig,
/// Truncated, canonicalized selector networks with their parsed
/// addresses, in declaration order.
selectors: Vec<(IpNet, AdvertisedAddress)>,
/// Parsed catch-all: [`ClusterNodeConfig::advertised_address`], else the
- /// roster [`ClusterNodeConfig::ip`]. `None` when the configured value
- /// does not parse - a set `advertised_address` never falls through to
- /// the private roster ip.
- catch_all: Option<AdvertisedAddress>,
+ /// roster [`ClusterNodeConfig::ip`]. A set `advertised_address` never
+ /// falls through to the private roster ip.
+ catch_all: AdvertisedAddress,
/// Parsed roster [`ClusterNodeConfig::ip`], the replica-plane dial
- /// address. `None` when the roster ip is not a literal IP (boot only
- /// requires it non-empty); internal forwarding then has no dial target.
- replica_ip: Option<IpAddr>,
+ /// address.
+ replica_ip: IpAddr,
}
-impl From<ClusterNodeConfig> for ResolvedClusterNode {
- fn from(config: ClusterNodeConfig) -> Self {
- let selectors = config
- .advertised_addresses
- .iter()
- .filter_map(|selector| {
- let network = selector.client_cidr.parse::<IpNet>().ok()?;
- let address =
selector.address.parse::<AdvertisedAddress>().ok()?;
- Some((canonical_ip_net(network.trunc()), address))
- })
- .collect();
- let catch_all = match config.advertised_address.as_deref() {
- Some(advertised_address) => advertised_address.parse().ok(),
- None => config.ip.parse().ok(),
- };
- let replica_ip = config.ip.parse().ok();
- Self {
+impl TryFrom<ClusterNodeConfig> for ResolvedClusterNode {
Review Comment:
`TryFrom` checks only that the sources parse; it doesn't reject unspecified
addresses the way the validator does for roster ip, catch-all, and selectors.
The "always dialable" invariant therefore holds only because validation runs
before conversion - a direct `TryFrom` caller can still build a node
advertising `0.0.0.0`. Mirroring the unspecified rejection here (on the
canonical form) would make the type self-defending.
##########
core/configs/src/server_config/cluster.rs:
##########
@@ -629,6 +647,10 @@ pub enum AdvertisedAddress {
}
impl AdvertisedAddress {
+ pub fn is_unspecified(&self) -> bool {
Review Comment:
`Ipv6Addr::is_unspecified` is true only for `::`, so the v4-mapped
`::ffff:0.0.0.0` slips through this check and through every gate built on it
(node.advertised_address, roster ip, selector addresses, and the new
tcp.address validation, where `[::ffff:0.0.0.0]:8090` binds the v4 wildcard on
a dual-stack Linux host - exactly the bug class this PR is fixing).
`ip.to_canonical().is_unspecified()` closes it; `canonical_ip_net` above
already canonicalizes mapped forms for selector CIDRs. A doc comment on this fn
would also match its documented siblings.
##########
helm/charts/iggy/templates/deployment.yaml:
##########
@@ -89,6 +89,14 @@ spec:
name: {{ include "iggy.fullname" . }}-root-credentials
key: password
{{- end }}{{- end}}
+ {{- $declaredInEnv := false }}
+ {{- range .Values.server.env }}
+ {{- if eq .name "IGGY_NODE_ADVERTISED_ADDRESS" }}{{- $declaredInEnv = true
}}{{- end }}
+ {{- end }}
+ {{- if not $declaredInEnv }}
+ - name: IGGY_NODE_ADVERTISED_ADDRESS
Review Comment:
When `server.env` already carries `IGGY_NODE_ADVERTISED_ADDRESS`,
`server.advertisedAddress` is silently ignored - this branch never runs and the
value is read nowhere else. Worth a precedence note on
`server.advertisedAddress` in values.yaml or the chart README.
##########
core/server/src/cluster_meta.rs:
##########
@@ -43,6 +44,34 @@ const SELF_NODE_NAME: &str = "iggy-node";
/// single-node label.
const SINGLE_NODE_CLUSTER_NAME: &str = "single-node";
+pub fn self_advertised_address(declared: Option<&str>, bind: IpAddr) -> String
{
Review Comment:
The declared address is published verbatim while the roster path normalizes
through `AdvertisedAddress`'s Display (lowercased hostnames, canonical IPv6 -
pinned by the tests in this file). A declared `Broker.Example.COM` or
`[2001:db8::1]` reaches the wire as typed, so the same address can publish in
two spellings depending on the path. `NodeConfig::validate` already guarantees
the parse, so parse-then-Display here is infallible.
--
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]
