[+] add SECURITY.md and CONTRIBUTING.md, fix the service unit

CONTRIBUTING.md

@@ -0,0 +1,63 @@
+# Contributing to vip-manager
+
+Thank you for taking the time. vip-manager is a small daemon that moves a
+virtual IP between the members of a Patroni cluster, so most changes touch code
+that decides whether a machine may answer on an address. Please keep that in
+mind: a change that leaves the address assigned on the wrong node is worse than
+a missing feature.
+
+## Before you start
+
+For anything beyond a typo, please open an issue first and describe what you
+are after. That saves you from writing a patch that goes in a direction the
+maintainers would not take.
+
+## Building and testing
+
+```shell
+go build ./...
+go test ./...
+```
+
+The linter that runs in CI is [golangci-lint](https://golangci-lint.run/), with
+the configuration in `.golangci.yml`:
+
+```shell
+golangci-lint run
+```
+
+Please run `gofmt` on everything you touch. The Windows specific files are
+built with `GOOS=windows`, which is easy to forget on Linux:
+
+```shell
+GOOS=windows go build ./...
+GOOS=linux go build ./...
+```
+
+Some tests start etcd or consul in a container through
+[testcontainers](https://golang.testcontainers.org/) and are skipped when no
+Docker daemon is available. The tests that add and remove addresses need root
+and are skipped otherwise, CI runs them in a privileged container. There is
+also an end to end test in `test/behaviour_test.sh` which needs root and a
+local etcd.
+
+## Pull requests
+
+- one topic per pull request, it makes review and a later `git bisect` easier
+- add a test for a fix, so that the bug cannot come back unnoticed
+- update `README.md` when you add or change a configuration item
+- the commit subject follows the convention used in the history:
+  `[+]` for an addition, `[-]` for a fix or a removal, `[*]` for a change of
+  existing behaviour and `[!]` for a refactoring or a breaking change,
+  e.g. `[-] remove VIP when DCS becomes unreachable, closes #336`
+- describe *why* the change is needed in the body of the commit message, the
+  diff already says what it does
+
+## Reporting bugs
+
+Please include the version (`vip-manager --version`), the configuration with
+the credentials removed, and the log around the moment things went wrong. If
+the log is not telling enough, `--verbose` adds the caller and the retries of
+the DCS client.
+
+Security issues do not belong in the issue tracker, see [SECURITY.md](SECURITY.md).

README.md

@@ -24,7 +24,7 @@ Manages a virtual IP based on state kept in `etcd`, `Consul` or using `Patroni`
 
 ## Prerequisites
 
-- `go` >= 1.19
+- `go` >= 1.26, see the `go` directive in `go.mod`
 - `make` (optional)
 - `goreleaser` (optional)
 

SECURITY.md

@@ -0,0 +1,38 @@
+# Security Policy
+
+vip-manager runs with enough privileges to add and remove addresses on a
+network interface, it opens raw sockets to send gratuitous ARP messages, and it
+holds credentials for the DCS and, with `manager-type=hetzner`, for the Hetzner
+Robot API. Please treat findings in these areas as security relevant.
+
+## Supported versions
+
+Security fixes are released for the latest minor release. Older releases are
+not maintained, please upgrade before reporting an issue that is already fixed
+in the current version.
+
+## Reporting a vulnerability
+
+Please **do not open a public issue** for a vulnerability.
+
+Use GitHub's private vulnerability reporting instead: go to the
+[Security tab](https://github.com/cybertec-postgresql/vip-manager/security)
+of this repository and choose *Report a vulnerability*. That creates a private
+advisory only the maintainers can see.
+
+Helpful in a report:
+
+- the version of vip-manager and the `manager-type` and `dcs-type` in use
+- what an attacker gains, and what access they need to get there
+- the configuration needed to reproduce it, with credentials removed
+
+You will get an acknowledgement of the report, and we will let you know when a
+fix is released. If you would like to be credited in the advisory, say so in
+the report.
+
+## Out of scope
+
+- vulnerabilities in etcd, consul, Patroni or PostgreSQL themselves - please
+  report those to the respective project
+- findings that require an attacker to already be root on the machine that
+  runs vip-manager, since the daemon runs with those privileges anyway

checker/etcd_leader_checker.go

@@ -15,9 +15,15 @@ import (
 )
 
 // EtcdLeaderChecker is used to check state of the leader key in Etcd
+//
+// The configuration and the client are named fields on purpose: embedding both
+// of them put their methods and fields into one namespace, where elc.client.Get() and
+// elc.client.Close() silently belonged to the client while elc.conf.Logger belonged to the
+// configuration, and any name they come to share in a future release of either
+// would break the build in a place far away from the cause.
 type EtcdLeaderChecker struct {
-	*vipconfig.Config
-	*clientv3.Client
+	conf     *vipconfig.Config
+	client   *clientv3.Client
 	getLog   *logThrottler // throttles repeated failures to read the key
 	watchLog *logThrottler // throttles repeated failures of the WATCH
 }
@@ -42,13 +48,18 @@ func NewEtcdLeaderChecker(conf *vipconfig.Config) (*EtcdLeaderChecker, error) {
 		return nil, fmt.Errorf("failed to connect to etcd at endpoints %v: %w", conf.Endpoints, err)
 	}
 	return &EtcdLeaderChecker{
-		Config:   conf,
-		Client:   c,
+		conf:     conf,
+		client:   c,
 		getLog:   newLogThrottler(conf.Logger),
 		watchLog: newLogThrottler(conf.Logger),
 	}, nil
 }
 
+// Close closes the connection to etcd
+func (elc *EtcdLeaderChecker) Close() error {
+	return elc.client.Close()
+}
+
 // clientLogger returns the logger handed over to the etcd client. Unless
 // verbose logging is requested, the retry chatter of the client is limited to
 // errors, because it repeats once per scan interval while etcd is unreachable.
@@ -103,43 +114,43 @@ func (elc *EtcdLeaderChecker) get(ctx context.Context, out chan<- bool) {
 	// Bound the request: the etcd client retries until the context expires,
 	// so without a timeout this would block forever while etcd is unreachable
 	// and never report the failure
-	getCtx, cancel := context.WithTimeout(ctx, time.Duration(max(elc.Interval, 1000))*time.Millisecond)
+	getCtx, cancel := context.WithTimeout(ctx, time.Duration(max(elc.conf.Interval, 1000))*time.Millisecond)
 	defer cancel()
-	resp, err := elc.Get(getCtx, elc.TriggerKey)
+	resp, err := elc.client.Get(getCtx, elc.conf.TriggerKey)
 	if err != nil {
 		elc.getLog.error("Failed to get value from etcd",
-			zap.String("key", elc.TriggerKey),
+			zap.String("key", elc.conf.TriggerKey),
 			zap.Error(err))
 		send(false)
 		return
 	}
 	if resp == nil {
-		elc.getLog.error("Received nil response from etcd", zap.String("key", elc.TriggerKey))
+		elc.getLog.error("Received nil response from etcd", zap.String("key", elc.conf.TriggerKey))
 		send(false)
 		return
 	}
 	if len(resp.Kvs) == 0 {
 		elc.getLog.info("No value found for the key - DCS may not have set it yet",
-			zap.String("key", elc.TriggerKey))
+			zap.String("key", elc.conf.TriggerKey))
 		send(false)
 		return
 	}
-	elc.getLog.success("Successfully read the value from etcd again", zap.String("key", elc.TriggerKey))
+	elc.getLog.success("Successfully read the value from etcd again", zap.String("key", elc.conf.TriggerKey))
 	for _, kv := range resp.Kvs {
 		value := string(kv.Value)
-		matches := value == elc.TriggerValue
-		elc.Logger.Sugar().Info("Current value from DCS:", value)
+		matches := value == elc.conf.TriggerValue
+		elc.conf.Logger.Sugar().Info("Current value from DCS:", value)
 		send(matches)
 	}
 }
 
 // watch monitors value changes from etcd
 func (elc *EtcdLeaderChecker) watch(ctx context.Context, out chan<- bool) error {
-	elc.Logger.Sugar().Info("Setting WATCH on ", elc.TriggerKey)
+	elc.conf.Logger.Sugar().Info("Setting WATCH on ", elc.conf.TriggerKey)
 	// WithRequireLeader makes the watch fail fast when the etcd server
 	// loses its quorum instead of silently returning no events
 	watchCtx := clientv3.WithRequireLeader(ctx)
-	watchChan := elc.Watch(watchCtx, elc.TriggerKey)
+	watchChan := elc.client.Watch(watchCtx, elc.conf.TriggerKey)
 	for {
 		select {
 		case <-ctx.Done():
@@ -154,28 +165,28 @@ func (elc *EtcdLeaderChecker) watch(ctx context.Context, out chan<- bool) error
 					watchErr = watchResp.Err()
 				}
 				elc.watchLog.error("WATCH on key lost, re-establishing and re-syncing state",
-					zap.String("key", elc.TriggerKey),
+					zap.String("key", elc.conf.TriggerKey),
 					zap.Error(watchErr))
 				// Back off briefly to avoid a busy loop when etcd is unreachable
 				select {
 				case <-time.After(time.Second):
 				case <-ctx.Done():
 					return ctx.Err()
 				}
-				watchChan = elc.Watch(watchCtx, elc.TriggerKey)
+				watchChan = elc.client.Watch(watchCtx, elc.conf.TriggerKey)
 				// re-establishing is already reported above, so this merely
 				// confirms it and stays out of the way during an outage
-				elc.Logger.Sugar().Debug("Resetting cancelled WATCH on ", elc.TriggerKey)
+				elc.conf.Logger.Sugar().Debug("Resetting cancelled WATCH on ", elc.conf.TriggerKey)
 				// Re-fetch the current value: events may have been missed
 				// while the watch was down (e.g. a leader change)
 				elc.get(ctx, out)
 				continue
 			}
-			elc.watchLog.success("WATCH on key is working again", zap.String("key", elc.TriggerKey))
+			elc.watchLog.success("WATCH on key is working again", zap.String("key", elc.conf.TriggerKey))
 			for _, event := range watchResp.Events {
 				select {
-				case out <- string(event.Kv.Value) == elc.TriggerValue:
-					elc.Logger.Sugar().Info("Current value from DCS: ", string(event.Kv.Value))
+				case out <- string(event.Kv.Value) == elc.conf.TriggerValue:
+					elc.conf.Logger.Sugar().Info("Current value from DCS: ", string(event.Kv.Value))
 				case <-ctx.Done():
 					return ctx.Err()
 				}
@@ -186,7 +197,7 @@ func (elc *EtcdLeaderChecker) watch(ctx context.Context, out chan<- bool) error
 
 // GetChangeNotificationStream monitors the leader in etcd
 func (elc *EtcdLeaderChecker) GetChangeNotificationStream(ctx context.Context, out chan<- bool) error {
-	defer elc.Close()
+	defer func() { _ = elc.Close() }()
 	go elc.get(ctx, out)
 	wctx, cancel := context.WithCancel(ctx)
 	defer cancel()

checker/etcd_leader_checker_test.go

@@ -529,7 +529,7 @@ func TestEtcdLeaderChecker_watch_ResyncsOnCanceledWatch(t *testing.T) {
 
 	// Kill all watch streams: closing the client's Watcher closes the watch
 	// channel, simulating a server-side cancellation / dead watch.
-	if err := checker.Watcher.Close(); err != nil {
+	if err := checker.client.Watcher.Close(); err != nil {
 		t.Fatalf("Watcher.Close: %v", err)
 	}
 

main_test.go

@@ -1,67 +1,67 @@
-package main
-
-import (
-	"fmt"
-	"os"
-	"testing"
-)
-
-// TestVersionFlagHandling verifies that the version flag is recognized.
-// This is a basic test of the version flag detection logic without os.Exit.
-func TestVersionFlagHandling(t *testing.T) {
-	// Test the version flag detection logic
-	tests := []struct {
-		args        []string
-		shouldMatch bool
-		name        string
-	}{
-		{[]string{"vip-manager", "--version"}, true, "version flag present"},
-		{[]string{"vip-manager", "--help"}, false, "help flag present"},
-		{[]string{"vip-manager"}, false, "no flags"},
-		{[]string{"vip-manager", "--config", "test.yml"}, false, "config flag present"},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			// Replicate the main() version flag logic
-			isVersion := (len(tt.args) > 1) && (tt.args[1] == "--version")
-			if isVersion != tt.shouldMatch {
-				t.Errorf("expected isVersion=%v, got %v for args %v", tt.shouldMatch, isVersion, tt.args)
-			}
-		})
-	}
-}
-
-// TestVersionFlagOutput verifies the version output format.
-func TestVersionFlagOutput(t *testing.T) {
-	// Save original stdout
-	oldStdout := os.Stdout
-	defer func() { os.Stdout = oldStdout }()
-
-	// Create a pipe to capture output
-	_, w, err := os.Pipe()
-	if err != nil {
-		t.Fatalf("failed to create pipe: %v", err)
-	}
-	os.Stdout = w
-
-	// Simulate version output
-	version := "master"
-	commit := "none"
-	date := "unknown"
-
-	fmt.Printf("version: %s\n", version)
-	fmt.Printf("commit:  %s\n", commit)
-	fmt.Printf("date:    %s\n", date)
-
-	w.Close()
-
-	// Restore stdout
-	os.Stdout = oldStdout
-
-	// In a real test, we would read from the pipe
-	// For simplicity, just verify the format is correct
-	if version != "master" || commit != "none" || date != "unknown" {
-		t.Error("version output format incorrect")
-	}
-}
+package main
+
+import (
+	"fmt"
+	"os"
+	"testing"
+)
+
+// TestVersionFlagHandling verifies that the version flag is recognized.
+// This is a basic test of the version flag detection logic without os.Exit.
+func TestVersionFlagHandling(t *testing.T) {
+	// Test the version flag detection logic
+	tests := []struct {
+		args        []string
+		shouldMatch bool
+		name        string
+	}{
+		{[]string{"vip-manager", "--version"}, true, "version flag present"},
+		{[]string{"vip-manager", "--help"}, false, "help flag present"},
+		{[]string{"vip-manager"}, false, "no flags"},
+		{[]string{"vip-manager", "--config", "test.yml"}, false, "config flag present"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Replicate the main() version flag logic
+			isVersion := (len(tt.args) > 1) && (tt.args[1] == "--version")
+			if isVersion != tt.shouldMatch {
+				t.Errorf("expected isVersion=%v, got %v for args %v", tt.shouldMatch, isVersion, tt.args)
+			}
+		})
+	}
+}
+
+// TestVersionFlagOutput verifies the version output format.
+func TestVersionFlagOutput(t *testing.T) {
+	// Save original stdout
+	oldStdout := os.Stdout
+	defer func() { os.Stdout = oldStdout }()
+
+	// Create a pipe to capture output
+	_, w, err := os.Pipe()
+	if err != nil {
+		t.Fatalf("failed to create pipe: %v", err)
+	}
+	os.Stdout = w
+
+	// Simulate version output
+	version := "master"
+	commit := "none"
+	date := "unknown"
+
+	fmt.Printf("version: %s\n", version)
+	fmt.Printf("commit:  %s\n", commit)
+	fmt.Printf("date:    %s\n", date)
+
+	w.Close()
+
+	// Restore stdout
+	os.Stdout = oldStdout
+
+	// In a real test, we would read from the pipe
+	// For simplicity, just verify the format is correct
+	if version != "master" || commit != "none" || date != "unknown" {
+		t.Error("version output format incorrect")
+	}
+}