From 8a19bb719377a5d9e26bf63ae88e155f37a87701 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Fri, 1 Dec 2023 10:24:29 +0100 Subject: [PATCH] image/cache: Check image platform MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make sure the cache candidate platform matches the requested. Signed-off-by: Paweł Gronowski (cherry picked from commit 877ebbe038ee914e4243d5b7e198eda00494d757) Signed-off-by: Paweł Gronowski --- builder/builder.go | 3 ++- builder/dockerfile/copy.go | 17 ++------------ builder/dockerfile/imageprobe.go | 9 ++++---- builder/dockerfile/internals.go | 17 +++++++++++++- builder/dockerfile/mockbackend_test.go | 3 ++- daemon/containerd/cache.go | 14 ++++++++--- image/cache/cache.go | 32 +++++++++++++++++++------- image/cache/compare.go | 3 --- 8 files changed, 62 insertions(+), 36 deletions(-) diff --git a/builder/builder.go b/builder/builder.go index d3521ddfbb..79ba19d1a7 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -15,6 +15,7 @@ import ( "github.com/docker/docker/image" "github.com/docker/docker/layer" "github.com/opencontainers/go-digest" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) const ( @@ -89,7 +90,7 @@ type ImageCacheBuilder interface { type ImageCache interface { // GetCache returns a reference to a cached image whose parent equals `parent` // and runconfig equals `cfg`. A cache miss is expected to return an empty ID and a nil error. - GetCache(parentID string, cfg *container.Config) (imageID string, err error) + GetCache(parentID string, cfg *container.Config, platform ocispec.Platform) (imageID string, err error) } // Image represents a Docker image used by the builder. diff --git a/builder/dockerfile/copy.go b/builder/dockerfile/copy.go index 7919c972fd..f22b19ad86 100644 --- a/builder/dockerfile/copy.go +++ b/builder/dockerfile/copy.go @@ -8,7 +8,6 @@ import ( "net/url" "os" "path/filepath" - "runtime" "sort" "strings" "time" @@ -74,7 +73,7 @@ type copier struct { source builder.Source pathCache pathCache download sourceDownloader - platform *ocispec.Platform + platform ocispec.Platform // for cleanup. TODO: having copier.cleanup() is error prone and hard to // follow. Code calling performCopy should manage the lifecycle of its params. // Copier should take override source as input, not imageMount. @@ -83,19 +82,7 @@ type copier struct { } func copierFromDispatchRequest(req dispatchRequest, download sourceDownloader, imageSource *imageMount) copier { - platform := req.builder.platform - if platform == nil { - // May be nil if not explicitly set in API/dockerfile - platform = &ocispec.Platform{} - } - if platform.OS == "" { - // Default to the dispatch requests operating system if not explicit in API/dockerfile - platform.OS = req.state.operatingSystem - } - if platform.OS == "" { - // This is a failsafe just in case. Shouldn't be hit. - platform.OS = runtime.GOOS - } + platform := req.builder.getPlatform(req.state) return copier{ source: req.source, diff --git a/builder/dockerfile/imageprobe.go b/builder/dockerfile/imageprobe.go index 5ef622193b..8023bf356f 100644 --- a/builder/dockerfile/imageprobe.go +++ b/builder/dockerfile/imageprobe.go @@ -5,6 +5,7 @@ import ( "github.com/docker/docker/api/types/container" "github.com/docker/docker/builder" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/sirupsen/logrus" ) @@ -12,7 +13,7 @@ import ( // cache. type ImageProber interface { Reset(ctx context.Context) error - Probe(parentID string, runConfig *container.Config) (string, error) + Probe(parentID string, runConfig *container.Config, platform ocispec.Platform) (string, error) } type resetFunc func(context.Context) (builder.ImageCache, error) @@ -51,11 +52,11 @@ func (c *imageProber) Reset(ctx context.Context) error { // Probe checks if cache match can be found for current build instruction. // It returns the cachedID if there is a hit, and the empty string on miss -func (c *imageProber) Probe(parentID string, runConfig *container.Config) (string, error) { +func (c *imageProber) Probe(parentID string, runConfig *container.Config, platform ocispec.Platform) (string, error) { if c.cacheBusted { return "", nil } - cacheID, err := c.cache.GetCache(parentID, runConfig) + cacheID, err := c.cache.GetCache(parentID, runConfig, platform) if err != nil { return "", err } @@ -74,6 +75,6 @@ func (c *nopProber) Reset(ctx context.Context) error { return nil } -func (c *nopProber) Probe(_ string, _ *container.Config) (string, error) { +func (c *nopProber) Probe(_ string, _ *container.Config, _ ocispec.Platform) (string, error) { return "", nil } diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index 050deb1aad..42c17f8546 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -10,6 +10,7 @@ import ( "fmt" "strings" + "github.com/containerd/containerd/platforms" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/backend" "github.com/docker/docker/api/types/container" @@ -328,7 +329,7 @@ func getShell(c *container.Config, os string) []string { } func (b *Builder) probeCache(dispatchState *dispatchState, runConfig *container.Config) (bool, error) { - cachedID, err := b.imageProber.Probe(dispatchState.imageID, runConfig) + cachedID, err := b.imageProber.Probe(dispatchState.imageID, runConfig, b.getPlatform(dispatchState)) if cachedID == "" || err != nil { return false, err } @@ -388,3 +389,17 @@ func hostConfigFromOptions(options *types.ImageBuildOptions) *container.HostConf } return hc } + +func (b *Builder) getPlatform(state *dispatchState) ocispec.Platform { + // May be nil if not explicitly set in API/dockerfile + out := platforms.DefaultSpec() + if b.platform != nil { + out = *b.platform + } + + if state.operatingSystem != "" { + out.OS = state.operatingSystem + } + + return out +} diff --git a/builder/dockerfile/mockbackend_test.go b/builder/dockerfile/mockbackend_test.go index a9e43e9bd1..a561c7d77f 100644 --- a/builder/dockerfile/mockbackend_test.go +++ b/builder/dockerfile/mockbackend_test.go @@ -14,6 +14,7 @@ import ( "github.com/docker/docker/image" "github.com/docker/docker/layer" "github.com/opencontainers/go-digest" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) // MockBackend implements the builder.Backend interface for unit testing @@ -111,7 +112,7 @@ type mockImageCache struct { getCacheFunc func(parentID string, cfg *container.Config) (string, error) } -func (mic *mockImageCache) GetCache(parentID string, cfg *container.Config) (string, error) { +func (mic *mockImageCache) GetCache(parentID string, cfg *container.Config, _ ocispec.Platform) (string, error) { if mic.getCacheFunc != nil { return mic.getCacheFunc(parentID, cfg) } diff --git a/daemon/containerd/cache.go b/daemon/containerd/cache.go index 5e696c5967..4bab9c1dee 100644 --- a/daemon/containerd/cache.go +++ b/daemon/containerd/cache.go @@ -8,7 +8,9 @@ import ( "github.com/docker/docker/api/types/container" imagetype "github.com/docker/docker/api/types/image" "github.com/docker/docker/builder" + "github.com/docker/docker/errdefs" "github.com/docker/docker/image" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) // MakeImageCache creates a stateful image cache. @@ -29,7 +31,7 @@ type imageCache struct { c *ImageService } -func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID string, err error) { +func (ic *imageCache) GetCache(parentID string, cfg *container.Config, platform ocispec.Platform) (imageID string, err error) { ctx := context.TODO() if parentID == "" { @@ -37,8 +39,11 @@ func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID return "", nil } - parent, err := ic.c.GetImage(ctx, parentID, imagetype.GetImageOpts{}) + parent, err := ic.c.GetImage(ctx, parentID, imagetype.GetImageOpts{Platform: &platform}) if err != nil { + if errdefs.IsNotFound(err) { + return "", nil + } return "", err } @@ -54,8 +59,11 @@ func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID } for _, children := range children { - childImage, err := ic.c.GetImage(ctx, children.String(), imagetype.GetImageOpts{}) + childImage, err := ic.c.GetImage(ctx, children.String(), imagetype.GetImageOpts{Platform: &platform}) if err != nil { + if errdefs.IsNotFound(err) { + continue + } return "", err } diff --git a/image/cache/cache.go b/image/cache/cache.go index 77cb9d1706..ee89a171e2 100644 --- a/image/cache/cache.go +++ b/image/cache/cache.go @@ -1,18 +1,19 @@ package cache // import "github.com/docker/docker/image/cache" import ( - "context" "encoding/json" "fmt" "reflect" "strings" - "github.com/containerd/log" + "github.com/containerd/containerd/platforms" containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/dockerversion" "github.com/docker/docker/image" "github.com/docker/docker/layer" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" + "github.com/sirupsen/logrus" ) // NewLocal returns a local image cache, based on parent chain @@ -28,8 +29,8 @@ type LocalImageCache struct { } // GetCache returns the image id found in the cache -func (lic *LocalImageCache) GetCache(imgID string, config *containertypes.Config) (string, error) { - return getImageIDAndError(getLocalCachedImage(lic.store, image.ID(imgID), config)) +func (lic *LocalImageCache) GetCache(imgID string, config *containertypes.Config, platform ocispec.Platform) (string, error) { + return getImageIDAndError(getLocalCachedImage(lic.store, image.ID(imgID), config, platform)) } // New returns an image cache, based on history objects @@ -53,8 +54,8 @@ func (ic *ImageCache) Populate(image *image.Image) { } // GetCache returns the image id found in the cache -func (ic *ImageCache) GetCache(parentID string, cfg *containertypes.Config) (string, error) { - imgID, err := ic.localImageCache.GetCache(parentID, cfg) +func (ic *ImageCache) GetCache(parentID string, cfg *containertypes.Config, platform ocispec.Platform) (string, error) { + imgID, err := ic.localImageCache.GetCache(parentID, cfg, platform) if err != nil { return "", err } @@ -217,7 +218,7 @@ func getImageIDAndError(img *image.Image, err error) (string, error) { // of the image with imgID, that had the same config when it was // created. nil is returned if a child cannot be found. An error is // returned if the parent image cannot be found. -func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *containertypes.Config) (*image.Image, error) { +func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *containertypes.Config, platform ocispec.Platform) (*image.Image, error) { if config == nil { return nil, nil } @@ -225,7 +226,7 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain isBuiltLocally := func(id image.ID) bool { builtLocally, err := imageStore.IsBuiltLocally(id) if err != nil { - log.G(context.TODO()).WithFields(log.Fields{ + logrus.WithFields(logrus.Fields{ "error": err, "id": id, }).Warn("failed to check if image was built locally") @@ -247,6 +248,21 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain continue } + imgPlatform := ocispec.Platform{ + Architecture: img.Architecture, + OS: img.OS, + OSVersion: img.OSVersion, + OSFeatures: img.OSFeatures, + Variant: img.Variant, + } + // Discard old linux/amd64 images with empty platform. + if imgPlatform.OS == "" && imgPlatform.Architecture == "" { + continue + } + if !platforms.OnlyStrict(platform).Match(imgPlatform) { + continue + } + if compare(&img.ContainerConfig, config) { // check for the most up to date match if match == nil || match.Created.Before(img.Created) { diff --git a/image/cache/compare.go b/image/cache/compare.go index ec1971a207..d438b65be7 100644 --- a/image/cache/compare.go +++ b/image/cache/compare.go @@ -135,9 +135,6 @@ func compare(a, b *container.Config) bool { if a.Healthcheck.Interval != b.Healthcheck.Interval { return false } - if a.Healthcheck.StartInterval != b.Healthcheck.StartInterval { - return false - } if a.Healthcheck.StartPeriod != b.Healthcheck.StartPeriod { return false }