Skip to content

[24.0 backport] plugins: Fix panic when fetching by digest #47324

Merged
thaJeztah merged 2 commits intomoby:24.0from
thaJeztah:24.0_backport_plugin-install-digest
Feb 5, 2024
Merged

[24.0 backport] plugins: Fix panic when fetching by digest #47324
thaJeztah merged 2 commits intomoby:24.0from
thaJeztah:24.0_backport_plugin-install-digest

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Feb 5, 2024

- What I did

Added a test for, and fixed a panic at:

moby/plugin/fetch_linux.go

Lines 202 to 203 in 71fa3ab

tn := reference.TagNameOnly(ref)
tagged := tn.(reference.Tagged)

when doing something like:

docker plugin install vieux/sshfs@sha256:1d3c3e42c12138da5ef7873b97f7f32cf99fb6edde75fa4f0bcf9ed277855811

By checking if the cast is safe, and only printing the tag if there is a tag to print.

- How I did it
With tag:

 docker plugin install vieux/sshfs
Plugin "vieux/sshfs" is requesting the following privileges:
 - network: [host]
 - mount: [/var/lib/docker/plugins/]
 - mount: []
 - device: [/dev/fuse]
 - capabilities: [CAP_SYS_ADMIN]
Do you grant the above permissions? [y/N] y
latest: Pulling from vieux/sshfs
Digest: sha256:1d3c3e42c12138da5ef7873b97f7f32cf99fb6edde75fa4f0bcf9ed277855811
52d435ada6a4: Complete
Installed plugin vieux/sshfs

With digest:

 docker plugin install vieux/sshfs@sha256:1d3c3e42c12138da5ef7873b97f7f32cf99fb6edde75fa4f0bcf9ed277855811
Plugin "vieux/sshfs@sha256:1d3c3e42c12138da5ef7873b97f7f32cf99fb6edde75fa4f0bcf9ed277855811" is requesting the following privileges:
 - network: [host]
 - mount: [/var/lib/docker/plugins/]
 - mount: []
 - device: [/dev/fuse]
 - capabilities: [CAP_SYS_ADMIN]
Do you grant the above permissions? [y/N] y
Pulling from vieux/sshfs
Digest: sha256:1d3c3e42c12138da5ef7873b97f7f32cf99fb6edde75fa4f0bcf9ed277855811
52d435ada6a4: Complete
Installed plugin vieux/sshfs@sha256:1d3c3e42c12138da5ef7873b97f7f32cf99fb6edde75fa4f0bcf9ed277855811

- How to verify it

$ make DOCKER_GRAPHDRIVER=overlayfs TEST_FILTER=TestPluginInstall TEST_IGNORE_CGROUP_CHECK=1 test-integration

- Description for the changelog

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

Screenshot 2024-02-01 at 20 02 48

@thaJeztah thaJeztah added this to the 24.0.10 milestone Feb 5, 2024
@thaJeztah thaJeztah self-assigned this Feb 5, 2024
@thaJeztah thaJeztah requested a review from cpuguy83 as a code owner February 5, 2024 08:45
@thaJeztah thaJeztah requested a review from laurazard February 5, 2024 08:45
@thaJeztah
Copy link
Member Author

thaJeztah commented Feb 5, 2024

Ah, hm.. looks like 24.0 and older need some additional changes;

integration/plugin/common/main_test.go:1: : # github.com/docker/docker/integration/plugin/common [github.com/docker/docker/integration/plugin/common.test]
integration/plugin/common/plugin_test.go:128:24: cannot use ctx (variable of type func()) as context.Context value in argument to plugin.Create: func() does not implement context.Context (missing method Deadline)

thaJeztah
thaJeztah commented Feb 5, 2024
View reviewed changes
integration/plugin/common/plugin_test.go Outdated
})

t.Run("with digest", func(t *testing.T) {
ctx := setupTest(t)
Copy link
Member Author

@thaJeztah thaJeztah Feb 5, 2024

Choose a reason for hiding this comment

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

Changed this to;

Suggested change
ctx := setupTest(t)
defer setupTest(t)()

@laurazard @thaJeztah
tests: add plugin install test w/ digest
6f483e7 
Adds a test case for installing a plugin from a remote in the form
of `plugin-content-trust@sha256:d98f2f8061...`, which is currently
causing the daemon to panic, as we found while running the CLI e2e
tests:

```
docker plugin install registry:5000/plugin-content-trust@sha256:d98f2f806144bf4ba62d4ecaf78fec2f2fe350df5a001f6e3b491c393326aedb
```

Signed-off-by: Laura Brehm <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@laurazard @thaJeztah
plugins: fix panic installing from repo w/ digest
f406728 
Only print the tag when the received reference has a tag, if
we can't cast the received tag to a `reference.Tagged` then
skip printing the tag as it's likely a digest.

Fixes panic when trying to install a plugin from a reference
with a digest such as
`vieux/sshfs@sha256:1d3c3e42c12138da5ef7873b97f7f32cf99fb6edde75fa4f0bcf9ed277855811`

Signed-off-by: Laura Brehm <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the 24.0_backport_plugin-install-digest branch from eb2a269 to f406728 Compare February 5, 2024 09:11
@laurazard
Copy link
Member

laurazard commented Feb 5, 2024

Oooh. I'll TAL.

@thaJeztah
Copy link
Member Author

thaJeztah commented Feb 5, 2024

Jenkins failure is unrelated (CI sometimes attempts to run device-mapper tests in a container, which fails); I restarted it;

[2024-02-05T09:16:59.134Z] === Failed
[2024-02-05T09:16:59.134Z] === FAIL: daemon/graphdriver/devmapper TestDevmapperIncreaseLoopBackSize (0.29s)
[2024-02-05T09:16:59.134Z] time="2024-02-05T09:16:15Z" level=error msg="Udev sync is not supported, which will lead to data loss and unexpected behavior. Make sure you have a recent version of libdevmapper installed and are running a dynamic binary, or select a different storage driver. For more information, see https://docs.docker.com/go/storage-driver/" storage-driver=devicemapper
[2024-02-05T09:16:59.135Z] time="2024-02-05T09:16:15Z" level=warning msg="Usage of loopback devices is strongly discouraged for production use. Please use `--storage-opt dm.thinpooldev` or use `man dockerd` to refer to dm.thinpooldev section." storage-driver=devicemapper
[2024-02-05T09:16:59.135Z] time="2024-02-05T09:16:15Z" level=info msg="Creating filesystem xfs on device docker-0:67-17920-base, mkfs args: [/dev/mapper/docker-0:67-17920-base]" storage-driver=devicemapper
[2024-02-05T09:16:59.135Z] time="2024-02-05T09:16:15Z" level=info msg="Successfully created filesystem xfs on device docker-0:67-17920-base" storage-driver=devicemapper
[2024-02-05T09:16:59.135Z] time="2024-02-05T09:16:15Z" level=warning msg="unmountAndDeactivate: open /tmp/docker-graphtest-747194598/devicemapper/mnt: no such file or directory" storage-driver=devicemapper
[2024-02-05T09:16:59.135Z] time="2024-02-05T09:16:15Z" level=error msg="Udev sync is not supported, which will lead to data loss and unexpected behavior. Make sure you have a recent version of libdevmapper installed and are running a dynamic binary, or select a different storage driver. For more information, see https://docs.docker.com/go/storage-driver/" storage-driver=devicemapper
[2024-02-05T09:16:59.135Z] time="2024-02-05T09:16:15Z" level=warning msg="Usage of loopback devices is strongly discouraged for production use. Please use `--storage-opt dm.thinpooldev` or use `man dockerd` to refer to dm.thinpooldev section." storage-driver=devicemapper
[2024-02-05T09:16:59.135Z] time="2024-02-05T09:16:15Z" level=warning msg="Base device already exists and has filesystem xfs on it. User specified filesystem  will be ignored." storage-driver=devicemapper
[2024-02-05T09:16:59.135Z]     devmapper_test.go:125: data or metadata loop back size is incorrect
[2024-02-05T09:16:59.135Z] time="2024-02-05T09:16:15Z" level=warning msg="unmountAndDeactivate: open /tmp/docker-graphtest-747194598/devicemapper/mnt: no such file or directory" storage-driver=devicemapper
[2024-02-05T09:16:59.135Z] 
[2024-02-05T09:16:59.135Z] DONE 3046 tests, 26 skipped, 1 failure in 94.776s

laurazard
laurazard approved these changes Feb 5, 2024
View reviewed changes
Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 9d3a6a2 into moby:24.0 Feb 5, 2024
@thaJeztah thaJeztah deleted the 24.0_backport_plugin-install-digest branch February 5, 2024 10:08
@thaJeztah thaJeztah mentioned this pull request Feb 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@laurazard laurazard laurazard approved these changes

@cpuguy83 cpuguy83 Awaiting requested review from cpuguy83 cpuguy83 is a code owner

Assignees

@thaJeztah thaJeztah

Labels

area/plugins kind/bugfix PR's that fix bugs status/2-code-review

Projects

None yet

Milestone

24.0.10

Development

Successfully merging this pull request may close these issues.

2 participants

@thaJeztah @laurazard