search

coredns / coredns

CoreDNS is a DNS server that chains plugins
https://coredns.io
Apache License 2.0
12.44k stars 2.14k forks source link

Merging the coredns/multicluster plugin in tree #6921

Open MrFreezeex opened 1 month ago

MrFreezeex commented 1 month ago

What would you like to be added: Merging the coredns/multicluster plugin (https://github.com/coredns/multicluster) in-tree/in the coredns main branch.

Why is this needed: The CoreDNS multicluster plugin is the implementation of KEP-1645 (https://github.com/kubernetes/enhancements/blob/master/keps/sig-multicluster/1645-multi-cluster-services-api/README.md#dns) and from what is publicly available is used by

Outside of that the Kubernetes SIG handling this KEP (SIG Multicluster) is looking at graduating to beta in the coming months: https://github.com/kubernetes-sigs/mcs-api/issues/48.

On my side I represent mostly the Cilium side of things and we would aim to somehow simplify the installation process of the coredns multicluster plugin which triggered this proposal/question. We could start by providing a docker image with the multicluster plugin compiled in/included but unlike AWS and Alibaba we run on many different types of Kubernetes installation and we don't control tightly everyone's setup so this comes with the risk that the Kubernetes cluster has installed a specific version of CoreDNS with some additional plugins. Including this in-tree would increase significantly the chance that the multicluster plugin would be compiled-in in the long term in the coredns image already installed in the cluster.

I would be happy to do all the work of getting the plugin in-tree and any potential improvements in order to do that. And as Cilium is aiming to be conformant and follow closely the KEP I would be happy to work on the maintenance of the plugin to keep it up to date with the KEP.

Let me know if getting the plugin in-tree looks reasonable to you, if yes I would start drafting some PR/checking more in-depth how to do this. Thanks!

keithmattix commented 1 month ago

I'm also happy to help some here!

MrFreezeex commented 2 weeks ago

Hi @chrisohaver, sorry to ping you directly but as I noticed you contributed/were involved in the multicluster plugin I was wondering if you could ack / says if this would sounds reasonable to you (in a non binding way is fine too!). :pray:

If you have any preference on the way you would like to see this done feel free to share, otherwise no worries we can certainly figure things out on the PR itself!

chrisohaver commented 2 weeks ago

It sounds reasonable to me to include the multicluster plugin as an internal coredns plugin.

MrFreezeex commented 1 week ago

@chrisohaver thanks for the quick response :pray:! I do have an additional question though, I checked a bit more what this would implies and all the changes to the kubernetes plugin in the last ~2 years that would similarly be applicable to the multicluster plugin and there's probably ~10 individual changes that might be applicable. Most of them are individually somewhat small but combined I don't think it can be qualified as small anymore :/. Thus I have a few proposals on how to get this going and would love to have your opinion on which path do you prefer between:

1) Submit all those fixes/changes as individual PRs to the multicluster repo and when everything went through/been checked open the PR for merging the multicluster plugin against the main coredns repo 2) Submit all those fixes/changes as one PR with separated commit for each one to the multicluster repo and then open the PR for merging the multicluster plugin against the main coredns repo 3) Open the PR to merge the multicluster plugin in the main coredns repo with minimal changes/fixes and fix everything after this initial merge 4) Do these fixes within the same PR as the one to merge the multicluster plugin in the main repo with each of those in a separate commit

I am guessing the 1) option would be preferable but let me know if you prefer something else, I'm trying to make this as easy as it could be for reviewers :sweat_smile:.

chrisohaver commented 1 week ago

@chrisohaver thanks for the quick response 🙏! I do have an additional question though, I checked a bit more what this would implies and all the changes to the kubernetes plugin in the last ~2 years that would similarly be applicable to the multicluster plugin and there's probably ~10 individual changes that might be applicable. Most of them are individually somewhat small but combined I don't think it can be qualified as small anymore :/. Thus I have a few proposals on how to get this going and would love to have your opinion on which path do you prefer between:

  1. Submit all those fixes/changes as individual PRs to the multicluster repo and when everything went through/been checked open the PR for merging the multicluster plugin against the main coredns repo
  2. Submit all those fixes/changes as one PR with separated commit for each one to the multicluster repo and then open the PR for merging the multicluster plugin against the main coredns repo
  3. Open the PR to merge the multicluster plugin in the main coredns repo with minimal changes/fixes and fix everything after this initial merge
  4. Do these fixes within the same PR as the one to merge the multicluster plugin in the main repo with each of those in a separate commit

I am guessing the 1) option would be preferable but let me know if you prefer something else, I'm trying to make this as easy as it could be for reviewers 😅.

Is there a lot of code duplication between the two plugins? If so, they’d need to be consolidated when merging into coredns. Perhaps I am misreading.

(By this I mean that duplicated code should be refactored to be shared)

MrFreezeex commented 1 week ago

Is there a lot of code duplication between the two plugins? If so, they’d need to be consolidated when merging into coredns. Perhaps I am misreading.

(By this I mean that duplicated code should be refactored to be shared)

Yep that's correct there is some duplication the most obvious one would be the duplicated trimmed down kubernetes objects here https://github.com/coredns/multicluster/tree/main/object which I intend to share/refactor.

Then there is some similar-ish code not related to the objects which I am not entirely sure could/should be refactored to be shared ATM but I can check a bit more what I can do to share even more code.

The rest implies applying similar fixes:

Also I intend to:

And from my initial research that would be "all" I could grasp from the last 3 years of coredns/kubernetes plugin change not applied to the multicluster plugin.

MrFreezeex commented 1 day ago

So I created the first PR starting the cleanup/refactor to share code between the kubernetes and multicluster plugin: https://github.com/coredns/multicluster/pull/17.

For the next steps I am planning to open a few more PR also against coredns/multicluster to fix the various things mentioned above. And then once we have settled with those, I would open a PR against this repo/the main coredns repo for merging the plugin + probably doing a few update within the same PR that would requires to modify the kubernetes plugin (for instance exposing more functions to use them in the multicluster plugin).