WIP: Improve using coreDNS as a go package (lib)

[davidhadas]

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

This change is needed to allow using CoreDNS/core/dnsserver as a go package to be imported and used by other projects.

2. Which issues (if any) are related?

See #5853

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

An example of how to use dnsserver as a package and attach the forward plugin and plugin named myplug:

package main

import (
	"github.com/coredns/caddy"
	"github.com/coredns/coredns/core/dnsserver"
	"github.com/coredns/coredns/plugin"
	_ "github.com/coredns/coredns/plugin/forward"
	_ "..../pkg/myplug"
)

func startDnsServices() {
	caddy.Quiet = true
	caddy.AppName = "myApp"
	caddy.AppVersion = "0.0.1"

	// Register the plugins
	dnsserver.RegisterServers([]string{"myplug", "forward"})

	// Set the dnsserver configuration
	corefileinput := caddy.CaddyfileInput{
		Filepath:       "",
		Contents:       []byte(".:1253 {\n myplug \n forward . /etc/resolv.conf \n}\n"),
		ServerTypeName: "dns",
	}

	// Start your engines
	_, err := caddy.Start(corefileinput)
	if err != nil {
		...
	}
}

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

Please advise

[davidhadas]

An alternative shown by @miekg at https://coredns.io/2017/07/25/compile-time-enabling-or-disabling-plugins/ may also be used.

However:

1. The suggested change allows the main program to be the one controlling CLI flags - it is expected that the imported dnssserver package will not process (or be affected by) CLI flags.
2. The suggested change allows the main program to be the one controlling the configuration - it is expected that the imported dnssserver package will be configurable by the importing program.
3. It seems that the suggested change - removing the on-load init() is the right option to initialize dnssserver rather than initializing it on-load during init().

Please advise

[chrisohaver]

I'm unsure of backward compatibility of this change with existing used-as-library solutions. The most widely deployed of these is (probably) Kubernetes node local dns cache.

Some of the flags set unexposed vars, so setting them from another package would be problematic. Probably need to expose those.

Is using CoreDNS as a separate component in your solution instead of a compiled-in package a viable option for you?

[davidhadas]

I'm unsure of backward compatibility of this change with existing used-as-library solutions. The most widely deployed of these is (probably) Kubernetes node local dns cache.

If the existing used-as-library solutions import coremain, I believe this change should not affect them.

If the existing used-as-library solutions import core/dnsserver, without importing coremain I wonder what they do... why does it make sense to have the flags in core/dnsserver? why does it make sense to have core/dnsserver do init() and autoregister a fixed set of plugins that are created with coreDNS make... how is it created in that other project? I am somewhat doubtful that anyone does this - imports core/dnsserver, without importing coremain...

Some of the flags set unexposed vars, so setting them from another package would be problematic. Probably need to expose those.

Do you see any issue with the change in this PR in this respect? I have not exposed any new vars.

Is using CoreDNS as a separate component in your solution instead of a compiled-in package a viable option for you?

Not really. As a POC maybe, but not as a released solution :)

btw, to fix the tests, it seems changes are needed to the test files - specifically:

import  _ "github.com/coredns/coredns/coremain" 

should do the trick.

(Once the overall direction will make sense, I will get this fixed).

[chrisohaver]

Is using CoreDNS as a separate component in your solution instead of a compiled-in package a viable option for you?

Not really. As a POC maybe, but not as a released solution :)

Can you elaborate a bit as to why?

[davidhadas]

Can you elaborate a bit as to why?

We plan to use the DNS proxy in a sidecar - overhead is a critical aspect

Also, we expect Guard's monitoring and control to do a function call to the plugin package to gather the list of domains approached by the user container and the list of IPs provided to the user container from DNS. This works now in a quick POC after making the changes in this PR to CoreDNS code.

[davidhadas]

I believe the only reason for CoreDNS to do caddy.RegisterServerType() in the core/dnsservice package init() is if the community decided to disallow others from ever using the package with other Directives. I.e to enforce the community Directives to all be there whenever core/dnsservice is used and disallow additional Directives.

If this is the intention - so I am specifically asking the CoreDNS community to do what it decided not to do :) and allow the use of core/dnsservice as a go package with alternative Directives (and this PR shows one way to do that)

If not, then is there a reason why to continue to prevent others from (1) using the core/dnsservice as a go package and (2) control the list of Directives and the use of flags?

Btw, the current practice of using init() for Caddy initialization seems wrong in many accounts. One is that Caddy's code may panic - more specifically, caddy.RegisterServerType may panic - this panic will happen during go package import. This seems very messy.

[davidhadas]

One more direction is to allow a second call to RegisterServerType to replace the first call without panic.

I.e. remove the panic from [coredns/caddy] (https://github.com/coredns/caddy/blob/b1ad5f2b4194270ea4f2cdc87fbf0b02f8a715fd/plugins.go#L183) and knowingly allow a second call to RegisterServerType replace the server type registered in a previous call:

I.e. change the function to:

func RegisterServerType(typeName string, srv ServerType) {
	serverTypes[typeName] = srv
}

This will at least allow changing the list of Directives after import...

[chrisohaver]

Can you look into the test failures?

[davidhadas]

Ok, when working on the tests I realized that the list of Directives is never being read during init(). Instead, only the caddy.RegisterServerType() simply registers a function call for reading the list of Directives, but the actual list is not called read until later. This means that when using core/dnsservice as a package of some other program, the program using the package can change the Directives even after init() has called caddy.RegisterServerType().

This removes a concern that was a real blocker. I realized we can change the list of Directives, making CoreDNS much smaller in size, even when using core/dnsservice as is.

This leaves us with two issues:

1. flags - hopefully we can move it from core/dnsservice to coremain
2. The general concern I was raising about the way CoreDNS uses init() when it should not, opening itself to panic during init() and other mischiefs.

Here is what I suggest doing: a) create a second tiny PR only for the flags b) leave this PR for moving beyond init() - I will clean what I have done so far and suggest a minimal change for the community to decide() - (This is certainly not a blocker to our use of CoreDNS).

make sense?

[davidhadas]

See PR #5865