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.

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

Do not allocate IPs or prefixes to trunk ENIs; enable Custom Networking before Security Groups for Pods #2801

Merged
merged 1 commit into from Feb 26, 2024

Conversation

jdn5126
Copy link
Contributor

@jdn5126 jdn5126 commented Feb 20, 2024

What type of PR is this?
bug

Which issue does this PR fix:
#2792

What does this PR do / Why do we need it:
This PR contains two updates:

If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
N/A

Testing done on this change:
All existing unit tests and integration tests pass. I spun up a 1.28 cluster with Custom Networking and Security Groups for Pods configured, and I verified the security groups assigned to trunk ENIs, the placement of new pods, etc. From the issues described in #2792, everything seems to be resolved.

Will this PR introduce any new dependencies?:
No

Will this break upgrades or downgrades? Has updating a running cluster been tested?:
No, Yes

Does this change require updates to the CNI daemonset config files to work?:
No

Does this PR introduce any user-facing change?:
Yes

Stop IPAMD from assigning prefixes/IPs to trunk ENIs. Enable Custom Networking before Security Groups for Pods.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jchen6585
jchen6585 previously approved these changes Feb 21, 2024
Copy link
Contributor

@jchen6585 jchen6585 left a comment

Choose a reason for hiding this comment

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

LGTM

@orsenthil
Copy link
Contributor

Updating the README with this detail (keeping that comprehensive might help), and aws docs separately with this detail might help too.

orsenthil
orsenthil previously approved these changes Feb 21, 2024
Copy link
Contributor

@orsenthil orsenthil left a comment

Choose a reason for hiding this comment

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

Please see if you would like to update the README in appropriate section.

pkg/ipamd/datastore/data_store.go Outdated Show resolved Hide resolved
@jdn5126 jdn5126 dismissed stale reviews from orsenthil and jchen6585 via 03e7c21 February 22, 2024 17:35
@jdn5126 jdn5126 changed the title Do not allocate IPs or prefixes to trunk ENIs or EFA ENIs Do not allocate IPs or prefixes to trunk ENIs; enable Custom Networking before Security Groups for Pods Feb 22, 2024
Copy link
Contributor

@haouc haouc left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@jayanthvn jayanthvn left a comment

Choose a reason for hiding this comment

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

lgtm...

@jdn5126 jdn5126 merged commit 315d56a into aws:master Feb 26, 2024
6 checks passed
@jdn5126 jdn5126 deleted the trunk branch February 26, 2024 19:09
@Shadowssong
Copy link

Hey @jdn5126 , has this been officially released yet? I was trying to find it in the changelog or in the github diffs but I can't seem to find it anywhere. We ran into this when upgrading our CNI from 1.15 -> 1.16 -> 1.17. Thanks!

@jdn5126
Copy link
Contributor Author

jdn5126 commented May 15, 2024

@Shadowssong
Copy link

@jdn5126 thanks! We were on 1.17 and still experiencing the exact same issue described in the associated issue. We reverted to 1.15 and the issue resolved. I will open a support ticket to further debug this issue.

@jayanthvn
Copy link
Contributor

When you upgraded from 1.15 to 1.16, can you share which patch version of 1.15 and 1.16?

@Shadowssong
Copy link

@jayanthvn We specified v1.16.4-eksbuild.2 for 1.16 and then for 1.17 we did v1.17.1-eksbuild.1. We were on 1.17 when we observed the behavior was occurring and reverting the EKS addon back to 1.15 resolved the issue once the daemonset restarted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants