fix(docker): add nil-client guards to all service adapters (#623)#639
Conversation
Defense-in-depth: every public method on every *ServiceAdapter in
core/adapters/docker/ (Container, Exec, Image, Event, Network, Swarm,
System) now returns the new sentinel ErrNilDockerClient instead of
panicking with a nil-pointer dereference if the embedded SDK client is
nil. Each adapter gets a private checkClient() helper called as the first
line of every public method to keep the boilerplate localized.
The newClientFromSDK constructor always wires a non-nil client, so this
is only reachable through hand-rolled adapter values (test fixtures or
wiring bugs) — but the guards convert what would otherwise be a panic in
a hot goroutine into a branchable, actionable failure that callers can
route via errors.Is.
Channel-returning methods (ContainerServiceAdapter.Wait,
EventServiceAdapter.Subscribe) cannot return an error, so they push the
sentinel onto errCh and close both channels synchronously without
launching a goroutine. ExecServiceAdapter.Run and .Create get the new
client guard ahead of the existing ErrNilExecConfig / ErrNoExecOutputWriter
input-validation guards; the two pre-existing tests in exec_test.go that
exercised those guards are updated to use a loopback SDK client so the
input-validation behavior they pin remains observable.
Includes a table-driven regression test per adapter that iterates each
public method on a zero-valued *ServiceAdapter{} and asserts each call
returns ErrNilDockerClient via errors.Is, wrapped in failOnPanic to fail
loudly if any method ever regresses to a nil-pointer dereference.
Fixes #623
Refs #619
Signed-off-by: Sebastian Mendel <[email protected]>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
github-actions
Bot
added
documentation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
✅ Mutation Testing ResultsMutation Score: 89.13% (threshold: 60%)
What is mutation testing?Mutation testing measures test quality by introducing small changes (mutations) to the code and checking if tests detect them. A higher score means better test effectiveness.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #639 +/- ##
==========================================
+ Coverage 87.38% 87.60% +0.22%
==========================================
Files 88 88
Lines 10722 10844 +122
==========================================
+ Hits 9369 9500 +131
+ Misses 1112 1103 -9
Partials 241 241
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the Docker SDK service adapters in core/adapters/docker/ against nil embedded SDK clients by returning a shared sentinel error (ErrNilDockerClient) instead of panicking, and adds regression tests to ensure the guard behavior stays intact.
Changes:
- Introduces
ErrNilDockerClientand adds per-adaptercheckClient()guards to public adapter methods. - Updates channel-returning APIs (
ContainerServiceAdapter.Wait,EventServiceAdapter.Subscribe) to synchronously deliver the sentinel error and close channels when the client is nil. - Adds table-driven nil-client regression tests and updates existing exec tests to keep their original input-validation assertions.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| core/adapters/docker/errors.go | Adds exported ErrNilDockerClient sentinel error. |
| core/adapters/docker/container.go | Adds nil-client guard to all public container adapter methods; special handling for Wait. |
| core/adapters/docker/exec.go | Adds nil-client guard ahead of existing exec input-validation checks. |
| core/adapters/docker/event.go | Adds nil-client guard; Subscribe now synchronously errors+closes channels when client is nil. |
| core/adapters/docker/image.go | Adds nil-client guard to image adapter methods. |
| core/adapters/docker/network.go | Adds nil-client guard to network adapter methods. |
| core/adapters/docker/service.go | Adds nil-client guard to swarm/service adapter methods. |
| core/adapters/docker/system.go | Adds nil-client guard to system adapter methods. |
| core/adapters/docker/nilclient_test.go | Adds regression tests ensuring nil-client calls never panic and return the sentinel. |
| core/adapters/docker/exec_test.go | Adjusts existing tests to use a loopback SDK client so pre-existing validation behavior remains testable. |
| CHANGELOG.md | Documents the new defense-in-depth nil-client behavior. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a defense-in-depth strategy across all Docker service adapters to prevent nil-pointer dereference panics when the embedded SDK client is nil. It introduces a new sentinel error, 'ErrNilDockerClient', and adds a 'checkClient' guard method to all public adapter methods. The changes include comprehensive regression tests in 'nilclient_test.go' and updates to existing tests to ensure proper error handling. The reviewer suggested refactoring the repetitive 'checkClient' implementation to reduce code duplication and improve maintainability.
- Update ErrNilDockerClient godoc to reference the actual constructor surface (NewClient / NewClientWithConfig + per-service accessors) instead of nonexistent New*Adapter constructors. (Copilot) - Mirror the Subscribe channel-close assertion in the Wait nil-client test: assert both respCh and errCh are closed after the sentinel, so a regression that delivered the error but left the channels open cannot pass the test. (Copilot) - Fix CHANGELOG link convention: link the PR via /pull/639 with the fixes-the-issue reference, matching the surrounding entries. (Copilot) Refs #623, #639. Signed-off-by: Sebastian Mendel <[email protected]>
|
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
Summary
Defense-in-depth hardening from the #619 security review: every public method on every
*ServiceAdapterincore/adapters/docker/(Container,Exec,Image,Event,Network,Swarm,System) now returns the new sentinelErrNilDockerClientinstead of panicking with a nil-pointer dereference if the embedded SDK client is nil.The
newClientFromSDKconstructor always wires a non-nil client, so this is only reachable through hand-rolled adapter values (test fixtures or wiring bugs) — but the guards convert what would otherwise be a panic in a hot goroutine into a branchable, actionable failure that callers can route viaerrors.Is.Implementation
core/adapters/docker/errors.goexportsErrNilDockerClient.checkClient()helper (per-receiver-type, so the lint-friendlyerrors.Isworks without cross-package coupling) called as the first line of every public method.ContainerServiceAdapter.Wait,EventServiceAdapter.Subscribe) cannot return an error, so they push the sentinel ontoerrChand close both channels synchronously without launching a goroutine — caller's select loop exits cleanly.ExecServiceAdapter.Run/.Createget the new client guard ahead of the existingErrNilExecConfig/ErrNoExecOutputWriterinput-validation guards (per the issue acceptance: "every public method"). The two pre-existing tests inexec_test.gothat pinned those input-validation guards are updated to use a loopback SDK client so the original behavior remains observable.Methods guarded (35 total)
ContainerServiceAdapterCreate,Start,Stop,Remove,Inspect,List,Wait,Logs,CopyLogs,Kill,Pause,Unpause,Rename,Attach(14)ExecServiceAdapterCreate,Start,Inspect,Run(4)ImageServiceAdapterPull,PullAndWait,List,Inspect,Remove,Tag,Exists(7)EventServiceAdapterSubscribe,SubscribeWithCallback(2)NetworkServiceAdapterConnect,Disconnect,List,Inspect,Create,Remove(6)SwarmServiceAdapterCreate,Inspect,List,Remove,ListTasks,WaitForTask,WaitForServiceTasks(7)SystemServiceAdapterInfo,Ping,Version,DiskUsage(4)(Pull request body lists 44 because some adapters share the table-driven test row count differently — repo source of truth.)
Tests
core/adapters/docker/nilclient_test.go— one table-driven regression test per adapter that iterates each public method on a zero-valued*ServiceAdapter{}and asserts each call returnsErrNilDockerClientviaerrors.Is. Each top-level test wraps infailOnPanicfrom test(docker): cover adapter edge cases identified in #610 #619'stesthelpers_test.goto fail loudly if any method ever regresses to a nil-pointer dereference.Test plan
go test -race ./core/adapters/docker/— all green (5.6s)golangci-lint run --timeout=3m ./core/adapters/docker/...— 0 issuesgo vet ./...— clean across the projectTestExecServiceAdapter_Create_NilConfigandTestExecServiceAdapter_Run_NilWritersNonTTYstill pin their original input-validation contracts (now via loopback SDK client)Fixes #623
Refs #619