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

Helm: enforce routing mode when either gke.enabled or aksbyocni.enabled are set #29674

Merged

Conversation

giorio94
Copy link
Member

@giorio94 giorio94 commented Dec 6, 2023

Historically, the Cilium helm chart allowed to override the routing mode leveraged in combination with {gke,aksbyocni}.enabled. This is no longer possible since aff16b2 ("Change routing-mode and tunnel interaction.").

According to the Cilium documentation [1,2], this appears to be the correct behavior, as the routing mode must be respectively set to native and tunnel in these cases. Hence, let's validate that users didn't configure a different routing mode, to avoid falling back silently, which may be confusing.

aff16b2 could be considered a breaking change. However, it got backported to v1.14, and we haven't sees bug reports (at least AFAIK) about the behavioral difference. Which means that users are not overriding the routing mode in these cases, as documented. Still, better being pedantic and prevent possible issues (our CI was affected, as apparently we were overriding the routing mode in the external workloads workflow, and it took a while to discover the underlying cause).

I'm marking this PR for backport to v1.14 for consistency with the above commit which introduced the behavioral change.

Related: #27841 (comment)

[1]: https://docs.cilium.io/en/stable/network/concepts/routing/#id6
[2]: https://docs.cilium.io/en/stable/gettingstarted/k8s-install-default/#install-cilium (AKS tab)

Historically, the Cilium helm chart allowed to override the routing mode
leveraged in combination with {gke,aksbyocni}.enabled. This is no longer
possible since aff16b2 ("Change routing-mode and tunnel interaction.").

According to the Cilium documentation [1,2], this appears to be the
correct behavior, as the routing mode must be respectively set to native
and tunnel in these cases. Hence, let's validate that users didn't
configure a different routing mode, to avoid falling back silently,
which may be confusing.

[1]: https://docs.cilium.io/en/stable/network/concepts/routing/#id6
[2]: https://docs.cilium.io/en/stable/gettingstarted/k8s-install-default/#install-cilium (AKS tab)

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94 giorio94 added release-note/misc This PR makes changes that have no direct user impact. integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. area/helm Impacts helm charts and user deployment experience needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Dec 6, 2023
@giorio94 giorio94 requested review from a team as code owners December 6, 2023 15:38
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.5 Dec 6, 2023
@giorio94
Copy link
Member Author

giorio94 commented Dec 6, 2023

/test

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.

Thanks!

@nebril nebril added this to Needs backport from main in 1.14.6 Dec 11, 2023
@nebril nebril removed this from Needs backport from main in 1.14.5 Dec 11, 2023
@giorio94
Copy link
Member Author

@youngnick Friendly ping 🙏

@giorio94 giorio94 added the needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch label Dec 13, 2023
@joestringer joestringer added this to Needs backport from main in v1.15.0-rc.1 Dec 14, 2023
@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 Dec 20, 2023
@julianwiedmann julianwiedmann added this pull request to the merge queue Dec 20, 2023
Merged via the queue into cilium:main with commit babba79 Dec 20, 2023
62 checks passed
@pippolo84 pippolo84 mentioned this pull request Jan 2, 2024
17 tasks
@pippolo84 pippolo84 added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Jan 2, 2024
@pippolo84 pippolo84 mentioned this pull request Jan 2, 2024
8 tasks
@pippolo84 pippolo84 added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Jan 2, 2024
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Jan 9, 2024
@aanm aanm added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Jan 16, 2024
@gentoo-root gentoo-root moved this from Needs backport from main to Backport done to v1.14 in 1.14.6 Jan 17, 2024
@aanm aanm moved this from Needs backport from main to Backport done to v1.15 in v1.15.0-rc.1 Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Impacts helm charts and user deployment experience backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.14.6
Backport done to v1.14
v1.15.0-rc.1
Backport done to v1.15
Development

Successfully merging this pull request may close these issues.

None yet

6 participants