hubcio commented on code in PR #3923:
URL: https://github.com/apache/iggy/pull/3923#discussion_r3862134382
##########
core/configs/src/server_config/cluster.rs:
##########
@@ -990,10 +1047,23 @@ impl Validatable<ConfigurationError> for ClusterConfig {
return Err(ConfigurationError::InvalidConfigurationValue);
}
- if node.ip.trim().is_empty() {
+ // The roster ip is dialed verbatim for replica traffic and is
+ // never resolved, so no hostname can work here whatever its
+ // shape.
+ let node_ip = node.ip.parse::<IpAddr>().map_err(|error| {
eprintln!(
- "Invalid cluster configuration: IP cannot be empty for
node '{}'",
- node.name
+ "Invalid cluster configuration: IP '{}' for node '{}' is
not a literal IP \
+ address: {error}; set node.advertised_address for the
name clients dial",
Review Comment:
this branch only runs with `cluster.enabled = true`, where
`node.advertised_address` is ignored (bootstrap warns exactly that), so the
hint sends people to a dead knob. point at
`cluster.nodes[*].advertised_address`.
##########
core/integration/tests/server/cluster_metadata_advertised.rs:
##########
@@ -0,0 +1,81 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! `node.advertised_address` on a cluster-disabled server: what a client is
+//! told about the one node in the roster.
+//!
+//! Without a roster the server has only its own bind address to reason from,
+//! and a bind address answers which interfaces it accepts on - never where a
+//! client reaches it. Behind any NAT (published container ports, a Service, a
+//! load balancer) the two are different addresses, and this setting is the
+//! only way to state the second one. The bind-derived fallback and the empty
+//! answer for a wildcard bind are pinned at unit level (`cluster_meta.rs`)
+//! and across the SDKs in `bdd/`; what needs a real server is that a declared
Review Comment:
there's no "empty answer for a wildcard bind" anywhere - a wildcard bind is
refused at boot now, and `bdd/` doesn't assert on metadata addresses. drop that
sentence. also the `!metadata.name.is_empty()` assert at line 66 can't fail
(the name is a const); check for `"single-node"` instead.
##########
core/configs/src/server_config/node.rs:
##########
@@ -0,0 +1,98 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// This node's own client-facing identity, for the cluster-disabled server.
+
+use super::COMPONENT;
+use super::cluster::AdvertisedAddress;
+use crate::ConfigurationError;
+use configs::ConfigEnv;
+use iggy_common::Validatable;
+use serde::{Deserialize, Serialize};
+
+/// Named to match its roster counterpart: `advertised_address` here and
+/// `cluster.nodes[*].advertised_address` there are the same setting for the
+/// same question, and an operator moving between the two modes should not have
+/// to learn a second spelling.
+#[derive(Debug, Default, Deserialize, Serialize, Clone, ConfigEnv)]
+#[serde(deny_unknown_fields)]
+pub struct NodeConfig {
+ /// Client-facing address: a literal IP or a DNS hostname. `None` leaves
+ /// the server deriving one from its bind address.
+ #[serde(default)]
+ pub advertised_address: Option<String>,
+}
+
+impl Validatable<ConfigurationError> for NodeConfig {
+ fn validate(&self) -> Result<(), ConfigurationError> {
+ let Some(address) = self.advertised_address.as_deref() else {
+ return Ok(());
+ };
+
+ let parsed = address.parse::<AdvertisedAddress>().map_err(|error| {
+ eprintln!("{COMPONENT} - node.advertised_address '{address}':
{error}");
Review Comment:
for `localhost:8090` this prints "ports are configured in
cluster.nodes.ports", which is wrong here - the single-node port comes from
`tcp.address`. generalize the `PortNotAllowed` text so it fits both.
##########
core/server/config.toml:
##########
@@ -757,7 +768,7 @@ skip_shard_zero_for_clients = false
#
# [[cluster.nodes]]
# name = "iggy-node-1"
-# ip = "10.0.1.5" # replica plane + last-resort
fallback
+# ip = "10.0.1.5" # replica plane, literal IP only
Review Comment:
since the roster ip is now literal-ip only, the `ca_file` note at lines
693-696 ("use hostnames in the roster if the certificates only have DNS SANs")
can't be followed anymore: that ip string is also the tls server name. reword
it to say peer certs need IP SANs.
##########
core/server/src/args.rs:
##########
@@ -54,7 +54,7 @@ ENVIRONMENT VARIABLES:
Common examples:
IGGY_SYSTEM_PATH=/data/iggy # Data directory
- IGGY_TCP_ADDRESS=0.0.0.0:8090 # TCP listener address
+ IGGY_TCP_ADDRESS=127.0.0.1:8090 # TCP listener address
Review Comment:
add `IGGY_NODE_ADVERTISED_ADDRESS=localhost` to the examples - it's the one
variable the docker image now requires.
##########
core/configs/src/server_config/validators.rs:
##########
@@ -424,6 +429,38 @@ fn reject_unsupported(config: &ServerConfig) -> Result<(),
ConfigurationError> {
Ok(())
}
+impl ServerConfig {
+ /// `tcp.address` must name a bind address, and a wildcard one must be
+ /// paired with a declared client-facing address.
+ fn validate_client_facing_address(&self) -> Result<(), ConfigurationError>
{
+ let bind = self.tcp.address.parse::<SocketAddr>().map_err(|error| {
+ eprintln!(
+ "{COMPONENT} - tcp.address '{}' is not an address and port:
{error}. The host \
+ is required and must be a literal IP, so ':PORT' and
'hostname:PORT' are \
+ both rejected; use 127.0.0.1:PORT for loopback or
0.0.0.0:PORT to accept on \
+ every interface.",
+ self.tcp.address
+ );
+ ConfigurationError::InvalidConfigurationValue
+ })?;
+
+ if self.cluster.enabled || self.node.advertised_address.is_some() {
Review Comment:
this reads `tcp.address` even when `tcp.enabled = false`, and so does the
derived self address in bootstrap. `IGGY_TCP_ENABLED=false` with the image
default `0.0.0.0:8090` now refuses to boot over a listener that isn't running,
and an http-only server with the shipped `127.0.0.1:8090` boots but publishes
`127.0.0.1` with no warning (the warn loop skips wildcard listeners). derive
from the first enabled listener instead, gate the refusal on that one, and warn
when another listener binds a wildcard while the derived address is loopback.
##########
helm/charts/iggy/templates/deployment.yaml:
##########
@@ -89,6 +89,17 @@ 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 }}
Review Comment:
this only checks the name, so `{name: IGGY_NODE_ADVERTISED_ADDRESS, value:
""}` in `server.env` suppresses the chart default and renders an empty value,
which the server treats as unset - the pod then hits the refusal this is meant
to prevent. require a non-empty `.value` here.
##########
web/docker-compose.yml:
##########
@@ -24,6 +24,7 @@ services:
IGGY_HTTP_ADDRESS: 0.0.0.0:3000
IGGY_QUIC_ADDRESS: 0.0.0.0:8080
IGGY_TCP_ADDRESS: 0.0.0.0:8090
+ IGGY_NODE_ADVERTISED_ADDRESS: iggy-server
Review Comment:
ports 8090/8080/8092 are published to the host, so clients on the host get
`iggy-server` as an address they can't resolve (the go sdk keeps a single
node's advertised address as a reconnect target). the root compose uses
`localhost`; same here, the ui itself never reads this.
--
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]
