Conversation
|
@copilot build, run the tests, summarize failures and take them into account to apply a tentative fix. |
|
@achamayou I've opened a new pull request, #7732, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds initial end-to-end coverage for IPv6 loopback usage in the test suite, and updates host/node networking code paths to better handle IPv6 addresses (notably in TCP connection setup and certificate SAN generation).
Changes:
- Add an e2e test that starts a network with RPC interfaces bound to
::1and exercises common endpoints. - Adjust test infra to avoid binding the IPv4-only “client interface” used for partition simulation when the node RPC host is IPv6.
- Improve IPv6 handling in node certificate SAN construction and in host TCP client socket creation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/infra/node.py |
Skips node_client_host binding when RPC host is IPv6 to avoid incompatible partition-simulation client interface. |
tests/e2e_logging.py |
Registers a new common_ipv6 sub-test using the logging sample app. |
tests/e2e_common_endpoints.py |
Adds run_ipv6() which forces nodes to bind to ::1 (skipping if IPv6 loopback is unavailable). |
src/node/node_state.h |
Strips brackets from IPv6 hosts when constructing default SANs; updates IPv6-related comment in is_ip(). |
src/host/tcp.h |
Defers client socket creation to use the resolved address family (IPv4 vs IPv6). |
Comments suppressed due to low confidence (1)
src/node/node_state.h:2347
- The is_ip() heuristic does not correctly detect general IPv6 literals: it only returns true when the final ':' component is purely numeric, so valid IPv6 addresses ending with hex letters (eg "2001:db8::a") will be treated as DNS names. This can produce incorrect SAN types in generated node certificates and break TLS validation when connecting by IP. Consider replacing this heuristic with a real IPv4/IPv6 parse (eg inet_pton / uv_inet_pton) after stripping brackets (and any zone ID if present).
bool is_ip(const std::string_view& hostname)
{
// IP address components are purely numeric. DNS names may be largely
// numeric, but at least the final component (TLD) must not be
// all-numeric. So this distinguishes "1.2.3.4" (an IP address) from
// "1.2.3.c4m" (a DNS name). "1.2.3." is invalid for either, and will
// throw. Handles IPv6 by splitting on ':' after splitting on '.'.
const auto final_component =
ccf::nonstd::split(ccf::nonstd::split(hostname, ".").back(), ":")
.back();
if (final_component.empty())
{
throw std::runtime_error(fmt::format(
"{} has a trailing period, is not a valid hostname", hostname));
}
return std::ranges::all_of(
final_component, [](char c) { return c >= '0' && c <= '9'; });
You can also share your feedback on Copilot code review. Take the survey.
Agent-Logs-Url: https://github.com/microsoft/CCF/sessions/025065b0-abc1-490d-9d50-efba3e4d7373 Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/CCF/sessions/025065b0-abc1-490d-9d50-efba3e4d7373 Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Done in 1ae1a18. I ran local checks and reviewed CI failures:
Tentative fix applied in
|
|
|
||
| #include <netinet/in.h> | ||
| #include <optional> | ||
| #ifndef _WIN32 |
| // This is best-effort cleanup on an existing failure path: we only log | ||
| // close() errors (including EINTR). We intentionally do not retry | ||
| // close(), since retrying may close a reused fd. | ||
| #ifdef _WIN32 |
There was a problem hiding this comment.
Drop all WIN32 include guards, we don't want them.
There was a problem hiding this comment.
Done in 262f563: removed the remaining _WIN32 guards in src/host/tcp.h (timeout and socket-close paths).
|
@copilot add to the description of the PR mermaid flowcharts that show the connection flow before and after your changes. |
Agent-Logs-Url: https://github.com/microsoft/CCF/sessions/0cf0011c-18a2-4003-951c-55a903a0b6a5 Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
|
@copilot add to the description of the PR mermaid flowcharts that show the connection flow before and after your changes. |
Agent-Logs-Url: https://github.com/microsoft/CCF/sessions/2caf1046-8ae3-4198-98b2-d97b68196b3f Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Updated the PR description with Mermaid flowcharts showing the connection flow before and after the TCP IPv6 retry/timeout changes (update pushed with 262f563).
Updated the PR description with Mermaid flowcharts showing the connection flow before and after the TCP IPv6 retry/timeout changes (update pushed with 262f563). |
#3215