test/integration: Fix quic_http_integration_test on macOS#45318
test/integration: Fix quic_http_integration_test on macOS#45318filipcacky wants to merge 5 commits into
quic_http_integration_test on macOS#45318Conversation
…istenerFilterReceivesFirstPacketWithCmsg` Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>
UsesPreferredAddress{,DNAT,DualStack} configure 127.0.0.2 as the server's
preferred address. This requires 127.0.0.2 to be configured on a
local interface, which is not the case on macOS by default.
Probe at the start of each test by attempting to bind a UDP socket
to 127.0.0.2 and skip when bind fails, instead of letting the tests
fail. On Linux the entire 127/8 range is reachable, so the probe
always passes.
Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>
|
Hi @filipcacky, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
Even with and probably needs something like https://gist.github.com/filipcacky/ffbbae44c0b10ba3c07b4a7f537887cb, but then it fails on something else. Didn't dig too much into it yet. |
8e7962d to
e9a3c89
Compare
macOS rejects sendmsg with EINVAL when msg_name is AF_INET on an AF_INET6 fd, even when the socket is dual-stack. This breaks QUIC SPA replies on a dual-stack listener: PATH_RESPONSE never leaves the socket. Linux accepts both forms. Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>
e9a3c89 to
8638e35
Compare
|
@danzh2010, are you up for a review? |
This is not a test-only change. The change to sendmsg() applies everywhere on iOS and MacOS. |
| const auto* v4 = dynamic_cast<const Address::Ipv4Instance*>(&peer_address); | ||
| if (v4 != nullptr) { |
There was a problem hiding this comment.
Please check version() instead of relying on dynamic_cast.
| // macOS rejects an AF_INET msg_name on an AF_INET6 fd with EINVAL even when the | ||
| // socket is dual-stack. When sending an IPv4 destination on such a socket, swap msg_name | ||
| // for the v4-mapped sockaddr_in6 form. | ||
| sockaddr_in6 mapped_peer; |
There was a problem hiding this comment.
This tmp variable can be placed inside the if block below.
There was a problem hiding this comment.
Needs to outlive the if block because of message.msg_name = &mapped_peer;
| @@ -178,6 +178,17 @@ bool Ipv4Instance::operator==(const Instance& rhs) const { | |||
| (ip_.port() == rhs_casted->ip_.port()) && (networkNamespace() == rhs.networkNamespace())); | |||
| } | |||
|
|
|||
| sockaddr_in6 Ipv4Instance::v4MappedSockAddr() const { | |||
There was a problem hiding this comment.
If the v4 mapped address isn't retained, this method isn't necessary. You can just put this logic into a helper function of sendmsg().
| } | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
nits: for the whole block you can write:
#ifndef __APPLE__
message.msg_name = reinterpret_cast<void*>(sock_addr);
message.msg_namelen = address_base->sockAddrLen();
#else
// populate with mapped address
#endif
There was a problem hiding this comment.
message.msg_name = reinterpret_cast<void*>(sock_addr);
message.msg_namelen = address_base->sockAddrLen();
still needs to run if !(domain_ == AF_INET6 && sock_addr->sa_family == AF_INET), so i'd still need to put that in else branch.
| @@ -172,6 +172,22 @@ Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slic | |||
| msghdr message; | |||
| message.msg_name = reinterpret_cast<void*>(sock_addr); | |||
| message.msg_namelen = address_base->sockAddrLen(); | |||
|
|
|||
| #if defined(__APPLE__) | |||
There was a problem hiding this comment.
Is __APPLE__ necessarily macOS? How about iOS behavior?
There was a problem hiding this comment.
Not sure how to check iOS behaviour. But seems like the way to check target for apple is using TargetConditionals.h
There was a problem hiding this comment.
There are probably some macros to distinguish different apple platforms.
Actually we didn't notice any such write error in YT app on iOS but I would assume MacOS and iOS use the same network stack. Can you do some research about which apple platform has this issue?
There was a problem hiding this comment.
How about connect()? We also used connected UDP socket with writev().
There was a problem hiding this comment.
Didn't notice that, seems to have the same issue.
|
Also needs a main merge |
filipcacky
left a comment
There was a problem hiding this comment.
connect has the same EINVAL issue for both TCP and UDP and i'm not sure yet how to handle this properly together with envoy.reloadable_features.always_use_v6.
I decided to revert the io handle changes, seems to be more complicated and out of scope of wanting to fix a few tests. I'll file a new PR with io handle tests, seems like there are none for this at the moment, so it manifests in other unit tests instead. Sorry for the extra work.
| // macOS rejects an AF_INET msg_name on an AF_INET6 fd with EINVAL even when the | ||
| // socket is dual-stack. When sending an IPv4 destination on such a socket, swap msg_name | ||
| // for the v4-mapped sockaddr_in6 form. | ||
| sockaddr_in6 mapped_peer; |
There was a problem hiding this comment.
Needs to outlive the if block because of message.msg_name = &mapped_peer;
There was a problem hiding this comment.
Didn't notice that, seems to have the same issue.
| @@ -172,6 +172,22 @@ Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slic | |||
| msghdr message; | |||
| message.msg_name = reinterpret_cast<void*>(sock_addr); | |||
| message.msg_namelen = address_base->sockAddrLen(); | |||
|
|
|||
| #if defined(__APPLE__) | |||
There was a problem hiding this comment.
Not sure how to check iOS behaviour. But seems like the way to check target for apple is using TargetConditionals.h
| } | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
message.msg_name = reinterpret_cast<void*>(sock_addr);
message.msg_namelen = address_base->sockAddrLen();
still needs to run if !(domain_ == AF_INET6 && sock_addr->sa_family == AF_INET), so i'd still need to put that in else branch.
This reverts commit 8638e35. Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>
Signed-off-by: Filip Cacky <filip.cacky@cdn77.com>
64fbf6f to
bd406d4
Compare
Commit Message: test/integration: Fix Linux assumptions failing
quic_http_integration_teston macOSQuicListenerFilterReceivesFirstPacketWithCmsghardcodedIP_TOS,IP_RECVTOSandIPV6_PKTINFOLinux values which caused the test to fail on macOS.UsesPreferredAddress{,DNAT,DualStack}assumed 127.0.0.2 to be bindable, which is not the case on macOS by default.Risk Level: Low
Testing: Fixed/skipped tests for macOS, passes on Linux
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A