helm: clustermesh: add a job for autoconfiguring coredns mcsapi config #40506
helm: clustermesh: add a job for autoconfiguring coredns mcsapi config #40506giorio94 merged 4 commits intocilium:mainfrom
Conversation
7a95aa9 to
5198ce9
Compare
|
/test |
5da3923 to
210e5e5
Compare
|
/test |
There was a problem hiding this comment.
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)?
giorio94
left a comment
There was a problem hiding this comment.
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.
install/kubernetes/cilium/templates/clustermesh-coredns-mcsapi/job-role.yaml
Show resolved
Hide resolved
|
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 |
c978ddb to
9bae7da
Compare
|
/test |
|
/test |
giorio94
left a comment
There was a problem hiding this comment.
Thanks! A couple of final comments inline.
install/kubernetes/cilium/templates/clustermesh-coredns-mcsapi/job.yaml
Outdated
Show resolved
Hide resolved
install/kubernetes/cilium/templates/clustermesh-coredns-mcsapi/job.yaml
Outdated
Show resolved
Hide resolved
clustermesh-apiserver/mcsapi-coredns-cfg/testdata/configure.txtar
Outdated
Show resolved
Hide resolved
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]>
705e41b to
5929842
Compare
|
(push above is only a rebase because of the conflict related to upgrade notes) |
efeb9b5 to
d4b2ec5
Compare
|
/test |
Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>
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]>
Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>
d4b2ec5 to
f93a26e
Compare
|
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 👀 |
|
/test |
tommyp1ckles
left a comment
There was a problem hiding this comment.
Not as familiar with clustermesh but k8s helm changes seem reasonable to me. Approving for @cilium/sig-k8s.
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.