network: allow IP instance creation for unsupported IP families#42422
network: allow IP instance creation for unsupported IP families#42422bencebeky wants to merge 1 commit intoenvoyproxy:mainfrom
Conversation
bencebeky
force-pushed
the
bencebeky/ip-family
branch
from
f123ef1 to
1521e18
Compare
|
cc @agrawroh |
bencebeky
force-pushed
the
bencebeky/ip-family
branch
5 times, most recently
from
6613b7a to
a3c4661
Compare
bencebeky
force-pushed
the
bencebeky/ip-family
branch
from
a3c4661 to
caabb90
Compare
bencebeky
force-pushed
the
bencebeky/ip-family
branch
8 times, most recently
from
871875f to
528bf57
Compare
1. Allow Ipv4Instance and Ipv6Instance objects to be created even if host does not support IPv4 or IPv6. 2. Add support for SocketInterfaceImpl::socket() to return error status to signal unsupported IP family at socket creation time. Fixes envoyproxy#41826 Signed-off-by: Bence Béky <bence.beky@datadoghq.com>
bencebeky
force-pushed
the
bencebeky/ip-family
branch
from
b857332 to
b477bd7
Compare
| Network::ConnectionSocketPtr connection_socket = | ||
| connection_socket_or.ok() ? std::move(*connection_socket_or) | ||
| : std::make_unique<Network::ConnectionSocketImpl>( | ||
| std::make_unique<Network::IoSocketHandleImpl>(INVALID_SOCKET), |
There was a problem hiding this comment.
I am not sure why we would create a socket with invalid socket here. did you check the call path to see how this was handled?
There was a problem hiding this comment.
At
it is stated that the socket can be bad inside the CreationResult struct. I returned StatusOr in an earlier draft of this PR but that necessitates changing dozens of more files to handle something that can probably be lumped together with the invalid socket case. But I didn't yet investigate how a bad socket is handled.
| absl::StatusOr<Network::IoHandlePtr> io_handle_or = | ||
| socket_interface.socket(socket_type, address, creation_options); | ||
| RETURN_IF_NOT_OK(io_handle_or.status()); | ||
| if (*io_handle_or == nullptr) { |
There was a problem hiding this comment.
I wonder if this check might be redudant with previous RETURN IF NOT OK
There was a problem hiding this comment.
The existing code handles the nullptr case. I do not want to remove that logic unless we thoroughly investigate to make sure all previous nullptr paths are replaced with an error status. But yes, ideally every method that this PR converts to return StatusOr or StatusOr should then guarantee that the pointer is never nullptr.
|
Hi @bencebeky . Good to see you again. Thanks for making this change. There are a few significant behavioral changes that are present together in this PR making it hard to reason about correctness. Can you split this PR into smaller changes please? For example:
I think it will be easier to discuss these two changes in isolation. /wait-any |
Hello @yanavlasov. It is indeed good to see so many familiar GitHub handles. Thank you for taking a look at the first iteration of this change. I wanted to start by getting a sense of whether Envoy maintainers are generally supportive of this design, and am happy to see no objections so far. (I also communicated with Greg privately.) Thank you for your recommendation, I agree that splitting up this PR into two would make it easier to reason about. /wait |
4 participants
Commit Message:
Additional Description: This PR is inspired by #41836 (comment) and in its first iteration is a proof of concept as an alternative to #41836. In order to be considered for merging, it will require flag protection and extensive testing.
Risk Level: medium
Testing: TODO
Docs Changes: n/a
Release Notes: allow proxy_protocol to parse IPv4 or IPv6 addresses even when not supported by host
Platform Specific Features: n/a
Optional Runtime guard: TODO
Fixes #41826