EKS: improve rules for asymmetric routing (multi-node NodePort)#13234
Merged
joestringer merged 4 commits intomasterfrom Sep 24, 2020
Merged
EKS: improve rules for asymmetric routing (multi-node NodePort)#13234joestringer merged 4 commits intomasterfrom
joestringer merged 4 commits intomasterfrom
Conversation
Member
Author
|
test-me-please |
christarazi
reviewed
Sep 21, 2020
Member
christarazi
left a comment
There was a problem hiding this comment.
LGTM! Just a minor nit below.
joestringer
requested changes
Sep 21, 2020
Member
joestringer
left a comment
There was a problem hiding this comment.
Looks good overall, but I think it makes sense to fix up @christarazi's feedback.
80b1696 to
990a31c
Compare
Member
Author
Of course it does :) Done now. Incremental diffdiff --git a/pkg/datapath/loader/base.go b/pkg/datapath/loader/base.go
index 0703bcc6e34f..042d013176a4 100644
--- a/pkg/datapath/loader/base.go
+++ b/pkg/datapath/loader/base.go
@@ -116,7 +116,7 @@ type setting struct {
ignoreErr bool
}
-func addENIRules(sysSettings *[]setting) error {
+func addENIRules(sysSettings []setting) ([]setting, error) {
// AWS ENI mode requires symmetric routing, see
// iptables.addCiliumENIRules().
// The default AWS daemonset installs the following rules that are used
@@ -135,15 +135,15 @@ func addENIRules(sysSettings *[]setting) error {
//
// We want to reproduce equivalent rules to ensure correct routing.
if !option.Config.EnableIPv4 {
- return nil
+ return nil, nil
}
iface, err := route.NodeDeviceWithDefaultRoute(option.Config.EnableIPv4, option.Config.EnableIPv6)
if err != nil {
- return fmt.Errorf("failed to find interface with default route: %w", err)
+ return nil, fmt.Errorf("failed to find interface with default route: %w", err)
}
- *sysSettings = append(*sysSettings, setting{
+ retSettings := append(sysSettings, setting{
fmt.Sprintf("net.ipv4.conf.%s.rp_filter", iface.Attrs().Name),
"2",
false,
@@ -154,10 +154,10 @@ func addENIRules(sysSettings *[]setting) error {
Mask: linux_defaults.MaskMultinodeNodeport,
Table: route.MainTable,
}); err != nil {
- return fmt.Errorf("unable to install ip rule for ENI multi-node NodePort: %w", err)
+ return nil, fmt.Errorf("unable to install ip rule for ENI multi-node NodePort: %w", err)
}
- return nil
+ return retSettings, nil
}
// Reinitialize (re-)configures the base datapath configuration including global
@@ -344,7 +344,8 @@ func (l *Loader) Reinitialize(ctx context.Context, o datapath.BaseProgramOwner,
}).Info("Setting up BPF datapath")
if option.Config.IPAM == ipamOption.IPAMENI {
- if err := addENIRules(&sysSettings); err != nil {
+ var err error
+ if sysSettings, err = addENIRules(sysSettings); err != nil {
return fmt.Errorf("unable to install ip rule for ENI multi-node NodePort: %w", err)
}
} |
Member
Author
|
test-me-please |
tklauser
requested changes
Sep 22, 2020
EKS needs some specific rules for NodePort traffic (see PR #12770, or comments in the code, for details). The addition of part of these rules were added to the body of the Reinitialize() function in the loader. To make them easier to maintain or extend, let's move them to a dedicated function called by Reinitialize(). No functional change. Signed-off-by: Quentin Monnet <quentin@isovalent.com>
EKS needs some specific rules for asymmetric routing with multi-node NodePort traffic. These rules are implemented only for IPv4, so we can avoid installing them when IPv4 is disabled. This is what this commit does. Note that this check is, in fact, not necessary at the moment, because as the config package says: "IPv6 cannot be enabled in ENI IPAM mode". So we always run with IPv4. But let's have it for good measure, to avoid issues if IPv6 support comes in the future. For the same reason, we also do not have to implement equivalent rules for IPv6 at the moment. Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Multi-node NodePort traffic on EKS needs specific rules regarding asymmetric routing. These rules were implemented for the eth0 interface (namely), because this is what EKS uses. With the default Amazon Linux 2 distribution. But EKS can also run with Ubuntu for example, and the name of the interface is not the same in that case. Instead of "eth0", use the interface with the dafault route. This is a quick fix, and longer term we want to add the rules to all relevant interfaces, as discussed in #12770. Fixes: #12770 Fixes: #13143 Signed-off-by: Quentin Monnet <quentin@isovalent.com>
EKS requires some specific rules for asymmetric routing with multi-node NodePort traffic. These rules relies on the xt_connmark kernel module, which is usually loaded by iptables when necessary. The rules are installed when the selected IPAM is ENI, meaning they are installed on AWS (but not only EKS). The xt_connmark module should be loaded in a similar way, unless loading modules after boot has been disabled, in which case the setup fails and the agent crashes. Add a comment to at least help debug the issue. Longer term, we may want to add more explicit hints to the logs if too many users hit the issue, but that would require parsing iptables' output for the specific error, so let's see how it goes with a simple comment in the code for now. Signed-off-by: Quentin Monnet <quentin@isovalent.com>
990a31c to
bce6fa8
Compare
Member
Author
|
test-me-please |
tklauser
approved these changes
Sep 22, 2020
vadorovsky
approved these changes
Sep 23, 2020
christarazi
approved these changes
Sep 23, 2020
This was referenced Sep 30, 2020
This file contains hidden or 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
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.
Multi-node NodePort traffic on EKS needs specific rules regarding asymmetric routing. This PR improves the way those rules are implemented.
The most important change is related to the interface for which the rules are used. So far we have been creating them for the
eth0interface (namely), because this is what EKS uses... With the default Amazon Linux 2 distribution. But EKS can also run with Ubuntu for example, and the name of the interface is not the same in that case. Instead ofeth0, use the interface with the default route. I checked that this selects the same interfaces as the default AWS deamonset for the two AMIs Amazon Linux 2 (eth0) and Ubuntu 18.04 (ens5), and thatpod-to-b-multi-node-nodeportfrom connectivity tests gets ready in both cases.Other changes are minor, please refer to individual commits for details.
Fixes: #12770
Fixes: #13143