fix(sdk): unify connection string options across TCP, QUIC and WebSocket by SteveParadox · Pull Request #4872 · apache/iggy

SteveParadox · 2026-11-12T09:52:45Z

Which issue does this PR address?

Rationale

TCP, QUIC and WebSocket connection strings use inconsistent option names and certificate-validation behavior, standardizing these options improves configuration consistency while preserving compatibility with existing connection strings.

What changed?

TCP, QUIC and WebSocket previously used inconsistent reconnection and TLS option names, with incomplete validation and propagation of some settings into client configurations.

The parsers now support standardized option names while retaining deprecated aliases with warnings, duplicate options follow last-occurrence precedence, malformed values are rejected and existing transport-specific TLS defaults are preserved. Client configuration propagation, TLS connection behavior and the documented connection-string options have also been updated and covered by regression tests.

Local Execution

Passed:

cargo test -p iggy_common --lib — 545 passed, 0 failed.
TCP connection-string regression tests — 8 passed.
QUIC connection-string regression tests — 10 passed.
WebSocket connection-string regression tests — 4 passed.
TCP, QUIC, and WebSocket deprecation-warning integration tests — 3 passed.
Targeted SDK TLS and configuration-validation tests — 5 passed.
git diff --check upstream/master...HEAD — passed.
cargo test -p iggy_common --tests — passed.
cargo test -p iggy --lib — 770 passed.
cargo test -p iggy_common --tests — passed.

AI Usage

1. Which tools?

ChatGPT, Codex

2. Scope of usage?

Assistance with test strategy, regression verification commands, analysis of test results and preparation of the PR description.

Any AI-assisted generation or modification of implementation code must also be disclosed here before submission.

3. How did you verify the generated code works correctly?

Executed the targeted TCP, QUIC and WebSocket regression tests, deprecated-option warning tests and SDK TLS tests locally. The complete iggy_common library suite passed all 545 tests, additional SDK.

4. Can you explain every line of the code if asked?

I will be prepared to explain the parsing changes, deprecated aliases, precedence rules, configuration propagation, TLS behavior and regression tests.

Security consideration: TCP TLS certificate validation

Previously, TCP connections enforced certificate validation unconditionally.

This implementation introduces configurable tls_validate_certificate behavior as requested in #4210.

Certificate validation remains enabled by default (true).
Explicitly setting tls_validate_certificate=false disables certificate verification.
This is a security-sensitive behavioral change because it allows connections without server certificate verification.
Disabling certificate verification should only be considered for controlled testing environments.

Maintainer review and explicit approval are requested for this behavior before merging.

Error handling compatibility

Invalid retry-count values in the updated Rust connection-string parsers now return InvalidConnectionString instead of the previous InvalidNumberValue error, including for QUIC.

This is an intentional error-classification change associated with the connection-string validation updates in #4210.

Go and Swift currently keeps the previous error behavior, Cross-SDK error compatibility will be tracked in a separate follow-up issue.

Consumers matching specific error variants should be aware of this behavioral change.

…propagation

…setup

Accept the canonical reconnect retry key while warning on the legacy key. Reject malformed certificate validation settings so a typo cannot silently disable verification. Refs: apache#4210 Co-authored-by: Codex <codex@openai.com>

github-actions · 2026-11-12T09:53:46Z

Thanks for the PR. It is labeled S-waiting-on-review and queued for review.

If AI assistance was used to create this PR, you are required to run the team-review skill before opening it.

Slash commands (own line, regular comment) move it around the queue:

/ready - back to S-waiting-on-review after addressing feedback
/author - flip to S-waiting-on-author while you finish changes
/request-review @user-or-team - request a reviewer
/pin - exempt the PR from the stale bot, /unpin to undo

See CONTRIBUTING.md for details.

krishvishal

Requesting changes: the PR body doesn't mention that TCP can now skip cert checks or the new retry error, and other SDKs reject the new keys.
Also c2e791a has a Codex co-author, please name it under AI Usage.

krishvishal · 2026-11-12T12:11:28Z

            tls_ca_file: connection_string.options().tls_ca_file().to_owned(),
-            // Require certificate verification, including when trusting a private CA.
-            tls_validate_certificate: true,
+            tls_validate_certificate: connection_string.options().tls_validate_certificate(),


The old code forced true here on purpose (see the removed comment), and now a TCP connection string can turn cert checks off.
I get that #4210 asks for the key, but please call this out in the PR body so a maintainer can sign off on it.

krishvishal · 2026-11-12T12:11:28Z

+                        ),
+                    };
+                }
                "reconnection_retries" => {


Go, .NET, Node and Swift reject reconnection_max_retries and tls_validate_certificate as unknown keys, so following this warning breaks the same string there.
Please add the keys to those parsers, or open a follow-up issue and link it here.

Thanks for highlighting the cross-SDK compatibility issue, I’ve opened a dedicated follow-up issue to track support for the standardized connection-string options across Go, .NET, Node.js and Swift.

I’ll link it in the PR description as well, so i can keep #4210 focused on the current Rust implementation.

krishvishal · 2026-11-12T12:11:28Z

+                        value => Some(
+                            value
+                                .parse()
+                                .map_err(|_| IggyError::InvalidConnectionString)?,


A bad retry count now returns InvalidConnectionString instead of InvalidNumberValue (QUIC too), while Go and Swift still return the old one.
Please mention it in the PR body and cover Go/Swift in the follow-up.

I have explicitly documented the change from InvalidNumberValue to InvalidConnectionString, including its impact on QUIC, in the PR description.

I also include Go and Swift error compatibility and corresponding regression tests in the cross-SDK follow-up issue.

krishvishal · 2026-11-12T12:11:28Z

+}
+
+#[test]
+fn connection_string_hosts_preserve_the_existing_explicit_server_name_contract() {


Connection strings still pin QUIC server_name to localhost, so tls_validate_certificate=true can't work against a real remote host.
That's older than this PR and fine as a follow-up, but I'd drop this test instead of locking it in.

krishvishal · 2026-11-12T12:11:28Z

+    ///     "iggy+ws://user:secret@localhost:8092?reconnection_max_retries=5&reestablish_after=500ms&tls_validate_certificate=true&tls=true&tls_domain=localhost",
+    /// )?
+    /// .build()?;
+    /// let legacy_client = IggyClient::builder_from_connection_string(


legacy_client teaches the deprecated key with cert checks off, and the example lost most of the options the old one had.
Can you drop legacy_client and bring back the full example with the new keys?

krishvishal · 2026-11-12T12:11:28Z

     *
     * Durations use Iggy duration syntax, such as `500ms`, `5s`, or `1min`.
-     * Boolean values are `true` or `false`.
+     * Certificate validation accepts exactly `true` or `false`. Disabling it


nit: the "Boolean values are true or false" line got dropped, and now only cert validation is described as strict, though tls is strict too.
Please bring that line back and mention tls.

krishvishal · 2026-11-12T12:11:28Z

                }
                "tls" => {
-                    tls_enabled = option_parts[1] == "true";
+                    tls_enabled = option_parts[1]


The Swift notes are stale now that tls is strict: foreign/swift/README.md:125 and ConnectionString.swift:40,202 say Rust reads non-true as false.
Please update them in this PR.

krishvishal · 2026-11-12T12:11:28Z

     * - `tls_ca_file=<path>`
-     * - `reconnection_retries=<uint32|unlimited>`
+     * - `tls_validate_certificate=<bool>` (default `true`)
+     * - `reconnection_max_retries=<uint32|unlimited>` (`reconnection_retries` is deprecated)


The Python and PHP bindings don't install a tracing subscriber, so their users never see these deprecation warnings.
Could the Python and PHP connection string docs list the deprecated keys like this header does?

krishvishal · 2026-11-12T12:11:28Z

+                        .map_err(|_| IggyError::InvalidConnectionString)?;
                }
                "nodelay" => {
                    nodelay = option_parts[1] == "true";


nodelay still does == "true", so nodelay=1 quietly means false while the other SDKs reject it (same for accept_unmasked_frames).
Pre-existing and not blocking, but with tls strict now, parse::<bool>() here would match.

krishvishal · 2026-11-12T12:11:28Z

+                    reestablish_after = IggyDuration::from_str(option_parts[1])
+                        .map_err(|_| IggyError::InvalidConnectionString)?;
                }
                "reconnection_reestablish_after" => {


A few callers still use deprecated keys and will log warnings on every run: client_lifecycle.rs:137 (this one), postgres_source.rs:198,319, tcp_client.rs:1597, test_client_config.py:469.
Please switch them to the new keys.

krishvishal · 2026-11-12T12:26:36Z

/author

Match the QUIC and WebSocket warning format and update the TCP test.

SteveParadox and others added 11 commits November 6, 2026 16:44

fix(common): standardize TCP retry and TLS connection options

e27f421

fix(common): propagate TCP TLS certificate verification option

56bb7e1

test(common): match parent module in TCP deprecation warning capture

7576bee

docs(sdk): document canonical TCP retry key and TLS verification

3cd8056

fix(quic): support canonical connection options and strict TLS parsing

a136922

test(quic): correct warning-capture assertions and formatting

65e506e

test(quic): add scoped tracing subscriber for alias warning assertions

d860d02

test(quic): cover deprecated TLS false and mixed-alias configuration …

dcc4373

…propagation

test(quic): exercise connection-string propagation through transport …

fd1760a

…setup

test(sdk): verify connection options across transports

5b9133c

github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Nov 12, 2026

krishvishal requested changes Nov 12, 2026

View reviewed changes

Merge branch 'master' into fix/4210-unified-connection-options-pr

e6c2418

github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Nov 12, 2026

fix(sdk): align TCP deprecation warning wording

e9e206f

Match the QUIC and WebSocket warning format and update the TCP test.

SteveParadox force-pushed the fix/4210-unified-connection-options-pr branch from 6af630f to e9e206f Compare November 13, 2026 06:11

SteveParadox added 8 commits November 13, 2026 07:12

docs(sdk): align WebSocket connection option entries

9220796

docs(sdk): clarify C++ and Swift TLS boolean options

7a47a59

test(sdk): use canonical connection string options

b8b9674

fix(sdk): validate TCP and WebSocket boolean options

904062d

docs(sdk): list deprecated keys for Python and PHP

f755e35

docs(sdk): restore full WebSocket connection string example

625ae87

test(sdk): drop QUIC localhost server-name contract test

70a8de3

test(sdk): decouple QUIC hostname verification from default

a68c3b8

Conversation

SteveParadox commented Nov 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR address?

Rationale

What changed?

Local Execution

AI Usage

Uh oh!

github-actions Bot commented Nov 12, 2026

Uh oh!

krishvishal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krishvishal commented Nov 12, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SteveParadox commented Nov 12, 2026 •

edited

Loading