Skip to content
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.

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

Re-add tun/tap to default device rules #4555

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

kolyshkin
Copy link
Contributor

Since v1.2.0 was released, a number of users complained that the removal of tun/tap device access from the default device ruleset is causing a regression in their workloads.

Additionally, it seems that some upper-level orchestration tools (Docker Swarm, Kubernetes) makes it either impossible or impractical to supply additional device rules.

While it's probably not right to have /dev/net/tun in a default device list, it was there from the very beginning, and users rely on it.

This reverts commit 2ce40b6 / PR #3468.

@kolyshkin kolyshkin added the backport/1.2-todo A PR in main branch which needs to be backported to release-1.2 label Dec 16, 2024
@kroese
Copy link

kroese commented Dec 16, 2024

It took 2+ years between the date of the original commit and when it was rolled out in Docker. So if it will take another two years until this revert is effectuated, it will be a bit pointless because by that time everyone already changed their compose files to workaround it.

@@ -120,14 +120,21 @@ block-8:
51: MovImm32 dst: r0 imm: 1
52: Exit
block-9:
// /dev/pts (c, 136, wildcard, rwm, true)
// tuntap (c, 10, 200, rwm, true)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment line to describe the background of this revert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean here, in the test file?

I don't think it's needed here in the test -- in here we just check that the eBPF generated is as expected, and since we've re-added tuntap, we have to modify the expected program.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it should be in spec_linux.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added; PTAL

@AkihiroSuda
Copy link
Member

either impossible or impractical to supply additional device rules.

Probably s/impractical/cumbersome/.

For Kubernetes, an additional device rule can be specified via the Generic Device Plugin: https://github.com/squat/generic-device-plugin
Installing a plugin is not as straightforward as adding a single line to a Pod manifest, though.

@kolyshkin
Copy link
Contributor Author

It took 2+ years between the date of the original commit and when it was rolled out in Docker. So if it will take another two years until this revert is effectuated, it will be a bit pointless because by that time everyone already changed their compose files to workaround it.

Unfortunately, the scope of this is not limited to docker compose.

Since v1.2.0 was released, a number of users complained that the removal
of tun/tap device access from the default device ruleset is causing a
regression in their workloads.

Additionally, it seems that some upper-level orchestration tools
(Docker Swarm, Kubernetes) makes it either impossible or cumbersome
to supply additional device rules.

While it's probably not quite right to have /dev/net/tun in a default
device list, it was there from the very beginning, and users rely on it.
Let's keep it there for the sake of backward compatibility.

This reverts commit 2ce40b6.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin added this to the 1.2.4 milestone Dec 16, 2024
@kolyshkin kolyshkin requested a review from cyphar December 16, 2024 23:51
@aofei
Copy link

aofei commented Dec 17, 2024

either impossible or impractical to supply additional device rules.

Probably s/impractical/cumbersome/.

For Kubernetes, an additional device rule can be specified via the Generic Device Plugin: https://github.com/squat/generic-device-plugin Installing a plugin is not as straightforward as adding a single line to a Pod manifest, though.

Just FYI, I think this approach is broken too.

@cyphar
Copy link
Member

cyphar commented Dec 17, 2024

@kroese That's because of the long time between tunc 1.1 and tunc 1.2 releases (the patch was not backported to 1.1.z). This is going to be included in the 1.2.4 release and so it will take very little time for the official Docker builds to package the new runc (and if you are using distribution packages it will happen automatically).

@cyphar cyphar merged commit 3ddaa91 into opencontainers:main Dec 17, 2024
40 checks passed
@kolyshkin kolyshkin added backport/1.2-done A PR in main branch which has been backported to release-1.2 and removed backport/1.2-todo A PR in main branch which needs to be backported to release-1.2 labels Dec 17, 2024
@kolyshkin
Copy link
Contributor Author

1.2 backport: #4556

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.2-done A PR in main branch which has been backported to release-1.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants