[*] improve error messages

.github/workflows/build.yml

@@ -18,7 +18,7 @@ jobs:
     - name: Set up Golang
       uses: actions/setup-go@v5
       with:
-        go-version: '1.23' 
+        go-version: '1.26' 
 
     - name: Install etcd
       if: runner.os == 'Linux'
@@ -44,6 +44,15 @@ jobs:
         version: latest
         args: --verbose
 
+    - name: Test
+      run: |
+        go test -failfast -v -coverprofile profile.cov ./...
+
+    - name: Coveralls
+      uses: coverallsapp/github-action@v2
+      with:
+        file: profile.cov 
+
     - name: Test E2E
       if: runner.os == 'Linux'
       run: |

checker/consul_leader_checker.go

@@ -23,7 +23,11 @@ func NewConsulLeaderChecker(con *vipconfig.Config) (lc *ConsulLeaderChecker, err
 
 	url, err := url.Parse(con.Endpoints[0])
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to parse consul endpoint URL %s: %w", con.Endpoints[0], err)
+	}
+
+	if url.Hostname() == "" {
+		return nil, fmt.Errorf("invalid consul endpoint URL: hostname is empty in %s", con.Endpoints[0])
 	}
 
 	config := &api.Config{
@@ -34,7 +38,7 @@ func NewConsulLeaderChecker(con *vipconfig.Config) (lc *ConsulLeaderChecker, err
 	}
 
 	if lc.Client, err = api.NewClient(config); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to create consul client for endpoint %s: %w", con.Endpoints[0], err)
 	}
 
 	return lc, nil

checker/consul_leader_checker_test.go

@@ -0,0 +1,57 @@
+package checker
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/cybertec-postgresql/vip-manager/vipconfig"
+	"go.uber.org/zap"
+)
+
+func newTestConfig(endpoint string) *vipconfig.Config {
+	return &vipconfig.Config{
+		Endpoints: []string{endpoint},
+		Logger:    zap.NewNop(),
+	}
+}
+
+// TestNewConsulLeaderChecker_UnparseableURL verifies that a URL containing a
+// null byte (rejected by net/url) is wrapped with context.
+func TestNewConsulLeaderChecker_UnparseableURL(t *testing.T) {
+	t.Parallel()
+	_, err := NewConsulLeaderChecker(newTestConfig("http://invalid\x00host"))
+	if err == nil {
+		t.Fatal("expected error, got nil")
+	}
+	if !strings.Contains(err.Error(), "failed to parse consul endpoint URL") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+// TestNewConsulLeaderChecker_EmptyHostname verifies that a URL with no host
+// component (e.g. a bare path) is rejected with the empty-hostname sentinel.
+func TestNewConsulLeaderChecker_EmptyHostname(t *testing.T) {
+	t.Parallel()
+	// "localhost" without a scheme parses successfully but Hostname() == ""
+	_, err := NewConsulLeaderChecker(newTestConfig("localhost"))
+	if err == nil {
+		t.Fatal("expected error, got nil")
+	}
+	if !strings.Contains(err.Error(), "hostname is empty") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+// TestNewConsulLeaderChecker_ValidURL verifies that a well-formed endpoint
+// does not produce a construction error (api.NewClient never fails for valid
+// address strings).
+func TestNewConsulLeaderChecker_ValidURL(t *testing.T) {
+	t.Parallel()
+	lc, err := NewConsulLeaderChecker(newTestConfig("http://127.0.0.1:8500"))
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if lc == nil {
+		t.Fatal("expected non-nil checker")
+	}
+}

checker/etcd_leader_checker.go

@@ -23,7 +23,7 @@ type EtcdLeaderChecker struct {
 func NewEtcdLeaderChecker(conf *vipconfig.Config) (*EtcdLeaderChecker, error) {
 	tlsConfig, err := getTransport(conf)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to create TLS transport for etcd: %w", err)
 	}
 	cfg := clientv3.Config{
 		Endpoints:            conf.Endpoints,
@@ -35,7 +35,10 @@ func NewEtcdLeaderChecker(conf *vipconfig.Config) (*EtcdLeaderChecker, error) {
 		Logger:               conf.Logger,
 	}
 	c, err := clientv3.New(cfg)
-	return &EtcdLeaderChecker{conf, c}, err
+	if err != nil {
+		return nil, fmt.Errorf("failed to connect to etcd at endpoints %v: %w", conf.Endpoints, err)
+	}
+	return &EtcdLeaderChecker{conf, c}, nil
 }
 
 func getTransport(conf *vipconfig.Config) (*tls.Config, error) {
@@ -74,7 +77,17 @@ func getTransport(conf *vipconfig.Config) (*tls.Config, error) {
 func (elc *EtcdLeaderChecker) get(ctx context.Context, out chan<- bool) {
 	resp, err := elc.Get(ctx, elc.TriggerKey)
 	if err != nil {
-		elc.Logger.Error("Failed to get etcd value:", zap.Error(err))
+		elc.Logger.Error("Failed to get etcd value", zap.String("key", elc.TriggerKey), zap.Error(err))
+		out <- false
+		return
+	}
+	if resp == nil {
+		elc.Logger.Error("Received nil response from etcd", zap.String("key", elc.TriggerKey))
+		out <- false
+		return
+	}
+	if len(resp.Kvs) == 0 {
+		elc.Logger.Sugar().Info("No value found for key ", elc.TriggerKey, " - DCS may not have a leader yet")
 		out <- false
 		return
 	}
@@ -99,6 +112,7 @@ func (elc *EtcdLeaderChecker) watch(ctx context.Context, out chan<- bool) error
 				continue
 			}
 			if err := watchResp.Err(); err != nil {
+				elc.Logger.Error("Watch error for key "+elc.TriggerKey+":", zap.Error(err))
 				elc.get(ctx, out) // RPC failed, try to get the key directly to be on the safe side
 				continue
 			}

checker/etcd_leader_checker_test.go

@@ -0,0 +1,137 @@
+package checker
+
+import (
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+
+	"github.com/cybertec-postgresql/vip-manager/vipconfig"
+	"go.uber.org/zap"
+)
+
+// certsDir returns the absolute path to the shared test certificates.
+func certsDir() string {
+	_, file, _, _ := runtime.Caller(0)
+	return filepath.Join(filepath.Dir(file), "..", "test", "certs")
+}
+
+func etcdConfig() *vipconfig.Config {
+	return &vipconfig.Config{
+		Endpoints: []string{"http://127.0.0.1:2379"},
+		Logger:    zap.NewNop(),
+	}
+}
+
+// ---------------------------------------------------------------------------
+// getTransport
+// ---------------------------------------------------------------------------
+
+// TestGetTransport_NoTLS verifies that an empty TLS config is accepted and
+// returns a non-nil (but empty) *tls.Config.
+func TestGetTransport_NoTLS(t *testing.T) {
+	t.Parallel()
+	cfg, err := getTransport(etcdConfig())
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if cfg == nil {
+		t.Fatal("expected non-nil tls.Config")
+	}
+}
+
+// TestGetTransport_MissingCAFile verifies the error when the CA file path does
+// not exist.
+func TestGetTransport_MissingCAFile(t *testing.T) {
+	t.Parallel()
+	conf := etcdConfig()
+	conf.EtcdCAFile = "/nonexistent/ca.crt"
+	_, err := getTransport(conf)
+	if err == nil {
+		t.Fatal("expected error, got nil")
+	}
+	if !strings.Contains(err.Error(), "cannot load CA file") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+// TestGetTransport_MissingCertFiles verifies the error when the client cert or
+// key file is missing.
+func TestGetTransport_MissingCertFiles(t *testing.T) {
+	t.Parallel()
+	conf := etcdConfig()
+	conf.EtcdCertFile = "/nonexistent/client.crt"
+	conf.EtcdKeyFile = "/nonexistent/client.key"
+	_, err := getTransport(conf)
+	if err == nil {
+		t.Fatal("expected error, got nil")
+	}
+	if !strings.Contains(err.Error(), "cannot load client cert or key file") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+// TestGetTransport_ValidCAFile verifies that a real CA certificate file is
+// loaded without error.
+func TestGetTransport_ValidCAFile(t *testing.T) {
+	t.Parallel()
+	conf := etcdConfig()
+	conf.EtcdCAFile = filepath.Join(certsDir(), "etcd_server_ca.crt")
+	cfg, err := getTransport(conf)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if cfg.RootCAs == nil {
+		t.Error("expected RootCAs to be populated")
+	}
+}
+
+// TestGetTransport_ValidCertAndKey verifies that a real client cert+key pair
+// is loaded without error.
+func TestGetTransport_ValidCertAndKey(t *testing.T) {
+	t.Parallel()
+	conf := etcdConfig()
+	conf.EtcdCAFile = filepath.Join(certsDir(), "etcd_server_ca.crt")
+	conf.EtcdCertFile = filepath.Join(certsDir(), "etcd_client.crt")
+	conf.EtcdKeyFile = filepath.Join(certsDir(), "etcd_client.key")
+	cfg, err := getTransport(conf)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(cfg.Certificates) == 0 {
+		t.Error("expected certificates to be populated")
+	}
+}
+
+// ---------------------------------------------------------------------------
+// NewEtcdLeaderChecker
+// ---------------------------------------------------------------------------
+
+// TestNewEtcdLeaderChecker_TLSError verifies that a TLS config error is
+// wrapped with "failed to create TLS transport for etcd".
+func TestNewEtcdLeaderChecker_TLSError(t *testing.T) {
+	t.Parallel()
+	conf := etcdConfig()
+	conf.EtcdCAFile = "/nonexistent/ca.crt"
+	_, err := NewEtcdLeaderChecker(conf)
+	if err == nil {
+		t.Fatal("expected error, got nil")
+	}
+	if !strings.Contains(err.Error(), "failed to create TLS transport for etcd") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+// TestNewEtcdLeaderChecker_ValidConfig verifies that the checker is created
+// without error when endpoints and TLS are valid. The etcd client connects
+// lazily so no live server is required.
+func TestNewEtcdLeaderChecker_ValidConfig(t *testing.T) {
+	t.Parallel()
+	checker, err := NewEtcdLeaderChecker(etcdConfig())
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if checker == nil {
+		t.Fatal("expected non-nil checker")
+	}
+}

checker/patroni_leader_checker.go

@@ -47,12 +47,17 @@ func (c *PatroniLeaderChecker) GetChangeNotificationStream(ctx context.Context,
 		case <-ctx.Done():
 			return nil
 		case <-time.After(time.Duration(c.Interval) * time.Millisecond):
-			r, err := c.Get(c.Endpoints[0] + c.TriggerKey)
+			url := c.Endpoints[0] + c.TriggerKey
+			r, err := c.Get(url)
 			if err != nil {
-				c.Logger.Sugar().Error("patroni REST API error:", err)
+				c.Logger.Sugar().Errorf("patroni REST API error connecting to %s: %v", url, err)
+				out <- false
 				continue
 			}
 			r.Body.Close() //throw away the body
+			if r.StatusCode < 200 || r.StatusCode >= 300 {
+				c.Logger.Sugar().Warnf("patroni REST API returned non-success status code %d for %s (expected %s)", r.StatusCode, url, c.TriggerValue)
+			}
 			out <- strconv.Itoa(r.StatusCode) == c.TriggerValue
 		}
 	}