-
Notifications
You must be signed in to change notification settings - Fork 3.4k
eni: use sane defaults when enabling ENI via helm #40445
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
Conversation
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>
There was a problem hiding this 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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks
/test |
Is the issue also present on supported stable versions? Please check, and add the relevant |
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. |
There was a problem hiding this 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
.
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 tonative
, 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 defaultcluster-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
.