Skip to content

EKS: improve rules for asymmetric routing (multi-node NodePort)#13234

Merged
joestringer merged 4 commits intomasterfrom
pr/qmonnet/eks_cond_rules
Sep 24, 2020
Merged

EKS: improve rules for asymmetric routing (multi-node NodePort)#13234
joestringer merged 4 commits intomasterfrom
pr/qmonnet/eks_cond_rules

Conversation

@qmonnet
Copy link
Copy Markdown
Member

@qmonnet qmonnet commented Sep 21, 2020

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 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 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 that pod-to-b-multi-node-nodeport from connectivity tests gets ready in both cases.

Other changes are minor, please refer to individual commits for details.

Fixes: #12770
Fixes: #13143

@qmonnet qmonnet added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. area/eni Impacts ENI based IPAM. integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. needs-backport/1.7 labels Sep 21, 2020
@qmonnet qmonnet requested review from a team and joestringer September 21, 2020 13:36
@qmonnet
Copy link
Copy Markdown
Member Author

qmonnet commented Sep 21, 2020

test-me-please

Copy link
Copy Markdown
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just a minor nit below.

Comment thread pkg/datapath/loader/base.go Outdated
Copy link
Copy Markdown
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, but I think it makes sense to fix up @christarazi's feedback.

@qmonnet qmonnet force-pushed the pr/qmonnet/eks_cond_rules branch from 80b1696 to 990a31c Compare September 22, 2020 10:04
@qmonnet
Copy link
Copy Markdown
Member Author

qmonnet commented Sep 22, 2020

Looks good overall, but I think it makes sense to fix up @christarazi's feedback.

Of course it does :) Done now.

Incremental diff
diff --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)
 		}
 	}

@qmonnet
Copy link
Copy Markdown
Member Author

qmonnet commented Sep 22, 2020

test-me-please

Comment thread pkg/datapath/loader/base.go Outdated
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>
@qmonnet qmonnet force-pushed the pr/qmonnet/eks_cond_rules branch from 990a31c to bce6fa8 Compare September 22, 2020 14:12
@qmonnet qmonnet requested a review from tklauser September 22, 2020 14:13
@qmonnet
Copy link
Copy Markdown
Member Author

qmonnet commented Sep 22, 2020

test-me-please

@joestringer joestringer merged commit 815be6a into master Sep 24, 2020
@joestringer joestringer deleted the pr/qmonnet/eks_cond_rules branch September 24, 2020 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/eni Impacts ENI based IPAM. integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Breaking error around "open /proc/sys/net/ipv4/conf/eth0/rp_filter: no such file or directory" in 1.8.3

6 participants