Add service virtual IP to sandbox's loopback address #1877
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Change LGTM. This will change the VIP assignment behavior that we have had since 1.12.x and can impact apps that make assumptions about the alias IP. Generally such assumptions are bad practices and its unlikely there are many such apps. So this looks ok to me. @mavenugo wdyt ? |
|
@mavenugo PTAL |
mavenugo
reviewed
Aug 3, 2017
| 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})) | ||
| } |
There was a problem hiding this comment.
@fcrisciani am yet to go over the details of this. But it seems like we are missing calling the method to add the vip to the loopback interface ?
There was a problem hiding this comment.
With the loopback interface replacing the endpoint interface to host the virtual-ip, there is no need for restoreOslSandbox to restore the states, since there is no state to be restored in OSL Sandbox :-)
The change is good to go as is.
|
Updated the commit but I would like to discuss the case here with you @mavenugo before the merge |
fcrisciani
force-pushed
the
viplo
branch
2 times, most recently
from
b05dac9
to
6a8bd67
Compare
Refreshed the PR: moby#1585 Addressed comments suggesting to remove the IPAlias logic not anymore used Signed-off-by: Flavio Crisciani <[email protected]>
mavenugo
approved these changes
Aug 9, 2017
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Dec 30, 2017
Today when binding to local (the default) we bind to any address that is a loopback address, or any address on an interface that declares itself as a loopback interface. Yet, not all addresses on loopback interfaces are loopback addresses. This arises on macOS where there is a link-local address assigned to the loopback interface (fe80::1%lo0) and in Docker services where virtual IPs of the service are assigned to the loopback interface (moby/libnetwork#1877). These situations cause problems: - because we do not handle the scope ID of a link-local address, we end up bound to an address for which publishing of that address does not allow that address to be reached (since we drop the scope) - the virtual IPs in the Docker situation are not loopback addresses, they are not link-local addresses, so we end up bound to interfaces that cause the bootstrap checks to be enforced even though the instance is only bound to local We address this by only binding to actual loopback addresses, and skip binding to any address on a loopback interface that is not a loopback address. This lets us simplify some code where in the bootstrap checks we were skipping link-local addresses, and in writing the ports file where we had to skip link-local addresses because again the formatting of them does not allow them to be connected to by another node (to be clear, they could be connected to via the scope-qualified address, but that information is not written out).
jasontedor
added a commit
to elastic/elasticsearch
that referenced
this pull request
Jan 2, 2018
* Only bind loopback addresses when binding to local Today when binding to local (the default) we bind to any address that is a loopback address, or any address on an interface that declares itself as a loopback interface. Yet, not all addresses on loopback interfaces are loopback addresses. This arises on macOS where there is a link-local address assigned to the loopback interface (fe80::1%lo0) and in Docker services where virtual IPs of the service are assigned to the loopback interface (moby/libnetwork#1877). These situations cause problems: - because we do not handle the scope ID of a link-local address, we end up bound to an address for which publishing of that address does not allow that address to be reached (since we drop the scope) - the virtual IPs in the Docker situation are not loopback addresses, they are not link-local addresses, so we end up bound to interfaces that cause the bootstrap checks to be enforced even though the instance is only bound to local We address this by only binding to actual loopback addresses, and skip binding to any address on a loopback interface that is not a loopback address. This lets us simplify some code where in the bootstrap checks we were skipping link-local addresses, and in writing the ports file where we had to skip link-local addresses because again the formatting of them does not allow them to be connected to by another node (to be clear, they could be connected to via the scope-qualified address, but that information is not written out). Relates #28029
jasontedor
added a commit
to elastic/elasticsearch
that referenced
this pull request
Jan 2, 2018
* Only bind loopback addresses when binding to local Today when binding to local (the default) we bind to any address that is a loopback address, or any address on an interface that declares itself as a loopback interface. Yet, not all addresses on loopback interfaces are loopback addresses. This arises on macOS where there is a link-local address assigned to the loopback interface (fe80::1%lo0) and in Docker services where virtual IPs of the service are assigned to the loopback interface (moby/libnetwork#1877). These situations cause problems: - because we do not handle the scope ID of a link-local address, we end up bound to an address for which publishing of that address does not allow that address to be reached (since we drop the scope) - the virtual IPs in the Docker situation are not loopback addresses, they are not link-local addresses, so we end up bound to interfaces that cause the bootstrap checks to be enforced even though the instance is only bound to local We address this by only binding to actual loopback addresses, and skip binding to any address on a loopback interface that is not a loopback address. This lets us simplify some code where in the bootstrap checks we were skipping link-local addresses, and in writing the ports file where we had to skip link-local addresses because again the formatting of them does not allow them to be connected to by another node (to be clear, they could be connected to via the scope-qualified address, but that information is not written out). Relates #28029
jasontedor
added a commit
to elastic/elasticsearch
that referenced
this pull request
Jan 2, 2018
* Only bind loopback addresses when binding to local Today when binding to local (the default) we bind to any address that is a loopback address, or any address on an interface that declares itself as a loopback interface. Yet, not all addresses on loopback interfaces are loopback addresses. This arises on macOS where there is a link-local address assigned to the loopback interface (fe80::1%lo0) and in Docker services where virtual IPs of the service are assigned to the loopback interface (moby/libnetwork#1877). These situations cause problems: - because we do not handle the scope ID of a link-local address, we end up bound to an address for which publishing of that address does not allow that address to be reached (since we drop the scope) - the virtual IPs in the Docker situation are not loopback addresses, they are not link-local addresses, so we end up bound to interfaces that cause the bootstrap checks to be enforced even though the instance is only bound to local We address this by only binding to actual loopback addresses, and skip binding to any address on a loopback interface that is not a loopback address. This lets us simplify some code where in the bootstrap checks we were skipping link-local addresses, and in writing the ports file where we had to skip link-local addresses because again the formatting of them does not allow them to be connected to by another node (to be clear, they could be connected to via the scope-qualified address, but that information is not written out). Relates #28029
jasontedor
added a commit
to elastic/elasticsearch
that referenced
this pull request
Jan 2, 2018
* Only bind loopback addresses when binding to local Today when binding to local (the default) we bind to any address that is a loopback address, or any address on an interface that declares itself as a loopback interface. Yet, not all addresses on loopback interfaces are loopback addresses. This arises on macOS where there is a link-local address assigned to the loopback interface (fe80::1%lo0) and in Docker services where virtual IPs of the service are assigned to the loopback interface (moby/libnetwork#1877). These situations cause problems: - because we do not handle the scope ID of a link-local address, we end up bound to an address for which publishing of that address does not allow that address to be reached (since we drop the scope) - the virtual IPs in the Docker situation are not loopback addresses, they are not link-local addresses, so we end up bound to interfaces that cause the bootstrap checks to be enforced even though the instance is only bound to local We address this by only binding to actual loopback addresses, and skip binding to any address on a loopback interface that is not a loopback address. This lets us simplify some code where in the bootstrap checks we were skipping link-local addresses, and in writing the ports file where we had to skip link-local addresses because again the formatting of them does not allow them to be connected to by another node (to be clear, they could be connected to via the scope-qualified address, but that information is not written out). Relates #28029
jasontedor
added a commit
to elastic/elasticsearch
that referenced
this pull request
Jan 2, 2018
* Only bind loopback addresses when binding to local Today when binding to local (the default) we bind to any address that is a loopback address, or any address on an interface that declares itself as a loopback interface. Yet, not all addresses on loopback interfaces are loopback addresses. This arises on macOS where there is a link-local address assigned to the loopback interface (fe80::1%lo0) and in Docker services where virtual IPs of the service are assigned to the loopback interface (moby/libnetwork#1877). These situations cause problems: - because we do not handle the scope ID of a link-local address, we end up bound to an address for which publishing of that address does not allow that address to be reached (since we drop the scope) - the virtual IPs in the Docker situation are not loopback addresses, they are not link-local addresses, so we end up bound to interfaces that cause the bootstrap checks to be enforced even though the instance is only bound to local We address this by only binding to actual loopback addresses, and skip binding to any address on a loopback interface that is not a loopback address. This lets us simplify some code where in the bootstrap checks we were skipping link-local addresses, and in writing the ports file where we had to skip link-local addresses because again the formatting of them does not allow them to be connected to by another node (to be clear, they could be connected to via the scope-qualified address, but that information is not written out). Relates #28029
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refreshed the PR: #1585
Addressed comments suggesting to remove the IPAlias logic not anymore used
Signed-off-by: Flavio Crisciani [email protected]