fix(sdk): allow hostnames in server address configuration#2923
fix(sdk): allow hostnames in server address configuration#2923hubcio merged 12 commits intoapache:masterfrom
Conversation
Signed-off-by: Faisal Ahmed <faisalahmedfarooq46@gmail.com>
Signed-off-by: Faisal Ahmed <faisalahmedfarooq46@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2923 +/- ##
============================================
+ Coverage 70.60% 70.61% +0.01%
Complexity 943 943
============================================
Files 1113 1114 +1
Lines 94590 94778 +188
Branches 71790 71994 +204
============================================
+ Hits 66782 66929 +147
- Misses 25338 25354 +16
- Partials 2470 2495 +25
🚀 New features to boost your workflow:
|
hubcio
left a comment
There was a problem hiding this comment.
hey, thanks for the pr! the intent is solid but there's a fundamental issue with the approach.
to_socket_addrs() calls getaddrinfo() under the hood, which is a synchronous blocking dns lookup. doing this inside build() means config construction now requires network access and can block the thread for seconds if dns is slow. config builders hould be pure/deterministic - th quic builder already gets this right by doing zero address validation in build(). we cannot accept this resolution.
validate the format only (valid host:port structure, port is a valid u16) and leave actual dns resolution to connect time. TcpStream::connect already does resolution internally anyway.
|
hello @felixfaisal do you plan to continue? |
Hey @hubcio Sorry for not paying attention here, I was a bit caught up this week, I plan to address the comments and update the logic by tonight or late tomorrow. I mostly agree with your review and will do the changes. |
Signed-off-by: Faisal Ahmed <faisalahmedfarooq46@gmail.com>
|
@felixfaisal both of these comments are not fixed. do you plan do to so? |
Signed-off-by: Faisal Ahmed <faisalahmedfarooq46@gmail.com>
core/common/src/types/configuration/tcp_config/tcp_client_config_builder.rs
Outdated
Show resolved
Hide resolved
- Rename function to better reflect its purpose (validation only, not parsing) - Make net module private and only export validate_server_address publicly - Update config builders to normalize and validate server addresses - Remove duplicate address validation from examples Signed-off-by: Faisal Ahmed <faisalahmedfarooq46@gmail.com>
hubcio
left a comment
There was a problem hiding this comment.
besides that, it seems like quic is not properly validating this:
QuicClientConfigBuilder::build()andHttpClientConfigBuilder::build()have zero address validation, making them inconsistent with the now-validated tcp/ws builders. http uses a url format so it needs different validation. consider a follow-up to add appropriate validat
…s for invalid addresses Hostnames with underscores are now accepted to support Docker Compose service names and SRV records. Error messages for missing ports/brackets now show the full original address with a placeholder instead of a dangling colon. Signed-off-by: Faisal Ahmed <faisalahmedfarooq46@gmail.com>
|
Created follow-up issue - #3075 |
Which issue does this PR close?
Closes #2882
Rationale
Server address validation previously required a literal socket address, preventing the use of DNS hostnames. This change allows hostnames (e.g.,
localhost:8080) in addition to IP addresses.What changed?
Server address validation previously relied on
SocketAddrparsing, which only accepts literal IP addresses and rejected DNS hostnames (e.g.,localhost:8080). This prevented using hostnames when configuring client connections.Validation now uses
ToSocketAddrs, which performs address resolution and returns the list of socket addresses for a given host.TcpStream::connect(...)already performs this resolution internally and attempts to connect to each resolved address.Local Execution
AI Usage
If AI tools were used, please answer:
cargo buildTCPConfigBuilderand added an additional unit test, also modifying another