address: IPv4/IPv6 dual bind. by htuch · Pull Request #2201 · envoyproxy/envoy

htuch · 2017-12-13T07:23:52Z

This adds support for listening for both IPv4 and IPv6 when binding to ::.

Risk Level: Low
Testing: Added unit tests for resolver and address implementations. No integration tests yet, need to convert the admin integration test to v2 first to allow for ipv4_compat to be set on the admin bind address.
API Changes: envoyproxy/data-plane-api#332

Signed-off-by: Harvey Tuch [email protected]

This adds support for listening for both IPv4 and IPv6 when binding to ::. Signed-off-by: Harvey Tuch <[email protected]>

htuch · 2017-12-13T07:26:21Z

Feel free to review, I will work on an independent PR to convert the admin integration test to v2 so I can get the integration tests happening.

alyssawilk · 2017-12-13T15:23:18Z

FWIW I have that test 90% ported but the patch depends on 2185 landing

ggreenway · 2017-12-13T17:41:56Z

include/envoy/network/address.h

+  /**
+   * @return IPv4-IPv6 mapping disabled for IPv6 addresses?
+   */
+  virtual bool v6only() const PURE;


Should this be on class Ipv6 instead of here?

Maybe? Need to do some dynamic casting to make use of where I needed it, but I agree it's cleaner at the interface level.

Can't you pivot off of ipv6() being not-null in the cases that you need?

ggreenway · 2017-12-13T17:45:19Z

source/common/network/address_impl.cc

 }

-InstanceConstSharedPtr addressFromFd(int fd) {
+InstanceConstSharedPtr addressFromFd(int fd, bool v6only) {


In this case, shouldn't we getsockopt to determine if it is set, instead of having this as a parameter?

Yep, this makes sense.

Signed-off-by: Harvey Tuch <[email protected]>

ggreenway · 2017-12-21T17:38:40Z

source/common/network/address_impl.cc

+  int socket_v6only = 0;
+  socklen_t size_int = sizeof(socket_v6only);
+  rc = ::getsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &socket_v6only, &size_int);
+  if (rc != 0 && errno != EOPNOTSUPP) {


Is this safe on all platforms, ie do all platforms that don't support this option return EOPNOTSUPP? As much as it is distasteful, it's tempting to ignore all errors from this getsockopt and just assume the default value if it fails.

ggreenway · 2017-12-21T17:49:17Z

source/common/network/address_impl.cc

+  int socket_v6only = 0;
+  socklen_t size_int = sizeof(socket_v6only);
+  rc = ::getsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &socket_v6only, &size_int);
+  if (rc != 0 && errno != EOPNOTSUPP) {


On second thought, there is a RELEASE_ASSERT that setting this sockopt does not fail, so I think you can treat any failure (ie ignore value of errno) as a failure here.

Signed-off-by: Harvey Tuch <[email protected]>

Changes made.

mattklein123

LGTM, small question.

mattklein123 · 2017-12-29T18:02:59Z

source/common/network/address_impl.cc

+  if (ss.ss_family == AF_INET6) {
+    socklen_t size_int = sizeof(socket_v6only);
+    rc = ::getsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &socket_v6only, &size_int);
+    if (rc != 0) {


realistically can this ever fail? Should this be RELEASE_ASSERT?

Signed-off-by: Harvey Tuch <[email protected]>

jrajahalme · 2018-01-03T21:39:48Z

AFAIK this represents IPv4 connections received on a [::] listener as having (IPv4 compatible) IPv6 addresses. I have an implementation on my work queue that returns an Address::Ipv4Instance instead in that case. I thought otherwise logging and possibly listener filter chain address matching would be impacted.

Also, I would have assumed risk of existing deployments breaking due to v6 sockets accepting v4 connections being low, especially if the address would be internally treated as an IPv4 address like above. If this assumption holds there would be no need to do V6ONLY syscalls at all on platforms that default to accepting v4 connections on v6 sockets.

Thoughs?

htuch · 2018-01-03T21:46:33Z

I think for listener filter chain address matching, you just use the IPv4-in-IPv6 encoding in your configs and you're fine. The idea being you use the correct IP address representation matching what you are binding the listener to. For logging, I can see some advantage in either using an Ipv4Instance or just having an Address::Instance representation function that can do the IPv4 representation.

If you have an implementation already in-flight, maybe post a rough PR and we can discuss? Otherwise I would be inclined to consider whether what we have there today is good enough.

We agreed in envoyproxy/data-plane-api#332 to follow the haproxy convention of explicitly calling out dual bind in the listener socket options, is there a reason to change this?

jrajahalme · 2018-01-03T22:16:40Z

You can see my patch here, very simple but needs rebasing after these changes:

jrajahalme@baa4785

jrajahalme · 2018-01-03T22:18:58Z

As for the explicit dual bind, I had not thought it is necessary, but have no reason to request a change to the agreed solution. Apart from code complexity, but that is in the eye of the beholder anyway :-)

jrajahalme · 2018-01-03T23:01:06Z

On Jan 3, 2018, at 2:16 PM, jrajahalme ***@***.***> wrote: You can see my patch here, very simple but needs rebasing after these changes: ***@***.*** <jrajahalme@baa4785> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2201 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGN127mRk2Bmen1yji1epXMrZ7AWLjTwks5tG_xVgaJpZM4RAHLA>.

I’m rebasing this on top of the current master, will issue a PR after some local testing. Jarno

jrajahalme · 2018-01-04T00:36:13Z

Issued this PR: #2309 <#2309> Jarno

…

On Jan 3, 2018, at 3:01 PM, jrajahalme ***@***.***> wrote: > On Jan 3, 2018, at 2:16 PM, jrajahalme ***@***.***> wrote: > > You can see my patch here, very simple but needs rebasing after these changes: > > ***@***.*** <jrajahalme@baa4785> > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub <#2201 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGN127mRk2Bmen1yji1epXMrZ7AWLjTwks5tG_xVgaJpZM4RAHLA>. > I’m rebasing this on top of the current master, will issue a PR after some local testing. Jarno — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2201 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGN12w9EXpMIhd-1QLQWGrUKW6LNjxsxks5tHAbCgaJpZM4RAHLA>.

htuch · 2018-01-04T11:44:05Z

@jrajahalme I think there are some valid reasons for explicit dual bind; you might want to allow multiple processes to listen on the same port and have one process pick up the IPv4 incoming and the other the IPv6. Having the ability to decide whether you are listening only for IPv6 becomes useful to allow that to happen. So, yeah, I think we should keep that.

Thanks for #2309, we can continue this thread there.

Adding changes since 0.4.5 to release notes Signed-off-by: JP Simard <[email protected]>

address: IPv4/IPv6 dual bind.

456bdbb

This adds support for listening for both IPv4 and IPv6 when binding to ::. Signed-off-by: Harvey Tuch <[email protected]>

alyssawilk mentioned this pull request Dec 13, 2017

Moving more tests over to the v2 API #2204

Merged

ggreenway previously requested changes Dec 13, 2017

View reviewed changes

htuch added 4 commits December 20, 2017 23:10

Merge remote-tracking branch 'upstream/master' into ipv46-bind

3e4a245

addressFromFd() can infer IPv6-only from socket options.

00852fb

Signed-off-by: Harvey Tuch <[email protected]>

Drop v6only() on address instance interface.

fb3503f

Signed-off-by: Harvey Tuch <[email protected]>

Admin interface integration test.

2b1202b

Signed-off-by: Harvey Tuch <[email protected]>

ggreenway reviewed Dec 21, 2017

View reviewed changes

htuch added 3 commits December 22, 2017 13:46

Merge remote-tracking branch 'upstream/master' into ipv46-bind

91a6a2a

Merge remote-tracking branch 'upstream/master' into ipv46-bind

6dff12c

Review feedback.

e944e63

Signed-off-by: Harvey Tuch <[email protected]>

mattklein123 reviewed Dec 29, 2017

View reviewed changes

htuch added 2 commits December 31, 2017 00:31

Merge remote-tracking branch 'upstream/master' into ipv46-bind

c8ac37a

RELEASE_ASSERT getsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, ..).

25dbb15

Signed-off-by: Harvey Tuch <[email protected]>

ggreenway approved these changes Jan 2, 2018

View reviewed changes

htuch merged commit 12101e5 into envoyproxy:master Jan 3, 2018

htuch deleted the ipv46-bind branch January 3, 2018 11:38

jpsim added a commit that referenced this pull request Nov 28, 2022

Release 0.4.6 (#2201)

627793d

Adding changes since 0.4.5 to release notes Signed-off-by: JP Simard <[email protected]>

jpsim added a commit that referenced this pull request Nov 29, 2022

Release 0.4.6 (#2201)

026fb34

Adding changes since 0.4.5 to release notes Signed-off-by: JP Simard <[email protected]>

Conversation

htuch commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Dec 13, 2017

Uh oh!

alyssawilk commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Jan 3, 2018

Uh oh!

htuch commented Jan 3, 2018

Uh oh!

jrajahalme commented Jan 3, 2018

Uh oh!

jrajahalme commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrajahalme commented Jan 3, 2018 via email

Uh oh!

jrajahalme commented Jan 4, 2018 via email

Uh oh!

htuch commented Jan 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

htuch commented Dec 13, 2017 •

edited

Loading

jrajahalme commented Jan 3, 2018 •

edited

Loading