Skip to content

aws: Add ability to mark ENIs as unmanaged#19096

Merged
qmonnet merged 3 commits intocilium:masterfrom
gandro:pr/gandro/eni-exclude-tags
Mar 30, 2022
Merged

aws: Add ability to mark ENIs as unmanaged#19096
qmonnet merged 3 commits intocilium:masterfrom
gandro:pr/gandro/eni-exclude-tags

Conversation

@gandro
Copy link
Copy Markdown
Member

@gandro gandro commented Mar 9, 2022 •
edited
Loading

This PR adds the ability to specify a set of tags in the ENI spec to
exclude certain ENIs from Cilium's IP allocation. To exclude interfaces
based on tags, a user may set the exclude-interface-tags option in the
CNI config (same as other ENI IPAM parameters). If an interface matches
all tags specified in the parameter, it will be ignored by Cilium.

This new mechanism works similar to the existing
first-interface-index value, but is able to also exclude interfaces
which are added to worker nodes dynamically after Cilium is already
running.

@gandro gandro added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/eni Impacts ENI based IPAM. labels Mar 9, 2022
@gandro
Copy link
Copy Markdown
Member Author

gandro commented Mar 9, 2022

/ci-eks

@gandro gandro force-pushed the pr/gandro/eni-exclude-tags branch from 81bcae4 to c492154 Compare March 10, 2022 08:58
@gandro
Copy link
Copy Markdown
Member Author

gandro commented Mar 10, 2022

/ci-eks

@gandro gandro force-pushed the pr/gandro/eni-exclude-tags branch 2 times, most recently from e7c3b2e to b9d0149 Compare March 17, 2022 16:55
@gandro gandro marked this pull request as ready for review March 17, 2022 16:55
@gandro gandro requested review from a team March 17, 2022 16:55
@gandro gandro requested review from a team as code owners March 17, 2022 16:55
@gandro
Copy link
Copy Markdown
Member Author

gandro commented Mar 17, 2022

/ci-eks

christarazi
christarazi approved these changes Mar 17, 2022
View reviewed changes
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 thanks!

qmonnet
qmonnet approved these changes Mar 17, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@qmonnet qmonnet 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, but Travis failure might be related to the changes?

ldelossa
ldelossa approved these changes Mar 17, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

Don't have a ton of knowledge with ENI but I think I get the gist here. Changes look good 👍

sayboras
sayboras approved these changes Mar 18, 2022
View reviewed changes
@twpayne
Copy link
Copy Markdown
Contributor

twpayne commented Mar 18, 2022

Note that the Travis failure seems relevant to this PR:

----------------------------------------------------------------------
FAIL: node_manager_test.go:540: ENISuite.TestNodeManagerENIExcludeInterfaceTags
node_manager_test.go:583:
    c.Assert(testutils.WaitUntil(func() bool { return reachedAddressesNeeded(mngr, "node1", 0) }, 5*time.Second), check.IsNil)
... value *errors.errorString = &errors.errorString{s:"timeout reached while waiting for condition"} ("timeout reached while waiting for condition")

gandro added 3 commits March 22, 2022 15:21
@gandro
aws: Add ability to excude ENIs for IPAM
7882c79 
This commit adds the ability to specify a set of tags in the ENI spec to
exclude certain ENIs from Cilium's IP allocation. To exclude interfaces
based on tags, a user may set the `exclude-interface-tags` option in the
CNI config (same as other ENI IPAM parameters). If an interface matches
all tags specified in the parameter, it will be ignored by Cilium.

This new mechanism works similar to the existing
`first-interface-index` value, but is able to also exclude interfaces
which are added to worker nodes dynamically after Cilium is already
running.

Signed-off-by: Sebastian Wicki <[email protected]>
@gandro
eni: Add unit test for exclude-interface-tags
01e5d40 
This adds a unit test where a node contains an ignored ENI. We ensure
that no secondary IPs are assigned to that ENI and that new ENIs are
attached and managed instead.

Signed-off-by: Sebastian Wicki <[email protected]>
@gandro
docs: Add docs for exclude-interface-tags
428f3aa 
Signed-off-by: Sebastian Wicki <[email protected]>
@gandro gandro force-pushed the pr/gandro/eni-exclude-tags branch from b9d0149 to 428f3aa Compare March 22, 2022 14:22
@gandro
Copy link
Copy Markdown
Member Author

gandro commented Mar 22, 2022

Thanks for the reviews and the feedback. Not sure why the unit test didn't break before for me, might have been racy. Fixed it as per Tam's suggestion to add a manual Resync.

@gandro
Copy link
Copy Markdown
Member Author

gandro commented Mar 22, 2022

/test

@gandro
Copy link
Copy Markdown
Member Author

gandro commented Mar 30, 2022

Marking ready to merge. The relevant CI test suite (ci-eks) passed multiple times in a row.

@gandro gandro added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 30, 2022
@qmonnet qmonnet merged commit c1aee70 into cilium:master Mar 30, 2022
gandro added a commit to gandro/cilium that referenced this pull request Mar 30, 2022
@gandro
eni: Fix broken build due to unit test
27262ee 
PR cilium#18463 changed the signature of the CreateNetworkInterface and
NewNodeManager methods. This change was merged last week.  PR cilium#19096
added a unit test in TestNodeManagerENIExcludeInterfaceTags which calls
those methods with the old signature.

Because the latter PR was not rebased after a merge, we accidentally
merged a non-compilable unit test into master, even though CI was green
(as CI was run before the signature was changed).

Signed-off-by: Sebastian Wicki <[email protected]>
@gandro gandro mentioned this pull request Mar 30, 2022
Merged
pchaigno pushed a commit that referenced this pull request Mar 30, 2022
@gandro @pchaigno
eni: Fix broken build due to unit test
c667a3f 
PR #18463 changed the signature of the CreateNetworkInterface and
NewNodeManager methods. This change was merged last week.  PR #19096
added a unit test in TestNodeManagerENIExcludeInterfaceTags which calls
those methods with the old signature.

Because the latter PR was not rebased after a merge, we accidentally
merged a non-compilable unit test into master, even though CI was green
(as CI was run before the signature was changed).

Signed-off-by: Sebastian Wicki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christarazi christarazi christarazi approved these changes

@ldelossa ldelossa ldelossa approved these changes

@sayboras sayboras sayboras approved these changes

@qmonnet qmonnet qmonnet approved these changes

+1 more reviewer

@twpayne twpayne twpayne approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/eni Impacts ENI based IPAM. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gandro @twpayne @christarazi @ldelossa @sayboras @qmonnet