Update network patches for TSI and add NetworkSuite from shimtest#250
Draft
dmcgowan wants to merge 4 commits into
Draft
Update network patches for TSI and add NetworkSuite from shimtest#250dmcgowan wants to merge 4 commits into
dmcgowan wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the vendored shimtest dependency and extends the shim conformance coverage with a new network-focused suite intended to catch regressions in default outbound connectivity (notably for TSI). It also tightens kernel patch application and adds/updates shim-side networking parsing and resolv.conf handling tests/logic.
Changes:
- Vendor-update
github.com/containerd/shimtestand addNetworkSuite(TCP, UDP, DNS) to the repository’s shim test runner. - Enhance the embedded
testbinutilities (ncgains TCP/UDP support; addhost) and wire the new commands into the shimtest rootfs. - Update TSI / vsock kernel patch set and make Docker kernel patch application strict (
--fuzz=0).
Reviewed changes
Copilot reviewed 10 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vendor/modules.txt | Updates vendored module version metadata for shimtest. |
| vendor/github.com/containerd/shimtest/testbin/testbin.go | Extends nc modes (unix/TCP/UDP) and adds host command. |
| vendor/github.com/containerd/shimtest/rootfs.go | Adds host symlink command into the embedded rootfs. |
| vendor/github.com/containerd/shimtest/README.md | Documents new net feature and the NetworkSuite subtests. |
| vendor/github.com/containerd/shimtest/network_suite.go | Adds NetworkSuite conformance tests for outbound TCP/UDP and DNS resolution. |
| vendor/github.com/containerd/shimtest/helpers.go | Adds helper to create tasks with stdin FIFO configured. |
| test/shim/shim_test.go | Runs the new NetworkSuite as part of the shim test suite. |
| kernel/patches/0012-tsi-allow-hijacking-sockets-tsi_hijack.patch | Updates TSI socket hijacking patch content (now includes inet6/unix options). |
| kernel/patches/0011-Transparent-Socket-Impersonation-implementation.patch | Updates the main TSI implementation patch (multi-family support, UDP behavior changes, etc.). |
| kernel/patches/0010-virtio-vsock-implement-has_data-for-DGRAM.patch | Adds new vsock DGRAM “has_data” support patch. |
| kernel/patches/0009-vsock-Add-support-for-SIOCINQ-ioctl.patch | Adds new vsock SIOCINQ support patch. |
| internal/shim/task/networking_test.go | Adds unit tests for parsing VM network annotations and provider behavior. |
| internal/shim/task/ctrnetworking.go | Improves resolv.conf host fallback logic (systemd-resolved stub detection). |
| internal/shim/task/ctrnetworking_test.go | Adds tests for container network annotation parsing and resolv.conf logic. |
| go.mod | Bumps github.com/containerd/shimtest dependency version. |
| go.sum | Updates checksums for the new shimtest version. |
| Dockerfile | Applies kernel patches with --fuzz=0 and fails fast (set -e). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+254
to
+258
| fields := strings.Fields(line) | ||
| if len(fields) != 2 || fields[0] != "nameserver" { | ||
| continue | ||
| } | ||
| ip := net.ParseIP(fields[1]) |
dmcgowan
force-pushed
the
network-update-and-tests
branch
from
57de218 to
2a45af7
Compare
Comment on lines
+250
to
+265
| func onlyLoopbackNameservers(resolvConf []byte) bool { | ||
| found := false | ||
| for _, line := range strings.Split(string(resolvConf), "\n") { | ||
| line = strings.TrimSpace(line) | ||
| fields := strings.Fields(line) | ||
| if len(fields) != 2 || fields[0] != "nameserver" { | ||
| continue | ||
| } | ||
| ip := net.ParseIP(fields[1]) | ||
| if ip == nil || !ip.IsLoopback() { | ||
| return false | ||
| } | ||
| found = true | ||
| } | ||
| return found | ||
| } |
dmcgowan
force-pushed
the
network-update-and-tests
branch
from
2a45af7 to
845fa67
Compare
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
cloneMntNs unconditionally added CLONE_NEWUSER even when the shim process already had real root (euid 0). Entering a *new* user namespace — even one mapping a UID to itself — demotes the process to a non-initial user namespace, and the kernel restricts mounting real block-device-backed filesystems (e.g. ext4) to the initial user namespace regardless of the effective capabilities held within a descendant namespace. When the shim already has real root, skip CLONE_NEWUSER and use CLONE_NEWNS alone: this still provides the mount-namespace isolation and cleanup-on-exit benefit without losing the ability to mount real filesystems. Found and fixed while working on the multi worktree's sandbox rootfs assembly path (which does need real ext4/loop mounts and hit this as "operation not permitted" when run as real root), but the bug applies here identically: this file only differs from multi's in UID-mapping style, not in the underlying logic. Verified: full conformance suite passes as both root and non-root (44/44 both ways). Signed-off-by: Derek McGowan <derek@mcg.dev>
dmcgowan
force-pushed
the
network-update-and-tests
branch
from
845fa67 to
b5be35c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 19 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
kernel/patches/0011-Transparent-Socket-Impersonation-implementation.patch:1703
- This patch registers the AF_TSIU address family and tsi_un_proto in tsi_init, but tsi_exit only unregisters AF_TSI/AF_TSI6 and their protos. If the module is unloaded, AF_TSIU and tsi_un_proto remain registered, which can lead to leaks or undefined behavior on reload.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Awaiting containerd/shimtest#5 merge to update vendor