Avoid applying SOURCE_DATE_EPOCH to base images
#4663
Conversation
AkihiroSuda
force-pushed
the
fix-4614
branch
from
1902aaf
to
4932ec6
Compare
AkihiroSuda
force-pushed
the
fix-4614
branch
2 times, most recently
from
3c33c2e
to
14960a0
Compare
examples/dockerfile2llb/main.go
Outdated
| @@ -31,6 +32,7 @@ func xmain() error { | |||
| var opt buildOpt | |||
| flag.StringVar(&opt.target, "target", "", "target stage") | |||
| flag.StringVar(&opt.partialImageConfigFile, "partial-image-config-file", "", "Output partial image config as a JSON file") | |||
| flag.StringVar(&opt.sourceImageConfigFile, "source-image-config-file", "", "Output source image config as a JSON file") | |||
There was a problem hiding this comment.
I think it should be called base image, not source image here and everywhere else.
There was a problem hiding this comment.
Isn't "base image" confusing in this case?
FROM busybox AS foo
# the "base image" of bar is busybox, not foo
FROM foo AS bar
There was a problem hiding this comment.
But isn't the point of this feature to keep the timestamps from "busybox" in this case? So yes it is the base image.
In here I think it makes sense to look at the base image based on the layers from the result artifact, not as a structure of Dockerfile stages. This is also how it works in provenance when we export layers or how Docker Scout detects if the base image should be updated (as opposed to materials that is an array and not a single image).
There was a problem hiding this comment.
Renamed to "base" images
In here I think it makes sense to look at the base image based on the layers from the result artifact, not as a structure of Dockerfile stages.
I'm not sure about this. Can this be discussed in a separate PR?
AkihiroSuda
force-pushed
the
fix-4614
branch
2 times, most recently
from
586a136
to
acaafbf
Compare
The base image config will be used later for avoiding applying `SOURCE_DATE_EPOCH` to the base image layers (issue 4614). The exporter stores this as the `ExporterImageBaseConfigKey` metadata. NOTE: For a multi-stage Dockerfile like below, the base image refers to `busybox`, not to `foo`: ```dockerfile FROM busybox AS foo FROM foo AS bar ``` Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
AkihiroSuda
force-pushed
the
fix-4614
branch
from
acaafbf
to
baaa64d
Compare
AkihiroSuda
changed the title
SOURCE_DATE_EPOCH to source imagesSOURCE_DATE_EPOCH to base images
| var immDiffID digest.Digest | ||
| if baseImg != nil && i < len(baseImg.RootFS.DiffIDs) { | ||
| immDiffID = baseImg.RootFS.DiffIDs[i] | ||
| if immDiffID == diffID { |
There was a problem hiding this comment.
iiuc this looks for the index match for individual layers. But it should check that full baseImg roofs is a subset in the beginning of the exported image.
There was a problem hiding this comment.
Added var divergedFromBase bool to check this
There was a problem hiding this comment.
This should be a check before processing. Atm some layers can already be processed before the validation fails.
The exporter fetches the base image config via the `ExporterImageBaseConfigKey` metadata to detect the immutable layer diffIDs and the history objects. Fix issue 4614 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
AkihiroSuda
force-pushed
the
fix-4614
branch
from
baaa64d
to
cc14a06
Compare
| 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{} |
There was a problem hiding this comment.
I don't understand this construction in here. Why is a map with a single item passed? Instead of for example just setting epoch nil?
There was a problem hiding this comment.
Why is a map with a single item passed?
The converter pkg is designed to cover wider usecases that are not needed by exporter/containerimage.
Instead of for example just setting epoch nil?
Because the converter may not make the final decision until computing the diff ID (not always available in the label)
The exporter fetches the base image config via the
ExporterImageBaseConfigKeymetadata to detect the immutable layer diffIDs and the history objects.Fix #4614
NOTE: For a multi-stage Dockerfile like below, the base image refers to
busybox, not tofoo: