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.

Sign up for GitHub

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

Jump to bottom

fix: deduplicate ConfigMap key if ENI mode and endpointRoutes are enabled #31891

Merged
merged 1 commit into from
Apr 22, 2024
Merged

fix: deduplicate ConfigMap key if ENI mode and endpointRoutes are enabled #31891

merged 1 commit into from
Apr 22, 2024

Conversation

remi-gelinas
Copy link
Contributor

@remi-gelinas remi-gelinas commented Apr 10, 2024 •
edited

Currently, if the Helm values eni.enabled and endpointRoutes.enabled are both explicitly enabled, the enable-endpoint-routes key in the main ConfigMap will be duplicated and fail schema validation. This PR adds an explicit check for endpointRoutes.enabled when eni.enabled is true to avoid duplicating the key, as discussed in #31868.

Fixes: #31868

@remi-gelinas remi-gelinas requested review from a team as code owners April 10, 2024 16:42
@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 Apr 10, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Apr 10, 2024
gandro
gandro reviewed Apr 10, 2024
View reviewed changes
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.

Great, thank you! One minor nit, otherwise this looks good to me

install/kubernetes/cilium/templates/cilium-configmap.yaml Outdated Show resolved Hide resolved
@gandro gandro added area/eni Impacts ENI based IPAM. release-note/misc This PR makes changes that have no direct user impact. area/helm Impacts helm charts and user deployment experience labels Apr 10, 2024
@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 Apr 10, 2024
@remi-gelinas remi-gelinas force-pushed the remi-gelinas/31868-deduplicate-enable-endpoint-routes branch from 27d53f4 to 49b0855 Compare April 10, 2024 18:18
gandro
gandro approved these changes Apr 11, 2024
View reviewed changes
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.

Great, thank you!

@gandro
Copy link
Member

gandro commented Apr 11, 2024

/test

@remi-gelinas
Copy link
Contributor Author

@gandro Hey, just wanted to follow up on this - is there a reason image builds are failing on this branch when the only changes are to the Helm chart? Is there any action I need to take to get this mergeable?

@gandro
Copy link
Member

gandro commented Apr 18, 2024

@gandro Hey, just wanted to follow up on this - is there a reason image builds are failing on this branch when the only changes are to the Helm chart? Is there any action I need to take to get this mergeable?

Let me restart the affected workflow, it might have been a flake.

As for next steps, there is nothing required from you at this point. We're still waiting on a review from sig-k8s. Once that is in (and assuming passing CI), we can merge then

@gandro
Copy link
Member

gandro commented Apr 22, 2024 •
edited

@remi-gelinas Would you mind rebasing on main? It seems the build commit error is persistent, it might be because your branch is lagging behind a bit

Edit: Andre rebased the branch for you!

@aanm aanm added this pull request to the merge queue Apr 22, 2024
@aanm aanm removed this pull request from the merge queue due to a manual request Apr 22, 2024
@remi-gelinas @aanm
helm: deduplicate ConfigMap key if ENI mode and endpointRoutes are en…
f8bc3cb 
…abled

Currently, if the Helm values `eni.enabled` and `endpointRoutes.enabled` are both explicitly enabled, the `enable-endpoint-routes` key in the Cilium
ConfigMap will be duplicated and fail schema validation. This commit adds an explicit check for `endpointRoutes.enabled` when `eni.enabled` is true to
avoid duplicating the key.

Fixes: cilium#31868

Signed-off-by: Remi Gelinas <mail@remigelin.as>
@aanm aanm force-pushed the remi-gelinas/31868-deduplicate-enable-endpoint-routes branch from 49b0855 to f8bc3cb Compare April 22, 2024 09:23
@aanm
Copy link
Member

aanm commented Apr 22, 2024

/test

@aanm aanm enabled auto-merge April 22, 2024 09:23
@aanm aanm added this pull request to the merge queue Apr 22, 2024
Merged via the queue into cilium:main with commit 62e211e Apr 22, 2024
62 checks passed
@remi-gelinas remi-gelinas deleted the remi-gelinas/31868-deduplicate-enable-endpoint-routes branch April 22, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@aanm aanm aanm approved these changes

@tommyp1ckles tommyp1ckles Awaiting requested review from tommyp1ckles tommyp1ckles is a code owner automatically assigned from cilium/sig-k8s

Assignees
No one assigned
Labels
area/eni Impacts ENI based IPAM. area/helm Impacts helm charts and user deployment experience kind/community-contribution This was a contribution made by a community member. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Helm chart generates duplicate ConfigMap value
3 participants
@remi-gelinas @gandro @aanm