Testing CoreDNS

Hi,

I’m testing CoreDNS, by building a new server and calling InspectServerBlocks and MakeServer myself. But I’ve run into a snag that the middleware isn’t chained in. This is because I can’t call the non-exported:

err = executeDirectives(inst, cdyfile.Path(), stype.Directives, sblocks)

That Caddy uses to set this all up.

How do I work around this? I also feel I’m copying a lot of stuff and there should be a more easy way? Code I’m talking about is here: https://github.com/miekg/coredns/blob/caddy/test/server.go#L61

1 Like

Good question. Let me get Caddy 0.9.1 out the door and then I’ll revisit this! Ping me if you don’t hear about it for a few days…

1 Like

I’ve disabled a bunch of integration tests, which make caddy-coredns pass the remaining testsuite \o/.
But I’m anxious to merge :slight_smile:

Do you have any (vague) ideas what you want to do here? I.e. I can give it a whack and send a PR your way (don’t hold your breath though).

What is it that you’re trying to test, exactly? Getting a list of servers will only allow you to Listen() and Serve(), basically. Is that what you’re after?

Yes, the whole stack basically, Corefile → []caddy.Servers and then have some control over them. So indeed Listen() and Serve() (and Shutdown() but that is a dnsserver.Server method).

In that case, would you be opposed to just testing by calling caddy.Start() and passing in your Corefile? It calls Listen and Serve for you, so depending on what you wanted to test, I guess you could do it that way? And call Stop() on the Instance when you’re done.

Just trying to keep the surface area of the exported package small as possible. But if we need to open it up a bit more to benefit testing, we can look into that.

That might worked, yes. I agree with keeping the API small.

And, FYI, adding more tests to Caddy core and the HTTP server is on my roadmap too, so either way, tests will be more filled out / robust in the future.

What I do need is the address the server is listening on (both UDP and TCP).

Right now I get this by making the interface *dnsserver.Server and using my local methods.

Do *Instance type does not seem to have methods to get that info?

Hmm, the HTTP server defines its listener address; does yours not do that?

So, no it doesn’t – I haven’t needed it to be able to do that because the HTTP server knows what it’s listening on. Or we know the addresses to check based on the input Caddyfile. We could probably write one I guess… but did my answer help spur any thoughts?

No, in tests I listen on :0 and let the system decide which port to open. But then I need to go back and figure out what that port is…

I see… okay… well, I will be happy to accept proposals, it’s something I can look at over the next few days as well.

Case in point: https://github.com/miekg/coredns/blob/caddy/test/proxy_test.go#L30

OK, there are 3 way to fix this:

  1. extending the Server interface and adding extra methods to extract this (don’t like this solution)

  2. exposing, Instance.servers and the []serverListeners in a neat way and then the server contained in serverListeners. This could be done by just exporting these fields…?

  3. same as 2, but instead of exporting server we could export the listener and packet to you can then just can LocalAddr and Addr on them?

How about this:

diff --git a/caddy.go b/caddy.go
index 401c6fc..5dcc14b 100644
--- a/caddy.go
+++ b/caddy.go
@@ -80,6 +80,9 @@ type Instance struct {
        onFinalShutdown []func() error // stopping, not as part of a restart
 }

+// Servers returns all servers from the instance i.
+func (i *Instance) Servers() []serverListener { return i.servers }
+
 // Stop stops all servers contained in i. It does NOT
 // execute shutdown callbacks.
 func (i *Instance) Stop() error {
diff --git a/plugins.go b/plugins.go
index 0226747..b7f680b 100644
--- a/plugins.go
+++ b/plugins.go
@@ -89,6 +89,12 @@ type serverListener struct {
        packet   net.PacketConn
 }

+// Listener returns the net.Listener from s.
+func (s serverListener) Listener() net.Listener { return s.listener }
+
+// PacketConn returns the net.PacketConn from s.
+func (s serverListener) PacketConn() net.PacketConn { return s.packet }
+
 // Context is a type which carries a server type through
 // the load and setup phase; it maintains the state
 // between loading the Caddyfile, then executing its

Untested, not compiled and not yet used by me.

Lint will complain about returning an unexported type. If we go this route, we should just export the serverListener type and forego those extra two accessor methods.

Lint! :slight_smile:
Mind you, this is just for testing (but there good be other uses)

I’m (obviously) fine with exporting the type. Do you also want to export the struct members in the type (listener and packet)?

Actually on second thought maybe we should leave those fields unexported and use getter methods so that the value is read-only. (Am I right in thinking that’s the result? I need to double check what is a pointer or something. It’s late here…)

Edit: ALSO we need to make sure it’s not going to have race conditions.

yes, read-only is fine, but the type is an interface type to the actual type might be a pointer.

Maybe we could test an ugly in between: only implement my use case? I want Addr and LocalAddr, we could add those methods to serverListener?

Is it that ugly? If that’s all you need, let’s see what the diff looks like. Might be a good balance.