Initial implementation of ForwardCRD plugin #4512

christianang · 2021-03-09T19:57:40Z

1. Why is this pull request needed and what does it do?

This PR is the initial implementation of the KubernetesCRD plugin that sets out to implement #4382. Namely, it allows CoreDNS to watch a DNSZone CRD to configure stub-domains for Kubernetes clusters.

Edit: This PR has been updated from just the readme to the initial implementation of the plugin.

2. Which issues (if any) are related?

#4382

3. Which documentation changes (if any) need to be made?

4. Does this introduce a backward incompatible change or deprecation?

plugin/forward/instance.go

plugin/kubernetescrd/apis/coredns/v1alpha1/dnszone_types.go

plugin/kubernetescrd/apis/coredns/v1alpha1/groupversion_info.go

plugin/kubernetescrd/kubernetescrd.go

miekg · 2021-03-16T10:02:33Z

very interesting idea. What the data model for the zone data? And how do you envision lookup taking weird DNS stuff into account, like wildcards and in-zone CNAMEs? You want to depend on file.Lookup ? Would not be a bad idea to make the interface suitable for different backends (long standing idea in another issue somewhere)

christianang · 2021-03-16T21:34:03Z

Currently I'm thinking this would handle the very narrow case of forwarding dns messages to other dns servers based on the zone name (basically implementing stub-domains, but through crds). Not currently thinking of handling any individual records for a zone. I imagine handling requests at that point would be similar to how CoreDNS handles dns mux-ing now for server blocks. E.g there will be a map of zone names that map to forward plugins and we would loop through the map looking for a match, popping off subdomains of the zone until a match is found and we call that forward plugin's ServeDNS function. I think this sidesteps a lot of dns complexity, but let me know if I'm overlooking something.

I can envision that this plugin can become more flexible, through additional crd(s). Not sure what this might look like yet, since there are probably a few ways to slice it so starting with a pretty limited crd.

plugin/kubernetescrd/controller.go

christianang · 2021-03-22T22:55:44Z

I've updated the PR to include my initial implementation of the plugin. Apologies for what is now a massive PR, it felt like it made sense to continue iterating on the single PR, but let me know if you prefer breaking this down in any way. I did try and keep commits focused, which may help digest the PR.

Would appreciate feedback, I imagine being new to this codebase there were design decisions I made that might not be the best approach for a given problem, so let me know if you spot ways this can be improved.

One design decision that might be worth pointing out, I added a function to the forward plugin NewWithConfig that allows the kubernetescrd plugin to create instances of forward on-demand. This duplicated some code, which could be "DRY"-ed up, but in favor of not making too far reaching changes to the code base I left it duplicated and am happy with making follow up PRs to de-duplicate code if the approach I used makes sense.

plugin/kubernetescrd/README.md

chrisohaver

Thanks @christianang! Lots to digest, but from what I understand, it looks good so far.

core/dnsserver/zdirectives.go

chrisohaver · 2021-04-05T14:11:44Z

plugin/forward/forward.go

 // New returns a new Forward.
 func New() *Forward {
 	f := &Forward{maxfails: 2, tlsConfig: new(tls.Config), expire: defaultExpire, p: new(random), from: ".", hcInterval: hcInterval, opts: options{forceTCP: false, preferUDP: false, hcRecursionDesired: true}}
 	return f
 }

+// NewWithConfig returns a new Forward configured by the provided
+// ForwardConfig.
+func NewWithConfig(config ForwardConfig) (*Forward, error) {


I think I like this direction. I presume forward's existing Corefile parsing functions will also be refactored to create a ForwardConfig, and then use NewWithConfig to create the Forward struct. This also opens the way for other plugins to reuse the "forwarding" function of the forward plugin (e.g. adding a forwarding action to coredns/policy).

@miekg, WDYT?

@chrisohaver I love this idea and implemented a similar solution in #4583. Anyway config parsing logic should be separated from plugin logic according to the single-responsibility principle. It will not only decouple components but also allow to thoroughly cover the parsing functionality with tests.

plugin/kubernetescrd/controller.go

Co-authored-by: Aidan Obley <aobley@vmware.com> Signed-off-by: Christian Ang <angc@vmware.com>

- Place forwardcrd before forward plugin in plugin list. This will avoid forward from preventing the forwardcrd plugin from handling any queries in the case of having a default upstream forwarder in a server block (as is the case in the default kubernetes Corefile). Co-authored-by: Aidan Obley <aobley@vmware.com> Signed-off-by: Christian Ang <angc@vmware.com>

Signed-off-by: Christian Ang <angc@vmware.com>

- allows external packages to instanciate forward plugins Co-authored-by: Aidan Obley <aobley@vmware.com> Signed-off-by: Christian Ang <angc@vmware.com>

- add a Kubernetes controller that can read Forward CRs - instances of the forward plugin are created based on Forward CRs from the Kubernetes controller - DNS requests are handled by calling matching Forward plugin instances based on zone name - Defaults to the kube-system namespace to align with Corefile RBAC Signed-off-by: Christian Ang <angc@vmware.com> Use klog v2 in forwardcrd plugin

Co-authored-by: Christian Ang <angc@vmware.com> Signed-off-by: Edwin Xie <exie@vmware.com>

- to ensure that the bitsize is 32 for later casting to uint32 Signed-off-by: Christian Ang <angc@vmware.com>

christianang · 2021-08-16T18:56:44Z

Hi, revisiting this PR since its been some time. @miekg do you think you can give this a review? I would appreciate it.

chrisohaver · 2021-09-09T19:56:32Z

@christianang, can you add this plugin and yourself as the owner, in the CODEOWNERS file?

Signed-off-by: Christian Ang <angc@vmware.com>

christianang · 2021-09-09T22:46:00Z

Done!

chrisohaver · 2021-10-25T14:02:33Z

@miekg, is it OK with you if we merge this new plugin without your review, or would you like to review it first?

miekg · 2021-11-12T16:18:52Z

[ Quoting ***@***.***> in "Re: [coredns/coredns] Initial imple..." ] ***@***.***, if it OK with you if we merge this new plugin without your review, or

would you like to review it first?

I don't have the time to thorougly review this. If I can find time for deeper coredns reviews it would be focussed on some long standing PRs in the *file* plugin.

miekg · 2021-11-12T16:32:54Z

ok, wsn't expediting that,. Think this is perfectly fine as an external plugin. Never saw the use-case why this needs to be in coredns proper

chrisohaver · 2021-11-12T16:35:11Z

ok, wsn't expediting that,. Think this is perfectly fine as an external plugin. Never saw the use-case why this needs to be in coredns proper

Sorry - I misinterpreted your response as an OK. I'll revert it.

This reverts commit 2e6953c.

chrisohaver · 2021-11-12T16:45:23Z

@christianang, sorry for the mixup. If you have time, please register the plugin as an external plugin on coredns.io ...

chrisohaver · 2021-11-12T16:49:03Z

... please register the plugin as an external plugin on coredns.io ...

Sorry - its been so long since I had looked at the code... quickly scanning it over, I remember now that this plugin required some refactoring of the forward plugin... so it cannot stand as-is as an external plugin unless we also create a new external plugin from a copy of the forward plugin with those modifications ... or we merge the mods to the forward plugin.

This reverts commit 2e6953c.

johnbelamaric · 2021-11-13T00:58:14Z

Alternatives to consider, that (maybe) won't require changes to any existing plugins:

Operator that consumes CRDs and rewrites the ConfigMap of the Corefile;
New input flag(s) that read read directly from k8s CRDs to create a special CaddyfileInput a-la this example: https://github.com/coredns/learning-coredns/blob/master/dnscached/main.go (may require using CoreDNS as a library rather than directly)
Some sort of tweak to coredns/caddyfile.Dispenser to construct a Corefile out of CRD input (similar to (2) but different approach internally

Of course (1) requires zero changes to CoreDNS.

chrisohaver · 2021-11-13T01:01:19Z

1 was originally suggested, but I had suggested a coredns plugin idea. Sorry folks!

christianang · 2021-12-01T18:49:31Z

Sad to hear. I'll hit pause on making any attempts at this for now since (as @chrisohaver brought up), this does require a refactor to the forward plugin so it can be used by the newly introduced crd plugin so just making it an external plugin will take some effort.

I am open to continuing discussion on the feature and how best to implement if someone wants to continue (perhaps it makes sense to reopen and continue on #4382?), but for now I'll put down the effort.

@christianang

* Add forwardcrd plugin README.md Co-authored-by: Aidan Obley <aobley@vmware.com> Signed-off-by: Christian Ang <angc@vmware.com> * Create forwardcrd plugin - Place forwardcrd before forward plugin in plugin list. This will avoid forward from preventing the forwardcrd plugin from handling any queries in the case of having a default upstream forwarder in a server block (as is the case in the default kubernetes Corefile). Co-authored-by: Aidan Obley <aobley@vmware.com> Signed-off-by: Christian Ang <angc@vmware.com> * Add Forward CRD Signed-off-by: Christian Ang <angc@vmware.com> * Add NewWithConfig to forward plugin - allows external packages to instanciate forward plugins Co-authored-by: Aidan Obley <aobley@vmware.com> Signed-off-by: Christian Ang <angc@vmware.com> * ForwardCRD plugin handles requests for Forward CRs - add a Kubernetes controller that can read Forward CRs - instances of the forward plugin are created based on Forward CRs from the Kubernetes controller - DNS requests are handled by calling matching Forward plugin instances based on zone name - Defaults to the kube-system namespace to align with Corefile RBAC Signed-off-by: Christian Ang <angc@vmware.com> Use klog v2 in forwardcrd plugin * Refactor forward setup to use NewWithConfig Co-authored-by: Christian Ang <angc@vmware.com> Signed-off-by: Edwin Xie <exie@vmware.com> * Use ParseInt instead of Atoi - to ensure that the bitsize is 32 for later casting to uint32 Signed-off-by: Christian Ang <angc@vmware.com> * Add @christianang to CODEOWNERS for forwardcrd Signed-off-by: Christian Ang <angc@vmware.com> Co-authored-by: Edwin Xie <exie@vmware.com> Signed-off-by: jinglinax@163.com <jinglinax@163.com>

…oredns#4981) This reverts commit 2e6953c. Signed-off-by: jinglinax@163.com <jinglinax@163.com>

stickler-ci reviewed Mar 9, 2021

View reviewed changes

christianang force-pushed the add-kubernetes-crd-plugin branch 2 times, most recently from 53e9e14 to 855394a Compare March 15, 2021 21:19

christianang changed the title ~~Initial Readme/Spike of KubernetesCRD plugin~~ Initial Readme of KubernetesCRD plugin Mar 15, 2021

christianang marked this pull request as ready for review March 15, 2021 21:30

christianang requested review from bradbeam, chrisohaver, dilyevsky, greenpau, Isolus, johnbelamaric, miekg, pmoroney, rajansandeep, stp-ip, SuperQ and yongtang as code owners March 15, 2021 21:30

christianang force-pushed the add-kubernetes-crd-plugin branch from 855394a to 35f4471 Compare March 22, 2021 21:50

stickler-ci reviewed Mar 22, 2021

View reviewed changes

plugin/kubernetescrd/controller.go Outdated Show resolved Hide resolved

plugin/kubernetescrd/controller.go Outdated Show resolved Hide resolved

christianang force-pushed the add-kubernetes-crd-plugin branch from 35f4471 to 9af4761 Compare March 22, 2021 21:53

christianang changed the title ~~Initial Readme of KubernetesCRD plugin~~ Initial implementation of KubernetesCRD plugin Mar 22, 2021

christianang force-pushed the add-kubernetes-crd-plugin branch from 9af4761 to 2806df8 Compare March 22, 2021 22:38

christianang force-pushed the add-kubernetes-crd-plugin branch from 2806df8 to 2e96adc Compare March 22, 2021 23:32

christianang mentioned this pull request Mar 23, 2021

CoreDNS plugin to watch Kubernetes CRD for stub-domains #4382

Open

rosenhouse reviewed Mar 25, 2021

View reviewed changes

plugin/kubernetescrd/README.md Outdated Show resolved Hide resolved

chrisohaver reviewed Apr 5, 2021

View reviewed changes

christianang force-pushed the add-kubernetes-crd-plugin branch from 4253808 to ff56369 Compare April 7, 2021 19:14

christianang added 4 commits August 16, 2021 18:21

Add forwardcrd plugin README.md

c681279

Co-authored-by: Aidan Obley <aobley@vmware.com> Signed-off-by: Christian Ang <angc@vmware.com>

Add Forward CRD

159fdca

Signed-off-by: Christian Ang <angc@vmware.com>

Add NewWithConfig to forward plugin

ff2fb75

- allows external packages to instanciate forward plugins Co-authored-by: Aidan Obley <aobley@vmware.com> Signed-off-by: Christian Ang <angc@vmware.com>

christianang force-pushed the add-kubernetes-crd-plugin branch 2 times, most recently from 8ab82af to 7dc40a0 Compare August 16, 2021 18:32

christianang and others added 3 commits August 16, 2021 18:43

Refactor forward setup to use NewWithConfig

fdeeb1c

Co-authored-by: Christian Ang <angc@vmware.com> Signed-off-by: Edwin Xie <exie@vmware.com>

Use ParseInt instead of Atoi

58974b2

- to ensure that the bitsize is 32 for later casting to uint32 Signed-off-by: Christian Ang <angc@vmware.com>

christianang force-pushed the add-kubernetes-crd-plugin branch from 7dc40a0 to 58974b2 Compare August 16, 2021 18:43

chrisohaver mentioned this pull request Sep 9, 2021

1.8.5 release tracking #4853

Closed

3 tasks

Add @christianang to CODEOWNERS for forwardcrd

a017315

Signed-off-by: Christian Ang <angc@vmware.com>

chrisohaver merged commit 2e6953c into coredns:master Nov 12, 2021

chrisohaver added a commit that referenced this pull request Nov 12, 2021

Revert "Initial implementation of ForwardCRD plugin (#4512)"

67219cb

This reverts commit 2e6953c.

chrisohaver mentioned this pull request Nov 12, 2021

Revert "Initial implementation of ForwardCRD plugin" #4981

Merged

chrisohaver added a commit that referenced this pull request Nov 12, 2021

Revert "Initial implementation of ForwardCRD plugin (#4512)" (#4981)

3288b11

This reverts commit 2e6953c.

jinglina pushed a commit to jinglina/coredns that referenced this pull request Dec 23, 2021

Revert "Initial implementation of ForwardCRD plugin (coredns#4512)" (c…

242b76e

…oredns#4981) This reverts commit 2e6953c. Signed-off-by: jinglinax@163.com <jinglinax@163.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of ForwardCRD plugin #4512

Initial implementation of ForwardCRD plugin #4512

christianang commented Mar 9, 2021 •

edited

miekg commented Mar 16, 2021

christianang commented Mar 16, 2021

christianang commented Mar 22, 2021

chrisohaver left a comment

chrisohaver Apr 5, 2021

v-byte-cpu May 13, 2021 •

edited

christianang commented Aug 16, 2021

chrisohaver commented Sep 9, 2021

christianang commented Sep 9, 2021

chrisohaver commented Oct 25, 2021 •

edited

miekg commented Nov 12, 2021 via email

miekg commented Nov 12, 2021

chrisohaver commented Nov 12, 2021 •

edited

chrisohaver commented Nov 12, 2021

chrisohaver commented Nov 12, 2021 •

edited

johnbelamaric commented Nov 13, 2021

chrisohaver commented Nov 13, 2021 •

edited

christianang commented Dec 1, 2021

Initial implementation of ForwardCRD plugin #4512

Initial implementation of ForwardCRD plugin #4512

Conversation

christianang commented Mar 9, 2021 • edited

1. Why is this pull request needed and what does it do?

2. Which issues (if any) are related?

3. Which documentation changes (if any) need to be made?

4. Does this introduce a backward incompatible change or deprecation?

miekg commented Mar 16, 2021

christianang commented Mar 16, 2021

christianang commented Mar 22, 2021

chrisohaver left a comment

Choose a reason for hiding this comment

chrisohaver Apr 5, 2021

Choose a reason for hiding this comment

v-byte-cpu May 13, 2021 • edited

Choose a reason for hiding this comment

christianang commented Aug 16, 2021

chrisohaver commented Sep 9, 2021

christianang commented Sep 9, 2021

chrisohaver commented Oct 25, 2021 • edited

miekg commented Nov 12, 2021 via email

miekg commented Nov 12, 2021

chrisohaver commented Nov 12, 2021 • edited

chrisohaver commented Nov 12, 2021

chrisohaver commented Nov 12, 2021 • edited

johnbelamaric commented Nov 13, 2021

chrisohaver commented Nov 13, 2021 • edited

christianang commented Dec 1, 2021

christianang commented Mar 9, 2021 •

edited

v-byte-cpu May 13, 2021 •

edited

chrisohaver commented Oct 25, 2021 •

edited

chrisohaver commented Nov 12, 2021 •

edited

chrisohaver commented Nov 12, 2021 •

edited

chrisohaver commented Nov 13, 2021 •

edited