Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
|
/ready |
FergusMok
changed the title
|
Thanks @FergusMok, and for keeping the old keys working with a deprecation warning |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4304 +/- ##
============================================
- Coverage 87.56% 86.79% -0.77%
Complexity 1575 1575
============================================
Files 1284 1283 -1
Lines 225457 219877 -5580
Branches 188821 183226 -5595
============================================
- Hits 197413 190837 -6576
- Misses 23309 24076 +767
- Partials 4735 4964 +229
🚀 New features to boost your workflow:
|
hubcio
changed the title
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
three inline notes below. two things that do not attach to a line: the description says the deprecated key wins when both are passed, but the parsers take the last occurrence, which should_use_last_connection_option_including_aliases asserts - please fix that sentence. optional drive-bys: core/connectors/runtime/src/stream.rs:207,214 and four connector tests still pass reconnection_retries, so each start now logs a deprecation warning, and the quic deprecated aliases arm at connection_string.rs:230 also carries reconnection_max_retries, which is quic's canonical key, not a deprecated one.
| "Connection string option 'validate_certificate' is deprecated; use 'tls_validate_certificate'" | ||
| ); | ||
| } | ||
| validate_certificate = option_parts[1] == "true"; |
There was a problem hiding this comment.
warning: any value other than an exact true reads as false, so a typo like tls_validate_certificate=ture turns certificate verification off and lands on SkipServerVerification at quic_client.rs:1198. parse the value with parse::<bool>() and reject the rest, like tcp does at tcp_connection_string_options.rs:97.
also at websocket_connection_string_options.rs:191.
| } | ||
| }, | ||
| "validate_certificate" => { | ||
| "tls_validate_certificate" | "validate_certificate" => { |
There was a problem hiding this comment.
warning: the key is tls_validate_certificate now, but the quic field and builder still say validate_certificate (quic_client_config.rs:54, quic_client_config_builder.rs:147), while tcp and websocket say tls_validate_certificate. renaming breaks the api, so either note it on #4210 or leave it for a follow-up.
| let mut tls_enabled = false; | ||
| let mut tls_domain = "".to_string(); | ||
| let mut tls_ca_file = None; | ||
| let mut tls_validate_certificate = true; |
There was a problem hiding this comment.
warning: this key defaults to true on tcp and false on quic and websocket (quic_connection_string_options.rs:100, websocket_connection_string_options.rs:215), so one key now means two security postures. the rustdoc covers both, but foreign/cpp/include/iggy.hpp:2067 states it for tcp only.
github-actions
Bot
added
S-waiting-on-author
This branch has not been deployed
4 participants
Which issue does this PR address?
Closes #4210
Rationale
Currently the different transport protocols (TCP QUIC and Websocket) use different keys to refer to the same thing.
Standardize the configuration connection keys between them.
reconnection_retriesreconnection_max_retriesreconnection_retriesreconnection_max_retriesreestablish_afterreconnection_reestablish_afterreestablish_afterreestablish_aftervalidate_certificatetls_validate_certificatetls_validate_certificateWhat changed?
Changed the connection keys among the different clients. Deprecated connection keys will still work, albeit with a warn log.
The deprecated keys will be removed in a future release, as mentioned in the issue.
Current behavior is that if both the new and deprecated keys are passed into connection, the latter will be used.
Updated the relevant documentation and tests
Local Execution
AI Usage:
For the addition of docs and tests