fix(proxy): deterministic routing, service hardening, unit tests, and CI go-checks

[paolomainardi]

User description

🤖 This was written by an AI agent on behalf of @paolomainardi.

Closes #101

Summary

This addresses the code-quality findings from a full read of the Go codebase. It fixes two latent routing bugs, hardens the event-driven services, removes dead code, and adds the project's first unit tests plus a CI gate to run them.

Correctness fixes

• Deterministic backend IP selection (cmd/dinghy-layer): getContainerIP ranged over the container's networks map and returned the first non-empty IP. Go randomizes map iteration, so a container attached to multiple networks could be routed to a different network IP on each restart or event. Network names are now sorted before selection.
• Deterministic default port selection (cmd/dinghy-layer): getDefaultPort had the same map-iteration problem over exposed ports. It now picks the lowest exposed TCP port (then the lowest bound TCP port), so the choice is stable.
• DNS A-record TTL lowered from 3600s to 60s (cmd/dns-server): the old value was the dns.NewRR default, not a deliberate choice. On a local development resolver a one-hour TTL means a changed HTTP_PROXY_DNS_TARGET_IP stays cached by the OS stub resolver. The record is now built directly as a *dns.A instead of being parsed from a string on every query.
• Nil-pointer guard (cmd/join-networks): getContainerInfo dereferenced NetworkSettings without a nil check, unlike the equivalent code in dinghy-layer.
• Signal handling (pkg/service): Run and RunWithSignalHandling each installed their own signal.Notify for the same signals, so two goroutines raced for each signal. Signal handling now has a single owner, and the event-stream reconnect backoff exits promptly on shutdown instead of always sleeping five seconds.

Cleanup

• Removed unused code: GetEnvOrDefaultInt, SliceToSet, the TraefikLabels struct (its own comment noted it was unused), and the write-only NetworkInfo struct.
• Extracted a shared HasTraefikLabel helper used by both dinghy-layer and utils.ShouldManageContainer.

Tests and CI

• Added 61 unit tests covering the pure parsing and config helpers, including the wildcard HostRegexp branch, the ReDoS guard, and the two determinism fixes above. These need no Docker, unlike the existing integration suite.
• Added a go-checks CI job running gofmt, go vet, and go test -race on every non-main branch. CI previously ran no Go-level checks.

Verification

• go test -race ./...: 61 passing.
• gofmt -l and go vet ./...: clean.
• make test (full integration suite, as CI runs it): all 6 suites pass (HTTP 5/5, HSTS 5/5, DNS 15/15, upstream DNS 3/3, forwarding 2/2, DNS config 3/3) against images rebuilt from this branch.

Notes

• golangci-lint was intentionally left out of the CI job. Its default staticcheck would fail immediately on the pre-existing types.ContainerJSON / types.Container deprecations across the codebase, which belong to a separate Docker SDK migration rather than this change.

---

PR Type

Bug fix, Tests, Enhancement

---

Description

• Fix nondeterministic backend IP and port selection by sorting map keys before iteration

• Remove dead code: TraefikLabels, NetworkInfo, GetEnvOrDefaultInt, SliceToSet; extract shared HasTraefikLabel helper

• Add first unit tests for pure parsing/config helpers across dinghy-layer, dns-server, config, and utils

• Add CI go-checks job enforcing gofmt, go vet, and go test -race; harden signal handling and DNS A-record construction

---

Diagram Walkthrough

flowchart LR
  A["Map iteration\n(nondeterministic)"] -- "sort keys before select" --> B["Deterministic IP/port\nselection"]
  C["Duplicate signal handlers\nin Run + RunWithSignalHandling"] -- "single owner in\nRunWithSignalHandling" --> D["Single signal handler\n+ prompt reconnect backoff"]
  E["Dead code\n(TraefikLabels, NetworkInfo,\nGetEnvOrDefaultInt, SliceToSet)"] -- "removed" --> F["Shared HasTraefikLabel\nhelper"]
  G["No unit tests"] -- "add test files" --> H["Tests: dinghy-layer,\ndns-server, config, utils"]
  H -- "enforced by" --> I["CI go-checks job\n(gofmt, vet, test -race)"]

Loading

File Walkthrough

	Relevant files
Bug fix	4 files main.go Deterministic IP/port selection, remove dead struct, extract helper +55/-34  main.go Lower A-record TTL, build record directly, log write errors +32/-17  main.go Guard nil NetworkSettings, remove unused NetworkInfo struct +5/-16    docker_event_service.go Single signal owner, extract filter helper, prompt reconnect backoff +23/-44
Tests	4 files main_test.go Add unit tests for parsing, routing, and config helpers    +325/-0  main_test.go Add unit tests for isDomainHandled                                              +30/-0    config_test.go Add unit tests for config env helpers                                        +63/-0    utils_test.go Add unit tests for utils helpers                                                  +92/-0
Miscellaneous	1 files config.go Remove unused GetEnvOrDefaultInt function                                +0/-11
Enhancement	1 files utils.go Extract HasTraefikLabel helper, remove SliceToSet dead code +11/-18
Configuration changes	1 files ci.yml Add go-checks CI job for fmt, vet, and unit tests                +30/-0
Documentation	1 files CHANGELOG.md Document all fixes and additions for this PR                          +9/-0

---

[sparkfabrik-ai-bot]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 101 - Partially compliant Compliant requirements: Fix getContainerIP nondeterministic map iteration (sort by network name) Fix getDefaultPort nondeterministic map iteration (sort, pick lowest TCP port) Fix double signal handling: Run no longer installs its own signal.Notify; single owner in RunWithSignalHandling Fix unguarded nil deref in join-networks getContainerInfo Remove dead code: GetEnvOrDefaultInt, SliceToSet, TraefikLabels struct, NetworkInfo struct Add unit tests for parseVirtualHosts, convertWildcardToRegex, generateServiceName, getEffectivePort, isPort, isDomainHandled, GetDockerEnvVar, HasTraefikLabel, ShouldManageContainer, FormatDockerID Add CI gate: gofmt -l, go vet ./..., go test -race Extract duplicated filters.NewArgs block into containerEventOptions() Fix reconnect backoff to respect ctx cancellation via select Extract HasTraefikLabel shared helper used by both ShouldManageContainer and processContainer Fix createARecord to construct *dns.A directly instead of string-parsing on every query Fix ignored WriteMsg errors via writeMsg helper Non-compliant requirements: golangci-lint not added to CI (only gofmt, go vet, go test) dns-server startup-error detection race (time.After(100ms)) not addressed dns-server signal handling still hand-rolled without SIGQUIT (not unified with other binaries) Requires further human verification: Double signal handling fix: verify no regression in shutdown behavior under real Docker environments (signal delivery, goroutine lifecycle) Deterministic IP selection by alphabetical network name — confirm this is the desired policy (ticket says "sort by network name or prefer a shared network"; the harder "prefer shared network" option was not implemented)

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Swallowed context.Canceled The old code checked err != nil && err != context.Canceled before logging/exiting. The new code checks only err != nil, so a normal context cancellation (e.g., signal-driven shutdown) will now log "Service failed" and call os.Exit(1) instead of exiting cleanly. This means every graceful shutdown will be reported as a failure. case err := <-errChan: if err != nil { service.GetLogger().Error("Service failed", "error", err) os.Exit(1) } Nil pointer in getContainerIP After sorting network names, the code indexes inspect.NetworkSettings.Networks[name] without checking whether the value pointer is nil. If Docker returns a network entry with a nil *network.EndpointSettings, this will panic. The old code iterated values directly and would also have panicked, but the new explicit map lookup makes it slightly more visible. for _, name := range names { if ip := inspect.NetworkSettings.Networks[name].IPAddress; ip != "" { return ip } Pinned to non-existent action version actions/checkout@v6 is used, but the latest stable release is v4. v6 does not exist; the workflow step will fail at runtime with a resolution error. Should be actions/checkout@v4. uses: actions/checkout@v6

[sparkfabrik-ai-bot]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix non-existent action version reference actions/checkout@v6 does not exist; the latest major version is v4. Using a non-existent action version will cause the workflow to fail immediately. .github/workflows/ci.yml [23] - name: Checkout repository - uses: actions/checkout@v6 + uses: actions/checkout@v4 Suggestion importance[1-10]: 9 __ Why: actions/checkout@v6 does not exist (latest is v4), so the go-checks CI job will fail immediately on every run. This is a critical correctness issue for the newly added CI workflow.	High
	Use 4-byte IPv4 form for DNS A records net.ParseIP returns a 16-byte IPv6-mapped representation for IPv4 addresses. For a DNS A record the field must be a 4-byte IPv4 address; passing the 16-byte form will produce a malformed record. Use .To4() to ensure the correct representation. cmd/dns-server/main.go [138-147] func (s *DNSServer) createARecord(question dns.Question) dns.RR { return &dns.A{ Hdr: dns.RR_Header{ Name: question.Name, Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: defaultRecordTTL, }, - A: net.ParseIP(s.targetIP), + A: net.ParseIP(s.targetIP).To4(), } } Suggestion importance[1-10]: 8 __ Why: net.ParseIP returns a 16-byte IPv6-mapped representation for IPv4 addresses. For DNS A records, the dns.A struct's A field must be a 4-byte IPv4 address; passing the 16-byte form can produce malformed records. Using .To4() ensures correctness.	Medium
General	Avoid treating context cancellation as a fatal error After the signal-handling refactor, service.Run (and thus runEventLoop) returns nil on a clean context cancellation. However, if the event loop exits due to ctx.Done() before a signal is received (e.g., the context was cancelled externally), err will be nil and the code logs "Service completed successfully" and exits 0, which is correct. But context.Canceled can still be returned in some paths; without the previous guard, any context.Canceled error now causes os.Exit(1). Consider re-adding the context.Canceled / context.DeadlineExceeded check to avoid treating a normal shutdown as a failure. pkg/service/docker_event_service.go [188-193] case err := <-errChan: - if err != nil { + if err != nil && err != context.Canceled && err != context.DeadlineExceeded { service.GetLogger().Error("Service failed", "error", err) os.Exit(1) } service.GetLogger().Info("Service completed successfully") Suggestion importance[1-10]: 6 __ Why: The PR removed the context.Canceled guard from the error check. If runEventLoop returns context.Canceled (e.g., from an external cancellation), the service will now call os.Exit(1) instead of treating it as a clean shutdown. Re-adding the guard prevents false failure exits.	Low

[Copilot]

Pull request overview

This PR improves the Spark HTTP Proxy’s Go sidecars by making routing decisions
deterministic, hardening event-loop shutdown behavior, removing confirmed dead
code, and adding the project’s first Go unit tests plus a CI gate to run them.

Changes:

• Make dinghy-layer backend IP/port selection deterministic (sorted selection
  rather than randomized map iteration).
• Improve service framework behavior (signal ownership + reconnect backoff) and
  harden join-networks against nil network settings.
• Add Go unit tests + a CI go-checks job (gofmt, go vet, go test -race),
  and update the changelog.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/utils/utils.go	Adds HasTraefikLabel helper and removes unused SliceToSet.
pkg/utils/utils_test.go	Adds unit tests for env parsing, label detection, and log level validation.
pkg/service/docker_event_service.go	Refactors signal handling ownership and reconnect logic.
pkg/config/config.go	Removes unused GetEnvOrDefaultInt.
pkg/config/config_test.go	Adds unit tests for env helpers.
cmd/join-networks/main.go	Removes unused network struct and adds nil guard for network settings.
cmd/dns-server/main.go	Lowers A-record TTL, avoids string-parsing RR creation, and logs write errors.
cmd/dns-server/main_test.go	Adds unit tests for domain-matching logic.
cmd/dinghy-layer/main.go	Sorts networks/ports for deterministic routing; uses shared Traefik-label helper.
cmd/dinghy-layer/main_test.go	Adds unit tests for parsing, determinism fixes, and config generation.
CHANGELOG.md	Documents the new tests/CI job and correctness fixes.
.github/workflows/ci.yml	Adds go-checks job for formatting/vet/unit-test gating.

Comments suppressed due to low confidence (1)

pkg/service/docker_event_service.go:129

• The Docker events loop reads from eventsChan / errChan without checking whether either channel has been closed. In Go, receiving from a closed channel returns the zero value immediately, so if Docker closes either channel (e.g. daemon restart / connection drop) this loop can become a tight spin (high CPU) and never reach the reconnect/backoff path.

	for {
		select {
		case <-ctx.Done():
			return nil
		case event := <-eventsChan:

[paolomainardi]

🤖 This was written by an AI agent on behalf of @paolomainardi.

Fresh-eyes review (self-review with subagents)

Ran a high-recall review over the diff. One real regression and two simplifications were found and fixed in 2497a33.

Correctness regression (fixed)

The rewritten createARecord builds &dns.A{A: net.ParseIP(targetIP)} directly. Startup validation only checked net.ParseIP(cfg.DNSIP) == nil, which also passes for IPv6. An IPv6 HTTP_PROXY_DNS_TARGET_IP (for example ::1) would therefore survive validation and be silently truncated into a 4-byte A record (::1 becomes 0.0.0.1). The previous dns.NewRR path rejected IPv6 loudly. Startup now requires IPv4 via ip.To4() != nil.

Simplifications (applied)

• performInitialNetworkJoin rebuilt a NetworkSet from containerInfo.Networks, which is already a NetworkSet after this PR. The copy loop is removed.
• The duplicated sort-and-format tail in getDefaultPort is folded into a small lowestTCPPort helper.

Checked and confirmed correct

The signal-handling refactor has a single owner with no double-close or missing close (both binaries call RunWithSignalHandling, never Run directly), the getDefaultPort precedence (exposed before bound) is preserved, and all four dead-code removals have no remaining callers.

Verification after the fixes: go test -race ./... 61 passing, gofmt/go vet clean, and the full make test integration suite re-run green.