From f1a63e87dc9895b14e75e75b4695249a3929b0c8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Mar 2026 11:41:21 +0000 Subject: [PATCH 1/7] Initial plan From 042aa374de93eba567e66e687dd30726534f2413 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Mar 2026 11:46:59 +0000 Subject: [PATCH 2/7] fix: tolerate default health port conflict Co-authored-by: tma <4719+tma@users.noreply.github.com> --- README.md | 3 +++ SPEC.md | 1 + cmd/mbproxy/main.go | 40 +++++++++++++++++++++++++++++---------- internal/config/config.go | 4 +++- 4 files changed, 37 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 48cb48b..4112a80 100644 --- a/README.md +++ b/README.md @@ -37,8 +37,11 @@ All configuration is via environment variables: | `MODBUS_REQUEST_DELAY` | Delay after each upstream request | `0` (disabled) | | `MODBUS_CONNECT_DELAY` | Silent period after connecting to upstream | `0` (disabled) | | `MODBUS_SHUTDOWN_TIMEOUT` | Graceful shutdown timeout | `30s` | +| `HEALTH_LISTEN` | HTTP health endpoint listen address | `:8080` | | `LOG_LEVEL` | Log level: `INFO`, `DEBUG` | `INFO` | +If the default health listener (`:8080`) is already in use, mbproxy keeps the proxy running and disables the health endpoint for that instance. Set `HEALTH_LISTEN` to another free address if you need a health endpoint per instance. + ### Read-Only Modes - `false`: Full read/write passthrough to upstream device diff --git a/SPEC.md b/SPEC.md index 701a651..674c3f7 100644 --- a/SPEC.md +++ b/SPEC.md @@ -106,6 +106,7 @@ Three modes: | `MODBUS_REQUEST_DELAY` | Delay after each upstream request | `0` (disabled) | `100ms`, `500ms` | | `MODBUS_CONNECT_DELAY` | Silent period after connecting to upstream | `0` (disabled) | `500ms`, `2s` | | `MODBUS_SHUTDOWN_TIMEOUT` | Graceful shutdown timeout | `30s` | `10s`, `60s` | +| `HEALTH_LISTEN` | HTTP health endpoint listen address | `:8080` | `:8080`, `127.0.0.1:18080` | | `LOG_LEVEL` | Log level | `INFO` | `INFO`, `DEBUG` | ## Implementation Details diff --git a/cmd/mbproxy/main.go b/cmd/mbproxy/main.go index 22c40f5..af97e11 100644 --- a/cmd/mbproxy/main.go +++ b/cmd/mbproxy/main.go @@ -2,9 +2,11 @@ package main import ( "context" + "errors" "flag" "fmt" "log/slog" + "net" "os" "os/signal" "syscall" @@ -52,16 +54,18 @@ func main() { // Start health server hs := health.NewServer(cfg.HealthListen, p, logger) - hsLn, err := hs.Listen() + hsLn, err := listenHealthServer(hs, cfg.HealthListen, cfg.HealthListen == config.DefaultHealthListen, logger) if err != nil { logger.Error("failed to start health server", "error", err) os.Exit(1) } - go func() { - if err := hs.Serve(hsLn); err != nil { - logger.Error("health server error", "error", err) - } - }() + if hsLn != nil { + go func() { + if err := hs.Serve(hsLn); err != nil { + logger.Error("health server error", "error", err) + } + }() + } // Start proxy in background errCh := make(chan error, 1) @@ -83,10 +87,12 @@ func main() { // Graceful shutdown cancel() - shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 5*time.Second) - defer shutdownCancel() - if err := hs.Shutdown(shutdownCtx); err != nil { - logger.Error("health server shutdown error", "error", err) + if hsLn != nil { + shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer shutdownCancel() + if err := hs.Shutdown(shutdownCtx); err != nil { + logger.Error("health server shutdown error", "error", err) + } } if err := p.Shutdown(cfg.ShutdownTimeout); err != nil { @@ -96,3 +102,17 @@ func main() { logger.Info("shutdown complete") } + +func listenHealthServer(hs *health.Server, addr string, allowAddrInUseFallback bool, logger *slog.Logger) (net.Listener, error) { + ln, err := hs.Listen() + if err == nil { + return ln, nil + } + + if allowAddrInUseFallback && errors.Is(err, syscall.EADDRINUSE) { + logger.Warn("health server disabled: listen address already in use", "addr", addr, "error", err) + return nil, nil + } + + return nil, err +} diff --git a/internal/config/config.go b/internal/config/config.go index 80a38cf..43da640 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -34,6 +34,8 @@ type Config struct { LogLevel string } +const DefaultHealthListen = ":8080" + // Load reads configuration from environment variables. func Load() (*Config, error) { cfg := &Config{ @@ -47,7 +49,7 @@ func Load() (*Config, error) { RequestDelay: 0, ConnectDelay: 0, ShutdownTimeout: 30 * time.Second, - HealthListen: GetEnv("HEALTH_LISTEN", ":8080"), + HealthListen: GetEnv("HEALTH_LISTEN", DefaultHealthListen), LogLevel: GetEnv("LOG_LEVEL", "INFO"), } From bb27315c45dda575e13f18335c076e6827c59546 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Mar 2026 11:51:44 +0000 Subject: [PATCH 3/7] refactor: make health checks internal Co-authored-by: tma <4719+tma@users.noreply.github.com> --- .gitignore | 2 +- Dockerfile | 2 -- README.md | 3 +- SPEC.md | 3 +- cmd/mbproxy/main.go | 52 ++++++++---------------------- cmd/mbproxy/main_test.go | 59 ++++++++++++++++++++++++++++++++++ internal/config/config.go | 4 --- internal/config/config_test.go | 9 ------ 8 files changed, 77 insertions(+), 57 deletions(-) create mode 100644 cmd/mbproxy/main_test.go diff --git a/.gitignore b/.gitignore index 87636fc..d49d582 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,5 @@ # Build artifacts -mbproxy +/mbproxy *.exe # Go specific diff --git a/Dockerfile b/Dockerfile index 8576f26..1a26c59 100644 --- a/Dockerfile +++ b/Dockerfile @@ -31,8 +31,6 @@ FROM scratch COPY --from=builder /app/mbproxy /mbproxy -EXPOSE 8080 - HEALTHCHECK --interval=5s --timeout=3s --start-period=10s --retries=3 \ CMD ["/mbproxy", "-health"] diff --git a/README.md b/README.md index 4112a80..96533fe 100644 --- a/README.md +++ b/README.md @@ -37,10 +37,9 @@ All configuration is via environment variables: | `MODBUS_REQUEST_DELAY` | Delay after each upstream request | `0` (disabled) | | `MODBUS_CONNECT_DELAY` | Silent period after connecting to upstream | `0` (disabled) | | `MODBUS_SHUTDOWN_TIMEOUT` | Graceful shutdown timeout | `30s` | -| `HEALTH_LISTEN` | HTTP health endpoint listen address | `:8080` | | `LOG_LEVEL` | Log level: `INFO`, `DEBUG` | `INFO` | -If the default health listener (`:8080`) is already in use, mbproxy keeps the proxy running and disables the health endpoint for that instance. Set `HEALTH_LISTEN` to another free address if you need a health endpoint per instance. +`/mbproxy -health` performs an internal upstream connectivity check and does not open a separate local TCP health port. ### Read-Only Modes diff --git a/SPEC.md b/SPEC.md index 674c3f7..aa61fd1 100644 --- a/SPEC.md +++ b/SPEC.md @@ -106,9 +106,10 @@ Three modes: | `MODBUS_REQUEST_DELAY` | Delay after each upstream request | `0` (disabled) | `100ms`, `500ms` | | `MODBUS_CONNECT_DELAY` | Silent period after connecting to upstream | `0` (disabled) | `500ms`, `2s` | | `MODBUS_SHUTDOWN_TIMEOUT` | Graceful shutdown timeout | `30s` | `10s`, `60s` | -| `HEALTH_LISTEN` | HTTP health endpoint listen address | `:8080` | `:8080`, `127.0.0.1:18080` | | `LOG_LEVEL` | Log level | `INFO` | `INFO`, `DEBUG` | +The container health check runs `mbproxy -health`, which performs an internal upstream connectivity check without binding a separate local TCP port. + ## Implementation Details ### Dependencies diff --git a/cmd/mbproxy/main.go b/cmd/mbproxy/main.go index af97e11..050ebfe 100644 --- a/cmd/mbproxy/main.go +++ b/cmd/mbproxy/main.go @@ -2,19 +2,16 @@ package main import ( "context" - "errors" "flag" "fmt" "log/slog" - "net" "os" "os/signal" "syscall" - "time" "github.com/tma/mbproxy/internal/config" - "github.com/tma/mbproxy/internal/health" "github.com/tma/mbproxy/internal/logging" + "github.com/tma/mbproxy/internal/modbus" "github.com/tma/mbproxy/internal/proxy" ) @@ -23,8 +20,7 @@ func main() { flag.Parse() if *healthCheck { - addr := config.GetEnv("HEALTH_LISTEN", ":8080") - if err := health.CheckHealth(addr); err != nil { + if err := runHealthCheck(); err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(1) } @@ -52,21 +48,6 @@ func main() { os.Exit(1) } - // Start health server - hs := health.NewServer(cfg.HealthListen, p, logger) - hsLn, err := listenHealthServer(hs, cfg.HealthListen, cfg.HealthListen == config.DefaultHealthListen, logger) - if err != nil { - logger.Error("failed to start health server", "error", err) - os.Exit(1) - } - if hsLn != nil { - go func() { - if err := hs.Serve(hsLn); err != nil { - logger.Error("health server error", "error", err) - } - }() - } - // Start proxy in background errCh := make(chan error, 1) go func() { @@ -87,14 +68,6 @@ func main() { // Graceful shutdown cancel() - if hsLn != nil { - shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 5*time.Second) - defer shutdownCancel() - if err := hs.Shutdown(shutdownCtx); err != nil { - logger.Error("health server shutdown error", "error", err) - } - } - if err := p.Shutdown(cfg.ShutdownTimeout); err != nil { logger.Error("shutdown error", "error", err) os.Exit(1) @@ -103,16 +76,19 @@ func main() { logger.Info("shutdown complete") } -func listenHealthServer(hs *health.Server, addr string, allowAddrInUseFallback bool, logger *slog.Logger) (net.Listener, error) { - ln, err := hs.Listen() - if err == nil { - return ln, nil +func runHealthCheck() error { + cfg, err := config.Load() + if err != nil { + return err } - if allowAddrInUseFallback && errors.Is(err, syscall.EADDRINUSE) { - logger.Warn("health server disabled: listen address already in use", "addr", addr, "error", err) - return nil, nil - } + return checkUpstreamHealth(cfg, logging.New(cfg.LogLevel)) +} - return nil, err +func checkUpstreamHealth(cfg *config.Config, logger *slog.Logger) error { + client := modbus.NewClient(cfg.Upstream, cfg.Timeout, cfg.RequestDelay, cfg.ConnectDelay, logger) + if err := client.Connect(); err != nil { + return err + } + return client.Close() } diff --git a/cmd/mbproxy/main_test.go b/cmd/mbproxy/main_test.go new file mode 100644 index 0000000..d305c12 --- /dev/null +++ b/cmd/mbproxy/main_test.go @@ -0,0 +1,59 @@ +package main + +import ( + "io" + "log/slog" + "net" + "testing" + "time" + + "github.com/tma/mbproxy/internal/config" +) + +func testLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +func TestCheckUpstreamHealth_Success(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("failed to listen: %v", err) + } + defer ln.Close() + + acceptDone := make(chan struct{}) + go func() { + defer close(acceptDone) + conn, err := ln.Accept() + if err == nil { + conn.Close() + } + }() + + cfg := &config.Config{ + Upstream: ln.Addr().String(), + Timeout: time.Second, + } + if err := checkUpstreamHealth(cfg, testLogger()); err != nil { + t.Fatalf("expected health check to succeed, got %v", err) + } + + <-acceptDone +} + +func TestCheckUpstreamHealth_Failure(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("failed to reserve port: %v", err) + } + addr := ln.Addr().String() + ln.Close() + + cfg := &config.Config{ + Upstream: addr, + Timeout: 100 * time.Millisecond, + } + if err := checkUpstreamHealth(cfg, testLogger()); err == nil { + t.Fatal("expected health check to fail") + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 43da640..1c86d1b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -30,12 +30,9 @@ type Config struct { RequestDelay time.Duration ConnectDelay time.Duration ShutdownTimeout time.Duration - HealthListen string LogLevel string } -const DefaultHealthListen = ":8080" - // Load reads configuration from environment variables. func Load() (*Config, error) { cfg := &Config{ @@ -49,7 +46,6 @@ func Load() (*Config, error) { RequestDelay: 0, ConnectDelay: 0, ShutdownTimeout: 30 * time.Second, - HealthListen: GetEnv("HEALTH_LISTEN", DefaultHealthListen), LogLevel: GetEnv("LOG_LEVEL", "INFO"), } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 6a85e69..55cc946 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -19,7 +19,6 @@ func TestLoad_Defaults(t *testing.T) { os.Unsetenv("MODBUS_READONLY") os.Unsetenv("MODBUS_TIMEOUT") os.Unsetenv("MODBUS_SHUTDOWN_TIMEOUT") - os.Unsetenv("HEALTH_LISTEN") os.Unsetenv("LOG_LEVEL") cfg, err := Load() @@ -57,9 +56,6 @@ func TestLoad_Defaults(t *testing.T) { if cfg.ShutdownTimeout != 30*time.Second { t.Errorf("expected 30s shutdown timeout, got %v", cfg.ShutdownTimeout) } - if cfg.HealthListen != ":8080" { - t.Errorf("expected :8080, got %s", cfg.HealthListen) - } if cfg.LogLevel != "INFO" { t.Errorf("expected INFO log level, got %s", cfg.LogLevel) } @@ -85,7 +81,6 @@ func TestLoad_CustomValues(t *testing.T) { os.Setenv("MODBUS_REQUEST_DELAY", "100ms") os.Setenv("MODBUS_CONNECT_DELAY", "200ms") os.Setenv("MODBUS_SHUTDOWN_TIMEOUT", "60s") - os.Setenv("HEALTH_LISTEN", ":9090") os.Setenv("LOG_LEVEL", "DEBUG") defer func() { @@ -99,7 +94,6 @@ func TestLoad_CustomValues(t *testing.T) { os.Unsetenv("MODBUS_REQUEST_DELAY") os.Unsetenv("MODBUS_CONNECT_DELAY") os.Unsetenv("MODBUS_SHUTDOWN_TIMEOUT") - os.Unsetenv("HEALTH_LISTEN") os.Unsetenv("LOG_LEVEL") }() @@ -135,9 +129,6 @@ func TestLoad_CustomValues(t *testing.T) { if cfg.ShutdownTimeout != 60*time.Second { t.Errorf("expected 60s shutdown timeout, got %v", cfg.ShutdownTimeout) } - if cfg.HealthListen != ":9090" { - t.Errorf("expected :9090, got %s", cfg.HealthListen) - } if cfg.LogLevel != "DEBUG" { t.Errorf("expected DEBUG log level, got %s", cfg.LogLevel) } From 4147d8fe4f515c90b7f11023b7d50971cdeebd90 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Mar 2026 11:53:31 +0000 Subject: [PATCH 4/7] chore: address review follow-ups Co-authored-by: tma <4719+tma@users.noreply.github.com> --- cmd/mbproxy/main.go | 14 +++++++++----- cmd/mbproxy/main_test.go | 6 +++--- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/cmd/mbproxy/main.go b/cmd/mbproxy/main.go index 050ebfe..ebe21a0 100644 --- a/cmd/mbproxy/main.go +++ b/cmd/mbproxy/main.go @@ -85,10 +85,14 @@ func runHealthCheck() error { return checkUpstreamHealth(cfg, logging.New(cfg.LogLevel)) } -func checkUpstreamHealth(cfg *config.Config, logger *slog.Logger) error { +func checkUpstreamHealth(cfg *config.Config, logger *slog.Logger) (err error) { client := modbus.NewClient(cfg.Upstream, cfg.Timeout, cfg.RequestDelay, cfg.ConnectDelay, logger) - if err := client.Connect(); err != nil { - return err - } - return client.Close() + defer func() { + closeErr := client.Close() + if err == nil && closeErr != nil { + err = closeErr + } + }() + + return client.Connect() } diff --git a/cmd/mbproxy/main_test.go b/cmd/mbproxy/main_test.go index d305c12..0bc9260 100644 --- a/cmd/mbproxy/main_test.go +++ b/cmd/mbproxy/main_test.go @@ -10,7 +10,7 @@ import ( "github.com/tma/mbproxy/internal/config" ) -func testLogger() *slog.Logger { +func newTestLogger() *slog.Logger { return slog.New(slog.NewTextHandler(io.Discard, nil)) } @@ -34,7 +34,7 @@ func TestCheckUpstreamHealth_Success(t *testing.T) { Upstream: ln.Addr().String(), Timeout: time.Second, } - if err := checkUpstreamHealth(cfg, testLogger()); err != nil { + if err := checkUpstreamHealth(cfg, newTestLogger()); err != nil { t.Fatalf("expected health check to succeed, got %v", err) } @@ -53,7 +53,7 @@ func TestCheckUpstreamHealth_Failure(t *testing.T) { Upstream: addr, Timeout: 100 * time.Millisecond, } - if err := checkUpstreamHealth(cfg, testLogger()); err == nil { + if err := checkUpstreamHealth(cfg, newTestLogger()); err == nil { t.Fatal("expected health check to fail") } } From c3f1356fcafc8f4e943e9c53205e1e84e242fe22 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Mar 2026 12:06:03 +0000 Subject: [PATCH 5/7] chore: slow docker healthcheck cadence Co-authored-by: tma <4719+tma@users.noreply.github.com> --- Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index 1a26c59..abf1edc 100644 --- a/Dockerfile +++ b/Dockerfile @@ -31,7 +31,7 @@ FROM scratch COPY --from=builder /app/mbproxy /mbproxy -HEALTHCHECK --interval=5s --timeout=3s --start-period=10s --retries=3 \ +HEALTHCHECK --interval=30s --timeout=3s --start-period=10s --retries=3 \ CMD ["/mbproxy", "-health"] ENTRYPOINT ["/mbproxy"] From 62d188facef82d23a646287b54fa5befeef5737d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 10 Mar 2026 07:46:14 +0000 Subject: [PATCH 6/7] fix: avoid ghcr push on pr builds Co-authored-by: tma <4719+tma@users.noreply.github.com> --- .github/workflows/docker.yml | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 9a345f4..a185716 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -47,12 +47,25 @@ jobs: type=semver,pattern={{major}}.{{minor}} type=semver,pattern={{major}} + - name: Build image + if: github.event_name == 'pull_request' + uses: docker/build-push-action@v6 + with: + context: . + platforms: linux/amd64,linux/arm64 + push: false + tags: ${{ steps.meta.outputs.tags }} + labels: ${{ steps.meta.outputs.labels }} + cache-from: type=gha + cache-to: type=gha,mode=max + - name: Build and push + if: github.event_name != 'pull_request' uses: docker/build-push-action@v6 with: context: . platforms: linux/amd64,linux/arm64 - push: ${{ github.event_name != 'pull_request' }} + push: true tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }} cache-from: type=registry,ref=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:buildcache From c889f03a6c4c54ef4951e4218f97eb9a60ab771c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 10 Mar 2026 08:12:57 +0000 Subject: [PATCH 7/7] fix: simplify docker workflow cache logic Co-authored-by: tma <4719+tma@users.noreply.github.com> --- .github/workflows/docker.yml | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index a185716..88f10c2 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -17,6 +17,9 @@ jobs: permissions: contents: read packages: write + env: + CACHE_FROM: ${{ github.event_name == 'pull_request' && 'type=gha' || format('type=registry,ref=ghcr.io/{0}:buildcache', github.repository) }} + CACHE_TO: ${{ github.event_name == 'pull_request' && 'type=gha,mode=max' || format('type=registry,ref=ghcr.io/{0}:buildcache,mode=max', github.repository) }} steps: - uses: actions/checkout@v6 @@ -47,26 +50,13 @@ jobs: type=semver,pattern={{major}}.{{minor}} type=semver,pattern={{major}} - - name: Build image - if: github.event_name == 'pull_request' - uses: docker/build-push-action@v6 - with: - context: . - platforms: linux/amd64,linux/arm64 - push: false - tags: ${{ steps.meta.outputs.tags }} - labels: ${{ steps.meta.outputs.labels }} - cache-from: type=gha - cache-to: type=gha,mode=max - - name: Build and push - if: github.event_name != 'pull_request' uses: docker/build-push-action@v6 with: context: . platforms: linux/amd64,linux/arm64 - push: true + push: ${{ github.event_name != 'pull_request' }} tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }} - cache-from: type=registry,ref=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:buildcache - cache-to: type=registry,ref=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:buildcache,mode=max + cache-from: ${{ env.CACHE_FROM }} + cache-to: ${{ env.CACHE_TO }}