Cilium eni #8316

olemarkus · 2020-01-11T20:03:06Z

With this feature enabled, Cilium will create and allocate ENIs for pods similar to LyftVPC and AmazonVPC.

Marked this as work in progress since there are a couple of other Cilium PRs that should go in first.

k8s-ci-robot · 2020-01-11T20:03:15Z

Hi @olemarkus. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hakman · 2020-01-12T03:00:06Z

/ok-to-test

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12.yaml.template

johngmyers · 2020-01-12T03:06:56Z

Need to make crds

johngmyers

The duplicate masquerade keys in the configmap need to be addressed.

I'm still not comfortable with always requiring the operator to run on the master. This doesn't exist in the upstream charts at all. As I see it, the only reason to do this is to give the IAM roles needed for ENI mode. Perhaps bring this up at the Kops Office Hours meeting?

johngmyers · 2020-02-08T21:31:06Z

nodeup/pkg/model/context.go

-	}
-
-	return false
+	return (c.Cluster.Spec.Networking.CNI != nil && c.Cluster.Spec.Networking.CNI.UsesSecondaryIP) || c.Cluster.Spec.Networking.AmazonVPC != nil || c.Cluster.Spec.Networking.LyftVPC != nil || (c.Cluster.Spec.Networking.Cilium != nil && c.Cluster.Spec.Networking.Cilium.Ipam == "eni")


Perhaps define a constant for this?

agreed, I think another constant along side this one would be good.

johngmyers · 2020-02-08T23:14:19Z

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12.yaml.template

@@ -127,6 +127,7 @@ data:
  enable-endpoint-routes: "true"
  auto-create-cilium-node-resource: "true"
  blacklist-conflicting-routes: "false"
+  masquerade: "false"


Doesn't this conflict with the masquerade on line 120?

This seems a reasonable thing to do, but I don't see it in the upstream charts.

It isn't directly in the charts, but it is in the documentation: https://docs.cilium.io/en/v1.6/gettingstarted/aws-eni/#prepare-deploy-cilium

I will change this to forcing the config on line 120 instead.

olemarkus · 2020-02-09T11:40:21Z

On Cilium operator running on master nodes: there are two alternatives
a) the privileged IAM permissions is given to all nodes (and thus all pods by default)
b) cilium operator runs on master and only master nodes have privileged IAM permissions

Cilium operator is something I would call a system component as without it running, normal pods won't spawn.

The question then becomes what kind of security concerns do we have that outweights pods having the escalated privileges on normal nodes.

olemarkus · 2020-02-09T12:20:42Z

What I can do is only force the operator on masters if ipam is set to ENI or gate it with a dedicated config option

johngmyers · 2020-02-11T04:33:41Z

@k8s-ci-robot you should label this needs-rebase.

johngmyers · 2020-02-12T05:17:18Z

pkg/apis/kops/validation/legacy.go

+		ciliumSpec := c.Spec.Networking.Cilium
+		if ciliumSpec.Ipam == "eni" {
+			if c.Spec.CloudProvider != "aws" {
+				allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Cilium").Child("Ipam"), "eni", "Cilum ENI IPAM is supported only in AWS"))


Use field.Forbidden()

johngmyers · 2020-02-12T05:17:53Z

pkg/apis/kops/validation/legacy.go

+				allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Cilium").Child("Ipam"), "eni", "Cilum ENI IPAM is supported only in AWS"))
+			}
+			if !ciliumSpec.DisableMasquerade {
+				allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Cilium").Child("DisableMasquerade"), "false", "Masquerade must be disabled when ENI IPAM is used"))


Also field.Forbidden()

Is masquerading forbidden with ENI? the Note block at https://docs.cilium.io/en/v1.6/gettingstarted/aws-eni/#prepare-deploy-cilium seems to suggest either way is possible.

If I masq, cluster networking doesn't work. I am guessing Cilium is masking behind the wrong interface or something. I couldn't quite figure out why. It may be that one have to explicitly set which interface to mask behind. But then I am unsure if we can really know that across all distros.

Masqing only makes sense if the SG on the ENI cilium creates have egress rules. But there isn't really any benefit of doing that since because of the masq, traffic through the ENI reaches internet anyway.

So I think the easiest is to force masqing off for now and if anyone has any good reason for having masq on, we loosen that restriction.

In ENI mode, there will be multiple possible egress paths for traffic leaving a node. Based on the destination, the right egress interface will be chosen. Masquerading is typically only ever needed for traffic heading out to the interface. This is typically routed via the default route pointing out eth0 although there can be node configurations where this is different.

For this reason, the Cilium ENI guide sets the egressMasqueradingInterfaces helm option to limit masquerading to traffic leaving on eth0:

--set global.egressMasqueradeInterfaces=eth0 \

Masquerading is only required if no NAT/internet gateway is set up in AWS which will perform the masquerading.

johngmyers · 2020-02-12T05:22:21Z

pkg/apis/kops/validation/legacy.go

+		}
+	}
+
+	if c.Spec.Networking != nil && c.Spec.Networking.Cilium != nil && c.Spec.Networking.Cilium.Ipam == "eni" && c.Spec.CloudProvider != "aws" {


Appears to be a duplicate of the check/error at line 608.

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12.yaml.template

olemarkus · 2020-02-12T10:09:19Z

I ran into an issue where etcd-manager is listening on one IP, but both primary IPs are added to the etcd-manager hosts file:

xxx.141   etcd-events-b.internal.xxx
xxx.229   etcd-events-b.internal.xxx
xxx.24    etcd-events-c.internal.xxx
xxx.157   etcd-events-c.internal.xxx
xxx.236    etcd-events-a.internal.xxx
xxx.181    etcd-events-a.internal.xxx

This will randomly break the etcd-cluster. Need to look into how to change this behaviour.
I would think this also breaks the other ENI CNIs
/hold

rifelpet · 2020-02-12T13:56:26Z

If it helps your troubleshooting, I just confirmed that amazonvpc CNI does not have that problem with kopeio/etcd-manager:3.0.20190930

# Begin host entries managed by etcd-manager[etcd] - do not edit
xxx.255	etcd-a.internal.xxx
xxx.142	etcd-b.internal.xxx
xxx.110	etcd-c.internal.xxx
# End host entries managed by etcd-manager[etcd]

# Begin host entries managed by etcd-manager[etcd-events] - do not edit
xxx.255	etcd-events-a.internal.xxx
xxx.142	etcd-events-b.internal.xxx
xxx.110	etcd-events-c.internal.xxx
# End host entries managed by etcd-manager[etcd-events]

olemarkus · 2020-02-12T14:11:42Z

Note that it took quite a while for this to happen. But thanks, that helps.

Would be useful with some clues on how etcd manager gets the info it uses for hosts. That has been quite hard to track

rifelpet · 2020-02-12T15:03:09Z

I believe this is where etcd-manager writes the map of hosts to /etc/hosts and I believe it comes from the member list within the etcd cluster itself. Can you use etcdctl to see the list of members?

olemarkus · 2020-02-12T18:23:40Z

From what I can tell, it looks like the issue only happens when using coreos. When using the default kops AMIs, I cannot reproduce this behaviour

/hold cancel

rifelpet · 2020-02-13T17:18:18Z

pkg/apis/kops/validation/legacy.go

+	if c.Spec.Networking != nil && c.Spec.Networking.Cilium != nil {
+		ciliumSpec := c.Spec.Networking.Cilium
+		if ciliumSpec.Ipam == "eni" {
+			if c.Spec.CloudProvider != "aws" {


nit: could use kops.CloudProviderAWS here.

rifelpet · 2020-02-15T14:32:45Z

pkg/model/iam/iam_builder.go

@@ -861,6 +865,34 @@ func addLyftVPCPermissions(p *Policy, resource stringorslice.StringOrSlice, lega
 	)
 }

+func addCiliumEniPermissions(p *Policy, resource stringorslice.StringOrSlice, legacyIAM bool, clusterName string) {


this clusterName parameter is unused and can probably be removed.

pkg/apis/kops/validation/legacy.go

* Force cilium-operator run on master nodes * Add option for setting cilium ipam mode * If cilium ipam mode is eni, add additional permissions to master nodes * Allow NonMasqueradeCIDR overlap with NetworkCIDR when Cilium ENI is enabled

rifelpet

Looks great, thanks for sticking with it!
/lgtm
/approve

k8s-ci-robot · 2020-02-16T20:13:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus, rifelpet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rifelpet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 11, 2020

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 11, 2020

k8s-ci-robot requested review from joshbranham and robinpercy January 11, 2020 20:03

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 12, 2020

johngmyers reviewed Jan 12, 2020

View reviewed changes

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12.yaml.template Outdated Show resolved Hide resolved

olemarkus changed the title ~~WIP: Cilium eni~~ Cilium eni Jan 27, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 27, 2020

olemarkus force-pushed the cilium-eni branch from 8df2bf5 to 756cc32 Compare February 8, 2020 13:35

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 8, 2020

johngmyers requested changes Feb 8, 2020

View reviewed changes

olemarkus force-pushed the cilium-eni branch from e52ebc2 to 618cfcd Compare February 9, 2020 15:05

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2020

olemarkus force-pushed the cilium-eni branch from 618cfcd to 4cfa762 Compare February 11, 2020 12:01

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2020

johngmyers requested changes Feb 12, 2020

View reviewed changes

olemarkus force-pushed the cilium-eni branch from 4cfa762 to 871c04b Compare February 12, 2020 10:04

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2020

olemarkus mentioned this pull request Feb 13, 2020

Cilium nodeport #8220

Merged

rifelpet reviewed Feb 13, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2020

olemarkus force-pushed the cilium-eni branch from e7f4886 to e84a4f2 Compare February 13, 2020 19:32

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2020

rifelpet reviewed Feb 15, 2020

View reviewed changes

johngmyers requested changes Feb 15, 2020

View reviewed changes

pkg/apis/kops/validation/legacy.go Outdated Show resolved Hide resolved

pkg/apis/kops/validation/legacy.go Outdated Show resolved Hide resolved

pkg/apis/kops/validation/legacy.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2020

Add option to use ENI as IPAM mode for Cilium

ced8f00

* Force cilium-operator run on master nodes * Add option for setting cilium ipam mode * If cilium ipam mode is eni, add additional permissions to master nodes * Allow NonMasqueradeCIDR overlap with NetworkCIDR when Cilium ENI is enabled

olemarkus force-pushed the cilium-eni branch from bf35c38 to ced8f00 Compare February 16, 2020 18:12

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2020

rifelpet reviewed Feb 16, 2020

View reviewed changes

k8s-ci-robot assigned rifelpet Feb 16, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 16, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 16, 2020

k8s-ci-robot merged commit 5a5eb67 into kubernetes:master Feb 16, 2020

k8s-ci-robot added this to the v1.18 milestone Feb 16, 2020

olemarkus deleted the cilium-eni branch March 25, 2020 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cilium eni #8316

Cilium eni #8316

olemarkus commented Jan 11, 2020

k8s-ci-robot commented Jan 11, 2020

hakman commented Jan 12, 2020

johngmyers commented Jan 12, 2020

johngmyers left a comment

johngmyers Feb 8, 2020

rifelpet Feb 13, 2020

johngmyers Feb 8, 2020

olemarkus Feb 9, 2020

olemarkus commented Feb 9, 2020

olemarkus commented Feb 9, 2020

johngmyers commented Feb 11, 2020

johngmyers Feb 12, 2020

johngmyers Feb 12, 2020

olemarkus Feb 12, 2020

tgraf Feb 14, 2020

johngmyers Feb 12, 2020

olemarkus commented Feb 12, 2020

rifelpet commented Feb 12, 2020 •

edited

Loading

olemarkus commented Feb 12, 2020

rifelpet commented Feb 12, 2020

olemarkus commented Feb 12, 2020

rifelpet Feb 13, 2020

rifelpet Feb 15, 2020

rifelpet left a comment

k8s-ci-robot commented Feb 16, 2020

Cilium eni #8316

Cilium eni #8316

Conversation

olemarkus commented Jan 11, 2020

k8s-ci-robot commented Jan 11, 2020

hakman commented Jan 12, 2020

johngmyers commented Jan 12, 2020

johngmyers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olemarkus commented Feb 9, 2020

olemarkus commented Feb 9, 2020

johngmyers commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olemarkus commented Feb 12, 2020

rifelpet commented Feb 12, 2020 • edited Loading

olemarkus commented Feb 12, 2020

rifelpet commented Feb 12, 2020

olemarkus commented Feb 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rifelpet left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 16, 2020

rifelpet commented Feb 12, 2020 •

edited

Loading