updating docker to moby. Removing windows support for lifecycle#10096
Conversation
![gemini-code-assist[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the codebase from the deprecated github.com/docker/docker package to github.com/moby/moby and github.com/containerd/errdefs. This migration affects numerous imports, client configurations, and error handling paths across the skaffold, buildpacks, and pack components. Two critical issues were identified in the review: first, the use of client.ImageInspectWithRawResponse in pkg/skaffold/docker/image.go causes a SIGSEGV panic in tests due to mock client limitations, which can be resolved by directly marshaling the inspect response to JSON; second, the mock errors in pkg/skaffold/build/docker/docker_test.go need to be updated to use Internal() and Canceled() to align with the new containerd/errdefs expectations.
| func (l *localDaemon) ImageInspectWithRaw(ctx context.Context, img string) (image.InspectResponse, []byte, error) { | ||
| var raw bytes.Buffer | ||
| res, err := l.apiClient.ImageInspect(ctx, img, client.ImageInspectWithRawResponse(&raw)) | ||
| if err != nil { | ||
| return image.InspectResponse{}, nil, err | ||
| } | ||
| return res.InspectResponse, raw.Bytes(), nil | ||
| } |
There was a problem hiding this comment.
Using client.ImageInspectWithRawResponse causes a SIGSEGV panic in tests (as seen in test.log) because the fake/mock client (FakeAPIClient) cannot safely extract the unexported rawResponse field from the unexported imageInspectConfig struct across package boundaries, leading to memory corruption or nil pointer dereferences.
We can completely avoid this fragile dependency on client.ImageInspectWithRawResponse by simply marshaling the returned res.InspectResponse to JSON directly using json.Marshal. This is fully compatible, safer, and fixes the test failures.
func (l *localDaemon) ImageInspectWithRaw(ctx context.Context, img string) (image.InspectResponse, []byte, error) {
res, err := l.apiClient.ImageInspect(ctx, img)
if err != nil {
return image.InspectResponse{}, nil, err
}
raw, err := json.Marshal(res.InspectResponse)
if err != nil {
return image.InspectResponse{}, nil, err
}
return res.InspectResponse, raw, nil
}There was a problem hiding this comment.
We need the option for fakes. In fakes, we don't set the config, which ends up causing issues.
| type systemError struct { | ||
| error | ||
| } | ||
|
|
||
| func (systemError) System() {} | ||
|
|
||
| type cancelledError struct { | ||
| error | ||
| } | ||
|
|
||
| func (cancelledError) Cancelled() {} |
There was a problem hiding this comment.
The mock errors systemError and cancelledError are using method names (System() and Cancelled()) from the old docker/errdefs package. Since this PR migrates to containerd/errdefs, these mocks will no longer be recognized by errdefs.IsInternal and errdefs.IsCanceled.
Specifically:
errdefs.IsInternalchecks for anInternal()method, notSystem().errdefs.IsCanceledchecks for aCanceled()method (spelled with one 'l'), notCancelled().
We should update these mock methods to match the containerd/errdefs expectations so that the error classification behaves correctly in tests.
| type systemError struct { | |
| error | |
| } | |
| func (systemError) System() {} | |
| type cancelledError struct { | |
| error | |
| } | |
| func (cancelledError) Cancelled() {} | |
| type systemError struct { | |
| error | |
| } | |
| func (systemError) Internal() {} | |
| type cancelledError struct { | |
| error | |
| } | |
| func (cancelledError) Canceled() {} |
There was a problem hiding this comment.
errdefs.IsInternal / IsCancelled requires System / Canceled. This comment is incorrect.
Darien-Lin
force-pushed
the
main
branch
16 times, most recently
from
abb88b5 to
9ba2d30
Compare
…e upgrade requires lifecycle package update
3 participants
Overview
This PR updates Skaffold's Docker SDK dependencies from the legacy github.com/docker/docker package to the modern github.com/moby/moby/client package to address a security vulnerability (CVE-2026-34040).
As part of this upgrade, downstream dependency updates (including go-containerregistry and lifecycle) have been integrated.
Key Changes
Migrated Skaffold's Docker client initialization and structures from github.com/docker/docker to github.com/moby/moby/client.
Minimally refactored pkg/skaffold/deploy/docker/deploy.go and related tasks to adapt to the new Moby API types (such as container.Summary and client.Filters) while preserving 100% of the original logic and removing any compile-time dependencies on the legacy docker/docker repository.
Upgrading the dependencies introduced a critical deadlock during local Minikube/sideloading operations in GHA integration tests (TestRun/ko test hung indefinitely). This was caused by a leaked layer stream reader bug in go-containerregistry v0.21.6's newly added concurrency pullLimiter.
Resolution: Pinned go-containerregistry to v0.21.4 in go.mod. This version fully supports the new Moby client migration but does not include the concurrency limiter feature, eliminating the deadlock natively and ensuring all integration suites pass cleanly. go-containerregistry already has the fix in @ HEAD.
Bumping the lifecycle package past v0.20.6 (required for Moby alignment) introduces breaking changes regarding native Windows support:
Native Buildpacks on Windows Hosts are No Longer Supported
Natively running Buildpack-based builds on a Windows host is no longer supported in Skaffold.
Impacted Skaffold Features & Workarounds:
Skaffold Buildpacks Builder:
Native Windows Execution: Running skaffold build or skaffold dev on a native Windows machine targeting buildpacks will fail.
Workaround: Windows users must run Buildpack-based builds inside a Linux environment.