Add service virtual IP to sandbox's loopback address#1585
Conversation
|
@aboch @mavenugo It looks like the elasticsearch does allow binding to |
|
@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. |
|
@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. |
|
@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. |
|
@sanimej @mavenugo In other words, the service VIP has only significance inside the service container. 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. |
| AddVirtualIP(ip *net.IPNet) error | ||
|
|
||
| // RemoveVirtualIP removes the passes IP address from the sandbox loopback interface | ||
| RemoveVirtualIP(ip *net.IPNet) error |
There was a problem hiding this comment.
Sticking to the convention, lets call these functions AddAliasIP or more explicitly, AddLoopbackAlias
There was a problem hiding this comment.
Agree, from the sandbox pov this is an alias IP on the loopback interface. I will change to AddLookbakcAliasIP. Thanks.
|
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. |
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I would retain that code instead. It's only few lines and generic enough that we could need it in future.
There was a problem hiding this comment.
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 <[email protected]>
| 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 <[email protected]>
Refreshed the PR: moby#1585 Addressed comments suggesting to remove the IPAlias logic not anymore used Signed-off-by: Flavio Crisciani <[email protected]>
Refreshed the PR: moby#1585 Addressed comments suggesting to remove the IPAlias logic not anymore used Signed-off-by: Flavio Crisciani <[email protected]>
Refreshed the PR: moby#1585 Addressed comments suggesting to remove the IPAlias logic not anymore used Signed-off-by: Flavio Crisciani <[email protected]>
Refreshed the PR: moby#1585 Addressed comments suggesting to remove the IPAlias logic not anymore used Signed-off-by: Flavio Crisciani <[email protected]>
Refreshed the PR: moby#1585 Addressed comments suggesting to remove the IPAlias logic not anymore used Signed-off-by: Flavio Crisciani <[email protected]>
|
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 <[email protected]>
5 participants
Related to moby/moby#29022
Signed-off-by: Alessandro Boch [email protected]