Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Always remove AWS-SNAT-CHAIN rules when running in ENI mode. #17845

Merged
merged 2 commits into from Nov 22, 2021
Merged

Always remove AWS-SNAT-CHAIN rules when running in ENI mode. #17845

merged 2 commits into from Nov 22, 2021

Conversation

bmcustodio
Copy link
Contributor

@bmcustodio bmcustodio commented Nov 10, 2021 •
edited

Please see each individual commit for details.

Fixes an issue which can cause traffic to be dropped when running Cilium in ENI mode due to the presence of iptables rules left over by the AWS VPC CNI plugin. Notable features that could be impacted include the egress gateway functionality.

@bmcustodio
helm: remove the masquerade check in poststart
346059d 
The current understanding is that we want to remove these rules all the
time, as they can cause issues with features such as egress gateway.

Signed-off-by: Bruno M. Custódio <brunomcustodio@gmail.com>
@bmcustodio
helm: rename 'poststart.sh' to 'poststart-eni.sh'
037e143 
To make room in the future for different post-start scripts depending on
different Helm values, and also because the current contents of the file
really only make sense if ENI mode is enabled.

Signed-off-by: Bruno M. Custódio <brunomcustodio@gmail.com>
@bmcustodio bmcustodio requested a review from a team as a code owner November 10, 2021 14:42
@bmcustodio bmcustodio requested review from a team, kaworu and nathanjsweet November 10, 2021 14:42
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 10, 2021
@bmcustodio bmcustodio changed the title Pr/bruno/always remove aws snat in eni mode Always remove AWS-SNAT-CHAIN rules when running in ENI mode. Nov 10, 2021
@bmcustodio
Copy link
Contributor Author

@brb @jibi ☝️

jibi
jibi approved these changes Nov 10, 2021
View reviewed changes
Copy link
Member

@jibi jibi left a comment

Choose a reason for hiding this comment

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

Thanks! 🚀

@gandro gandro added area/eni Impacts ENI based IPAM. area/helm Impacts helm charts and user deployment experience release-blocker/1.11 This issue will prevent the release of the next version of Cilium. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Nov 10, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 10, 2021
@pchaigno pchaigno self-requested a review November 11, 2021 09:05
pchaigno
pchaigno requested changes Nov 11, 2021
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

The code looks good to me!

Given this is fixing a bug with egress gateway on EKS, shouldn't it be release-note/bug with a release note clarifying the bug impact for users and backport to v1.10?

@pchaigno
Copy link
Member

pchaigno commented Nov 11, 2021 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-Runtime-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

RuntimeKVStoreTest KVStore tests Consul KVStore

Failure Output

FAIL: Found 2 Cilium logs matching list of errors that must be investigated:

If it is a flake, comment /mlh new-flake Cilium-PR-Runtime-net-next so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sChaosTest Connectivity demo application Endpoint can still connect while Cilium is not running

Failure Output

FAIL: Cannot ping from "testclient-pbb4j" to "10.84.2.79"

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.6 Nov 11, 2021
@bmcustodio bmcustodio added release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Nov 11, 2021
@jibi jibi requested a review from pchaigno November 17, 2021 14:38
@jibi jibi added the feature/egress-gateway Impacts the egress IP gateway feature. label Nov 17, 2021
@pchaigno pchaigno requested a review from aanm November 17, 2021 15:02
@pchaigno pchaigno removed the request for review from nathanjsweet November 17, 2021 15:02
@jibi
Copy link
Member

jibi commented Nov 22, 2021

/test-runtime

@jibi
Copy link
Member

jibi commented Nov 22, 2021

GKE failure is unrelated, marking as ready

@jibi jibi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 22, 2021
@gandro gandro merged commit 2373cbe into cilium:master Nov 22, 2021
@aanm aanm mentioned this pull request Nov 23, 2021
Merged
This was referenced Nov 23, 2021
Closed
Merged
@gandro gandro mentioned this pull request Nov 23, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.6 Nov 23, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.6 Nov 23, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.6 Nov 30, 2021
@joestringer joestringer mentioned this pull request Dec 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jibi jibi jibi approved these changes

@kaworu kaworu kaworu approved these changes

@brb brb brb approved these changes

@pchaigno pchaigno pchaigno approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
area/eni Impacts ENI based IPAM. area/helm Impacts helm charts and user deployment experience feature/egress-gateway Impacts the egress IP gateway feature. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.11 This issue will prevent the release of the next version of Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.10.6
Backport done to v1.10
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@bmcustodio @pchaigno @jibi @kaworu @brb @aanm @gandro @nathanjsweet @qmonnet