AWS EC2 Instance tag filter #19181

prune998 · 2022-03-18T01:33:17Z

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Thanks for contributing!

maintainer-s-little-helper · 2022-03-18T01:33:19Z

Commit 96133ab6cdcacf9eb4b9a91e00240502b91cb5a7 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

christarazi

Why is the subnet tag filter not enough to do this job? AFAIU, interfaces are attached to subnets, so filtering out certain subnets allows you to filter out interfaces as well.

prune998 · 2022-03-18T20:43:06Z

Why is the subnet tag filter not enough to do this job? AFAIU, interfaces are attached to subnets, so filtering out certain subnets allows you to filter out interfaces as well.

The theory is that you want Cilium to sync AWS resources that are used by the cluster.
While filtering by subnets may help in some situations, it can also be problematic:

you may have multiple clusters using the same subnet
your ec2 instances may not have any interface in the filtered subnet before Cilium kicks in (which is in fact the issue described in Cilium 1.11.0 does not start on EKS with pod subnet-filter #18239)

The real good way to filter resources is by targeting things that are in play in the cluster: instances / nodegroups.
The actual subnet filtering is actually a dirty hack, badly named and documented, racing and confusing with the CNI filter.

Ideally I would PR to remove the subnet filter, but that would be a breaking change. This PR is adding an instance filter that should be able to replace the subnet filter in most if not all of the possible scenarios.

I'm also wondering about the other part of the Resync function, as we're maintaining all the VPCs, subnets and securityGroups of the whole account... while there's almost no chances that they will be used/at play in one specific cluster.
While some people have one VPC, few subnets and one cluster per AWS account, some have TONS of stuff in a single account that may have an impact on Cilium-Operator performance.

Personally, in my DEV AWS account, I have currently 8 clusters * 5 nodes * 3 private subnets for ec2 instance IPs * 3 pod subnets for pod IPs... For this setup to work, I have to either don't use a filter, or filter on both instances and pods subnets -> each Cilium-Operator will maintain all the ENIs that existe in the account, instead of the ones from the cluster it's running in.

Note that same issue seems to exist with Alibaba, Azure and all.

I'm opened to any suggestion here. And if you think the subnet-filter hack is enough (once well documented) then I'll close this PR :)

maintainer-s-little-helper · 2022-03-25T14:18:55Z

Commit ef1fe5b831034982f3f0c6989ff0376041c8a3f2 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

prune998 · 2022-03-25T14:32:40Z

latest commit msg with some infos:

This PR is adding the basic code to add a new command line option of the Cilium-Operator to filter AWS EC2 instances (and available ENI/subnets) based on EC2 instance tags

Technically, if an instance tag filter is provided, instead of searching ALL the ENIs that exist in the AWS account, we:

search all instances matching the tags
grab all the ENIS of the selected instances

We have to search twice, once to filter instances and ENIS (giving us ec2_types.InstanceNetworkInterface) and sadly, another one to grab the ec2_types.NetworkInterface that is needed down the line.

This could also be refactored... We could filter both on instance tags and subnets in the same function (even if it seems useless to do so).

prune998 · 2022-03-25T14:45:58Z

Usage:
your EKS cluster where you're deploying Cilium is made on EC2 Instances. Those Instances have some tags set at creation time (for unmanaged instances, set some tags if needed !)
ex:

When starting Cilium, you can use the new --instance-tags-filter option to filter the instances by tags:

--instance-tags-filter=eks:cluster-name=prune-feature-cluster-us-east-1

gandro

Overall this looks very solid to me, thanks a lot for taking care of this! 🚀

I have a few minor nits that need to be addressed, and an overall question about what the interaction with this and the old subnet tag filter should be.

operator/option/config.go

pkg/aws/ec2/ec2.go

prune998 · 2022-04-05T00:01:25Z

/test

gandro · 2022-04-05T08:56:21Z

/test

[ upstream commit 212d6e7 ] The `eni.subnetTagsFilter` option is notoriously hard to use correctly. If it is used with tags that don't match the subnet of the pre-attached ENI, Cilium agent will never become ready (cilium#18239). This PR removes it from the ENI documentation (which most users will use as a reference configuration) such that no one enables this option without being aware of its requirements. This PR also adds additional context the Helm value. We might deprecate and remove the option in the future as well (cilium#19181). Signed-off-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: Paul Chaignon <paul@cilium.io>

prune998 · 2022-04-05T11:49:06Z

I just corrected to docs wordlist, but i'm clueless about the other tests failing... how to get this PR merged ?

gandro · 2022-04-05T12:10:49Z

I just corrected to docs wordlist, but i'm clueless about the other tests failing... how to get this PR merged ?

Thanks! So to get the PR merged, we want to make sure CI runs completely and doesn't uncover any issues. Our CI tends to be unreliable at times, so not every failure is necessarily related to the PR. I'll trigger a CI run (the /test is only available to Cilium core committers) and once it finishes, we can triage any failing tests.

As a side note: Please avoid pushing to the branch before we have had a change to look at any failing tests. Re-pushing invalidates the CI results.

gandro · 2022-04-05T12:10:57Z

/test

Job 'Cilium-PR-K8s-GKE' hit: #19014 (97.44% similarity)

next-next hit #19264

[ upstream commit 212d6e7 ] The `eni.subnetTagsFilter` option is notoriously hard to use correctly. If it is used with tags that don't match the subnet of the pre-attached ENI, Cilium agent will never become ready (#18239). This PR removes it from the ENI documentation (which most users will use as a reference configuration) such that no one enables this option without being aware of its requirements. This PR also adds additional context the Helm value. We might deprecate and remove the option in the future as well (#19181). Signed-off-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: Paul Chaignon <paul@cilium.io>

prune998 · 2022-04-07T23:56:51Z

@gandro any news ?

This PR is adding a new command line option `instance-tags-filter` to the Cilium-Operator to filter maintained AWS EC2 instances (and available ENIs) based on EC2 instance tags Technically, if an instance tag filter is provided, instead of searching ALL the ENIs that exist in the AWS account, we: - skip the `instance-subnet-filter` - search all instances matching the tags - grab all the ENIS of the selected instances The code will search for matching instances and will grab all instance ENIS (giving us `ec2_types.InstanceNetworkInterface`) and will then search ENIs to get `ec2_types.NetworkInterface` that is needed down the line. Signed-off-by: Sebastien Prune THOMAS <prune@lecentre.net>

sayboras · 2022-04-08T01:11:18Z

/test

sayboras · 2022-04-08T01:12:18Z

I have re-triggered the tests, normally it will take 2-3 hours for full CI. Will check back once it's completed.

pchaigno · 2022-04-08T09:07:02Z

GKE failed with known flake #17307 but shouldn't be affected by these changes anyway. Other tests are passing and reviews are in. Marking ready to merge.

nbusseneau · 2022-04-08T12:16:51Z

@prune998 Thanks for the contribution!

… used PR cilium#19181 introduced option `instance-tags-filter` for filtering instances that need to be kept in sync. The implementation uses AWS EC2 API DescribeInstances. Without the permission, the operator would fail with message: level=warning msg="Unable to synchronize EC2 interface list" error="operation error EC2: DescribeInstances, https response error StatusCode: 403, RequestID: <snap>, api error UnauthorizedOperation: You are not authorized to perform this operation." subsys=eni This patches documents the necessary permission the operator needs to be granted when using option `instance-tags-filter`. Signed-off-by: Haitao Li <lihaitao@gmail.com>

… used PR #19181 introduced option `instance-tags-filter` for filtering instances that need to be kept in sync. The implementation uses AWS EC2 API DescribeInstances. Without the permission, the operator would fail with message: level=warning msg="Unable to synchronize EC2 interface list" error="operation error EC2: DescribeInstances, https response error StatusCode: 403, RequestID: <snap>, api error UnauthorizedOperation: You are not authorized to perform this operation." subsys=eni This patches documents the necessary permission the operator needs to be granted when using option `instance-tags-filter`. Signed-off-by: Haitao Li <lihaitao@gmail.com>

… used PR cilium#19181 introduced option `instance-tags-filter` for filtering instances that need to be kept in sync. The implementation uses AWS EC2 API DescribeInstances. Without the permission, the operator would fail with message: level=warning msg="Unable to synchronize EC2 interface list" error="operation error EC2: DescribeInstances, https response error StatusCode: 403, RequestID: <snap>, api error UnauthorizedOperation: You are not authorized to perform this operation." subsys=eni This patches documents the necessary permission the operator needs to be granted when using option `instance-tags-filter`. Signed-off-by: Haitao Li <lihaitao@gmail.com>

… used [ upstream commit c7ebfee ] PR cilium#19181 introduced option `instance-tags-filter` for filtering instances that need to be kept in sync. The implementation uses AWS EC2 API DescribeInstances. Without the permission, the operator would fail with message: level=warning msg="Unable to synchronize EC2 interface list" error="operation error EC2: DescribeInstances, https response error StatusCode: 403, RequestID: <snap>, api error UnauthorizedOperation: You are not authorized to perform this operation." subsys=eni This patches documents the necessary permission the operator needs to be granted when using option `instance-tags-filter`. Signed-off-by: Haitao Li <lihaitao@gmail.com> Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>

… used [ upstream commit c7ebfee ] PR #19181 introduced option `instance-tags-filter` for filtering instances that need to be kept in sync. The implementation uses AWS EC2 API DescribeInstances. Without the permission, the operator would fail with message: level=warning msg="Unable to synchronize EC2 interface list" error="operation error EC2: DescribeInstances, https response error StatusCode: 403, RequestID: <snap>, api error UnauthorizedOperation: You are not authorized to perform this operation." subsys=eni This patches documents the necessary permission the operator needs to be granted when using option `instance-tags-filter`. Signed-off-by: Haitao Li <lihaitao@gmail.com> Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>

… used [ upstream commit c7ebfee ] PR cilium#19181 introduced option `instance-tags-filter` for filtering instances that need to be kept in sync. The implementation uses AWS EC2 API DescribeInstances. Without the permission, the operator would fail with message: level=warning msg="Unable to synchronize EC2 interface list" error="operation error EC2: DescribeInstances, https response error StatusCode: 403, RequestID: <snap>, api error UnauthorizedOperation: You are not authorized to perform this operation." subsys=eni This patches documents the necessary permission the operator needs to be granted when using option `instance-tags-filter`. Signed-off-by: Haitao Li <lihaitao@gmail.com> Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com> Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

prune998 requested a review from a team March 18, 2022 01:33

prune998 requested review from a team as code owners March 18, 2022 01:33

prune998 requested review from twpayne and christarazi March 18, 2022 01:33

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 18, 2022

prune998 mentioned this pull request Mar 18, 2022

Cilium 1.11.0 does not start on EKS with pod subnet-filter #18239

Closed

2 tasks

prune998 force-pushed the prune/tagFilter branch from 96133ab to 6ca153c Compare March 18, 2022 01:40

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 18, 2022

prune998 force-pushed the prune/tagFilter branch from 6ca153c to abdbefd Compare March 18, 2022 01:40

christarazi requested changes Mar 18, 2022

View reviewed changes

twpayne added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Mar 22, 2022

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 22, 2022

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 25, 2022

prune998 changed the title ~~WIP: AWS EC2 Instance tag filter~~ AWS EC2 Instance tag filter Mar 25, 2022

prune998 force-pushed the prune/tagFilter branch from ef1fe5b to d3e901a Compare March 25, 2022 14:31

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 25, 2022

gandro requested changes Mar 28, 2022

View reviewed changes

operator/option/config.go Outdated Show resolved Hide resolved

pkg/aws/ec2/ec2.go Show resolved Hide resolved

pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved

pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved

pkg/aws/ec2/ec2.go Show resolved Hide resolved

gandro added the area/eni Impacts ENI based IPAM. label Mar 28, 2022

prune998 force-pushed the prune/tagFilter branch from d3e901a to aec5404 Compare March 28, 2022 19:17

christarazi requested changes Mar 28, 2022

View reviewed changes

pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved

pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved

pkg/aws/ec2/ec2.go Outdated Show resolved Hide resolved

pkg/aws/ec2/ec2.go Show resolved Hide resolved

prune998 requested a review from a team March 29, 2022 12:12

prune998 requested review from a team as code owners March 29, 2022 12:12

kaworu approved these changes Apr 5, 2022

View reviewed changes

prune998 force-pushed the prune/tagFilter branch from cfe11d4 to 51d6d57 Compare April 5, 2022 11:47

maintainer-s-little-helper bot mentioned this pull request Apr 5, 2022

CI: K8sDatapathConfig Etcd Check connectivity #19014

Closed

prune998 force-pushed the prune/tagFilter branch from 51d6d57 to 495be70 Compare April 8, 2022 00:01

pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 8, 2022

nbusseneau merged commit 36ea7d2 into cilium:master Apr 8, 2022

lht mentioned this pull request Jul 28, 2022

docs: Add required ec2:DescribeInstances when instance-tags-filter is used #20703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS EC2 Instance tag filter #19181

AWS EC2 Instance tag filter #19181

prune998 commented Mar 18, 2022 •

edited by gandro

maintainer-s-little-helper bot commented Mar 18, 2022

christarazi left a comment

prune998 commented Mar 18, 2022

maintainer-s-little-helper bot commented Mar 25, 2022

prune998 commented Mar 25, 2022

prune998 commented Mar 25, 2022

gandro left a comment

prune998 commented Apr 5, 2022

gandro commented Apr 5, 2022

prune998 commented Apr 5, 2022

gandro commented Apr 5, 2022 •

edited

gandro commented Apr 5, 2022 •

edited by christarazi

prune998 commented Apr 7, 2022

sayboras commented Apr 8, 2022

sayboras commented Apr 8, 2022

pchaigno commented Apr 8, 2022

nbusseneau commented Apr 8, 2022

AWS EC2 Instance tag filter #19181

AWS EC2 Instance tag filter #19181

Conversation

prune998 commented Mar 18, 2022 • edited by gandro

maintainer-s-little-helper bot commented Mar 18, 2022

christarazi left a comment

Choose a reason for hiding this comment

prune998 commented Mar 18, 2022

maintainer-s-little-helper bot commented Mar 25, 2022

prune998 commented Mar 25, 2022

prune998 commented Mar 25, 2022

gandro left a comment

Choose a reason for hiding this comment

prune998 commented Apr 5, 2022

gandro commented Apr 5, 2022

prune998 commented Apr 5, 2022

gandro commented Apr 5, 2022 • edited

gandro commented Apr 5, 2022 • edited by christarazi

prune998 commented Apr 7, 2022

sayboras commented Apr 8, 2022

sayboras commented Apr 8, 2022

pchaigno commented Apr 8, 2022

nbusseneau commented Apr 8, 2022

prune998 commented Mar 18, 2022 •

edited by gandro

gandro commented Apr 5, 2022 •

edited

gandro commented Apr 5, 2022 •

edited by christarazi