diff --git a/pkg/kubelet/dockershim/docker_image.go b/pkg/kubelet/dockershim/docker_image.go index e4c450bc8b0c6..7ba73a803da66 100644 --- a/pkg/kubelet/dockershim/docker_image.go +++ b/pkg/kubelet/dockershim/docker_image.go @@ -120,24 +120,27 @@ func (ds *dockerService) RemoveImage(_ context.Context, r *runtimeapi.RemoveImag // TODO: We assume image.Image is image ID here, which is true in the current implementation // of kubelet, but we should still clarify this in CRI. imageInspect, err := ds.client.InspectImageByID(image.Image) - if err == nil && imageInspect != nil && len(imageInspect.RepoTags) > 1 { - for _, tag := range imageInspect.RepoTags { - if _, err := ds.client.RemoveImage(tag, dockertypes.ImageRemoveOptions{PruneChildren: true}); err != nil && !libdocker.IsImageNotFoundError(err) { - return nil, err - } - } - return &runtimeapi.RemoveImageResponse{}, nil - } + // dockerclient.InspectImageByID doesn't work with digest and repoTags, // it is safe to continue removing it since there is another check below. if err != nil && !libdocker.IsImageNotFoundError(err) { return nil, err } - _, err = ds.client.RemoveImage(image.Image, dockertypes.ImageRemoveOptions{PruneChildren: true}) - if err != nil && !libdocker.IsImageNotFoundError(err) { - return nil, err + // An image can have different numbers of RepoTags and RepoDigests. + // Iterating over both of them plus the image ID ensures the image really got removed. + // It also prevents images from being deleted, which actually are deletable using this approach. + var images []string + images = append(images, imageInspect.RepoTags...) + images = append(images, imageInspect.RepoDigests...) + images = append(images, image.Image) + + for _, image := range images { + if _, err := ds.client.RemoveImage(image, dockertypes.ImageRemoveOptions{PruneChildren: true}); err != nil && !libdocker.IsImageNotFoundError(err) { + return nil, err + } } + return &runtimeapi.RemoveImageResponse{}, nil } diff --git a/pkg/kubelet/dockershim/docker_image_test.go b/pkg/kubelet/dockershim/docker_image_test.go index 51363cab4bf10..3dbd5ef2b3a60 100644 --- a/pkg/kubelet/dockershim/docker_image_test.go +++ b/pkg/kubelet/dockershim/docker_image_test.go @@ -30,22 +30,57 @@ import ( ) func TestRemoveImage(t *testing.T) { - ds, fakeDocker, _ := newTestDockerService() - id := "1111" - fakeDocker.InjectImageInspects([]dockertypes.ImageInspect{{ID: id, RepoTags: []string{"foo"}}}) - ds.RemoveImage(getTestCTX(), &runtimeapi.RemoveImageRequest{Image: &runtimeapi.ImageSpec{Image: id}}) - fakeDocker.AssertCallDetails(libdocker.NewCalledDetail("inspect_image", nil), - libdocker.NewCalledDetail("remove_image", []interface{}{id, dockertypes.ImageRemoveOptions{PruneChildren: true}})) -} + tests := map[string]struct { + image dockertypes.ImageInspect + calledDetails []libdocker.CalledDetail + }{ + "single tag": { + dockertypes.ImageInspect{ID: "1111", RepoTags: []string{"foo"}}, + []libdocker.CalledDetail{ + libdocker.NewCalledDetail("inspect_image", nil), + libdocker.NewCalledDetail("remove_image", []interface{}{"foo", dockertypes.ImageRemoveOptions{PruneChildren: true}}), + libdocker.NewCalledDetail("remove_image", []interface{}{"1111", dockertypes.ImageRemoveOptions{PruneChildren: true}}), + }, + }, + "multiple tags": { + dockertypes.ImageInspect{ID: "2222", RepoTags: []string{"foo", "bar"}}, + []libdocker.CalledDetail{ + libdocker.NewCalledDetail("inspect_image", nil), + libdocker.NewCalledDetail("remove_image", []interface{}{"foo", dockertypes.ImageRemoveOptions{PruneChildren: true}}), + libdocker.NewCalledDetail("remove_image", []interface{}{"bar", dockertypes.ImageRemoveOptions{PruneChildren: true}}), + libdocker.NewCalledDetail("remove_image", []interface{}{"2222", dockertypes.ImageRemoveOptions{PruneChildren: true}}), + }, + }, + "single tag multiple repo digests": { + dockertypes.ImageInspect{ID: "3333", RepoTags: []string{"foo"}, RepoDigests: []string{"foo@3333", "example.com/foo@3333"}}, + []libdocker.CalledDetail{ + libdocker.NewCalledDetail("inspect_image", nil), + libdocker.NewCalledDetail("remove_image", []interface{}{"foo", dockertypes.ImageRemoveOptions{PruneChildren: true}}), + libdocker.NewCalledDetail("remove_image", []interface{}{"foo@3333", dockertypes.ImageRemoveOptions{PruneChildren: true}}), + libdocker.NewCalledDetail("remove_image", []interface{}{"example.com/foo@3333", dockertypes.ImageRemoveOptions{PruneChildren: true}}), + libdocker.NewCalledDetail("remove_image", []interface{}{"3333", dockertypes.ImageRemoveOptions{PruneChildren: true}}), + }, + }, + "no tags multiple repo digests": { + dockertypes.ImageInspect{ID: "4444", RepoTags: []string{}, RepoDigests: []string{"foo@4444", "example.com/foo@4444"}}, + []libdocker.CalledDetail{ + libdocker.NewCalledDetail("inspect_image", nil), + libdocker.NewCalledDetail("remove_image", []interface{}{"foo@4444", dockertypes.ImageRemoveOptions{PruneChildren: true}}), + libdocker.NewCalledDetail("remove_image", []interface{}{"example.com/foo@4444", dockertypes.ImageRemoveOptions{PruneChildren: true}}), + libdocker.NewCalledDetail("remove_image", []interface{}{"4444", dockertypes.ImageRemoveOptions{PruneChildren: true}}), + }, + }, + } -func TestRemoveImageWithMultipleTags(t *testing.T) { - ds, fakeDocker, _ := newTestDockerService() - id := "1111" - fakeDocker.InjectImageInspects([]dockertypes.ImageInspect{{ID: id, RepoTags: []string{"foo", "bar"}}}) - ds.RemoveImage(getTestCTX(), &runtimeapi.RemoveImageRequest{Image: &runtimeapi.ImageSpec{Image: id}}) - fakeDocker.AssertCallDetails(libdocker.NewCalledDetail("inspect_image", nil), - libdocker.NewCalledDetail("remove_image", []interface{}{"foo", dockertypes.ImageRemoveOptions{PruneChildren: true}}), - libdocker.NewCalledDetail("remove_image", []interface{}{"bar", dockertypes.ImageRemoveOptions{PruneChildren: true}})) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + ds, fakeDocker, _ := newTestDockerService() + fakeDocker.InjectImageInspects([]dockertypes.ImageInspect{test.image}) + ds.RemoveImage(getTestCTX(), &runtimeapi.RemoveImageRequest{Image: &runtimeapi.ImageSpec{Image: test.image.ID}}) + err := fakeDocker.AssertCallDetails(test.calledDetails...) + assert.NoError(t, err) + }) + } } func TestPullWithJSONError(t *testing.T) { diff --git a/pkg/kubelet/dockershim/libdocker/fake_client.go b/pkg/kubelet/dockershim/libdocker/fake_client.go index 54dafcaf767b7..5e14a63a0486d 100644 --- a/pkg/kubelet/dockershim/libdocker/fake_client.go +++ b/pkg/kubelet/dockershim/libdocker/fake_client.go @@ -37,14 +37,14 @@ import ( "k8s.io/apimachinery/pkg/util/clock" ) -type calledDetail struct { +type CalledDetail struct { name string arguments []interface{} } // NewCalledDetail create a new call detail item. -func NewCalledDetail(name string, arguments []interface{}) calledDetail { - return calledDetail{name: name, arguments: arguments} +func NewCalledDetail(name string, arguments []interface{}) CalledDetail { + return CalledDetail{name: name, arguments: arguments} } // FakeDockerClient is a simple fake docker client, so that kubelet can be run for testing without requiring a real docker setup. @@ -58,7 +58,7 @@ type FakeDockerClient struct { Images []dockertypes.ImageSummary ImageIDsNeedingAuth map[string]dockertypes.AuthConfig Errors map[string]error - called []calledDetail + called []CalledDetail pulled []string EnableTrace bool RandGenerator *rand.Rand @@ -132,7 +132,7 @@ func (f *FakeDockerClient) WithRandSource(source rand.Source) *FakeDockerClient return f } -func (f *FakeDockerClient) appendCalled(callDetail calledDetail) { +func (f *FakeDockerClient) appendCalled(callDetail CalledDetail) { if f.EnableTrace { f.called = append(f.called, callDetail) } @@ -183,7 +183,7 @@ func (f *FakeDockerClient) ClearErrors() { func (f *FakeDockerClient) ClearCalls() { f.Lock() defer f.Unlock() - f.called = []calledDetail{} + f.called = []CalledDetail{} f.pulled = []string{} f.Created = []string{} f.Started = []string{} @@ -286,7 +286,7 @@ func (f *FakeDockerClient) AssertCalls(calls []string) (err error) { return } -func (f *FakeDockerClient) AssertCallDetails(calls ...calledDetail) (err error) { +func (f *FakeDockerClient) AssertCallDetails(calls ...CalledDetail) (err error) { f.Lock() defer f.Unlock() @@ -390,7 +390,7 @@ func (f *FakeDockerClient) popError(op string) error { func (f *FakeDockerClient) ListContainers(options dockertypes.ContainerListOptions) ([]dockertypes.Container, error) { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "list"}) + f.appendCalled(CalledDetail{name: "list"}) err := f.popError("list") containerList := append([]dockertypes.Container{}, f.RunningContainerList...) if options.All { @@ -470,7 +470,7 @@ func toDockerContainerStatus(state string) string { func (f *FakeDockerClient) InspectContainer(id string) (*dockertypes.ContainerJSON, error) { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "inspect_container"}) + f.appendCalled(CalledDetail{name: "inspect_container"}) err := f.popError("inspect_container") if container, ok := f.ContainerMap[id]; ok { return container, err @@ -487,7 +487,7 @@ func (f *FakeDockerClient) InspectContainer(id string) (*dockertypes.ContainerJS func (f *FakeDockerClient) InspectContainerWithSize(id string) (*dockertypes.ContainerJSON, error) { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "inspect_container_withsize"}) + f.appendCalled(CalledDetail{name: "inspect_container_withsize"}) err := f.popError("inspect_container_withsize") if container, ok := f.ContainerMap[id]; ok { return container, err @@ -504,7 +504,7 @@ func (f *FakeDockerClient) InspectContainerWithSize(id string) (*dockertypes.Con func (f *FakeDockerClient) InspectImageByRef(name string) (*dockertypes.ImageInspect, error) { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "inspect_image"}) + f.appendCalled(CalledDetail{name: "inspect_image"}) if err := f.popError("inspect_image"); err != nil { return nil, err } @@ -519,7 +519,7 @@ func (f *FakeDockerClient) InspectImageByRef(name string) (*dockertypes.ImageIns func (f *FakeDockerClient) InspectImageByID(name string) (*dockertypes.ImageInspect, error) { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "inspect_image"}) + f.appendCalled(CalledDetail{name: "inspect_image"}) if err := f.popError("inspect_image"); err != nil { return nil, err } @@ -555,7 +555,7 @@ func GetFakeContainerID(name string) string { func (f *FakeDockerClient) CreateContainer(c dockertypes.ContainerCreateConfig) (*dockercontainer.ContainerCreateCreatedBody, error) { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "create"}) + f.appendCalled(CalledDetail{name: "create"}) if err := f.popError("create"); err != nil { return nil, err } @@ -581,7 +581,7 @@ func (f *FakeDockerClient) CreateContainer(c dockertypes.ContainerCreateConfig) func (f *FakeDockerClient) StartContainer(id string) error { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "start"}) + f.appendCalled(CalledDetail{name: "start"}) if err := f.popError("start"); err != nil { return err } @@ -619,7 +619,7 @@ func (f *FakeDockerClient) StartContainer(id string) error { func (f *FakeDockerClient) StopContainer(id string, timeout time.Duration) error { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "stop"}) + f.appendCalled(CalledDetail{name: "stop"}) if err := f.popError("stop"); err != nil { return err } @@ -657,7 +657,7 @@ func (f *FakeDockerClient) StopContainer(id string, timeout time.Duration) error func (f *FakeDockerClient) RemoveContainer(id string, opts dockertypes.ContainerRemoveOptions) error { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "remove"}) + f.appendCalled(CalledDetail{name: "remove"}) err := f.popError("remove") if err != nil { return err @@ -693,7 +693,7 @@ func (f *FakeDockerClient) UpdateContainerResources(id string, updateConfig dock func (f *FakeDockerClient) Logs(id string, opts dockertypes.ContainerLogsOptions, sopts StreamOptions) error { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "logs"}) + f.appendCalled(CalledDetail{name: "logs"}) return f.popError("logs") } @@ -710,7 +710,7 @@ func (f *FakeDockerClient) isAuthorizedForImage(image string, auth dockertypes.A func (f *FakeDockerClient) PullImage(image string, auth dockertypes.AuthConfig, opts dockertypes.ImagePullOptions) error { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "pull"}) + f.appendCalled(CalledDetail{name: "pull"}) err := f.popError("pull") if err == nil { if !f.isAuthorizedForImage(image, auth) { @@ -742,21 +742,21 @@ func (f *FakeDockerClient) CreateExec(id string, opts dockertypes.ExecConfig) (* f.Lock() defer f.Unlock() f.execCmd = opts.Cmd - f.appendCalled(calledDetail{name: "create_exec"}) + f.appendCalled(CalledDetail{name: "create_exec"}) return &dockertypes.IDResponse{ID: "12345678"}, nil } func (f *FakeDockerClient) StartExec(startExec string, opts dockertypes.ExecStartCheck, sopts StreamOptions) error { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "start_exec"}) + f.appendCalled(CalledDetail{name: "start_exec"}) return nil } func (f *FakeDockerClient) AttachToContainer(id string, opts dockertypes.ContainerAttachOptions, sopts StreamOptions) error { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "attach"}) + f.appendCalled(CalledDetail{name: "attach"}) return nil } @@ -767,7 +767,7 @@ func (f *FakeDockerClient) InspectExec(id string) (*dockertypes.ContainerExecIns func (f *FakeDockerClient) ListImages(opts dockertypes.ImageListOptions) ([]dockertypes.ImageSummary, error) { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "list_images"}) + f.appendCalled(CalledDetail{name: "list_images"}) err := f.popError("list_images") return f.Images, err } @@ -775,7 +775,7 @@ func (f *FakeDockerClient) ListImages(opts dockertypes.ImageListOptions) ([]dock func (f *FakeDockerClient) RemoveImage(image string, opts dockertypes.ImageRemoveOptions) ([]dockertypes.ImageDeleteResponseItem, error) { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "remove_image", arguments: []interface{}{image, opts}}) + f.appendCalled(CalledDetail{name: "remove_image", arguments: []interface{}{image, opts}}) err := f.popError("remove_image") if err == nil { for i := range f.Images { @@ -833,14 +833,14 @@ func (f *FakeDockerClient) updateContainerStatus(id, status string) { func (f *FakeDockerClient) ResizeExecTTY(id string, height, width uint) error { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "resize_exec"}) + f.appendCalled(CalledDetail{name: "resize_exec"}) return nil } func (f *FakeDockerClient) ResizeContainerTTY(id string, height, width uint) error { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "resize_container"}) + f.appendCalled(CalledDetail{name: "resize_container"}) return nil } @@ -884,7 +884,7 @@ func dockerTimestampToString(t time.Time) string { func (f *FakeDockerClient) ImageHistory(id string) ([]dockerimagetypes.HistoryResponseItem, error) { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "image_history"}) + f.appendCalled(CalledDetail{name: "image_history"}) history := f.ImageHistoryMap[id] return history, nil } @@ -916,6 +916,6 @@ func (f *FakeDockerPuller) GetImageRef(image string) (string, error) { func (f *FakeDockerClient) GetContainerStats(id string) (*dockertypes.StatsJSON, error) { f.Lock() defer f.Unlock() - f.appendCalled(calledDetail{name: "getContainerStats"}) + f.appendCalled(CalledDetail{name: "getContainerStats"}) return nil, fmt.Errorf("not implemented") }