Skip to content

helm: clustermesh: add a job for autoconfiguring coredns mcsapi config #40506

Merged
giorio94 merged 4 commits intocilium:mainfrom
MrFreezeex:mcsapi-coredns-autoinstall
Jul 24, 2025
Merged

helm: clustermesh: add a job for autoconfiguring coredns mcsapi config #40506
giorio94 merged 4 commits intocilium:mainfrom
MrFreezeex:mcsapi-coredns-autoinstall

Conversation

@MrFreezeex
Copy link
Copy Markdown
Member

@MrFreezeex MrFreezeex commented Jul 13, 2025

First commit move the existing mcsapi option into a subsection for regrouping all mcsapi configuration together.

Second commit add a new command to autoconfigure CoreDNS to configure CoreDNS with the required MCS-API configuration.

Third commit add the helm code to wire up the command for the previous commit in helm installation and recommend installing CoreDNS v1.12.2+ (will hopefully be included in Kubernetes 1.35+ by default) that now support natively MCS-API.

clustermesh: helm: move MCS-API helm config and add a job to autoconfigure CoreDNS for MCS-API for CoreDNS v1.12.2+

@MrFreezeex MrFreezeex requested review from a team as code owners July 13, 2025 18:08
@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 13, 2025
@MrFreezeex MrFreezeex added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/clustermesh Relates to multi-cluster routing functionality in Cilium. area/helm Impacts helm charts and user deployment experience and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 13, 2025
@MrFreezeex MrFreezeex force-pushed the mcsapi-coredns-autoinstall branch 2 times, most recently from 7a95aa9 to 5198ce9 Compare July 13, 2025 18:18
@MrFreezeex
Copy link
Copy Markdown
Member Author

/test

@MrFreezeex MrFreezeex force-pushed the mcsapi-coredns-autoinstall branch 2 times, most recently from 5da3923 to 210e5e5 Compare July 13, 2025 19:28
@MrFreezeex
Copy link
Copy Markdown
Member Author

/test

Copy link
Copy Markdown
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! Helm-wise this looks okay to me on a first glance, but I'm not a fan of us pulling in yet another external image.

Have you investigated if this could be implemented in Go and make the job invoke a clustermesh-apiserver subcommand (or perhaps even run it a go-routine of one of the existing components)?

Copy link
Copy Markdown
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks! A couple of initial comments inline.

Generally speaking, I'd also prefer to avoid pulling in yet another image, and replace the bash script with a sub-command of one of the already existing images. That would also make the overall command more robust compared to bash scripting, and easier to extend in future, especially in case more advanced tasks need to be performed. Additionally, kubectl has technically a relatively strict compatibility policy, making it tricky to pick the correct version (even though the commands here are simple enough that real problems are unlikely).

I'm not totally sure about the best image to use though. The clustermesh-apiserver seems the most likely candidate, even though it is not strictly necessary to run it to make MCS-API work. However, that would not be a significant problem as long as it is run as a separate job, which to me makes sense given its one-off nature.

@MrFreezeex
Copy link
Copy Markdown
Member Author

Ah yes right, I was wondering of doing this in go but presupposed that I needed to create a new component entirely which seems harder but If I could do that in the clustermesh-apiserver it seems easier indeed 👀. Thank you both for the suggestion will try that!

@MrFreezeex MrFreezeex marked this pull request as draft July 14, 2025 11:48
@MrFreezeex MrFreezeex force-pushed the mcsapi-coredns-autoinstall branch 6 times, most recently from c978ddb to 9bae7da Compare July 14, 2025 20:52
@MrFreezeex
Copy link
Copy Markdown
Member Author

/test

@MrFreezeex
Copy link
Copy Markdown
Member Author

/test

@MrFreezeex MrFreezeex marked this pull request as ready for review July 20, 2025 19:40
@MrFreezeex MrFreezeex requested a review from giorio94 July 20, 2025 19:40
Copy link
Copy Markdown
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks! A couple of final comments inline.

Create a new dedicated subsection for mcsapi and deprecated the old
option to prepare for more mcsapi related options grouped together.

Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>
@MrFreezeex MrFreezeex force-pushed the mcsapi-coredns-autoinstall branch from 705e41b to 5929842 Compare July 21, 2025 20:09
@MrFreezeex
Copy link
Copy Markdown
Member Author

(push above is only a rebase because of the conflict related to upgrade notes)

@MrFreezeex MrFreezeex force-pushed the mcsapi-coredns-autoinstall branch 2 times, most recently from efeb9b5 to d4b2ec5 Compare July 21, 2025 20:35
@MrFreezeex
Copy link
Copy Markdown
Member Author

/test

Starting in CoreDNS v1.12.2, CoreDNS natively supports MCS-API. This
commit update the existing documentation to reflect and recommend that.

This is also wiring a opt-in job in the helm installation to auto configure
CoreDNS that was added in the previous commit.

Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>
@MrFreezeex MrFreezeex force-pushed the mcsapi-coredns-autoinstall branch from d4b2ec5 to f93a26e Compare July 22, 2025 10:06
@MrFreezeex
Copy link
Copy Markdown
Member Author

I fixed one more thing about the new image version check in the last push to strip the image digest if present. It should cover most coredns installation now I believe 👀

@MrFreezeex
Copy link
Copy Markdown
Member Author

/test

Copy link
Copy Markdown
Contributor

@tommyp1ckles tommyp1ckles left a comment

Choose a reason for hiding this comment

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

Not as familiar with clustermesh but k8s helm changes seem reasonable to me. Approving for @cilium/sig-k8s.

@giorio94 giorio94 added dont-merge/bad-bot To prevent MLH from marking ready-to-merge. and removed dont-merge/bad-bot To prevent MLH from marking ready-to-merge. labels Jul 23, 2025
@giorio94 giorio94 enabled auto-merge July 23, 2025 08:57
@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 24, 2025
@giorio94 giorio94 added this pull request to the merge queue Jul 24, 2025
Merged via the queue into cilium:main with commit d8dd4bb Jul 24, 2025
68 checks passed
@cilium-release-bot cilium-release-bot bot moved this to Released in cilium v1.19.0 Feb 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/clustermesh Relates to multi-cluster routing functionality in Cilium. area/helm Impacts helm charts and user deployment experience 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

No open projects
Status: Released

Development

Successfully merging this pull request may close these issues.

5 participants