From 2a6ff3c24fd790e5d42d2eabaf6acf06edfe6975 Mon Sep 17 00:00:00 2001 From: Tianon Gravi Date: Mon, 8 May 2023 11:13:46 -0700 Subject: [PATCH] Use OCI "History" type instead of inventing our own copy The most notable change here is that the OCI's type uses a pointer for `Created`, which we probably should've been too, so most of these changes are accounting for that (and embedding our `Equal` implementation in the one single place it was used). Signed-off-by: Tianon Gravi --- api/server/router/image/image_routes.go | 7 +++++- daemon/containerd/image.go | 22 ++--------------- daemon/containerd/image_builder.go | 16 ++---------- daemon/images/image_import.go | 4 +-- daemon/images/image_list.go | 18 ++++++++------ daemon/images/image_prune.go | 2 +- daemon/images/image_squash.go | 4 +-- image/cache/cache.go | 2 +- image/image.go | 32 +++--------------------- image/image_test.go | 23 ----------------- image/tarexport/load.go | 13 ++++++++-- image/tarexport/save.go | 33 +++++++++++++++---------- 12 files changed, 62 insertions(+), 114 deletions(-) diff --git a/api/server/router/image/image_routes.go b/api/server/router/image/image_routes.go index 9b37d76c56..5c1288f7d4 100644 --- a/api/server/router/image/image_routes.go +++ b/api/server/router/image/image_routes.go @@ -294,13 +294,18 @@ func (ir *imageRouter) toImageInspect(img *image.Image) (*types.ImageInspect, er repoDigests = []string{} } + var created string + if img.Created != nil { + created = img.Created.Format(time.RFC3339Nano) + } + return &types.ImageInspect{ ID: img.ID().String(), RepoTags: repoTags, RepoDigests: repoDigests, Parent: img.Parent.String(), Comment: comment, - Created: img.Created.Format(time.RFC3339Nano), + Created: created, Container: img.Container, ContainerConfig: &img.ContainerConfig, DockerVersion: img.DockerVersion, diff --git a/daemon/containerd/image.go b/daemon/containerd/image.go index ee242132e1..df46b25963 100644 --- a/daemon/containerd/image.go +++ b/daemon/containerd/image.go @@ -68,30 +68,12 @@ func (i *ImageService) GetImage(ctx context.Context, refOrID string, options ima exposedPorts[nat.Port(k)] = v } - derefTimeSafely := func(t *time.Time) time.Time { - if t != nil { - return *t - } - return time.Time{} - } - - var imgHistory []image.History - for _, h := range ociimage.History { - imgHistory = append(imgHistory, image.History{ - Created: derefTimeSafely(h.Created), - Author: h.Author, - CreatedBy: h.CreatedBy, - Comment: h.Comment, - EmptyLayer: h.EmptyLayer, - }) - } - img := image.NewImage(image.ID(desc.Digest)) img.V1Image = image.V1Image{ ID: string(desc.Digest), OS: ociimage.OS, Architecture: ociimage.Architecture, - Created: derefTimeSafely(ociimage.Created), + Created: ociimage.Created, Config: &containertypes.Config{ Entrypoint: ociimage.Config.Entrypoint, Env: ociimage.Config.Env, @@ -106,7 +88,7 @@ func (i *ImageService) GetImage(ctx context.Context, refOrID string, options ima } img.RootFS = rootfs - img.History = imgHistory + img.History = ociimage.History if options.Details { lastUpdated := time.Unix(0, 0) diff --git a/daemon/containerd/image_builder.go b/daemon/containerd/image_builder.go index 05338da2da..3ee8aa7642 100644 --- a/daemon/containerd/image_builder.go +++ b/daemon/containerd/image_builder.go @@ -402,21 +402,9 @@ func (i *ImageService) CreateImage(ctx context.Context, config []byte, parent st exposedPorts[string(k)] = v } - var ociHistory []ocispec.History - for _, history := range imgToCreate.History { - created := history.Created - ociHistory = append(ociHistory, ocispec.History{ - Created: &created, - CreatedBy: history.CreatedBy, - Author: history.Author, - Comment: history.Comment, - EmptyLayer: history.EmptyLayer, - }) - } - // make an ocispec.Image from the docker/image.Image ociImgToCreate := ocispec.Image{ - Created: &imgToCreate.Created, + Created: imgToCreate.Created, Author: imgToCreate.Author, Platform: ocispec.Platform{ Architecture: imgToCreate.Architecture, @@ -437,7 +425,7 @@ func (i *ImageService) CreateImage(ctx context.Context, config []byte, parent st StopSignal: imgToCreate.Config.StopSignal, }, RootFS: rootfs, - History: ociHistory, + History: imgToCreate.History, } var layers []ocispec.Descriptor diff --git a/daemon/images/image_import.go b/daemon/images/image_import.go index dd32b11a90..5c26cec2fb 100644 --- a/daemon/images/image_import.go +++ b/daemon/images/image_import.go @@ -58,7 +58,7 @@ func (i *ImageService) ImportImage(ctx context.Context, newRef reference.Named, Architecture: platform.Architecture, Variant: platform.Variant, OS: platform.OS, - Created: created, + Created: &created, Comment: msg, }, RootFS: &image.RootFS{ @@ -66,7 +66,7 @@ func (i *ImageService) ImportImage(ctx context.Context, newRef reference.Named, DiffIDs: []layer.DiffID{l.DiffID()}, }, History: []image.History{{ - Created: created, + Created: &created, Comment: msg, }}, }) diff --git a/daemon/images/image_list.go b/daemon/images/image_list.go index 00161434c4..682d5f9ea1 100644 --- a/daemon/images/image_list.go +++ b/daemon/images/image_list.go @@ -53,8 +53,8 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions) } // Resolve multiple values to the oldest image, // equivalent to ANDing all the values together. - if beforeFilter.IsZero() || beforeFilter.After(img.Created) { - beforeFilter = img.Created + if img.Created != nil && (beforeFilter.IsZero() || beforeFilter.After(*img.Created)) { + beforeFilter = *img.Created } return nil }) @@ -69,8 +69,8 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions) } // Resolve multiple values to the newest image, // equivalent to ANDing all the values together. - if sinceFilter.Before(img.Created) { - sinceFilter = img.Created + if img.Created != nil && sinceFilter.Before(*img.Created) { + sinceFilter = *img.Created } return nil }) @@ -97,10 +97,10 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions) default: } - if !beforeFilter.IsZero() && !img.Created.Before(beforeFilter) { + if !beforeFilter.IsZero() && (img.Created == nil || !img.Created.Before(beforeFilter)) { continue } - if !sinceFilter.IsZero() && !img.Created.After(sinceFilter) { + if !sinceFilter.IsZero() && (img.Created == nil || !img.Created.After(sinceFilter)) { continue } @@ -256,10 +256,14 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions) } func newImageSummary(image *image.Image, size int64) *types.ImageSummary { + var created int64 + if image.Created != nil { + created = image.Created.Unix() + } summary := &types.ImageSummary{ ParentID: image.Parent.String(), ID: image.ID().String(), - Created: image.Created.Unix(), + Created: created, Size: size, // -1 indicates that the value has not been set (avoids ambiguity // between 0 (default) and "not set". We cannot use a pointer (nil) diff --git a/daemon/images/image_prune.go b/daemon/images/image_prune.go index 4aefd201e6..174ff3f70e 100644 --- a/daemon/images/image_prune.go +++ b/daemon/images/image_prune.go @@ -75,7 +75,7 @@ func (i *ImageService) ImagesPrune(ctx context.Context, pruneFilters filters.Arg if len(i.referenceStore.References(dgst)) == 0 && len(i.imageStore.Children(id)) != 0 { continue } - if !until.IsZero() && img.Created.After(until) { + if !until.IsZero() && (img.Created == nil || img.Created.After(until)) { continue } if img.Config != nil && !matchLabels(pruneFilters, img.Config.Labels) { diff --git a/daemon/images/image_squash.go b/daemon/images/image_squash.go index 71d7a6488e..aa53bf90ea 100644 --- a/daemon/images/image_squash.go +++ b/daemon/images/image_squash.go @@ -76,10 +76,10 @@ func (i *ImageService) SquashImage(id, parent string) (string, error) { } newImage.History = append(newImage.History, image.History{ - Created: now, + Created: &now, Comment: historyComment, }) - newImage.Created = now + newImage.Created = &now b, err := json.Marshal(&newImage) if err != nil { diff --git a/image/cache/cache.go b/image/cache/cache.go index 6d3f4c57b5..b36534e572 100644 --- a/image/cache/cache.go +++ b/image/cache/cache.go @@ -227,7 +227,7 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain if compare(&img.ContainerConfig, config) { // check for the most up to date match - if match == nil || match.Created.Before(img.Created) { + if img.Created != nil && (match == nil || match.Created.Before(*img.Created)) { match = img } } diff --git a/image/image.go b/image/image.go index e904d1e7e6..d3624285fd 100644 --- a/image/image.go +++ b/image/image.go @@ -4,7 +4,6 @@ import ( "encoding/json" "errors" "io" - "reflect" "runtime" "strings" "time" @@ -53,7 +52,7 @@ type V1Image struct { Comment string `json:"comment,omitempty"` // Created is the timestamp at which the image was created - Created time.Time `json:"created"` + Created *time.Time `json:"created"` // Container is the ID of the container that was used to create the image. // @@ -261,44 +260,21 @@ func NewChildImage(img *Image, child ChildConfig, os string) *Image { } // History stores build commands that were used to create an image -type History struct { - // Created is the timestamp at which the image was created - Created time.Time `json:"created"` - // Author is the name of the author that was specified when committing the - // image, or as specified through MAINTAINER (deprecated) in the Dockerfile. - Author string `json:"author,omitempty"` - // CreatedBy keeps the Dockerfile command used while building the image - CreatedBy string `json:"created_by,omitempty"` - // Comment is the commit message that was set when committing the image - Comment string `json:"comment,omitempty"` - // EmptyLayer is set to true if this history item did not generate a - // layer. Otherwise, the history item is associated with the next - // layer in the RootFS section. - EmptyLayer bool `json:"empty_layer,omitempty"` -} +type History = ocispec.History // NewHistory creates a new history struct from arguments, and sets the created // time to the current time in UTC func NewHistory(author, comment, createdBy string, isEmptyLayer bool) History { + now := time.Now().UTC() return History{ Author: author, - Created: time.Now().UTC(), + Created: &now, CreatedBy: createdBy, Comment: comment, EmptyLayer: isEmptyLayer, } } -// Equal compares two history structs for equality -func (h History) Equal(i History) bool { - if !h.Created.Equal(i.Created) { - return false - } - i.Created = h.Created - - return reflect.DeepEqual(h, i) -} - // Exporter provides interface for loading and saving images type Exporter interface { Load(io.ReadCloser, io.Writer, bool) error diff --git a/image/image_test.go b/image/image_test.go index 21f81c768d..b174b80908 100644 --- a/image/image_test.go +++ b/image/image_test.go @@ -56,29 +56,6 @@ func TestMarshalKeyOrder(t *testing.T) { } } -const sampleHistoryJSON = `{ - "created": "2021-01-13T09:35:56Z", - "created_by": "image_test.go" -}` - -func TestHistoryEqual(t *testing.T) { - h := historyFromJSON(t, sampleHistoryJSON) - hCopy := h - assert.Check(t, h.Equal(hCopy)) - - hUTC := historyFromJSON(t, `{"created": "2021-01-13T14:00:00Z"}`) - hOffset0 := historyFromJSON(t, `{"created": "2021-01-13T14:00:00+00:00"}`) - assert.Check(t, hUTC.Created != hOffset0.Created) - assert.Check(t, hUTC.Equal(hOffset0)) -} - -func historyFromJSON(t *testing.T, historyJSON string) History { - var h History - err := json.Unmarshal([]byte(historyJSON), &h) - assert.Check(t, err) - return h -} - func TestImage(t *testing.T) { cid := "50a16564e727" config := &container.Config{ diff --git a/image/tarexport/load.go b/image/tarexport/load.go index d9f3873081..4cc1c2263b 100644 --- a/image/tarexport/load.go +++ b/image/tarexport/load.go @@ -7,6 +7,7 @@ import ( "io" "os" "path/filepath" + "reflect" "runtime" "github.com/docker/distribution" @@ -396,8 +397,16 @@ func checkValidParent(img, parent *image.Image) bool { if len(img.History)-len(parent.History) != 1 { return false } - for i, h := range parent.History { - if !h.Equal(img.History[i]) { + for i, hP := range parent.History { + hC := img.History[i] + if (hP.Created == nil) != (hC.Created == nil) { + return false + } + if hP.Created != nil && !hP.Created.Equal(*hC.Created) { + return false + } + hC.Created = hP.Created + if !reflect.DeepEqual(hP, hC) { return false } } diff --git a/image/tarexport/save.go b/image/tarexport/save.go index 1caa92351a..d6067c86e1 100644 --- a/image/tarexport/save.go +++ b/image/tarexport/save.go @@ -379,10 +379,11 @@ func (s *saveSession) saveImage(id image.ID) (map[layer.DiffID]distribution.Desc var layers []digest.Digest var foreignSrcs map[layer.DiffID]distribution.Descriptor for i, diffID := range img.RootFS.DiffIDs { + v1ImgCreated := time.Unix(0, 0) v1Img := image.V1Image{ // This is for backward compatibility used for // pre v1.9 docker. - Created: time.Unix(0, 0), + Created: &v1ImgCreated, } if i == len(img.RootFS.DiffIDs)-1 { v1Img = img.V1Image @@ -422,26 +423,30 @@ func (s *saveSession) saveImage(id image.ID) (map[layer.DiffID]distribution.Desc if err := os.MkdirAll(blobDir, 0o755); err != nil { return nil, err } - if err := system.Chtimes(blobDir, img.Created, img.Created); err != nil { - return nil, err - } - if err := system.Chtimes(filepath.Dir(blobDir), img.Created, img.Created); err != nil { - return nil, err + if img.Created != nil { + if err := system.Chtimes(blobDir, *img.Created, *img.Created); err != nil { + return nil, err + } + if err := system.Chtimes(filepath.Dir(blobDir), *img.Created, *img.Created); err != nil { + return nil, err + } } configFile := filepath.Join(blobDir, dgst.Encoded()) if err := os.WriteFile(configFile, img.RawJSON(), 0o644); err != nil { return nil, err } - if err := system.Chtimes(configFile, img.Created, img.Created); err != nil { - return nil, err + if img.Created != nil { + if err := system.Chtimes(configFile, *img.Created, *img.Created); err != nil { + return nil, err + } } s.images[id].layers = layers return foreignSrcs, nil } -func (s *saveSession) saveLayer(id layer.ChainID, legacyImg image.V1Image, createdTime time.Time) (distribution.Descriptor, error) { +func (s *saveSession) saveLayer(id layer.ChainID, legacyImg image.V1Image, createdTime *time.Time) (distribution.Descriptor, error) { if _, exists := s.savedLayers[legacyImg.ID]; exists { return distribution.Descriptor{}, nil } @@ -499,10 +504,12 @@ func (s *saveSession) saveLayer(id layer.ChainID, legacyImg image.V1Image, creat return distribution.Descriptor{}, err } - for _, fname := range []string{outDir, configPath, layerPath} { - // todo: maybe save layer created timestamp? - if err := system.Chtimes(fname, createdTime, createdTime); err != nil { - return distribution.Descriptor{}, errors.Wrap(err, "could not set layer timestamp") + if createdTime != nil { + for _, fname := range []string{outDir, configPath, layerPath} { + // todo: maybe save layer created timestamp? + if err := system.Chtimes(fname, *createdTime, *createdTime); err != nil { + return distribution.Descriptor{}, errors.Wrap(err, "could not set layer timestamp") + } } }