[gateway] Validate configuration and redact secrets - #3970
Conversation
e39da12 to
0f10251
Compare
74b9853 to
4112339
Compare
cae7418 to
082458b
Compare
There was a problem hiding this comment.
Pull request overview
This PR strengthens the Fluss Gateway’s configuration layer by adding typed per-cluster settings, cross-field validation, and consistent secret redaction in logs/errors/diagnostics, building on the gateway runtime foundation added previously.
Changes:
- Introduces typed cluster, security, metrics, and request-limit configuration parsing/validation, including cluster declaration enforcement and reserved-but-unsupported
gateway.cluster.<id>.client.*overrides. - Adds a
Secretnewtype and ensures sensitive values are redacted in startup diagnostics and failure paths; logs the effective redacted configuration at startup. - Expands process/config tests for invalid configs and redaction guarantees; updates gateway CI and dependency metadata to account for the new
fluss-rsworkspace dependency.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| fluss-gateway/tests/process.rs | Adds process-level tests for invalid config exit codes and secret redaction behavior. |
| fluss-gateway/src/lifecycle.rs | Logs the effective (redacted) configuration at debug level during startup. |
| fluss-gateway/src/config.rs | Implements typed config loading with precedence, per-cluster config model, validation, reserved client override rejection, and secret redaction. |
| fluss-gateway/justfile | Normalizes dependency inventory generation to avoid trailing whitespace drift. |
| fluss-gateway/DEPENDENCIES.rust.tsv | Updates the checked-in dependency/license inventory after dependency graph changes. |
| fluss-gateway/Cargo.toml | Adds a path dependency on fluss-rs to build native client connection settings from typed config. |
| fluss-gateway/Cargo.lock | Updates lockfile for the new fluss-rs dependency and its transitive crates. |
| .github/workflows/gateway-ci.yml | Extends CI path triggers for fluss-rs/protos and ensures protoc is installed for builds/checks; updates inventory drift check. |
Suppressed comments (2)
fluss-gateway/src/config.rs:962
- This error message for
identity-mode: userrefers to{CLUSTER_SERVICE_ACCOUNT_KEY}/{CLUSTER_SERVICE_SECRET_KEY}without the cluster prefix, so the rendered message won’t clearly point to the public config keys. Prefer printing the fully qualified keys for all referenced options.
problems.push(format!(
"{} user requires {CLUSTER_SERVICE_ACCOUNT_KEY} and {CLUSTER_SERVICE_SECRET_KEY}",
cluster_key(id, CLUSTER_IDENTITY_MODE_KEY)
));
fluss-gateway/src/config.rs:1047
- This startup warning mixes a fully-qualified key with bare suffix keys (
connection.idle-timeout,connection.identity-mode). Since this is operator-facing diagnostics, it should consistently print the full public keys (including the cluster prefix) for all referenced options.
warnings.push(format!(
"{} and {CLUSTER_CONNECTION_IDLE_TIMEOUT_KEY} are ignored because \
{CLUSTER_IDENTITY_MODE_KEY} is service",
cluster_key(id, CLUSTER_CONNECTION_MAX_KEY)
));
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@beryllw Thank you for the PR, left some comments , PTAL
| - main | ||
| paths: | ||
| - 'fluss-gateway/**' | ||
| - 'fluss-rust/crates/fluss/**' |
There was a problem hiding this comment.
fluss-rs takes arrow, tokio and others as workspace = true, so what the gateway compiles is pinned in fluss-rust/Cargo.toml, which isn't in this list. Shall we add it?
| .map_err(|e| format!("invalid byte size {s:?}: {e}"))?; | ||
| let multiplier: u64 = match unit { | ||
| "" | "B" => 1, | ||
| "KB" => 1000, |
There was a problem hiding this comment.
KiB is fine, it's KB that worries me. Fluss's MemorySize lowercases the unit and treats kb as 1024, so 64KB is 64000 bytes here and 65536 in server.yaml. Shall we match java or nb add more descriptive error?
Also 1d and 1 s parse in TimeUtils but not here.
| } | ||
| } | ||
|
|
||
| impl fmt::Debug for ClusterConfig { |
There was a problem hiding this comment.
Secret already redacts itself in Debug, so #[derive(Debug)] prints the same thing
| } | ||
|
|
||
| /// Returns non-fatal configuration advisories that should be logged at startup. | ||
| pub fn warnings(&self) -> Vec<String> { |
There was a problem hiding this comment.
We warn when gateway.rest.listen is not loopback, but not when the metrics listener is not loopback. The FIP default is 0.0.0.0:9095, so the metrics port can be open to the network with no warning.
Same for trusted-header auth: the principal comes from the x-forwarded-user header, so on a non-loopback listener any client can set it and become any user.
Should we warn for both here?
| .any(|server| !server.trim().is_empty()) | ||
| { | ||
| problems.push(format!( | ||
| "{} must configure at least one server", |
There was a problem hiding this comment.
This accepts anything non-empty, so host, host:99999 and http://h:9123 all load. The bad part is the process then starts clean and /ready says 200 while every request to that cluster fails.
Shall we parse each entry as host:port here?
Purpose
Linked issue: close #3969
Adds the typed, validated and redacted Gateway configuration on top of #3963.
Brief change log
gateway.clustersdeclaring which clusters may be configured.client.security.protocol: sasl.connection.service.account/.secretbecome canonical; the legacy SASL keys keep last-wins precedence and warn once per cluster.Secretnewtype, redacted in logs, errors and the startup dump.Documentation
Module rustdoc only; user documentation lands with the delivery task in #3957.