Implement KNE Packet Bridge daemon and CLI subcommand - #762
Merged
Merged
Conversation
kraney
force-pushed
the
feat/packet-bridge
branch
from
September 8, 2026 21:19
b16dcb0 to
e6328da
Compare
kraney
force-pushed
the
feat/packet-bridge
branch
3 times, most recently
from
September 14, 2026 21:20
a042c7b to
79bdea3
Compare
manizzzz
reviewed
Sep 16, 2026
manizzzz
reviewed
Sep 16, 2026
manizzzz
reviewed
Sep 16, 2026
manizzzz
reviewed
Sep 16, 2026
manizzzz
reviewed
Sep 16, 2026
manizzzz
reviewed
Sep 16, 2026
manizzzz
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the changes.
kraney
force-pushed
the
feat/packet-bridge
branch
2 times, most recently
from
September 22, 2026 21:11
ab68daf to
f5a190b
Compare
manizzzz
reviewed
Sep 22, 2026
manizzzz
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the super thorough fixes on the and new test suite all look great!
Holding LGTM for three small issues in the new diff before we merge:
- bridge/bridge.go (ReadPacket / WritePacket): retry unix.EINTR inside the s.rc.Read / s.rc.Write callbacks (RawConn doesn't ignore EINTR, so
Go's SIGURG preemption will cause readLoop to tear down the demux). - bridge/bridge.go (Server.Transmit): wait for the egress goroutine after demux.unsubscribe(pktChan) in the defer so stream.Send cannot race
with Transmit returning. - topo/node/forward/forward.go (CreatePod): preserve pb.Config.Args when ForwardConfig.wires is used so flags like --alts aren't dropped.
Left a few optional/non-blocking comments on forward.go and node.go as well. Once (1)–(3) are in, this is good to LGTM!
The bridge subcommand sets up a daemon process that will forward packets into / out of the KNE cluster over a gRPC streaming service. An external client can use this service to inject or receive packets to/from a particular interface defined in the topology.
This makes bridge usable in KNE-only situations, to link two separate clusters (like if one cluster has access another doesn't.) More importantly, it facilitates easy testing of the feature.
* use a pool for packet buffers * use cancel so waiting goroutines are freed if readLoop exits on a socket error * use signal.NotifyContext() * Make sure kne bridge runs root handlers * distinguish between normal and unexpected stream closure in the client * Better test coverage
1. Netpoller Integration & Interruptible ReadPacket:
• Refactored bridge.go:59 in bridge.go:69 to set the raw socket to non-blocking via unix.
SetNonblock, wrap the file descriptor in os.NewFile, and use syscall.RawConn (s.rc.Read and s.rc.
Write).
• Calling s.Close() (s.f.Close()) now immediately interrupts any pending or concurrent ReadPacket()
/ WritePacket() calls via the Go runtime netpoller, preventing socket pinning and kernel memory
leaks.
• Documented on bridge.go:54 that implementations MUST ensure Close() interrupts pending
ReadPacket() calls.
2. Removed Goroutine & Socket Leaks in Client runStream:
• In client.go:135, tracked child egress and ingress goroutines with a sync.WaitGroup.
• Deferred teardown cancels streamCancel(), calls handler.Close(), and waits on wg.Wait() so
reconnect loops cannot leave orphaned goroutines or open promiscuous sockets.
3. Demux and Socket Concurrency Guarding:
• Netpoller RawConn prevents raw file descriptor reuse races between concurrent WritePacket and
Close.
• Added bridge.go:183 which checks d.ctx.Err() prior to writing to the underlying handler. Transmit
now calls demux.write() rather than accessing demux.handler directly.
4. Explicit Demux Closed State & Safe Cache Teardown:
• Added an explicit closed bool flag to bridge.go:141. bridge.go:229 returns an error if the demux
is closed or cancelled, preventing orphaned subscriber channels.
• Updated bridge.go:281 to purge dead demux instances before returning and added a pointer identity
check (cur == d) in onClose to prevent late callbacks from evicting replacement demuxers.
5. Server Teardown Synchronization (Server.Close):
• Tracked demux read loops using a sync.WaitGroup in InterfaceDemux via bridge.go:251.
• bridge.go:404 now invokes d.close(), awaits completion with d.wait(), and aggregates any close
errors with errors.Join.
• Added an early s.ctx.Err() check in getOrCreateDemux returning codes.Unavailable status errors if
called during or after server shutdown.
6. Distinguished Fatal vs. Per-Packet Write Errors & Length Checks:
• Added bridge.go:423 and explicit MTU length validation against maxFrameSize.
• Non-fatal write errors (such as EMSGSIZE or ENOBUFS) log and bump a droppedWriteFrames counter
without tearing down the gRPC stream.
7. Buffer & Subscriber Management:
• Removed sync.Pool (packetPool) in favor of a dedicated per-SocketHandler 64 KB scratch buffer (s.
rbuf).
• Capped concurrent subscribers per interface to maxSubscribersPerDemux (32) and documented slice
immutability on subscribe().
8. Tests:
• Added unit tests in bridge_test.go and client_test.go testing closed demux subscribe rejection,
max subscriber limits, server shutdown waiting, dead demux cache replacement, and non-fatal error
continuation.
1. Explicit DeleteService Error Handling & DaemonSet Cleanup:
• In node.go:772, DeleteService no longer ignores all errors with _ =. It explicitly checks and
ignores only apierrors.IsNotFound(err) while collecting and returning all actionable failures (such
as RBAC Forbidden, API timeouts, or context cancellation) using errors.Join.
• Updated node.go:730 to propagate errors from DeleteService and DeleteResource using errors.Join
rather than discarding them with log.Warningf.
• Updated node.go:744 and node.go:811 to cleanly ignore NotFound and NotExist errors on already-
deleted resources.
2. Per-Type Service Grouping (Mixed-Type Support):
• In node.go:578, eliminated service type collapsing across ports. Ports are now partitioned by
service type:
• LoadBalancer ports are grouped into service-<node> (with AllocateLoadBalancerNodePorts:
false).
• NodePort ports are grouped into service-<node>-nodeport.
• ClusterIP ports are grouped into service-<node>-clusterip.
• Updated node.go:895 to list all services matching the pod=<node> label selector, with backward-
compatibility fallback.
• Added mixed-type service test cases and DeleteService RBAC error propagation tests in
node_test.go:424.
3. Demux Fan-out, Multi-Interface, and Stress Tests:
• Added bridge_test.go:574 in bridge/bridge_test.go verifying that concurrent Wire.Transmit streams
on the same interface both receive the broadcast frame.
• Added bridge_test.go:644 asserting socketOpener is called once per distinct interface across
concurrent/sequential streams with per-interface fakes.
• Added bridge_test.go:707 verifying subscription failure on a dead demux.
• Added bridge_test.go:723 testing concurrent subscriptions/unsubscriptions under packet flow.
• Verified that go test -race -count=10 ./bridge/... ./cmd/bridge/... ./topo/node passes cleanly.
### 1. Socat DaemonSet Hardening & Security (topo/node/node.go)
• Eliminated False-Positive Proxy Creation (Comment 11 / ID 4030589333):
• Finding: The fallback iteration over n.Proto.Services by service name caused services without
explicit names to match ("" == ""), opening unintended host IPv6 socat proxies.
• Fix: Removed the fallback search; only keyed services with NodePort > 0 create proxy containers,
and added a warning when v6_host_proxy: true is configured on non-NodePort services.
• Pinned & Configurable Image (Comment 14 / ID 4030627360):
• Finding: alpine/socat:latest was hardcoded with a floating tag, creating non-deterministic builds
and breaking offline/air-gapped clusters.
• Fix: Replaced with a pinned, configurable package variable node.go:575-580 ("alpine/socat:1.8.0.
0").
• Connection Flooding & Container Security (Comment 15 / ID 4030639450):
• Finding: Unbounded socat forks could trigger cgroup OOM kills during connection bursts, and
containers lacked explicit security contexts and resource requests.
• Fix: Added max-children=64 to TCP6-LISTEN, configured container SecurityContext (RunAsNonRoot:
true, AllowPrivilegeEscalation: false, Capabilities: {Drop: ["ALL"]}), and added explicit
CPU/memory Requests (50m/32Mi) alongside Limits (100m/64Mi).
### 2. Kubernetes Lifecycle & Resilience (topo/node/node.go)
• Atomic Service Creation Rollback (Comment 12 / ID 4030609747):
• Finding: If DaemonSets().Create() failed, previously created Service objects were left orphaned
in the cluster, blocking retries.
• Fix: Added a named-return deferred rollback in node.go:610-620 to delete partially created
services on failure.
• Garbage Collection & Scheduling (Comment 13 / ID 4030617630):
• Finding: The proxy DaemonSet lacked an OwnerReference (preventing k8s GC on service deletion),
lacked Tolerations (preventing scheduling on control-plane/master nodes in kind clusters), and
lacked DNSPolicy.
• Fix: Set OwnerReferences to the primary Service, added control-plane Tolerations (Operator:
Exists), and configured DNSPolicy: DNSClusterFirstWithHostNet.
──────
### 3. Topology Proto Legacy Semantics (topo/topo.go)
• Legacy Empty Type Handling (Comment 16 / ID 4030648120):
• Finding: An empty spec.type in Kubernetes defaults to ClusterIP, but topo.go:940-950 was treating
"" as LoadBalancer without explanation.
• Fix: Documented that spec.Type == "" is handled as LoadBalancer strictly for backward
compatibility with legacy test fixtures.
### 4. Test Suite Idempotency & New Unit Tests
• Multi-run Idempotency under -count=10:
• Finding: In-test calls to node.Vendor(...) in topo/topo_test.go caused panics due to duplicate
registration when executed with -count=10.
• Fix: Centralized vendor registrations in init() in topo_test.go:161-168.
• Added Unit Tests:
• Added node_test.go:607-675 verifying DaemonSet attributes (security context, requests/limits,
tolerations, owner references, and max-children).
• Added node_test.go:677-715 verifying automatic rollback on DaemonSet creation failure.
None of these are caught by the unit tests, since kfake neither runs a kubelet nor enforces RBAC. 1. v6 proxy DaemonSet could never start. The socat image declares no USER, so its configured UID is 0 and RunAsNonRoot alone makes the kubelet reject the container with "container has runAsNonRoot and image will run as root". Set an explicit non-root UID/GID; socat only binds a NodePort (>1024) and dials loopback, so it needs no privileges. 2. Missing RBAC for the v6 proxy. The podrunner ClusterRole grants only core pods/services/logs, so creating or deleting the DaemonSet is Forbidden for in-cluster KNE. Now that DeleteService correctly surfaces non-NotFound errors, this turned into a teardown failure rather than a silent leak. Grant apps/daemonsets. 3. Example topology dialed a service that no longer exists. Grouping ports into one Service per type renamed the NODE_PORT service to service-<node>-nodeport, but bridge-client still pointed at service-bridge-server. Fix the peer address and the two stale README references, and document the naming rule where users will hit it. Also gofmt the two new bridge test files (import ordering).
The bridge daemon was reachable only by hand-writing its command line into
a topology, which meant a topology had to know the daemon's flag names, the
wire port, and the name KNE happens to give a node's Service. None of that
is a contract worth asking callers to depend on, and the Service name in
particular is an implementation detail that changed within this branch.
FORWARD nodes already had a declarative model in forward.proto, added in
2024 but never implemented: a list of wires, each with an "a" and a "z"
endpoint, where an endpoint is either a local interface, another node in
the topology, or an address outside it. This implements it. The endpoint
written as an interface is the declaring node; if that is "a" the node
dials out, and if it is "z" the node listens. A wire with no "a" at all is
a server whose client lives outside the topology, which is how an external
process attaches. The remote_node case, declared but implemented nowhere,
now works too.
The daemon runs one mode per process, so a node that both serves and dials
gets a container per role, named <node>-<role>; a node needing only one
keeps the plain node name.
Naming a peer with local_node needs that name to resolve, and nothing
created a Service under a node's own name. Every FORWARD node now gets a
headless Service named after it. It declares no ports, because a headless
Service needs only a selector to publish A records, and ports would put it
in the map `kne show` reports and collide with the wire port a topology may
already expose. It carries the "pod" label so the node's teardown removes
it.
Alongside:
- Rename Service.v6_host_proxy to external_ipv6. The old name described
the mechanism KNE currently uses; the field states an intent that a
dual-stack cluster could satisfy natively. Breaking, and deliberately
done now while nothing depends on it.
- Document outside_ip as populated only for LOAD_BALANCER services.
- Point the FORWARD default image at the published bridge image, tagged
:ga to match the rest of KNE, instead of the never-published
forward:latest.
- Fix the peer name typos in examples/forward, which named "fw1"/"fw2".
super-linter runs prettier over every file a PR changes, not just the changed lines, so adding the daemonsets rule to the podrunner ClusterRole pulled the whole of serviceaccount.yaml into scope. Prettier indents block sequences under their mapping key, which this file never did. The reindent is mechanical and the parsed documents are unchanged; verified by comparing yaml.safe_load_all before and after. Also drops a stray blank line from the bridge example README.
`git describe --tags --abbrev=0` returns whichever component was tagged most recently, which in this repo is third_party/meshnet/vX.Y.Z. So `make bridge-docker` was labelling the bridge image v0.5.1, meshnet's version, and `make bridge-release` would have published it under that. Matching v* picks KNE's own release tags and skips the prefixed component ones. The bridge image is just the kne binary with `kne bridge` as its entrypoint, so KNE's version is the right one for it to carry; it does not need a series of its own. This also makes the $(notdir ...) wrapper dead, since the tags that needed stripping are now excluded. Also point bridge-release at :ga. bridge-docker builds :$(TAG) and :ga and no longer builds :latest, so pushing :latest would have shipped whatever stale image happened to still carry that tag.
`kne release bridge` minted a bridge/vX.Y.Z tag, copying the meshnet pattern. Meshnet earns a series of its own: it is a separate vendored component with its own source tree, Dockerfile and build. The bridge is not that. Its image is the kne binary with `kne bridge` as the entrypoint, so a bridge version distinct from KNE's describes nothing, and two series for one artifact is a second thing to bump and a way for the image to disagree with the binary inside it. Tag KNE as a whole instead. Together with the Makefile now matching v*, both release paths land on the same version for the same commit, where before cloudbuild used whatever was passed on the command line and make used meshnet's latest tag. Nothing else in the repo creates tags: createAndPushTag has exactly two callers, and no workflow under .github or cloudbuild tags anything. KNE's v* tags are cut by hand, so this does not race an automated tagger, and a collision with a hand-cut tag fails closed because `git tag` refuses to overwrite.
staticcheck QF1008: corev1.Service embeds ObjectMeta, so the selector in got.ObjectMeta.Labels is redundant. Only the lines this PR adds are checked, because .github/linters/.golangci.yml sets new-from-rev to origin/main. Prettier wanted the trailing document separator in serviceaccount.yaml not to be followed by a blank line. The earlier reformat added one because it was produced with prettier 3.6.2, which asks for the opposite; super-linter v8.6.0 pins ^3.8.1, and the two disagree on this one byte. `prettier --check` reports only that a file has issues and never a diff, so the failure had nothing in it to act on. Verified with the versions CI uses: prettier 3.8.1 clean on every yaml, md and json file the branch touches, and golangci-lint 2.12.2 reporting no issues under the repo's own config.
kraney
force-pushed
the
feat/packet-bridge
branch
from
September 23, 2026 21:11
8362d56 to
8d7d10a
Compare
bstoll
approved these changes
Sep 24, 2026
bstoll
left a comment
Contributor
There was a problem hiding this comment.
I didn't verify this but if GSO/TSO are enabled on the veth do TCP and UDP packets get forwarded with valid checksums? I think the way this reads packets they will forward with invalid checksums unless hardware offloads are disabled.
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.
Summary
Adds the KNE Packet Bridge: a daemon that carries raw Layer 2 Ethernet frames
between network segments over a gRPC stream, so that a KNE topology can be wired to
something that is not in the same cluster — another KNE cluster, a workstation
veth,or a process running outside Kubernetes entirely.
A
FORWARDnode declares the wires it terminates and KNE derives the daemoninvocations. The topology never names a flag, a port, or a Kubernetes object.
The contract
proto/forward.protoalready described this model — a list of wires, each with anaand a
zendpoint — but nothing implemented it. This PR implements it, including theRemoteNodeendpoint that was declared and unreachable.The endpoint written as an
interfaceis always the declaring node:a: { interface },z: { local_node }a: { interface },z: { remote_node }z: { interface }, noaSo a node that a Borg job attaches to is written as:
No image, no CLI flags, no Service names. Two consequences fall out of the model:
local_noderequires that name to resolve,and nothing created a Service under a node's own name. Every
FORWARDnode now getsa headless Service named after it. It declares no ports — a headless Service needs
only a selector to publish A records, and ports would place it in the map
kne showreports and collide with the wire port a topology may already expose. It carries the
podlabel so the node's existing teardown removes it.that both serves and dials gets one container per role, named
<node>-<role>. A nodeneeding only one keeps the plain node name.
Changes
Bridge daemon and client (
bridge/,cmd/bridge/)Bidirectional streaming of raw frames over the existing
Wireservice, with aper-interface demux so one server can serve many wires on one port.
kne bridge serverand
kne bridge client, with interface binding, reconnect, and optional ALTS transportcredentials for DirectPath on GCE.
Wire planning (
topo/node/forward/)ForwardConfigis translated into daemon invocations and containers, plus the headlesspeer Service described above. The default image points at the published bridge image, so
topologies no longer carry an
image:line.Service exposure (
proto/topo.proto,topo/node/,topo/topo.go)Service.TypegainsLOAD_BALANCER,NODE_PORTandCLUSTER_IP, with one KubernetesService created per requested type.
Service.external_ipv6states that a service shouldbe reachable over IPv6 from outside the cluster; on a single-stack IPv4 cluster KNE
satisfies that with a host-network
socatDaemonSet, and on a dual-stack cluster it maybecome a no-op.
outside_ipis now documented as populated only forLOAD_BALANCER.Build and release (
Makefile,deploy/bridge/,cloudbuild/,cmd/release/)A Dockerfile whose entrypoint is
kne bridge,maketargets for local kind testing, andkne release bridgedriving a Cloud Build pipeline. The bridge image is theknebinary,so it has no version of its own: releasing it tags KNE as a whole and labels the image
with that version, rather than minting a separate series.
Testing
Unit tests for
bridge/,cmd/bridge/, the wire planner and peer Service intopo/node/forward/, and the service-type handling intopo/. The planner tests covereach endpoint shape, both malformed-wire cases, and the mixed server-plus-client node.
Verified on kind:
kne deploy, thenkne createofexamples/bridge/paired-bridge.pb.txt,then
pingbetween the two hosts, which have no meshnet link and reach each other onlyacross the bridge. That exercises wire planning, peer resolution through the headless
Service, stream establishment, and bidirectional forwarding.
Also verified on kind with
external_ipv6: trueset on the service: thesocatDaemonSetis admitted, which exercises the
apps/daemonsetsgrant added to thepodrunnerClusterRole, and its pod reaches
Running, which exercises the explicit non-root UID thecontainer needs in order for the kubelet to accept it.
Not yet exercised on a cluster: multi-container
FORWARDnodes andremote_nodeendpoints. Both are covered by unit tests only.
Notes for reviewers
Service.v6_host_proxywas renamed toService.external_ipv6. This is a breaking protochange, made deliberately while nothing depends on the field, because the old name
described the mechanism rather than the intent.
deploy/ubuntu/serviceaccount.yamlis reindented throughout. super-linter runs prettierover every file a PR touches rather than only the changed lines, and this file did not
previously indent block sequences under their mapping key. The parsed documents are
unchanged.
x/wire/contains an earlier, unfinished implementation of the sameWireservice. Itis untouched here and nothing imports it; retiring it is left to a follow-up.