From 9b5ab5dce03c0e4c8453b72e5b91ab11e2ac44f5 Mon Sep 17 00:00:00 2001
From: Jaime Soriano Pastor <jaime.soriano@elastic.co>
Date: Thu, 1 Jul 2021 13:06:32 +0200
Subject: [PATCH] Fix memory leak in SQL helper when database is not available
 (#26607)

Explicitly close SQL connections when connectivity checks fail during their
creation, to avoid leaking resources when a connection is created but it
is not going to be used.

(cherry picked from commit e4d4f76706d70302fb6fe6c5efa0e8c9da51ef56)
---
 CHANGELOG.next.asciidoc           |  1 +
 metricbeat/helper/sql/sql.go      |  3 ++
 metricbeat/helper/sql/sql_test.go | 59 +++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)

diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc
index b4cbd7fed51..a5c86248496 100644
--- a/CHANGELOG.next.asciidoc
+++ b/CHANGELOG.next.asciidoc
@@ -385,6 +385,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
 - Fix azure billing dashboard. {pull}25554[25554]
 - Major refactor of system/cpu and system/core metrics. {pull}25771[25771]
 - Fix GCP Project ID being ingested as `cloud.account.id` in `gcp.billing` module {issue}26357[26357] {pull}26412[26412]
+- Fix memory leak in SQL module when database is not available. {issue}25840[25840] {pull}26607[26607]
 - Fix aws metric tags with resourcegroupstaggingapi paginator.  {issue}26385[26385] {pull}26443[26443]
 
 *Packetbeat*
diff --git a/metricbeat/helper/sql/sql.go b/metricbeat/helper/sql/sql.go
index b417e3dc923..521295b1b2d 100644
--- a/metricbeat/helper/sql/sql.go
+++ b/metricbeat/helper/sql/sql.go
@@ -52,6 +52,9 @@ func NewDBClient(driver, uri string, l *logp.Logger) (*DbClient, error) {
 	}
 	err = dbx.Ping()
 	if err != nil {
+		if closeErr := dbx.Close(); closeErr != nil {
+			return nil, errors.Wrapf(err, "failed to close with %s, after connection test failed", closeErr)
+		}
 		return nil, errors.Wrap(err, "testing connection")
 	}
 
diff --git a/metricbeat/helper/sql/sql_test.go b/metricbeat/helper/sql/sql_test.go
index 1045c8d5b87..713837a96fa 100644
--- a/metricbeat/helper/sql/sql_test.go
+++ b/metricbeat/helper/sql/sql_test.go
@@ -18,11 +18,18 @@
 package sql
 
 import (
+	"context"
+	"database/sql"
+	"database/sql/driver"
+	"fmt"
 	"math"
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/elastic/beats/v7/libbeat/common"
+	"github.com/elastic/beats/v7/libbeat/tests/resources"
 )
 
 type kv struct {
@@ -186,3 +193,55 @@ func TestToDotKeys(t *testing.T) {
 		t.Fail()
 	}
 }
+
+func TestNewDBClient(t *testing.T) {
+	t.Run("create and close", func(t *testing.T) {
+		goroutines := resources.NewGoroutinesChecker()
+		defer goroutines.Check(t)
+
+		client, err := NewDBClient("dummy", "localhost", nil)
+		require.NoError(t, err)
+
+		err = client.Close()
+		require.NoError(t, err)
+	})
+
+	t.Run("unavailable", func(t *testing.T) {
+		goroutines := resources.NewGoroutinesChecker()
+		defer goroutines.Check(t)
+
+		_, err := NewDBClient("dummy", "unavailable", nil)
+		require.Error(t, err)
+	})
+}
+
+func init() {
+	sql.Register("dummy", dummyDriver{})
+}
+
+type dummyDriver struct{}
+
+func (dummyDriver) Open(name string) (driver.Conn, error) {
+	if name == "error" {
+		return nil, fmt.Errorf("error")
+	}
+
+	return &dummyConnection{name: name}, nil
+}
+
+type dummyConnection struct {
+	name string
+}
+
+// Ensure that this dummy connection implements the pinger interface, used by the helper.
+var _ driver.Pinger = &dummyConnection{}
+
+func (*dummyConnection) Prepare(query string) (driver.Stmt, error) { return nil, nil }
+func (*dummyConnection) Close() error                              { return nil }
+func (*dummyConnection) Begin() (driver.Tx, error)                 { return nil, nil }
+func (c *dummyConnection) Ping(context.Context) error {
+	if c.name == "unavailable" {
+		return fmt.Errorf("database unavailable")
+	}
+	return nil
+}