From acaafbf8e1119274101a340bdfde67ff9ad7b6e5 Mon Sep 17 00:00:00 2001 From: Akihiro Suda Date: Mon, 19 Feb 2024 23:40:21 +0900 Subject: [PATCH] Avoid applying `SOURCE_DATE_EPOCH` to source images The exporter fetches the source image config via the `ExporterImageSourceConfigKey` metadata to detect the immutable layer diffIDs and the history objects. Fix issue 4614 Signed-off-by: Akihiro Suda --- exporter/containerimage/writer.go | 66 +++++++++++++++---- frontend/dockerfile/dockerfile2llb/convert.go | 19 +----- frontend/dockerfile/dockerfile_test.go | 51 +++++++++++--- util/converter/converter.go | 14 +++- 4 files changed, 111 insertions(+), 39 deletions(-) diff --git a/exporter/containerimage/writer.go b/exporter/containerimage/writer.go index 303b9ff065802..1ae0d9c33cd56 100644 --- a/exporter/containerimage/writer.go +++ b/exporter/containerimage/writer.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "reflect" "strconv" "strings" "time" @@ -34,6 +35,7 @@ import ( "github.com/moby/buildkit/util/purl" "github.com/moby/buildkit/util/system" "github.com/moby/buildkit/util/tracing" + dockerspec "github.com/moby/docker-image-spec/specs-go/v1" digest "github.com/opencontainers/go-digest" specs "github.com/opencontainers/image-spec/specs-go" ocispecs "github.com/opencontainers/image-spec/specs-go/v1" @@ -124,6 +126,15 @@ func (ic *ImageWriter) Commit(ctx context.Context, inp *exporter.Source, session ref = inp.Ref } config := exptypes.ParseKey(inp.Metadata, exptypes.ExporterImageConfigKey, p) + srcImgConfig := exptypes.ParseKey(inp.Metadata, exptypes.ExporterImageSourceConfigKey, p) + var srcImg *dockerspec.DockerOCIImage + if len(srcImgConfig) > 0 { + var srcImgX dockerspec.DockerOCIImage + if err := json.Unmarshal(srcImgConfig, &srcImgX); err != nil { + return nil, errors.Wrap(err, "failed to unmarshal source image config") + } + srcImg = &srcImgX + } remotes, err := ic.exportLayers(ctx, opts.RefCfg, session.NewGroup(sessionID), ref) if err != nil { @@ -131,7 +142,7 @@ func (ic *ImageWriter) Commit(ctx context.Context, inp *exporter.Source, session } remote := &remotes[0] if opts.RewriteTimestamp { - remote, err = ic.rewriteRemoteWithEpoch(ctx, opts, remote) + remote, err = ic.rewriteRemoteWithEpoch(ctx, opts, remote, srcImg) if err != nil { return nil, err } @@ -157,7 +168,7 @@ func (ic *ImageWriter) Commit(ctx context.Context, inp *exporter.Source, session } } - mfstDesc, configDesc, err := ic.commitDistributionManifest(ctx, opts, ref, config, remote, annotations, inlineCacheEntry, opts.Epoch, session.NewGroup(sessionID)) + mfstDesc, configDesc, err := ic.commitDistributionManifest(ctx, opts, ref, config, remote, annotations, inlineCacheEntry, opts.Epoch, session.NewGroup(sessionID), srcImg) if err != nil { return nil, err } @@ -222,6 +233,15 @@ func (ic *ImageWriter) Commit(ctx context.Context, inp *exporter.Source, session return nil, errors.Errorf("failed to find ref for ID %s", p.ID) } config := exptypes.ParseKey(inp.Metadata, exptypes.ExporterImageConfigKey, &p) + srcImgConfig := exptypes.ParseKey(inp.Metadata, exptypes.ExporterImageSourceConfigKey, &p) + var srcImg *dockerspec.DockerOCIImage + if len(srcImgConfig) > 0 { + var srcImgX dockerspec.DockerOCIImage + if err := json.Unmarshal(srcImgConfig, &srcImgX); err != nil { + return nil, errors.Wrap(err, "failed to unmarshal source image config") + } + srcImg = &srcImgX + } remote := &remotes[remotesMap[p.ID]] if remote == nil { @@ -230,7 +250,7 @@ func (ic *ImageWriter) Commit(ctx context.Context, inp *exporter.Source, session } } if opts.RewriteTimestamp { - remote, err = ic.rewriteRemoteWithEpoch(ctx, opts, remote) + remote, err = ic.rewriteRemoteWithEpoch(ctx, opts, remote, srcImg) if err != nil { return nil, err } @@ -241,7 +261,7 @@ func (ic *ImageWriter) Commit(ctx context.Context, inp *exporter.Source, session inlineCacheEntry, _ = inlineCacheResult.FindRef(p.ID) } - desc, _, err := ic.commitDistributionManifest(ctx, opts, r, config, remote, opts.Annotations.Platform(&p.Platform), inlineCacheEntry, opts.Epoch, session.NewGroup(sessionID)) + desc, _, err := ic.commitDistributionManifest(ctx, opts, r, config, remote, opts.Annotations.Platform(&p.Platform), inlineCacheEntry, opts.Epoch, session.NewGroup(sessionID), srcImg) if err != nil { return nil, err } @@ -369,8 +389,14 @@ func (ic *ImageWriter) exportLayers(ctx context.Context, refCfg cacheconfig.RefC // the new blob. // // If no conversion is needed, this returns nil without error. -func rewriteImageLayerWithEpoch(ctx context.Context, cs content.Store, desc ocispecs.Descriptor, comp compression.Config, epoch *time.Time) (*ocispecs.Descriptor, error) { - converterFn, err := converter.NewWithRewriteTimestamp(ctx, cs, desc, comp, epoch) +func rewriteImageLayerWithEpoch(ctx context.Context, cs content.Store, desc ocispecs.Descriptor, comp compression.Config, epoch *time.Time, immDiffID digest.Digest) (*ocispecs.Descriptor, error) { + var immDiffIDs map[digest.Digest]struct{} + if immDiffID != "" { + immDiffIDs = map[digest.Digest]struct{}{ + immDiffID: {}, + } + } + converterFn, err := converter.NewWithRewriteTimestamp(ctx, cs, desc, comp, epoch, immDiffIDs) if err != nil { return nil, err } @@ -380,7 +406,7 @@ func rewriteImageLayerWithEpoch(ctx context.Context, cs content.Store, desc ocis return converterFn(ctx, cs, desc) } -func (ic *ImageWriter) rewriteRemoteWithEpoch(ctx context.Context, opts *ImageCommitOpts, remote *solver.Remote) (*solver.Remote, error) { +func (ic *ImageWriter) rewriteRemoteWithEpoch(ctx context.Context, opts *ImageCommitOpts, remote *solver.Remote, srcImg *dockerspec.DockerOCIImage) (*solver.Remote, error) { if opts.Epoch == nil { bklog.G(ctx).Warn("rewrite-timestamp is specified, but no source-date-epoch was found") return remote, nil @@ -392,8 +418,21 @@ func (ic *ImageWriter) rewriteRemoteWithEpoch(ctx context.Context, opts *ImageCo fmt.Sprintf("rewriting layers with source-date-epoch %d (%s)", opts.Epoch.Unix(), opts.Epoch.String())) for i, desc := range remoteDescriptors { i, desc := i, desc + info, err := cs.Info(ctx, desc.Digest) + if err != nil { + return nil, err + } + diffID := digest.Digest(info.Labels[labels.LabelUncompressed]) // can be empty + var immDiffID digest.Digest + if srcImg != nil && i < len(srcImg.RootFS.DiffIDs) { + immDiffID = srcImg.RootFS.DiffIDs[i] + if immDiffID == diffID { + bklog.G(ctx).WithField("blob", desc).Debugf("Not rewriting to apply epoch (immutable diffID %q)", diffID) + continue + } + } eg.Go(func() error { - if rewrittenDesc, err := rewriteImageLayerWithEpoch(ctx, cs, desc, opts.RefCfg.Compression, opts.Epoch); err != nil { + if rewrittenDesc, err := rewriteImageLayerWithEpoch(ctx, cs, desc, opts.RefCfg.Compression, opts.Epoch, immDiffID); err != nil { bklog.G(ctx).WithError(err).Warnf("failed to rewrite layer %d/%d to match source-date-epoch %d (%s)", i+1, len(remoteDescriptors), opts.Epoch.Unix(), opts.Epoch.String()) } else if rewrittenDesc != nil { @@ -411,7 +450,7 @@ func (ic *ImageWriter) rewriteRemoteWithEpoch(ctx context.Context, opts *ImageCo }, nil } -func (ic *ImageWriter) commitDistributionManifest(ctx context.Context, opts *ImageCommitOpts, ref cache.ImmutableRef, config []byte, remote *solver.Remote, annotations *Annotations, inlineCache *exptypes.InlineCacheEntry, epoch *time.Time, sg session.Group) (*ocispecs.Descriptor, *ocispecs.Descriptor, error) { +func (ic *ImageWriter) commitDistributionManifest(ctx context.Context, opts *ImageCommitOpts, ref cache.ImmutableRef, config []byte, remote *solver.Remote, annotations *Annotations, inlineCache *exptypes.InlineCacheEntry, epoch *time.Time, sg session.Group, srcImg *dockerspec.DockerOCIImage) (*ocispecs.Descriptor, *ocispecs.Descriptor, error) { if len(config) == 0 { var err error config, err = defaultImageConfig() @@ -430,7 +469,7 @@ func (ic *ImageWriter) commitDistributionManifest(ctx context.Context, opts *Ima return nil, nil, err } - config, err = patchImageConfig(config, remote.Descriptors, history, inlineCache, epoch) + config, err = patchImageConfig(config, remote.Descriptors, history, inlineCache, epoch, srcImg) if err != nil { return nil, nil, err } @@ -658,7 +697,7 @@ func parseHistoryFromConfig(dt []byte) ([]ocispecs.History, error) { return config.History, nil } -func patchImageConfig(dt []byte, descs []ocispecs.Descriptor, history []ocispecs.History, cache *exptypes.InlineCacheEntry, epoch *time.Time) ([]byte, error) { +func patchImageConfig(dt []byte, descs []ocispecs.Descriptor, history []ocispecs.History, cache *exptypes.InlineCacheEntry, epoch *time.Time, srcImg *dockerspec.DockerOCIImage) ([]byte, error) { var img ocispecs.Image if err := json.Unmarshal(dt, &img); err != nil { return nil, errors.Wrap(err, "invalid image config for export") @@ -693,6 +732,11 @@ func patchImageConfig(dt []byte, descs []ocispecs.Descriptor, history []ocispecs if epoch != nil { for i, h := range history { + if srcImg != nil && i < len(srcImg.History) && reflect.DeepEqual(h, srcImg.History[i]) { + // Retain the timestamp for the source image layers + // https://github.com/moby/buildkit/issues/4614 + continue + } if h.Created == nil || h.Created.After(*epoch) { history[i].Created = epoch } diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 4bf27a94b723f..6fa9f04a3e5fb 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -270,7 +270,9 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS ds.noinit = true ds.state = *s if img != nil { - ds.image = clampTimes(*img, opt.Epoch) + // timestamps are inherited as-is, regardless to SOURCE_DATE_EPOCH + // https://github.com/moby/buildkit/issues/4614 + ds.image = *img if img.Architecture != "" && img.OS != "" { ds.platform = &ocispecs.Platform{ OS: img.OS, @@ -1885,21 +1887,6 @@ func commonImageNames() []string { return out } -func clampTimes(img dockerspec.DockerOCIImage, tm *time.Time) dockerspec.DockerOCIImage { - if tm == nil { - return img - } - for i, h := range img.History { - if h.Created == nil || h.Created.After(*tm) { - img.History[i].Created = tm - } - } - if img.Created != nil && img.Created.After(*tm) { - img.Created = tm - } - return img -} - func isHTTPSource(src string) bool { return strings.HasPrefix(src, "http://") || strings.HasPrefix(src, "https://") } diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index e49d85b392aa3..a36fe6b9b338a 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -53,6 +53,7 @@ import ( "github.com/moby/buildkit/util/testutil/httpserver" "github.com/moby/buildkit/util/testutil/integration" "github.com/moby/buildkit/util/testutil/workers" + digest "github.com/opencontainers/go-digest" ocispecs "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" "github.com/stretchr/testify/require" @@ -6866,8 +6867,16 @@ RUN rm -f /foo.1 RUN rm -f /foo-2010.1 RUN rm -f /foo-2030.1 `) + // https://explore.ggcr.dev/?image=amd64%2Fdebian%3Abullseye-20230109-slim + sourceImageLayers := []digest.Digest{ + "sha256:8740c948ffd4c816ea7ca963f99ca52f4788baa23f228da9581a9ea2edd3fcd7", + } + sourceImageHistoryTimestamps := []time.Time{ + timeMustParse(t, time.RFC3339Nano, "2023-01-11T02:34:44.402266175Z"), + timeMustParse(t, time.RFC3339Nano, "2023-01-11T02:34:44.829692296Z"), + } - const expectedDigest = "sha256:3eb3c164e3420bbfcf52c34f1e40ee66631d69445e934175b779551c729f80df" + const expectedDigest = "sha256:04e5d0cbee3317c79f50494cfeb4d8a728402a970ef32582ee47c62050037e3f" dir := integration.Tmpdir( t, @@ -6914,13 +6923,24 @@ RUN rm -f /foo-2030.1 _, err = f.Solve(ctx, c, solveOpt, nil) require.NoError(t, err) - desc, manifest := readImage(t, ctx, target) - _, cacheManifest := readImage(t, ctx, target+"-cache") + desc, manifest, img := readImage(t, ctx, target) + _, cacheManifest, _ := readImage(t, ctx, target+"-cache") t.Log("The digest may change depending on the BuildKit version, the snapshotter configuration, etc.") require.Equal(t, expectedDigest, desc.Digest.String()) - // Image layers must have rewritten-timestamp - for _, l := range manifest.Layers { - require.Equal(t, fmt.Sprintf("%d", tm.Unix()), l.Annotations["buildkit/rewritten-timestamp"]) + + // Image history from the source config must remain immutable + for i, tm := range sourceImageHistoryTimestamps { + require.True(t, img.History[i].Created.Equal(tm)) + } + + // Image layers, *except the source layers*, must have rewritten-timestamp + for i, l := range manifest.Layers { + if i < len(sourceImageLayers) { + require.Empty(t, l.Annotations["buildkit/rewritten-timestamp"]) + require.Equal(t, sourceImageLayers[i], l.Digest) + } else { + require.Equal(t, fmt.Sprintf("%d", tm.Unix()), l.Annotations["buildkit/rewritten-timestamp"]) + } } // Cache layers must *not* have rewritten-timestamp for _, l := range cacheManifest.Layers { @@ -6932,21 +6952,34 @@ RUN rm -f /foo-2030.1 delete(solveOpt2.Exports[0].Attrs, "rewrite-timestamp") _, err = f.Solve(ctx, c, solveOpt2, nil) require.NoError(t, err) - _, manifest2 := readImage(t, ctx, target) + _, manifest2, img2 := readImage(t, ctx, target) + for i, tm := range sourceImageHistoryTimestamps { + require.True(t, img2.History[i].Created.Equal(tm)) + } for _, l := range manifest2.Layers { require.Empty(t, l.Annotations["buildkit/rewritten-timestamp"]) } } +func timeMustParse(t *testing.T, layout, value string) time.Time { + tm, err := time.Parse(layout, value) + require.NoError(t, err) + return tm +} + //nolint:revive // context-as-argument: context.Context should be the first parameter of a function -func readImage(t *testing.T, ctx context.Context, ref string) (ocispecs.Descriptor, ocispecs.Manifest) { +func readImage(t *testing.T, ctx context.Context, ref string) (ocispecs.Descriptor, ocispecs.Manifest, ocispecs.Image) { desc, provider, err := contentutil.ProviderFromRef(ref) require.NoError(t, err) dt, err := content.ReadBlob(ctx, provider, desc) require.NoError(t, err) var manifest ocispecs.Manifest require.NoError(t, json.Unmarshal(dt, &manifest)) - return desc, manifest + imgDt, err := content.ReadBlob(ctx, provider, manifest.Config) + require.NoError(t, err) + var img ocispecs.Image + require.NoError(t, json.Unmarshal(imgDt, &img)) + return desc, manifest, img } func testNilContextInSolveGateway(t *testing.T, sb integration.Sandbox) { diff --git a/util/converter/converter.go b/util/converter/converter.go index 6bb8aa858e9ca..4b956f02f1060 100644 --- a/util/converter/converter.go +++ b/util/converter/converter.go @@ -26,12 +26,12 @@ import ( // New returns converter function according to the specified compression type. // If no conversion is needed, this returns nil without error. func New(ctx context.Context, cs content.Store, desc ocispecs.Descriptor, comp compression.Config) (converter.ConvertFunc, error) { - return NewWithRewriteTimestamp(ctx, cs, desc, comp, nil) + return NewWithRewriteTimestamp(ctx, cs, desc, comp, nil, nil) } // NewWithRewriteTimestamp returns converter function according to the specified compression type and the epoch. // If no conversion is needed, this returns nil without error. -func NewWithRewriteTimestamp(ctx context.Context, cs content.Store, desc ocispecs.Descriptor, comp compression.Config, rewriteTimestamp *time.Time) (converter.ConvertFunc, error) { +func NewWithRewriteTimestamp(ctx context.Context, cs content.Store, desc ocispecs.Descriptor, comp compression.Config, rewriteTimestamp *time.Time, immDiffIDs map[digest.Digest]struct{}) (converter.ConvertFunc, error) { needs, err := comp.Type.NeedsConversion(ctx, cs, desc) if err != nil { return nil, errors.Wrapf(err, "failed to determine conversion needs") @@ -53,6 +53,7 @@ func NewWithRewriteTimestamp(ctx context.Context, cs content.Store, desc ocispec c.compress, c.finalize = comp.Type.Compress(ctx, comp) c.decompress = from.Decompress c.rewriteTimestamp = rewriteTimestamp + c.immDiffIDs = immDiffIDs return (&c).convert, nil } @@ -63,6 +64,7 @@ type conversion struct { compress compression.Compressor finalize compression.Finalizer rewriteTimestamp *time.Time + immDiffIDs map[digest.Digest]struct{} // diffIDs of immutable layers } var bufioPool = sync.Pool{ @@ -116,6 +118,7 @@ func (c *conversion) convert(ctx context.Context, cs content.Store, desc ocispec // convert this layer diffID := digest.Canonical.Digester() + origDiffID := digest.Canonical.Digester() decR, err := c.decompress(ctx, cs, desc) if err != nil { return nil, err @@ -123,7 +126,7 @@ func (c *conversion) convert(ctx context.Context, cs content.Store, desc ocispec defer decR.Close() rdr := decR if c.rewriteTimestamp != nil { - tcR := tarconverter.NewReader(decR, rewriteTimestampInTarHeader(*c.rewriteTimestamp)) + tcR := tarconverter.NewReader(io.TeeReader(decR, origDiffID.Hash()), rewriteTimestampInTarHeader(*c.rewriteTimestamp)) defer tcR.Close() rdr = tcR } @@ -136,6 +139,11 @@ func (c *conversion) convert(ctx context.Context, cs content.Store, desc ocispec if err := bufW.Flush(); err != nil { // Flush the buffer return nil, errors.Wrap(err, "failed to flush diff during conversion") } + origDiffIDVal := origDiffID.Digest() + if _, ok := c.immDiffIDs[origDiffIDVal]; ok { + bklog.G(ctx).WithField("blob", desc).Debugf("Not rewriting to apply epoch (immutable diffID %q, computed during conversion)", origDiffIDVal) + return &desc, nil + } labelz[labels.LabelUncompressed] = diffID.Digest().String() // update diffID label if c.rewriteTimestamp != nil { labelz[labelRewrittenTimestamp] = fmt.Sprintf("%d", c.rewriteTimestamp.UTC().Unix())