Fix several bugs in minecraft input (#2970)

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) {

plugins/inputs/minecraft/minecraft.go

@@ -25,7 +25,7 @@ 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
@@ -34,6 +34,7 @@ type Minecraft struct {
 	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
 	}

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",

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

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")
+	}
+}

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)
 	}