From 27a7cc739e9a352296cc44c60b5bde5ff7e87973 Mon Sep 17 00:00:00 2001 From: houseme Date: Tue, 14 Jul 2026 00:07:08 +0800 Subject: [PATCH] fix(targets): keep pulsar target online after restart without TLS (#4798) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pulsar config validation rejected any non-`pulsar+ssl` broker whenever `tls_allow_insecure` was set or `tls_hostname_verification` was disabled. Both toggles are inert on a plaintext `pulsar://` broker — the Pulsar client only applies them for `pulsar+ssl` — so treating a non-default value as fatal is wrong. This surfaced as issue #4796: the console persists `tls_hostname_verification` as `false` (the checkbox defaults to unchecked while the server default is `on`). At creation time the value is not present in the unmerged request and falls back to the safe default, so the connectivity check passes and the target comes online. On restart the persisted config is merged and validated, the `false` is read back, and the target is rejected — permanently offline even though Pulsar is reachable. Relax both the loader-side (`validate_pulsar_broker_config`) and runtime-side (`PulsarArgs::validate`) checks so only a `tls_ca` bundle — real TLS trust material that is never defaulted to a non-empty value — requires a `pulsar+ssl` scheme. The inert toggles no longer fail the target. This also heals configs already persisted with the bad value. Add regression coverage for both paths. Co-authored-by: heihutu --- crates/targets/src/config/common.rs | 38 ++++++++++++++++++++++------- crates/targets/src/target/pulsar.rs | 34 ++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 11 deletions(-) diff --git a/crates/targets/src/config/common.rs b/crates/targets/src/config/common.rs index 7f9473e34..ff3e850da 100644 --- a/crates/targets/src/config/common.rs +++ b/crates/targets/src/config/common.rs @@ -19,7 +19,7 @@ use rustfs_config::server_config::KVS; use rustfs_config::{ DEFAULT_DELIMITER, ENABLE_KEY, EnableState, NATS_CREDENTIALS_FILE, NATS_PASSWORD, NATS_QUEUE_DIR, NATS_SUBJECT, NATS_TLS_CA, NATS_TLS_CLIENT_CERT, NATS_TLS_CLIENT_KEY, NATS_TOKEN, NATS_USERNAME, PULSAR_AUTH_TOKEN, PULSAR_PASSWORD, PULSAR_QUEUE_DIR, - PULSAR_TLS_ALLOW_INSECURE, PULSAR_TLS_CA, PULSAR_TLS_HOSTNAME_VERIFICATION, PULSAR_TOPIC, PULSAR_USERNAME, + PULSAR_TLS_CA, PULSAR_TOPIC, PULSAR_USERNAME, }; use rustfs_utils::egress::validate_outbound_url; use std::collections::HashSet; @@ -156,15 +156,20 @@ pub(super) fn validate_pulsar_broker_config(broker: &str, config: &KVS, default_ } let tls_ca = config.lookup(PULSAR_TLS_CA).unwrap_or_default(); - let tls_allow_insecure = parse_target_bool(config.lookup(PULSAR_TLS_ALLOW_INSECURE).as_deref()).unwrap_or(false); - let tls_hostname_verification = parse_target_bool(config.lookup(PULSAR_TLS_HOSTNAME_VERIFICATION).as_deref()).unwrap_or(true); if !tls_ca.is_empty() && !Path::new(&tls_ca).is_absolute() { return Err(TargetError::Configuration("Pulsar tls_ca must be an absolute path".to_string())); } - if url.scheme() != "pulsar+ssl" && (!tls_ca.is_empty() || tls_allow_insecure || !tls_hostname_verification) { + // A CA bundle is TLS trust material a plaintext `pulsar://` broker silently + // ignores, so treat it as a genuine misconfiguration. The + // `tls_allow_insecure` / `tls_hostname_verification` toggles, however, are + // inert on a non-TLS broker (the Pulsar client only honours them for + // `pulsar+ssl`); rejecting non-default values would leave a persisted + // target permanently offline after a restart even though the flags do + // nothing — see issue #4796. + if url.scheme() != "pulsar+ssl" && !tls_ca.is_empty() { return Err(TargetError::Configuration( - "Pulsar TLS settings are only allowed with pulsar+ssl brokers".to_string(), + "Pulsar tls_ca is only allowed with pulsar+ssl brokers".to_string(), )); } @@ -192,7 +197,8 @@ mod tests { use async_nats::ServerAddr; use rustfs_config::server_config::KVS; use rustfs_config::{ - NATS_PASSWORD, NATS_QUEUE_DIR, NATS_SUBJECT, NATS_TOKEN, NATS_USERNAME, PULSAR_TLS_ALLOW_INSECURE, PULSAR_TOPIC, + NATS_PASSWORD, NATS_QUEUE_DIR, NATS_SUBJECT, NATS_TOKEN, NATS_USERNAME, PULSAR_TLS_ALLOW_INSECURE, PULSAR_TLS_CA, + PULSAR_TLS_HOSTNAME_VERIFICATION, PULSAR_TOPIC, }; use std::str::FromStr; @@ -223,14 +229,28 @@ mod tests { } #[test] - fn validate_pulsar_broker_config_rejects_tls_flags_without_tls_scheme() { + fn validate_pulsar_broker_config_rejects_tls_ca_without_tls_scheme() { let mut config = KVS::new(); config.insert(PULSAR_TOPIC.to_string(), "events".to_string()); - config.insert(PULSAR_TLS_ALLOW_INSECURE.to_string(), "on".to_string()); + config.insert(PULSAR_TLS_CA.to_string(), "/etc/ssl/certs/ca.pem".to_string()); let err = validate_pulsar_broker_config("pulsar://127.0.0.1:6650", &config, "") - .expect_err("TLS flags should require pulsar+ssl"); + .expect_err("a CA bundle should require pulsar+ssl"); assert!(err.to_string().contains("only allowed with pulsar+ssl")); } + + #[test] + fn validate_pulsar_broker_config_accepts_inert_tls_toggles_without_tls_scheme() { + // The TLS toggles are no-ops on a plaintext broker, so a persisted + // config carrying non-default values must still load — otherwise the + // target is stuck offline after a restart (issue #4796). + let mut config = KVS::new(); + config.insert(PULSAR_TOPIC.to_string(), "events".to_string()); + config.insert(PULSAR_TLS_ALLOW_INSECURE.to_string(), "on".to_string()); + config.insert(PULSAR_TLS_HOSTNAME_VERIFICATION.to_string(), "off".to_string()); + + validate_pulsar_broker_config("pulsar://127.0.0.1:6650", &config, "") + .expect("inert TLS toggles must not fail a plaintext broker"); + } } diff --git a/crates/targets/src/target/pulsar.rs b/crates/targets/src/target/pulsar.rs index a9a1fc2f3..a12604691 100644 --- a/crates/targets/src/target/pulsar.rs +++ b/crates/targets/src/target/pulsar.rs @@ -113,9 +113,16 @@ impl PulsarArgs { let parsed = Url::parse(&self.broker) .map_err(|e| TargetError::Configuration(format!("Invalid Pulsar broker URL: {e} (value: '{}')", self.broker)))?; let tls_enabled = parsed.scheme() == "pulsar+ssl"; - if !tls_enabled && (!self.tls_ca.is_empty() || self.tls_allow_insecure || !self.tls_hostname_verification) { + // A CA bundle is TLS trust material a plaintext `pulsar://` broker + // silently ignores, so treat it as a genuine misconfiguration. The + // `tls_allow_insecure` / `tls_hostname_verification` toggles, however, + // are inert on a non-TLS broker (they only take effect for + // `pulsar+ssl`); rejecting non-default values would leave a persisted + // target permanently offline after a restart even though the flags do + // nothing — see issue #4796. + if !tls_enabled && !self.tls_ca.is_empty() { return Err(TargetError::Configuration( - "Pulsar TLS settings are only allowed with pulsar+ssl brokers".to_string(), + "Pulsar tls_ca is only allowed with pulsar+ssl brokers".to_string(), )); } @@ -540,4 +547,27 @@ mod tests { }; assert!(args.validate().is_err()); } + + #[test] + fn validate_pulsar_accepts_inert_tls_toggles_on_plaintext_broker() { + // `tls_allow_insecure` / `tls_hostname_verification` are no-ops on a + // `pulsar://` broker, so a target persisted with these non-default + // values must still validate — otherwise it stays offline after a + // restart (issue #4796). + let args = PulsarArgs { + tls_allow_insecure: true, + tls_hostname_verification: false, + ..base_args() + }; + args.validate().expect("inert TLS toggles must not fail a plaintext broker"); + } + + #[test] + fn validate_pulsar_rejects_tls_ca_on_plaintext_broker() { + let args = PulsarArgs { + tls_ca: "/etc/ssl/certs/ca.pem".to_string(), + ..base_args() + }; + assert!(args.validate().is_err()); + } }