Add AmneziaWG cable driver#4097
Conversation
|
🤖 Created branch: z_pr4097/sanek9/feat/amneziawg-cable-driver |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an AmneziaWG cable driver with userspace device management, configurable obfuscation, peer lifecycle handling, connection monitoring, driver registration, dependency updates, and mocked lifecycle tests. ChangesAmneziaWG cable driver
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LocalEndpoint
participant amneziawgDriver
participant startUserspaceDevice
participant AWGClient
participant Netlink
LocalEndpoint->>amneziawgDriver: provide endpoint specification
amneziawgDriver->>startUserspaceDevice: create userspace interface
amneziawgDriver->>AWGClient: configure device
amneziawgDriver->>LocalEndpoint: store device public key
amneziawgDriver->>Netlink: activate interface
amneziawgDriver->>AWGClient: configure peer
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Part of the proposal overview: #4101 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/cable/amneziawg/obfuscation.go (1)
28-30: 🩺 Stability & Availability | 🔵 TrivialCross-cluster consistency risk for
H1-H4/S1-S4via per-gateway env overrides.The comment correctly notes
H1-H4/S1-S4must be identical on every peer, butapplyCableDriverObfuscationsources overrides from each gateway's ownSUBMARINER_CABLEDRIVEROPTIONSindependently. In a multi-cluster mesh, if different clusters/gateways set different override values, handshakes will silently fail (or behave inconsistently) across the connection with no explicit cross-cluster validation or error surfaced. Worth documenting this operational requirement prominently for operators (e.g. in the CRD/API docs forcableDriverOptions), since a mismatch here is only discoverable at runtime as connectivity failures.Also applies to: 79-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/obfuscation.go` around lines 28 - 30, Document in the CRD/API documentation for cableDriverOptions that H1-H4 and S1-S4 overrides must be identical across every gateway and cluster in the mesh. Clearly distinguish these shared parameters from Jc/I* values, which may differ, and warn that inconsistent per-gateway SUBMARINER_CABLEDRIVEROPTIONS values cause runtime connectivity failures.pkg/cable/amneziawg/amneziawg_test.go (2)
162-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPre-populated fake peer has no effect and isn't verified.
The stale peer seeded on
t.client.devices[amneziawg.DefaultDeviceName](lines 163-165) is wiped out byConfigureDevice'sReplacePeers: truehandling (lines 354-356 in this same file) before theItblock runs, anddevice.Peersis never asserted. This setup currently does nothing and could mislead a reader into thinkingReplacePeersbehavior is exercised here.Either drop the dead setup or assert on it to get real coverage of the replace-peers path.
Suggested fix
It("should configure the device with obfuscation parameters and public key", func() { device := t.client.devices[amneziawg.DefaultDeviceName] Expect(device).ToNot(BeNil()) Expect(device.ListenPort).To(Equal(listenPort)) Expect(device.PublicKey).ToNot(Equal(wgtypes.Key{})) Expect(device.PrivateKey).ToNot(Equal(wgtypes.Key{})) Expect(device.Jc).To(Equal(7)) Expect(device.H1).To(Equal("200000000-280000000")) Expect(device.S3).To(Equal(35)) Expect(device.I1).To(Equal("<r 40>")) + Expect(device.Peers).To(BeEmpty(), "ReplacePeers should have cleared the stale peer")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/amneziawg_test.go` around lines 162 - 181, Remove the pre-populated peer setup from the BeforeEach block, or update the test to explicitly assert that ConfigureDevice replaces the seeded peer. Ensure the test meaningfully covers ReplacePeers behavior rather than retaining an unused device.Peers fixture.
94-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused error-injection scaffolding leaves
NewDrivererror paths untested.
checkNewDriverErr(fields at lines 100, default no-op at lines 142-144) andfakeClient.configureDeviceErr(line 315) are never overridden by any test, so branches such as the PSK validation failure (w.spec.PSK == "" || w.spec.PSK == "default psk") andConfigureDevicefailure propagation inNewDriver(per driver.go snippet) go untested.Consider adding a negative-path test using these hooks, or remove the scaffolding if it's not intended to be exercised.
Also applies to: 142-144, 313-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/amneziawg_test.go` around lines 94 - 101, Remove the unused NewDriver error-injection scaffolding, including testDriver.checkNewDriverErr and fakeClient.configureDeviceErr, unless you add negative-path tests that override these hooks and assert PSK validation and ConfigureDevice failures propagate through NewDriver. Ensure the resulting tests explicitly cover those error branches if retaining the hooks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cable/amneziawg/driver.go`:
- Around line 91-172: Register failure cleanup immediately after setupDevice
succeeds, before NewClient can fail. Extend the existing NewDriver error defer
to close w.userspaceDevice and delete w.link when err != nil, while preserving
client cleanup and nil assignment. Ensure all subsequent initialization failures
reclaim the userspace device, its goroutine/socket resources, and the netlink
link.
In `@pkg/cable/amneziawg/obfuscation.go`:
- Around line 83-132: Extend obfuscationFromOptions and setStringOption to
validate string options before assigning them: enforce the supported N-M syntax
for h1-h4 and AWG mini-syntax for i1-i5, warning and retaining defaults for
invalid values. After applying integer overrides, validate that Jmin is not
greater than Jmax; warn and ignore the invalid override(s), preserving a valid
fallback configuration.
---
Nitpick comments:
In `@pkg/cable/amneziawg/amneziawg_test.go`:
- Around line 162-181: Remove the pre-populated peer setup from the BeforeEach
block, or update the test to explicitly assert that ConfigureDevice replaces the
seeded peer. Ensure the test meaningfully covers ReplacePeers behavior rather
than retaining an unused device.Peers fixture.
- Around line 94-101: Remove the unused NewDriver error-injection scaffolding,
including testDriver.checkNewDriverErr and fakeClient.configureDeviceErr, unless
you add negative-path tests that override these hooks and assert PSK validation
and ConfigureDevice failures propagate through NewDriver. Ensure the resulting
tests explicitly cover those error branches if retaining the hooks.
In `@pkg/cable/amneziawg/obfuscation.go`:
- Around line 28-30: Document in the CRD/API documentation for
cableDriverOptions that H1-H4 and S1-S4 overrides must be identical across every
gateway and cluster in the mesh. Clearly distinguish these shared parameters
from Jc/I* values, which may differ, and warn that inconsistent per-gateway
SUBMARINER_CABLEDRIVEROPTIONS values cause runtime connectivity failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9fc8758a-4d17-49ec-9307-c6c6a904b75d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.modpkg/cable/amneziawg/amneziawg_test.gopkg/cable/amneziawg/client.gopkg/cable/amneziawg/daemon.gopkg/cable/amneziawg/driver.gopkg/cable/amneziawg/getconnections.gopkg/cable/amneziawg/obfuscation.gopkg/cable/options.gopkg/cable/options_test.gopkg/cableengine/cableengine.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/cable/amneziawg/driver.go (2)
242-242: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDiscarded
removePeererrors leave no diagnostic trail.Both call sites (
ConnectToEndpointon peer-key change,DisconnectFromEndpoint) discard theremovePeererror via_ =without even a warning log. If peer removal genuinely fails (e.g., device not reachable), there's no signal of a stale peer remaining configured.Also applies to: 317-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/driver.go` at line 242, Update the removePeer call sites in ConnectToEndpoint and DisconnectFromEndpoint to handle removal errors instead of discarding them. Log a warning containing the error and enough peer context to diagnose a potentially stale configured peer, while preserving the existing control flow.
336-341: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
setupDevicetreats anyLinkByNameerror as "no existing link", unlikecleanupDevice.
cleanupDevice(lines 461-468) correctly distinguishesIsLinkNotFoundErrorfrom a genuine lookup failure, butsetupDevicehere silently proceeds on any non-nil error fromLinkByName, including permission/netlink errors unrelated to "not found". This could mask a real lookup problem instead of surfacing it.🛠️ Proposed fix
- if link, err := w.netLink.LinkByName(DefaultDeviceName); err == nil { - if err := w.netLink.LinkDel(link); err != nil { - return errors.Wrap(err, "failed to delete existing AmneziaWG device") - } - } + link, err := w.netLink.LinkByName(DefaultDeviceName) + switch { + case err == nil: + if err := w.netLink.LinkDel(link); err != nil { + return errors.Wrap(err, "failed to delete existing AmneziaWG device") + } + case !netlinkAPI.IsLinkNotFoundError(err): + return errors.Wrapf(err, "error checking for existing AmneziaWG device %q", DefaultDeviceName) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/driver.go` around lines 336 - 341, Update setupDevice’s LinkByName error handling to ignore only IsLinkNotFoundError, matching cleanupDevice; return or wrap any other lookup error, while preserving deletion and existing delete-error handling when the link is found.pkg/cable/amneziawg/amneziawg_test.go (1)
319-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFake client only tracks a subset of AmneziaWG obfuscation fields.
ConfigureDevicecopies/tracks onlyJc,S3,H1,I1(plus key/peer fields). Per the PR's documented option set (jc,jmin,jmax,s1-s4,h1-h4,i1-i5), the remaining fields are never captured, sotestNewDriver's default/override assertions can't catch regressions in those defaults.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/amneziawg_test.go` around lines 319 - 397, Extend fakeClient.ConfigureDevice to copy every AmneziaWG obfuscation field from wgtypes.Config into the stored device, including Jmin, Jmax, S1-S4, H1-H4, and I1-I5 alongside the existing fields. Preserve the current handling of keys, listen port, peers, and existing obfuscation values so testNewDriver can verify both defaults and overrides for the complete option set.pkg/cable/amneziawg/getconnections.go (1)
61-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompare keys directly in
connectionByKey.wgtypes.Keyis comparable, so==avoids repeated string conversions in the loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/getconnections.go` around lines 61 - 70, Update amneziawgDriver.connectionByKey to compare the input key directly with the key returned by keyFromSpec, using wgtypes.Key equality instead of converting both keys to strings. Preserve the existing iteration and nil return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cable/amneziawg/daemon.go`:
- Around line 75-90: Close fileUAPI in the UAPIListen error branch before
returning. Update the failure handling around ipc.UAPIListen to release the
descriptor alongside tdev and dev, while preserving the existing wrapped error
return.
In `@pkg/cable/amneziawg/driver.go`:
- Around line 285-292: The connection flow after verifyNewPeer should not record
a successful connection when verification fails. Update the error branch
handling around verifyNewPeer to return or propagate the error before reaching
the success log and cable.RecordConnection call, preserving the existing
connected path only for successful verification.
---
Nitpick comments:
In `@pkg/cable/amneziawg/amneziawg_test.go`:
- Around line 319-397: Extend fakeClient.ConfigureDevice to copy every AmneziaWG
obfuscation field from wgtypes.Config into the stored device, including Jmin,
Jmax, S1-S4, H1-H4, and I1-I5 alongside the existing fields. Preserve the
current handling of keys, listen port, peers, and existing obfuscation values so
testNewDriver can verify both defaults and overrides for the complete option
set.
In `@pkg/cable/amneziawg/driver.go`:
- Line 242: Update the removePeer call sites in ConnectToEndpoint and
DisconnectFromEndpoint to handle removal errors instead of discarding them. Log
a warning containing the error and enough peer context to diagnose a potentially
stale configured peer, while preserving the existing control flow.
- Around line 336-341: Update setupDevice’s LinkByName error handling to ignore
only IsLinkNotFoundError, matching cleanupDevice; return or wrap any other
lookup error, while preserving deletion and existing delete-error handling when
the link is found.
In `@pkg/cable/amneziawg/getconnections.go`:
- Around line 61-70: Update amneziawgDriver.connectionByKey to compare the input
key directly with the key returned by keyFromSpec, using wgtypes.Key equality
instead of converting both keys to strings. Preserve the existing iteration and
nil return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5608666e-e67a-4a9e-8052-8d7e705325ae
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
go.modpkg/cable/amneziawg/amneziawg_test.gopkg/cable/amneziawg/client.gopkg/cable/amneziawg/daemon.gopkg/cable/amneziawg/driver.gopkg/cable/amneziawg/getconnections.gopkg/cable/amneziawg/obfuscation.gopkg/cable/amneziawg/obfuscation_test.gopkg/cable/options.gopkg/cable/options_test.gopkg/cableengine/cableengine.go
77787aa to
70af8b7
Compare
70af8b7 to
4945ece
Compare
| func init() { | ||
| kzerolog.AddFlags(nil) | ||
| } | ||
|
|
||
| var _ = BeforeSuite(func() { | ||
| flags := flag.NewFlagSet("kzerolog", flag.ExitOnError) | ||
| kzerolog.AddFlags(flags) | ||
| _ = flags.Parse([]string{"-v=4"}) | ||
|
|
||
| kzerolog.InitK8sLogging() | ||
| }) | ||
|
|
||
| func TestAmneziaWG(t *testing.T) { | ||
| RegisterFailHandler(Fail) | ||
| RunSpecs(t, "AmneziaWG Suite") | ||
| } |
There was a problem hiding this comment.
Since this package isn't trivial and there's more than one _test.go file, this should go in a separate amneziawg_suite_test.go file.
There was a problem hiding this comment.
Done. the Ginkgo suite entrypoint is in a separate file:
| type UserspaceDevice = userspaceDevice | ||
|
|
||
| // StartUserspaceDeviceForTest overrides userspace device startup in unit tests. | ||
| var StartUserspaceDeviceForTest func(ifaceName string) (UserspaceDevice, error) |
There was a problem hiding this comment.
Having both startUserspaceDevice and StartUserspaceDeviceForTest seems redundant. To simplify, you could make startUserspaceDevice public and remove StartUserspaceDeviceForTest entirely.
There was a problem hiding this comment.
Done. there is a single public StartUserspaceDevice (tests override the var); StartUserspaceDeviceForTest is gone:
Test override:
| return &w, nil | ||
| } | ||
|
|
||
| func (w *amneziawgDriver) Init(_ context.Context) error { |
There was a problem hiding this comment.
Nit: it seems w was borrowed from wireguard - let's use a
| func (w *amneziawgDriver) Init(_ context.Context) error { | |
| func (a *amneziawgDriver) Init(_ context.Context) error { |
There was a problem hiding this comment.
Done. receiver renamed from w to a throughout.
| } | ||
|
|
||
| defer func() { | ||
| if err == nil { |
There was a problem hiding this comment.
This is a bit fragile since it relies on err being the last assigned error variable before any return and variable shadowing with := can break this assumption (which line 144 does). At worst cleanup would be skipped on error which isn't catastrophic.
There was a problem hiding this comment.
Done. cleanup no longer depends on the last assigned err. NewDriver uses an explicit setupOK flag; the defer tears down only when setup did not complete:
Success path sets the flag before returning:
(Also removed the err shadowing on the backend-port path that made the old defer fragile)
| return nil, errors.Wrap(err, "error generating private key") | ||
| } | ||
|
|
||
| port, err := localEndpoint.Spec().GetBackendPort(v1.UDPPortConfig, w.spec.NATTPort) |
There was a problem hiding this comment.
Done. no longer shadows err with := on that path:
Together with the setupOK cleanup flag, this keeps failure teardown reliable in NewDriver.
| . "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| func TestIsValidHeaderValue(t *testing.T) { |
There was a problem hiding this comment.
Please use Ginkgo for all unit tests.
There was a problem hiding this comment.
Done. converted to Ginkgo (Describe / DescribeTable).
I also split driver options from obfuscation parameters: obfuscation knobs stay in obfuscation.go, non-obfuscation driver options (currently just keepalive) and the assemble/apply/validate path live in options.go. Internal tests for both are in options_internal_test.go, e.g.:
| @@ -0,0 +1,146 @@ | |||
| /* | |||
There was a problem hiding this comment.
Since these tests access private functions, it should be named obfuscation_internal_test.go.
There was a problem hiding this comment.
Done. same-package *_internal_test.go for unexported helpers.
I intentionally split driver options from obfuscation parameters into separate tables/files:
- Obfuscation knobs (
jc,jmin/jmax,s1..s4,h1..h4,i1..i5) live inobfuscation.go:
- Cross-field obfuscation checks (
jmin/jmax, overlappingh*) are also there:
- Non-obfuscation driver options live in
options.go. Right now that table has a single option,keepalive:
options.goalso owns the shared option type, assembles both tables, applies values, and invokes validations (includingvalidateObfuscationOptions):
Because of that layout I named the internal tests options_internal_test.go (rather than obfuscation_internal_test.go). they cover the combined apply/validate path plus the obfuscation parsers:
| "github.com/submariner-io/submariner/pkg/cable" | ||
| ) | ||
|
|
||
| func TestGetDriverOptions(t *testing.T) { |
There was a problem hiding this comment.
Done. pkg/cable/options_test.go is Ginkgo now:
Suite entrypoint:
|
|
||
| options := map[string]string{} | ||
| if err := json.Unmarshal([]byte(raw), &options); err != nil { | ||
| logger.Warningf("Ignoring invalid %s value: %v", CableDriverOptionsEnv, err) |
There was a problem hiding this comment.
I think we should propagate user configuration errors and fail-fast to provide clear indication that their configuration was rejected, instead of logging warning that they likely won't see.
There was a problem hiding this comment.
Done. invalid configuration fails fast instead of warn-and-ignore.
Invalid JSON from the env returns an error:
On the AmneziaWG side I also split driver options from obfuscation parameters. Obfuscation stays in obfuscation.go; options.go holds the shared option machinery, the non-obfuscation table (currently just keepalive), assembles both tables, applies values, and invokes validations.. unknown/invalid keys are collected and returned together:
NewDriver surfaces that as a hard failure:
4945ece to
e7fc459
Compare
|
@tpantelis
Separately, I also reworked option parsing for stricter validation, and so that all configuration errors are reported together instead of one at a time. Sorry if I misunderstood... after squashing and force-pushing, it’s hard to see what specifically changed for the review comments. |
e7fc459 to
d46bc5e
Compare
Actually I can see the changes via the Compare link provided by GitHub after each push. |
|
|
||
| It("should use the configured keepalive when connecting", func() { | ||
| natInfo := newNATInfo("remote", "172.16.0.0/16") | ||
| _, err := t.driver.ConnectToEndpoint(natInfo) |
There was a problem hiding this comment.
The outer context for this test case is intended to test NewDriver so I technically don't think we should call ConnectToEndpoint here. This test case should be in testConnectToEndpoint.
There was a problem hiding this comment.
Done. Moved this case into testConnectToEndpoint:
(Exact lines on tip after push; search for keepalive cable driver option is set under testConnectToEndpoint.)
|
@sanek9 Would it be feasible to add the new cable driver to the CI E2E test matrix? |
b14bae6 to
c738b3c
Compare
Add an AmneziaWG-based cable driver with obfuscation options, stricter multi-error option validation, and connection status handling aligned with the WireGuard driver. Also add amneziawg to the CI E2E cable-driver matrix. Signed-off-by: sanek9 <sanya0996@gmail.com>
c3592c1 to
b7210fc
Compare
|
@tpantelis Man, these AI tools are so smart... My employer has been paying for my Cursor subscription for half a year now, and I only started using it a week ago. All I really needed here was an AmneziaWG tunnel... and guess what? It found some repository, created another pull request, and didn't even ask me. Then it told me it wouldn't work without it and that we need to get it merged. It even wanted to leave a comment here on its own. You just give it an idea, and it runs the lab, writes the report, and then generates the code with tests. I really hope the code quality is good, though... I learned Go about 7 years ago, but I haven't had to write much in it since... |
3 participants
What this PR does / why we need it
Adds an AmneziaWG cable driver (WireGuard-compatible with DPI obfuscation) for environments where plain WireGuard is blocked or fingerprintable.
Also makes AmneziaWG obfuscation parameters (and peer keepalive) configurable via the gateway environment variable
SUBMARINER_CABLEDRIVEROPTIONS(JSON map). When unset/empty, built-in defaults are used so older operators remain compatible.More details
pkg/cable/amneziawgregistered in the cable engine (same pattern as WireGuard).amneziawg-gouserspace stack(linked into
submariner-gateway; no separate binary and no AmneziaWG kernel module on nodes).MIT is compatible with this Apache-2.0 project.
privileged+NET_ADMIN(and host networkpath as today), which is enough to create a TUN device — no extra RBAC/securityContext changes.
jc,jmin,jmax,s1–s4,h1–h4,i1–i5.keepalive(seconds; default10;0disables).single parse are returned together so they can be fixed in one pass.
H1–H4/S1–S4must match across peers in the mesh;Jc/I*may differ per gateway.spec.cableDriverOptionsand--cable-driver-optionto populate the env var.Related issues
Checklist
Summary by CodeRabbit