Add service virtual IP to sandbox's loopback address #1585

aboch · 2016-12-06T00:15:17Z

Signed-off-by: Alessandro Boch aboch@docker.com

sanimej · 2016-12-06T23:37:21Z

@aboch @mavenugo It looks like the elasticsearch does allow binding to 0.0.0.0 . Since we have the service VIP as alias on the overlay interface since 1.12 its less explaining to do if we keep it that way. wdyt ?

aboch · 2016-12-06T23:49:14Z

@sanimej True. But regardless of that specific issue, I feel it is logically more correct that the task's service VIP is programmed on the loopback interface.

It is also less confusing when debugging, the IP on the interface will be just that, and the ones on loopback are the services' VIPs.

Also I'd like to avoid in future to deal with a use case where an app fails to start or function properly because of the IPs ambiguity on an ethX interface if the app happens to make any such assumptions.

sanimej · 2016-12-07T01:11:04Z

@aboch I don't think it makes any difference for debugging or understanding; both needs some documentation. At least the current behavior is already familiar to most users. When a task is attached to multiple networks, currently the interface corresponding to a network will have only that VIP as alias IP. With the change VIPs in different subnets will be an alias IP in the loopback interface. I don't know if this could break any working scenarios. Unless it fixes any specific use case I am not sure if its needed.

mavenugo · 2016-12-07T01:51:42Z

@sanimej I think adding the virtual-ip in this case logically belongs to the loopback interface.

The only reason we added the virtual-ip to the container interface is to support the "loopback" functionality of the self reaching itself using this IP. If we read the purpose of having the loopback interface is exactly that. And it is assigned the 127.0.0.1 address to signify that.

Infact the loopback interface must contain all of the virtual-ips that it has to reach itself and it does logically make sense IMO.

aboch · 2016-12-07T04:34:45Z

@sanimej @mavenugo
I would also like to add that in our case (ingress load balancing) there is no reason for setting the service VIP as an IP alias to the interface. The container does not need to respond on VIP over the network. No packet with the service VIP as dest IP will ever be sent over the overlay networks, an originating packet with dest IP = VIP will never leave the container as is.

In other words, the service VIP has only significance inside the service container.
This is why I think it is natural for it to be assigned to the logical loopback interface.

I also think that if we had some more time to think about it when this was first added, we'd have agreed to set it on the loopback interface. But the fact that it was added to the interface as alias few months ago should not be a reason for now correct it now. Even more given this should not have any functionality change. And no inspect API change, it is transparent change to user.

But I agree, this is not about functionality or bug fix. It is about refining things before it is too late so that IMO they look easier, more natural to understand and verify during debugging.

sanimej · 2016-12-07T20:36:55Z

@aboch @mavenugo I still don't think this buys us anything. But its very unlikely that this change will break anything. So I am ok with it.. will review the change.

sanimej · 2016-12-07T20:41:45Z

osl/sandbox.go

+	AddVirtualIP(ip *net.IPNet) error
+
+	// RemoveVirtualIP removes the passes IP address from the sandbox  loopback interface
+	RemoveVirtualIP(ip *net.IPNet) error


Sticking to the convention, lets call these functions AddAliasIP or more explicitly, AddLoopbackAlias

Agree, from the sandbox pov this is an alias IP on the loopback interface. I will change to AddLookbakcAliasIP. Thanks.

sanimej · 2016-12-07T20:43:45Z

Its pretty common for apps to bind to a specific IP (in this case the overlay IP). In such cases I think accessing the service from within the container won't work ? We are expecting the service to be available on the service VIP. This comment is not specific to this PR.

sanimej · 2016-12-07T22:58:35Z

sandbox.go

-		if len(ep.virtualIP) != 0 {
-			vipAlias := &net.IPNet{IP: ep.virtualIP, Mask: net.CIDRMask(32, 32)}
-			ifaceOptions = append(ifaceOptions, sb.osSbox.InterfaceOptions().IPAliases([]*net.IPNet{vipAlias}))
-		}
 		Ifaces[fmt.Sprintf("%s+%s", i.srcName, i.dstPrefix)] = ifaceOptions


Since we decided to go with the alias IPs on the loopback interface lets remove the unused code from the original change introduced in #1321

I would retain that code instead. It's only few lines and generic enough that we could need it in future.

Previous code has the aliasIP on the osl interface and accessor functions for it. And in configureInterface we will be calling setInterfaceIPAliases expecting it to do nothing. Its confusing to leave such dead code. I think we should remove it.

Signed-off-by: Alessandro Boch <aboch@docker.com>

xproax · 2017-02-27T01:32:00Z

sandbox.go

-		if len(ep.virtualIP) != 0 {
-			vipAlias := &net.IPNet{IP: ep.virtualIP, Mask: net.CIDRMask(32, 32)}
-			ifaceOptions = append(ifaceOptions, sb.osSbox.InterfaceOptions().IPAliases([]*net.IPNet{vipAlias}))
-		}
 		Ifaces[fmt.Sprintf("%s+%s", i.srcName, i.dstPrefix)] = ifaceOptions


Refreshed the PR: moby#1585 Addressed comments suggesting to remove the IPAlias logic not anymore used Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

euanh · 2019-06-27T11:03:24Z

Merged in #1877

Refreshed the PR: moby/libnetwork#1585 Addressed comments suggesting to remove the IPAlias logic not anymore used Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

sanimej reviewed Dec 7, 2016

View reviewed changes

Add service virtual IP to sandbox's loopback address

9130352

Signed-off-by: Alessandro Boch <aboch@docker.com>

xproax approved these changes Jan 5, 2017

View reviewed changes

aboch mentioned this pull request Jan 26, 2017

Docker 1.13.0 in swarm mode fails to route packets to secondary IP of an interface that belongs to an overlay network. moby/moby#30461

Open

xproax approved these changes Feb 27, 2017

View reviewed changes

xproax reviewed Feb 27, 2017

View reviewed changes

fcrisciani mentioned this pull request Aug 1, 2017

Add service virtual IP to sandbox's loopback address #1877

Merged

euanh closed this Jun 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add service virtual IP to sandbox's loopback address #1585

Add service virtual IP to sandbox's loopback address #1585

aboch commented Dec 6, 2016

sanimej commented Dec 6, 2016

aboch commented Dec 6, 2016

sanimej commented Dec 7, 2016

mavenugo commented Dec 7, 2016

aboch commented Dec 7, 2016

sanimej commented Dec 7, 2016

sanimej Dec 7, 2016

aboch Dec 7, 2016

sanimej commented Dec 7, 2016

sanimej Dec 7, 2016

aboch Dec 7, 2016

sanimej Dec 7, 2016

xproax Feb 27, 2017

euanh commented Jun 27, 2019

Add service virtual IP to sandbox's loopback address #1585

Add service virtual IP to sandbox's loopback address #1585

Conversation

aboch commented Dec 6, 2016

sanimej commented Dec 6, 2016

aboch commented Dec 6, 2016

sanimej commented Dec 7, 2016

mavenugo commented Dec 7, 2016

aboch commented Dec 7, 2016

sanimej commented Dec 7, 2016

sanimej Dec 7, 2016

Choose a reason for hiding this comment

aboch Dec 7, 2016

Choose a reason for hiding this comment

sanimej commented Dec 7, 2016

sanimej Dec 7, 2016

Choose a reason for hiding this comment

aboch Dec 7, 2016

Choose a reason for hiding this comment

sanimej Dec 7, 2016

Choose a reason for hiding this comment

xproax Feb 27, 2017

Choose a reason for hiding this comment

euanh commented Jun 27, 2019