Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
thaJeztah merged 2 commits into from
Feb 5, 2024
Merged

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

thaJeztah merged 2 commits into from
Feb 5, 2024

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 <laurabrehm@hey.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@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 <laurabrehm@hey.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@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