From ee8084a08f8cfcd4357ae2ad0b6dff51ca322d3a Mon Sep 17 00:00:00 2001 From: Francois Tur Date: Wed, 14 Feb 2018 14:20:27 -0500 Subject: [PATCH] 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 | 22 ++-- plugin/proxy/grpc_test.go | 210 ++++++++++++++++++++++++++++++++------ 2 files changed, 193 insertions(+), 39 deletions(-) diff --git a/plugin/proxy/grpc.go b/plugin/proxy/grpc.go index b1ac1b1a8..2844a1c71 100644 --- a/plugin/proxy/grpc.go +++ b/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 + 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 } - d := new(dns.Msg) - err = d.Unpack(reply.Msg) - if err != nil { - return nil, err - } - return d, nil + return nil, fmt.Errorf("grpc exchange - no connection available for host: %s ", addr) } func (g *grpcClient) Transport() string { return "tcp" } diff --git a/plugin/proxy/grpc_test.go b/plugin/proxy/grpc_test.go index c6e6e20cc..801b6fd2f 100644 --- a/plugin/proxy/grpc_test.go +++ b/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 TestStartupShutdown(t *testing.T) { - grpclog.SetLogger(discard{}) +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 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) { } } -// discard is a Logger that outputs nothing. -type discard struct{} +func TestGRPCRunAQuery(t *testing.T) { -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{}) {} + 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 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-----` +)