-
Notifications
You must be signed in to change notification settings - Fork 5.5k
listener: Use v4-mapped addressing consistently. #2572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
0816aa3
59b1077
3d8e281
c23d305
659a3aa
0acec79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -154,5 +154,52 @@ TEST_P(ListenerImplTest, WildcardListenerUseActualDst) { | |
| dispatcher.run(Event::Dispatcher::RunType::Block); | ||
| } | ||
|
|
||
| // Test for the correct behavior when a listener is configured with an ANY address that allows | ||
| // receiving IPv4 connections on an IPv6 socket. In this case the address instances of both | ||
| // local and remote addresses of the connection should be IPv4 instances, as the connection really | ||
| // is an IPv4 connection. | ||
| TEST_P(ListenerImplTest, WildcardListenerIpv4Compat) { | ||
| Stats::IsolatedStoreImpl stats_store; | ||
| Event::DispatcherImpl dispatcher; | ||
| Network::TcpListenSocket socket(Network::Test::getAnyAddress(version_, true), true); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like a comment here (or at the top of the test) briefly explaining the circumstances this is testing. It's confusing to me since this is the only place that leads to invoking Network::Utility::getAnyIpv6Address(false), so is this code testing a case that cannot occur outside of tests?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test mocks a case where a listener would be configured with an IPv6 any address and have it's |
||
| Network::MockListenerCallbacks listener_callbacks; | ||
| Network::MockConnectionHandler connection_handler; | ||
|
|
||
| ASSERT_TRUE(socket.localAddress()->ip()->isAnyAddress()); | ||
|
|
||
| // Do not redirect since use_original_dst is false. | ||
| Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks, true, true); | ||
|
|
||
| auto listener_address = Network::Utility::getAddressWithPort( | ||
| *Network::Test::getCanonicalLoopbackAddress(version_), socket.localAddress()->ip()->port()); | ||
| auto local_dst_address = Network::Utility::getAddressWithPort( | ||
| *Network::Utility::getCanonicalIpv4LoopbackAddress(), socket.localAddress()->ip()->port()); | ||
| Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( | ||
| local_dst_address, Network::Address::InstanceConstSharedPtr(), | ||
| Network::Test::createRawBufferSocket(), nullptr); | ||
| client_connection->connect(); | ||
|
|
||
| EXPECT_CALL(listener, getLocalAddress(_)) | ||
| .WillOnce(Invoke( | ||
| [](int fd) -> Address::InstanceConstSharedPtr { return Address::addressFromFd(fd); })); | ||
|
|
||
| EXPECT_CALL(listener_callbacks, onAccept_(_, _)) | ||
| .WillOnce(Invoke([&](Network::ConnectionSocketPtr& socket, bool) -> void { | ||
| Network::ConnectionPtr new_connection = dispatcher.createServerConnection( | ||
| std::move(socket), Network::Test::createRawBufferSocket()); | ||
| listener_callbacks.onNewConnection(std::move(new_connection)); | ||
| })); | ||
| EXPECT_CALL(listener_callbacks, onNewConnection_(_)) | ||
| .WillOnce(Invoke([&](Network::ConnectionPtr& conn) -> void { | ||
| EXPECT_EQ(conn->localAddress()->ip()->version(), conn->remoteAddress()->ip()->version()); | ||
| EXPECT_EQ(*conn->localAddress(), *local_dst_address); | ||
| client_connection->close(ConnectionCloseType::NoFlush); | ||
| conn->close(ConnectionCloseType::NoFlush); | ||
| dispatcher.exit(); | ||
| })); | ||
|
|
||
| dispatcher.run(Event::Dispatcher::RunType::Block); | ||
| } | ||
|
|
||
| } // namespace Network | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,10 +116,24 @@ Address::InstanceConstSharedPtr getCanonicalLoopbackAddress(Address::IpVersion v | |
| return Network::Utility::getIpv6LoopbackAddress(); | ||
| } | ||
|
|
||
| Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version) { | ||
| // There is no portable way to initialize sockaddr_in6 with a static initializer, do it with a | ||
| // helper function instead | ||
| static sockaddr_in6 v6any_() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Envoy convention is to put this in the anonymous namespace rather than mark it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applied. |
||
| sockaddr_in6 v6any = {}; | ||
| v6any.sin6_family = AF_INET6; | ||
| v6any.sin6_addr = in6addr_any; | ||
|
|
||
| return v6any; | ||
| } | ||
|
|
||
| Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version, bool v4_compat) { | ||
| if (version == Address::IpVersion::v4) { | ||
| return Network::Utility::getIpv4AnyAddress(); | ||
| } | ||
| if (v4_compat) { | ||
| static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(v6any_(), false)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add some internal comments here explaining how this (maybe using ASCII address notation) differs from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added this: |
||
| return any; | ||
| } | ||
| return Network::Utility::getIpv6AnyAddress(); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,9 +73,13 @@ Address::InstanceConstSharedPtr getCanonicalLoopbackAddress(const Address::IpVer | |
| /** | ||
| * Returns the any address for the specified IP version. | ||
| * @param version the IP version of the any address. | ||
| * @param v4_compat determines whether a v4-mapped addresses bound to a socket listening on the | ||
| * returned ANY address are to be treated as IPv4 or IPv6 addresses. Defaults to 'false', | ||
| * has no effect with IPv4 ANY address. | ||
| * @returns the any address for the specified IP version. | ||
| */ | ||
| Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version); | ||
| Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version, | ||
| bool v4_compat = false); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Document v4_compat, please.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. |
||
|
|
||
| /** | ||
| * This function tries to create a socket of type IpVersion version and bind to it. If | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nit: prefer to negate the conditional here, easier to parse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.