diff --git a/AGENTS.md b/AGENTS.md index 8666f48..1490095 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,7 +15,7 @@ and exports Prometheus metrics for monitoring backup status and health. - **Logging**: `log/slog` (stdlib, text handler, defaults to stderr) - **Log rotation**: `gopkg.in/natefinch/lumberjack.v2` - **Testing**: `testing` + `github.com/stretchr/testify/assert` -- **Build**: GoReleaser, CGO_ENABLED=0, Linux only (amd64/arm64) +- **Build**: GoReleaser, CGO_ENABLED=0, Linux only (amd64/arm/arm64) - Avoid `github.com/sirupsen/logrus` (blocked by depguard linter) ## Project Structure @@ -39,9 +39,14 @@ and exports Prometheus metrics for monitoring backup status and health. ### Configuration (config/config.go) - CLI flag parsing: `-c/--config` (required), `-p/--port` (default 9289), `-V/--version` -- YAML configuration parsing using Koanf - Support for multiple backup definitions with URL, username, password, outputFile - Logging configuration per logger/LogOptions +- Use Koanf for all YAML parsing +- Use spf13/pflag for CLI flag parsing +- Provide sensible defaults for all config options +- Validate configuration values at startup +- Support both short and long CLI flags +- With Koanf, prefer getting typed values than using the Get() method. ### Main Application (main.go) - Prometheus metrics registry for backup status tracking @@ -82,14 +87,6 @@ and exports Prometheus metrics for monitoring backup status and health. - Logger: singleton via `logger.Get()`. - Context: pass `context.Context` to operations that may need cancellation. -### Configuration Management -- Use Koanf for all YAML parsing -- Use spf13/pflag for CLI flag parsing -- Provide sensible defaults for all config options -- Validate configuration values at startup -- Support both short and long CLI flags -- With Koanf, prefer getting typed values than using the Get() method. - ### Environment Variables - Use optional environment variables for configuration (e.g., `BRF_BACKUP_DIR`...) - Environment variables should override values from the config file @@ -102,6 +99,7 @@ and exports Prometheus metrics for monitoring backup status and health. - Register metrics immediately after creation - Assign labels consistently (e.g., "id" for backup identifiers) - Document what each metric represents in the Help text +- `backup_nb` counts every retrieval round (full runs **and** retry batches), not individual files or successful backups ### Error Handling - Use slog for error logging with context @@ -125,6 +123,17 @@ and exports Prometheus metrics for monitoring backup status and health. - Test data files must be placed in the `testdata/` directory - Unused testdata files must be removed +## HTTP Client Design + +The application creates a new HTTP client per request in `main.go`. This design is intentional and well-suited for this workload because: + +1. **Periodic backups**: Backups typically run every 24 hours or so, so connection pooling benefits are minimal +2. **Different hosts**: Each backup may target a different host, making a per-host pool impractical +3. **Isolation**: Fresh clients provide clean state per backup operation, avoiding resource leaks +4. **Simplicity**: Simpler code without complex connection management for edge cases + +This approach avoids the complexity of maintaining connection pools while still being performant for the usage pattern. + ## Common Tasks ### Adding a New Configuration Option diff --git a/config/config.go b/config/config.go index 8730ff4..50b16b3 100644 --- a/config/config.go +++ b/config/config.go @@ -271,7 +271,7 @@ func (c *Config) readBackups(k *koanf.Koanf, l *logger.Logger) error { return err } c.Backups = append(c.Backups, backup) - l.Info("Config: backup url", slog.String("url", backup.URL)) + l.Info("Config: backup url", slog.String("url", logger.RedactURL(backup.URL))) } return nil } diff --git a/logger/logger.go b/logger/logger.go index 7f90218..a9c7f67 100644 --- a/logger/logger.go +++ b/logger/logger.go @@ -9,6 +9,7 @@ import ( "io" "log" "log/slog" + "net/url" "os" "strings" "sync" @@ -166,3 +167,14 @@ func FromCtx(ctx context.Context) *Logger { func WithCtx(ctx context.Context, l *Logger) context.Context { return context.WithValue(ctx, ctxKey{}, l) } + +// RedactURL returns rawURL with any userinfo (username/password) stripped, +// so it can be logged without leaking credentials. +func RedactURL(rawURL string) string { + u, err := url.Parse(rawURL) + if err != nil { + return rawURL + } + u.User = nil + return u.String() +} diff --git a/logger/logger_test.go b/logger/logger_test.go index 385ffe5..c4768b0 100644 --- a/logger/logger_test.go +++ b/logger/logger_test.go @@ -223,3 +223,32 @@ func TestGet_RaceCondition(t *testing.T) { } wg.Wait() } + +func TestRedactURL(t *testing.T) { + tests := []struct { + name string + rawURL string + want string + }{ + { //nolint:gosec + name: "strips credentials", + rawURL: "https://user:secret@example.com/path", + want: "https://example.com/path", //nolint:goconst + }, + { + name: "no credentials unchanged", + rawURL: "https://example.com/path", + want: "https://example.com/path", + }, + { + name: "unparseable returned as-is", + rawURL: "://not a url", + want: "://not a url", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, RedactURL(tt.rawURL)) + }) + } +} diff --git a/main.go b/main.go index 379ccf8..d32276f 100644 --- a/main.go +++ b/main.go @@ -14,6 +14,7 @@ import ( "net" "net/http" "os" + "os/signal" "runtime" "strconv" "strings" @@ -87,7 +88,7 @@ func NewMetrics(reg prometheus.Registerer, namespace string) *metrics { prometheus.CounterOpts{ Namespace: namespace, Name: "backup_nb", - Help: "Number of retrievals", + Help: "Number of retrieval rounds, including retries", }, ), } @@ -129,6 +130,19 @@ type fsError struct{ error } func (e *fsError) Unwrap() error { return e.error } +// newClient builds an HTTP client for a single request. It is a variable so +// tests can substitute a fake transport. +var newClient = func(timeout time.Duration) *http.Client { + return &http.Client{ + Timeout: timeout, + Transport: &http.Transport{ + DialContext: (&net.Dialer{Timeout: timeout}).DialContext, + TLSHandshakeTimeout: timeout, + ResponseHeaderTimeout: timeout, + }, + } +} + func fetchURL( ctx context.Context, url, username, password string, @@ -143,14 +157,7 @@ func fetchURL( // Create a new client per request — these are periodic backups // (typically every 24h), so connection pooling is not beneficial // and each backup may target a different host. - client := &http.Client{ - Timeout: timeout, - Transport: &http.Transport{ - DialContext: (&net.Dialer{Timeout: timeout}).DialContext, - TLSHandshakeTimeout: timeout, - ResponseHeaderTimeout: timeout, - }, - } + client := newClient(timeout) resp, err := client.Do(req) if err != nil { @@ -159,14 +166,15 @@ func fetchURL( if resp.StatusCode >= http.StatusMultipleChoices { resp.Body.Close() - return nil, &httpError{error: errors.New("HTTP status >= 300")} + return nil, &httpError{error: fmt.Errorf("unexpected status %d %s", resp.StatusCode, resp.Status)} } return resp, nil } func saveToFile(r io.Reader, outputFile string) (int64, error) { - f, err := os.Create(outputFile + ".part") + partFile := outputFile + ".part" + f, err := os.Create(partFile) if err != nil { return 0, &fsError{error: err} } @@ -174,10 +182,12 @@ func saveToFile(r io.Reader, outputFile string) (int64, error) { written, err := io.Copy(f, r) if err != nil { + _ = os.Remove(partFile) return 0, &fsError{error: err} } - if err := os.Rename(outputFile+".part", outputFile); err != nil { + if err := os.Rename(partFile, outputFile); err != nil { + _ = os.Remove(partFile) return 0, &fsError{error: err} } @@ -195,7 +205,7 @@ func backupFile( if err != nil { l.Error("Failed to fetch URL", slog.String("id", id), - slog.String("url", url), + slog.String("url", logger.RedactURL(url)), slog.Any("error", err), ) return 0, err @@ -367,8 +377,11 @@ func run(ctx context.Context) error { } func main() { - if err := run(context.Background()); err != nil { - if !errors.Is(err, flag.ErrHelp) { + ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt) + + if err := run(ctx); err != nil { + cancel() + if !errors.Is(err, flag.ErrHelp) && !errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded) { fmt.Fprintf(os.Stderr, "error: %v\n", err) os.Exit(1) } diff --git a/main_test.go b/main_test.go index a949867..8b144e0 100644 --- a/main_test.go +++ b/main_test.go @@ -76,9 +76,7 @@ func TestRetrieveUrls_TargetIsDirectory(t *testing.T) { status := newBackupStatus(cfg.Backups) r := retrieveUrls(t.Context(), cfg, m, status, true) - if !assert.FileExists(t, outputFilename+".part") { - return - } + assert.NoFileExists(t, outputFilename+".part") assert.True(t, r) } @@ -775,3 +773,34 @@ func TestRunBackupLoop_Retry(t *testing.T) { t.Fatal("run() did not return after cancellation") } } + +// roundTripFunc adapts a function to an http.RoundTripper for tests. +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + +// failCloseBody is an io.ReadCloser whose Close always returns an error, +// used to exercise the body-close error path in backupFile. +type failCloseBody struct{} + +func (failCloseBody) Read(_ []byte) (int, error) { return 0, io.EOF } + +func (failCloseBody) Close() error { return errors.New("close failed") } + +func TestBackupFile_BodyCloseError(t *testing.T) { + testutil.UseTempDir(t) + + origNewClient := newClient + newClient = func(time.Duration) *http.Client { + return &http.Client{ + Transport: roundTripFunc(func(_ *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Body: failCloseBody{}}, nil + }), + } + } + t.Cleanup(func() { newClient = origNewClient }) + + _, err := backupFile(t.Context(), "test", "http://example.com", "", "", "test.out", 10*time.Second) + assert.NoError(t, err) + assert.FileExists(t, "test.out") +} diff --git a/testutil/testutil.go b/testutil/testutil.go index 89249b7..9b9651b 100644 --- a/testutil/testutil.go +++ b/testutil/testutil.go @@ -8,19 +8,35 @@ import ( "testing" ) -func UseTempDir(t *testing.T) { +var ( + getwd = os.Getwd + chdir = os.Chdir +) + +// UseTempDir changes the working directory to a fresh temporary directory for +// the duration of the test, restoring the original directory on cleanup. +func UseTempDir(t testing.TB) { t.Helper() + if err := useTempDir(t); err != nil { + t.Fatal(err) + } +} + +// useTempDir performs the directory switch and returns an error instead of +// aborting, so its failure paths can be tested without failing the test. +func useTempDir(t testing.TB) error { dir := t.TempDir() - oldDir, err := os.Getwd() + oldDir, err := getwd() if err != nil { - t.Fatal(err) + return err } - if err := os.Chdir(dir); err != nil { - t.Fatal(err) + if err := chdir(dir); err != nil { + return err } t.Cleanup(func() { - if err := os.Chdir(oldDir); err != nil { + if err := chdir(oldDir); err != nil { t.Logf("failed to restore working directory: %v", err) } }) + return nil } diff --git a/testutil/testutil_test.go b/testutil/testutil_test.go index 51632bc..273f47c 100644 --- a/testutil/testutil_test.go +++ b/testutil/testutil_test.go @@ -4,6 +4,7 @@ package testutil import ( + "errors" "os" "testing" ) @@ -67,3 +68,62 @@ func TestUseTempDir_CleanupLog(t *testing.T) { t.Errorf("working directory was not restored: got %q, want %q", restoredDir, originalDir) } } + +func TestUseTempDir_ErrorGetwd(t *testing.T) { + origGetwd := getwd + getwd = func() (string, error) { return "", errors.New("getwd failed") } + t.Cleanup(func() { getwd = origGetwd }) + + if err := useTempDir(t); err == nil { + t.Error("expected error when getwd fails") + } +} + +func TestUseTempDir_ErrorChdir(t *testing.T) { + origChdir := chdir + chdir = func(string) error { return errors.New("chdir failed") } + t.Cleanup(func() { chdir = origChdir }) + + if err := useTempDir(t); err == nil { + t.Error("expected error when chdir fails") + } +} + +func TestUseTempDir_ErrorRestore(t *testing.T) { + origChdir := chdir + calls := 0 + chdir = func(path string) error { + calls++ + err := origChdir(path) + if calls > 1 { + return errors.New("restore failed") + } + return err + } + t.Cleanup(func() { chdir = origChdir }) + + UseTempDir(t) +} + +// fakeTB is a testing.TB whose Fatal records the call instead of aborting, +// so the failure path of UseTempDir can be exercised without failing a test. +type fakeTB struct { + testing.TB + fatalCalled bool +} + +func (f *fakeTB) Fatal(_ ...any) { + f.fatalCalled = true +} + +func TestUseTempDir_Fatal(t *testing.T) { + origGetwd := getwd + getwd = func() (string, error) { return "", errors.New("getwd failed") } + t.Cleanup(func() { getwd = origGetwd }) + + ft := &fakeTB{TB: t} + UseTempDir(ft) + if !ft.fatalCalled { + t.Error("expected UseTempDir to call Fatal when useTempDir returns an error") + } +}