From c30124e192e8e9096e67e2bcd87780be2d0a5730 Mon Sep 17 00:00:00 2001 From: Adam Perlin Date: Tue, 27 Jun 2017 13:14:07 -0700 Subject: [PATCH] Fix several bugs in minecraft input (#2970) --- .../inputs/minecraft/internal/rcon/rcon.go | 10 +++++- plugins/inputs/minecraft/minecraft.go | 25 +++++++++---- plugins/inputs/minecraft/minecraft_test.go | 8 ++++- plugins/inputs/minecraft/rcon.go | 30 ++++++++++++++-- .../minecraft/rcon_disconnect_error_test.go | 36 +++++++++++++++++++ plugins/inputs/minecraft/rcon_test.go | 5 +-- 6 files changed, 100 insertions(+), 14 deletions(-) create mode 100644 plugins/inputs/minecraft/rcon_disconnect_error_test.go diff --git a/plugins/inputs/minecraft/internal/rcon/rcon.go b/plugins/inputs/minecraft/internal/rcon/rcon.go index f17163f8b..118771242 100644 --- a/plugins/inputs/minecraft/internal/rcon/rcon.go +++ b/plugins/inputs/minecraft/internal/rcon/rcon.go @@ -171,9 +171,17 @@ func (c *Client) Send(typ int32, command string) (response *Packet, err error) { } body := make([]byte, header.Size-int32(PacketHeaderSize)) - n, err = c.Connection.Read(body) + for n < len(body) { + var nBytes int + nBytes, err = c.Connection.Read(body[n:]) + if err != nil { + return + } + n += nBytes + } + if nil != err { return } else if n != len(body) { diff --git a/plugins/inputs/minecraft/minecraft.go b/plugins/inputs/minecraft/minecraft.go index d88f4373d..04b3f4ad1 100644 --- a/plugins/inputs/minecraft/minecraft.go +++ b/plugins/inputs/minecraft/minecraft.go @@ -25,15 +25,16 @@ var ( // Client is an interface for a client which gathers data from a minecraft server type Client interface { - Gather() ([]string, error) + Gather(producer RCONClientProducer) ([]string, error) } // Minecraft represents a connection to a minecraft server type Minecraft struct { - Server string - Port string - Password string - client Client + Server string + Port string + Password string + client Client + clientSet bool } // Description gives a brief description. @@ -48,16 +49,26 @@ func (s *Minecraft) SampleConfig() string { // Gather uses the RCON protocal to collect player and // scoreboard stats from a minecraft server. +//var hasClient bool = false func (s *Minecraft) Gather(acc telegraf.Accumulator) error { - if s.client == nil { + // can't simply compare s.client to nil, because comparing an interface + // to nil often does not produce the desired result + if !s.clientSet { var err error s.client, err = NewRCON(s.Server, s.Port, s.Password) if err != nil { return err } + s.clientSet = true } - scores, err := s.client.Gather() + // (*RCON).Gather() takes an RCONClientProducer for testing purposes + d := defaultClientProducer{ + Server: s.Server, + Port: s.Port, + } + + scores, err := s.client.Gather(d) if err != nil { return err } diff --git a/plugins/inputs/minecraft/minecraft_test.go b/plugins/inputs/minecraft/minecraft_test.go index 664ad4e81..c0a9e6cf5 100644 --- a/plugins/inputs/minecraft/minecraft_test.go +++ b/plugins/inputs/minecraft/minecraft_test.go @@ -161,7 +161,7 @@ type MockClient struct { Err error } -func (m *MockClient) Gather() ([]string, error) { +func (m *MockClient) Gather(d RCONClientProducer) ([]string, error) { return m.Result, m.Err } @@ -178,13 +178,19 @@ func TestGather(t *testing.T) { }, Err: nil, }, + clientSet: true, } err := testConfig.Gather(&acc) + if err != nil { t.Fatalf("gather returned error. Error: %s\n", err) } + if !testConfig.clientSet { + t.Fatalf("clientSet should be true, client should be set") + } + tags := map[string]string{ "player": "divislight", "server": "biffsgang.net:25575", diff --git a/plugins/inputs/minecraft/rcon.go b/plugins/inputs/minecraft/rcon.go index 6dbed47ad..219a1d14f 100644 --- a/plugins/inputs/minecraft/rcon.go +++ b/plugins/inputs/minecraft/rcon.go @@ -29,6 +29,22 @@ type RCON struct { client RCONClient } +// RCONClientProducer is an interface which defines how a new client will be +// produced in the event of a network disconnect. It exists mainly for +// testing purposes +type RCONClientProducer interface { + newClient() (RCONClient, error) +} + +type defaultClientProducer struct { + Server string + Port string +} + +func (d defaultClientProducer) newClient() (RCONClient, error) { + return newClient(d.Server, d.Port) +} + // NewRCON creates a new RCON func NewRCON(server, port, password string) (*RCON, error) { client, err := newClient(server, port) @@ -50,18 +66,26 @@ func newClient(server, port string) (*rcon.Client, error) { return nil, err } - return rcon.NewClient(server, p) + client, err := rcon.NewClient(server, p) + + // If we've encountered any error, the contents of `client` could be corrupted, + // so we must return nil, err + if err != nil { + return nil, err + } + return client, nil } // Gather recieves all player scoreboard information and returns it per player. -func (r *RCON) Gather() ([]string, error) { +func (r *RCON) Gather(producer RCONClientProducer) ([]string, error) { if r.client == nil { var err error - r.client, err = newClient(r.Server, r.Port) + r.client, err = producer.newClient() if err != nil { return nil, err } } + if _, err := r.client.Authorize(r.Password); err != nil { // Potentially a network problem where the client will need to be // re-initialized diff --git a/plugins/inputs/minecraft/rcon_disconnect_error_test.go b/plugins/inputs/minecraft/rcon_disconnect_error_test.go new file mode 100644 index 000000000..c89308e06 --- /dev/null +++ b/plugins/inputs/minecraft/rcon_disconnect_error_test.go @@ -0,0 +1,36 @@ +package minecraft + +import ( + "errors" + "testing" +) + +type MockRCONProducer struct { + Err error +} + +func (m *MockRCONProducer) newClient() (RCONClient, error) { + return nil, m.Err +} + +func TestRCONErrorHandling(t *testing.T) { + m := &MockRCONProducer{ + Err: errors.New("Error: failed connection"), + } + c := &RCON{ + Server: "craftstuff.com", + Port: "2222", + Password: "pass", + //Force fetching of new client + client: nil, + } + + _, err := c.Gather(m) + if err == nil { + t.Errorf("Error nil, unexpected result") + } + + if c.client != nil { + t.Fatal("c.client should be nil, unexpected result") + } +} diff --git a/plugins/inputs/minecraft/rcon_test.go b/plugins/inputs/minecraft/rcon_test.go index 011939894..1660b53f9 100644 --- a/plugins/inputs/minecraft/rcon_test.go +++ b/plugins/inputs/minecraft/rcon_test.go @@ -40,7 +40,8 @@ func TestRCONGather(t *testing.T) { client: mock, } - got, err := client.Gather() + d := defaultClientProducer{} + got, err := client.Gather(d) if err != nil { t.Fatalf("Gather returned an error. Error %s\n", err) } @@ -57,7 +58,7 @@ func TestRCONGather(t *testing.T) { Err: nil, } - got, err = client.Gather() + got, err = client.Gather(defaultClientProducer{}) if err != nil { t.Fatalf("Gather returned an error. Error %s\n", err) }