Skip to content

Conversation

f1ko
Copy link
Member

@f1ko f1ko commented Jul 9, 2025

Currently, enabling ENI in the Helm chart by setting eni.enabled=true is not sufficient. Users are also required to manually set additional values to avoid conflicting configurations:

  • routingMode must be explicitly set to native, otherwise the chart defaults to configuring a VXLAN interface — which is mutually exclusive with ENI in Cilium.

  • ipam.mode must be explicitly set to eni, otherwise the default cluster-pool IPAM remains enabled — which is also incompatible with ENI.

These extra required settings are not obvious and can lead to misconfiguration and deployment issues.

This PR updates the chart logic to automatically handle these dependencies when the user sets eni.enabled=true.

helm: use sane defaults in combination with `eni.enabled=true`

Currently, enabling ENI in the Helm chart by setting
eni.enabled=true is not sufficient. Users are also
required to manually set additional values to avoid
conflicting configurations:

- routingMode must be explicitly set to native,
otherwise the chart defaults to configuring a VXLAN
interface — which is mutually exclusive with ENI in Cilium.

- ipam.mode must be explicitly set to eni, otherwise
the default cluster-pool IPAM remains enabled — which
is also incompatible with ENI.

These extra required settings are not obvious and
can lead to misconfiguration and deployment issues.

Signed-off-by: Filip Nikolic <oss.filipn@gmail.com>
@f1ko f1ko requested review from a team as code owners July 9, 2025 16:10
@f1ko f1ko requested review from marseel and gandro July 9, 2025 16:10
@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 Jul 9, 2025
@marseel marseel requested a review from a team July 10, 2025 12:01
Copy link
Contributor

@marseel marseel left a comment

Choose a reason for hiding this comment

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

lgtm, I requested review from aws team, but I see Sebastian covers that already 😅

Signed-off-by: Filip Nikolic <oss.filipn@gmail.com>
@f1ko f1ko requested a review from a team as a code owner July 10, 2025 15:18
@f1ko f1ko requested a review from qmonnet July 10, 2025 15:18
Copy link
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, thanks

@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. area/eni Impacts ENI based IPAM. release-note/misc This PR makes changes that have no direct user impact. labels Jul 11, 2025
@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 Jul 11, 2025
@qmonnet
Copy link
Member

qmonnet commented Jul 11, 2025

/test

@qmonnet
Copy link
Member

qmonnet commented Jul 11, 2025

Is the issue also present on supported stable versions? Please check, and add the relevant needs-backport labels accordingly.

@gandro
Copy link
Member

gandro commented Jul 14, 2025

Is the issue also present on supported stable versions? Please check, and add the relevant needs-backport labels accordingly.

I would vote for not backporting this. I think this is a UX improvement, not a bug per se, as the old required values were documented.

@gandro gandro added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Jul 14, 2025
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Seems like a solid UX improvement, in particular since we already are auto-enabling certain features based on eni.enabled.

@gandro gandro enabled auto-merge July 14, 2025 08:00
@gandro gandro added this pull request to the merge queue Jul 14, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 14, 2025
Merged via the queue into cilium:main with commit 99bdf9a Jul 14, 2025
72 checks passed
@f1ko f1ko deleted the eni-sane-defaults branch July 14, 2025 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. 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
Development

Successfully merging this pull request may close these issues.

4 participants