Skip to content

replace json.Unmarshal with NewFromJSON in Create#41701

Merged
thaJeztah merged 1 commit into
moby:masterfrom
jimlinntu:reuse
Jan 12, 2021
Merged

replace json.Unmarshal with NewFromJSON in Create#41701
thaJeztah merged 1 commit into
moby:masterfrom
jimlinntu:reuse

Conversation

@jimlinntu

@jimlinntu jimlinntu commented Nov 21, 2020

Copy link
Copy Markdown
Contributor

fixes #41683

- What I did

Reuse NewFromJSON to handle no rootfs key situation in image/store.go to solve the issue in #41683

- How I did it

Simply replace the json.Unmarshal function with NewFromJSON in image/image.go

- How to verify it

Running this (mainly modified from #41683) will show invalid image JSON, no RootFS:

package main

import (
    "github.com/docker/docker/image"
    "os"
    "io/ioutil"
    "runtime"
    "github.com/docker/docker/layer"
    "fmt"
)


type mockLayerGetReleaser struct{}

func (ls *mockLayerGetReleaser) Get(layer.ChainID) (layer.Layer, error) {
        return nil, nil
}

func (ls *mockLayerGetReleaser) Release(layer.Layer) ([]layer.Metadata, error) {
        return nil, nil
}

func main(){
    tmpdir, err := ioutil.TempDir("", "images-fs-store")
    defer os.RemoveAll(tmpdir)
    if err != nil {
        fmt.Print(err)
        os.Exit(1)
    }
    fsBackend, err := image.NewFSStoreBackend(tmpdir)
    if err != nil {
        fmt.Print(err)
        os.Exit(1)
    }
    mlgrMap := make(map[string]image.LayerGetReleaser)
    mlgrMap[runtime.GOOS] = &mockLayerGetReleaser{}
    store, err := image.NewImageStore(fsBackend, mlgrMap)
    if err != nil {
        fmt.Print(err)
        os.Exit(1)
    }
    _, err = store.Create([]byte(`{}`))
    if err != nil {
        fmt.Print(err)
        os.Exit(1)
    }
}

- Description for the changelog

Handle an image json that rootfs key is not present.

- A picture of a cute animal (not mandatory but encouraged)
🐕

Comment thread image/store.go Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So one difference is that NewFromJSON also propagates the rawJSON field;

img.rawJSON = src

At a glance, I don't think it makes a difference for how it's used here (but thought I'd at least leave a comment)

@jimlinntu

Copy link
Copy Markdown
Contributor Author

Are there anything I can help for this issue?

@tianon

tianon commented Dec 31, 2020

Copy link
Copy Markdown
Member

Is it possible to write a unit test for the edge case this is fixing? 😇

@jimlinntu

jimlinntu commented Jan 10, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for your advice!
I have added a unit test in image/store_test.go
You can test that by TESTDIRS='github.com/docker/docker/image' TESTFLAGS="-test.run ^TestCreate$" hack/test/unit

ping @tianon

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like the linter was failing; could you also squash the commits? I think it's ok to have the test and code changes in a single commit for this PR

Comment thread image/store_test.go Outdated

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@tianon PTAL

@tianon tianon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅 LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/image/store.go Empty configuration causes runtime panic

3 participants