Plugin/Proxy - prevent nil pointer when query using a gRPC client that could not dial in (#1495)

* prevent nil pointer when query frpc client that could not open

* add UT checking we can now safely request DNS query on an invalid hostname, query for gRPC connection

* fix ortograph

* fix format

* fix package declaration, fix UT - grpclogger, use fatalf, build pool with known addresses

* type, useless error check - after review

plugin/proxy/grpc.go

@@ -2,6 +2,7 @@ package proxy
 
 import (
 	"crypto/tls"
+	"fmt"
 	"log"
 
 	"github.com/coredns/coredns/pb"
@@ -42,16 +43,19 @@ func (g *grpcClient) Exchange(ctx context.Context, addr string, state request.Re
 		return nil, err
 	}
 
-	reply, err := g.clients[addr].Query(ctx, &pb.DnsPacket{Msg: msg})
-	if err != nil {
-		return nil, err
-	}
-	d := new(dns.Msg)
-	err = d.Unpack(reply.Msg)
-	if err != nil {
-		return nil, err
+	if cl, ok := g.clients[addr]; ok {
+		reply, err := cl.Query(ctx, &pb.DnsPacket{Msg: msg})
+		if err != nil {
+			return nil, err
+		}
+		d := new(dns.Msg)
+		err = d.Unpack(reply.Msg)
+		if err != nil {
+			return nil, err
+		}
+		return d, nil
 	}
-	return d, nil
+	return nil, fmt.Errorf("grpc exchange - no connection available for host: %s ", addr)
 }
 
 func (g *grpcClient) Transport() string { return "tcp" }

plugin/proxy/grpc_test.go

@@ -2,33 +2,63 @@ package proxy
 
 import (
 	"testing"
-	"time"
 
 	"github.com/coredns/coredns/plugin/pkg/healthcheck"
 
+	"fmt"
+	"github.com/coredns/coredns/plugin/pkg/tls"
+	"github.com/coredns/coredns/plugin/test"
+	"github.com/coredns/coredns/request"
+	"github.com/miekg/dns"
+	"golang.org/x/net/context"
 	"google.golang.org/grpc/grpclog"
 )
 
-func pool() []*healthcheck.UpstreamHost {
-	return []*healthcheck.UpstreamHost{
-		{
-			Name: "localhost:10053",
-		},
-		{
-			Name: "localhost:10054",
-		},
+func init() {
+	grpclog.SetLoggerV2(discardV2{})
+}
+
+func buildPool(size int) ([]*healthcheck.UpstreamHost, func(), error) {
+	ups := make([]*healthcheck.UpstreamHost, size)
+	srvs := []*dns.Server{}
+	errs := []error{}
+	for i := 0; i < size; i++ {
+		srv, addr, err := test.TCPServer("localhost:0")
+		if err != nil {
+			errs = append(errs, err)
+			continue
+		}
+		ups[i] = &healthcheck.UpstreamHost{Name: addr}
+		srvs = append(srvs, srv)
+	}
+	stopIt := func() {
+		for _, s := range srvs {
+			s.Shutdown()
+		}
 	}
+	if len(errs) > 0 {
+		go stopIt()
+		valErr := ""
+		for _, e := range errs {
+			valErr += fmt.Sprintf("%v\n", e)
+		}
+		return nil, nil, fmt.Errorf("error at allocation of the pool : %v", valErr)
+	}
+	return ups, stopIt, nil
 }
 
-func TestStartupShutdown(t *testing.T) {
-	grpclog.SetLogger(discard{})
+func TestGRPCStartupShutdown(t *testing.T) {
+
+	pool, closePool, err := buildPool(2)
+	if err != nil {
+		t.Fatalf("error creating the pool of upstream for the test : %s", err)
+	}
+	defer closePool()
 
 	upstream := &staticUpstream{
 		from: ".",
 		HealthCheck: healthcheck.HealthCheck{
-			Hosts:       pool(),
-			FailTimeout: 10 * time.Second,
-			MaxFails:    1,
+			Hosts: pool,
 		},
 	}
 	g := newGrpcClient(nil, upstream)
@@ -37,19 +67,17 @@ func TestStartupShutdown(t *testing.T) {
 	p := &Proxy{}
 	p.Upstreams = &[]Upstream{upstream}
 
-	err := g.OnStartup(p)
+	err = g.OnStartup(p)
 	if err != nil {
-		t.Errorf("Error starting grpc client exchanger: %s", err)
-		return
+		t.Fatalf("Error starting grpc client exchanger: %s", err)
 	}
-	if len(g.clients) != len(pool()) {
-		t.Errorf("Expected %d grpc clients but found %d", len(pool()), len(g.clients))
+	if len(g.clients) != len(pool) {
+		t.Fatalf("Expected %d grpc clients but found %d", len(pool), len(g.clients))
 	}
 
 	err = g.OnShutdown(p)
 	if err != nil {
-		t.Errorf("Error stopping grpc client exchanger: %s", err)
-		return
+		t.Fatalf("Error stopping grpc client exchanger: %s", err)
 	}
 	if len(g.clients) != 0 {
 		t.Errorf("Shutdown didn't remove clients, found %d", len(g.clients))
@@ -59,12 +87,134 @@ func TestStartupShutdown(t *testing.T) {
 	}
 }
 
+func TestGRPCRunAQuery(t *testing.T) {
+
+	pool, closePool, err := buildPool(2)
+	if err != nil {
+		t.Fatalf("error creating the pool of upstream for the test : %s", err)
+	}
+	defer closePool()
+
+	upstream := &staticUpstream{
+		from: ".",
+		HealthCheck: healthcheck.HealthCheck{
+			Hosts: pool,
+		},
+	}
+	g := newGrpcClient(nil, upstream)
+	upstream.ex = g
+
+	p := &Proxy{}
+	p.Upstreams = &[]Upstream{upstream}
+
+	err = g.OnStartup(p)
+	if err != nil {
+		t.Fatalf("Error starting grpc client exchanger: %s", err)
+	}
+	// verify the client is usable, or an error is properly raised
+	state := request.Request{W: &test.ResponseWriter{}, Req: new(dns.Msg)}
+	g.Exchange(context.TODO(), "localhost:10053", state)
+
+	// verify that you have proper error if the hostname is unknwn or not registered
+	_, err = g.Exchange(context.TODO(), "invalid:10055", state)
+	if err == nil {
+		t.Errorf("Expecting a proper error when querying gRPC client with invalid hostname : %s", err)
+	}
+
+	err = g.OnShutdown(p)
+	if err != nil {
+		t.Fatalf("Error stopping grpc client exchanger: %s", err)
+	}
+}
+
+func TestGRPCRunAQueryOnSecureLinkWithInvalidCert(t *testing.T) {
+
+	pool, closePool, err := buildPool(1)
+	if err != nil {
+		t.Fatalf("error creating the pool of upstream for the test : %s", err)
+	}
+	defer closePool()
+
+	upstream := &staticUpstream{
+		from: ".",
+		HealthCheck: healthcheck.HealthCheck{
+			Hosts: pool,
+		},
+	}
+
+	filename, rmFunc, err := test.TempFile("", aCert)
+	if err != nil {
+		t.Errorf("Error saving file : %s", err)
+		return
+	}
+	defer rmFunc()
+
+	tls, _ := tls.NewTLSClientConfig(filename)
+	// ignore error as the certificate is known valid
+
+	g := newGrpcClient(tls, upstream)
+	upstream.ex = g
+
+	p := &Proxy{}
+	p.Upstreams = &[]Upstream{upstream}
+
+	// Although dial will not work, it is not expected to have an error
+	err = g.OnStartup(p)
+	if err != nil {
+		t.Fatalf("Error starting grpc client exchanger: %s", err)
+	}
+
+	// verify that you have proper error if the hostname is unknwn or not registered
+	state := request.Request{W: &test.ResponseWriter{}, Req: new(dns.Msg)}
+	_, err = g.Exchange(context.TODO(), pool[0].Name+"-whatever", state)
+	if err == nil {
+		t.Errorf("Error in Exchange process : %s ", err)
+	}
+
+	err = g.OnShutdown(p)
+	if err != nil {
+		t.Fatalf("Error stopping grpc client exchanger: %s", err)
+	}
+}
+
 // discard is a Logger that outputs nothing.
-type discard struct{}
-
-func (d discard) Fatal(args ...interface{})                 {}
-func (d discard) Fatalf(format string, args ...interface{}) {}
-func (d discard) Fatalln(args ...interface{})               {}
-func (d discard) Print(args ...interface{})                 {}
-func (d discard) Printf(format string, args ...interface{}) {}
-func (d discard) Println(args ...interface{})               {}
+type discardV2 struct{}
+
+func (d discardV2) Info(args ...interface{})                    {}
+func (d discardV2) Infoln(args ...interface{})                  {}
+func (d discardV2) Infof(format string, args ...interface{})    {}
+func (d discardV2) Warning(args ...interface{})                 {}
+func (d discardV2) Warningln(args ...interface{})               {}
+func (d discardV2) Warningf(format string, args ...interface{}) {}
+func (d discardV2) Error(args ...interface{})                   {}
+func (d discardV2) Errorln(args ...interface{})                 {}
+func (d discardV2) Errorf(format string, args ...interface{})   {}
+func (d discardV2) Fatal(args ...interface{})                   {}
+func (d discardV2) Fatalln(args ...interface{})                 {}
+func (d discardV2) Fatalf(format string, args ...interface{})   {}
+func (d discardV2) V(l int) bool                                { return true }
+
+const (
+	aCert = `-----BEGIN CERTIFICATE-----
+	MIIDlDCCAnygAwIBAgIJAPaRnBJUE/FVMA0GCSqGSIb3DQEBBQUAMEUxCzAJBgNV
+BAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5ldCBX
+aWRnaXRzIFB0eSBMdGQwHhcNMTcxMTI0MTM0OTQ3WhcNMTgxMTI0MTM0OTQ3WjBF
+MQswCQYDVQQGEwJBVTETMBEGA1UECAwKU29tZS1TdGF0ZTEhMB8GA1UECgwYSW50
+ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIB
+CgKCAQEAuTDeAoWS6tdZVcp/Vh3FlagbC+9Ohi5VjRXgkpcn9JopbcF5s2jpl1v+
+cRpqkrmNNKLh8qOhmgdZQdh185VNe/iZ94H42qwKZ48vvnC5hLkk3MdgUT2ewgup
+vZhy/Bb1bX+buCWkQa1u8SIilECMIPZHhBP4TuBUKJWK8bBEFAeUnxB5SCkX+un4
+pctRlcfg8sX/ghADnp4e//YYDqex+1wQdFqM5zWhWDZAzc5Kdkyy9r+xXNfo4s1h
+fI08f6F4skz1koxG2RXOzQ7OK4YxFwT2J6V72iyzUIlRGZTbYDvair/zm1kjTF1R
+B1B+XLJF9oIB4BMZbekf033ZVaQ8YwIDAQABo4GGMIGDMDMGA1UdEQQsMCqHBH8A
+AAGHBDR3AQGHBDR3AQCHBDR3KmSHBDR3KGSHBDR3KmWHBDR3KtIwHQYDVR0OBBYE
+FFAEccLm7D/rN3fEe1fwzH7p0spAMB8GA1UdIwQYMBaAFFAEccLm7D/rN3fEe1fw
+zH7p0spAMAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQEFBQADggEBAF4zqaucNcK2
+GwYfijwbbtgMqPEvbReUEXsC65riAPjksJQ9L2YxQ7K0RIugRizuD1DNQam+FSb0
+cZEMEKzvMUIexbhZNFINWXY2X9yUS/oZd5pWP0WYIhn6qhmLvzl9XpxNPVzBXYWe
+duMECCigU2x5tAGmFa6g/pXXOoZCBRzFXwXiuNhSyhJEEwODjLZ6vgbySuU2jso3
+va4FKFDdVM16s1/RYOK5oM48XytCMB/JoYoSJHPfpt8LpVNAQEHMvPvHwuZBON/z
+q8HFtDjT4pBpB8AfuzwtUZ/zJ5atwxa5+ahcqRnK2kX2RSINfyEy43FZjLlvjcGa
+UIRTUJK1JKg=
+-----END CERTIFICATE-----`
+)