feat: add ConntrackDelete command #1134
Conversation
WalkthroughAdds conntrack flow deletion APIs: a package-level Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PackageAPI as ConntrackDelete()
participant Handle as Handle.ConntrackDelete()
participant Netlink as Netlink Handle
User->>PackageAPI: ConntrackDelete(table, family, flow)
PackageAPI->>Handle: pkgHandle.ConntrackDelete(table, family, flow)
Handle->>Netlink: build DELETE netlink message
Note right of Handle: convert flow -> netlink attrs
Handle->>Netlink: attach attrs & execute request
alt success
Netlink-->>Handle: OK
Handle-->>PackageAPI: nil
else error
Netlink-->>Handle: error
Handle-->>PackageAPI: error
end
PackageAPI-->>User: return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
conntrack_test.go (2)
1501-1609: Misleading comment and unnecessary pkgHandle swap.The comment states that deletion is "via the package-level wrapper ConntrackDelete (which uses pkgHandle)" but Line 1595 calls
h.ConntrackDeletedirectly (the handle method). ThepkgHandleswap at lines 1533-1535 is therefore unused and misleading.Either:
- Update the comment to state deletion is via the handle method, and remove the pkgHandle swap (lines 1533-1535), or
- Call
ConntrackDelete(ConntrackTable, InetFamily(nl.FAMILY_V4), &flow)instead at Line 1595 to actually use the package-level wrapperOther tests in this file (e.g.,
TestConntrackCreateV4) consistently call handle methods directly without swappingpkgHandle, so the first option aligns better with existing patterns.Apply this diff to align with existing test patterns:
-// TestConntrackDeleteV4 creates an IPv4 conntrack entry, verifies it exists, -// deletes it via the package-level wrapper ConntrackDelete (which uses pkgHandle), -// and verifies it was removed. +// TestConntrackDeleteV4 creates an IPv4 conntrack entry, verifies it exists, +// deletes it via the handle method, and verifies it was removed. func TestConntrackDeleteV4(t *testing.T) { // Print timestamps in UTC os.Setenv("TZ", "") @@ -1530,10 +1530,6 @@ t.Fatalf("failed to create netlink handle: %s", err) } - // Point pkgHandle to the namespaced handle so the package-level wrapper acts in this ns. - orig := pkgHandle - pkgHandle = h - defer func() { pkgHandle = orig }() - flow := ConntrackFlow{ FamilyType: FAMILY_V4,
1611-1719: Same issue: misleading comment and unnecessary pkgHandle swap.Same issue as
TestConntrackDeleteV4. The comment claims the package-level wrapper is used but Line 1705 callsh.ConntrackDeletedirectly. ThepkgHandleswap at lines 1643-1645 is unused.Apply this diff:
-// TestConntrackDeleteV6 creates an IPv6 conntrack entry, verifies it exists, -// deletes it via the package-level wrapper ConntrackDelete (which uses pkgHandle), -// and verifies it was removed. +// TestConntrackDeleteV6 creates an IPv6 conntrack entry, verifies it exists, +// deletes it via the handle method, and verifies it was removed. func TestConntrackDeleteV6(t *testing.T) { // Print timestamps in UTC os.Setenv("TZ", "") @@ -1639,10 +1639,6 @@ t.Fatalf("failed to create netlink handle: %s", err) } - // Point pkgHandle to the namespaced handle so the package-level wrapper acts in this ns. - orig := pkgHandle - pkgHandle = h - defer func() { pkgHandle = orig }() - flow := ConntrackFlow{ FamilyType: FAMILY_V6,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
conntrack_linux.go(2 hunks)conntrack_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
conntrack_linux.go (3)
conntrack_unspecified.go (3)
ConntrackTableType(7-7)InetFamily(10-10)ConntrackFlow(13-13)handle_linux.go (1)
Handle(24-27)nl/conntrack_linux.go (1)
IPCTNL_MSG_CT_DELETE(68-68)
conntrack_test.go (8)
netlink_test.go (1)
KernelVersion(290-308)handle_linux.go (1)
NewHandleAt(139-141)nl/nl_linux.go (2)
FAMILY_V4(25-25)FAMILY_V6(26-26)conntrack_linux.go (18)
ConntrackFlow(321-332)IPTuple(279-287)ProtoInfo(242-244)ProtoInfoTCP(248-250)ProtoInfoTCP(253-253)ConntrackCreate(64-66)ConntrackTable(22-22)ConntrackTableList(51-53)ConntrackFilter(767-773)ConntrackFilterType(744-744)ConntrackOrigSrcIP(747-747)ConntrackOrigDstIP(748-748)ConntrackReplySrcIP(749-749)ConntrackReplyDstIP(750-750)ConntrackOrigSrcPort(752-752)ConntrackOrigDstPort(753-753)ConntrackDelete(76-78)InetFamily(34-34)conntrack_unspecified.go (4)
ConntrackFlow(13-13)ConntrackTableList(23-25)ConntrackFilter(19-19)InetFamily(10-10)nl/tc_linux.go (1)
IPPROTO_TCP(1247-1247)nl/conntrack_linux.go (1)
TCP_CONNTRACK_ESTABLISHED(38-38)netlink.go (1)
NewIPNet(35-40)
🔇 Additional comments (2)
conntrack_linux.go (2)
74-78: LGTM!The package-level function follows the established pattern of
ConntrackCreateandConntrackUpdate, correctly delegating to the handle method.
157-172: LGTM!The implementation correctly follows the pattern of
ConntrackCreateandConntrackUpdate, using the appropriateIPCTNL_MSG_CT_DELETEoperation. Reusingflow.toNlData()is appropriate since the flow tuple identifies which entry to delete.
|
/assign @aboch This is the primitive that was missing, similar to the The test failing does not look related |
aojea
force-pushed
the
conntrack_delete
branch
from
a0ec4a9 to
95ea400
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
conntrack_linux.go (1)
157-172: Harden delete: align family, include zone, and treat not-found as success
- Align flow.FamilyType with passed family to avoid mismatches.
- Include CTA_ZONE when set to delete flows in non-default zones.
- Mirror ConntrackDeleteFilters semantics: fs.ErrNotExist should be non-fatal.
Apply:
func (h *Handle) ConntrackDelete(table ConntrackTableType, family InetFamily, flow *ConntrackFlow) error { - req := h.newConntrackRequest(table, family, nl.IPCTNL_MSG_CT_DELETE, unix.NLM_F_ACK) - attr, err := flow.toNlData() + req := h.newConntrackRequest(table, family, nl.IPCTNL_MSG_CT_DELETE, unix.NLM_F_ACK) + // Ensure flow family matches header family without mutating caller's flow. + tmp := *flow + if tmp.FamilyType == 0 { + tmp.FamilyType = uint8(family) + } else if tmp.FamilyType != uint8(family) { + return fmt.Errorf("family mismatch: flow.FamilyType=%d, arg=%d", tmp.FamilyType, family) + } + attr, err := tmp.toNlData() if err != nil { return err } for _, a := range attr { req.AddData(a) } - _, err = req.Execute(unix.NETLINK_NETFILTER, 0) - return err + // Include zone when set to target zone-specific entries. + if tmp.Zone != 0 { + req.AddData(nl.NewRtAttr(nl.CTA_ZONE, nl.BEUint16Attr(tmp.Zone))) + } + + _, err = req.Execute(unix.NETLINK_NETFILTER, 0) + if err != nil && errors.Is(err, fs.ErrNotExist) { + // Be idempotent and consistent with ConntrackDeleteFilters. + return nil + } + return err }Please confirm tests for zoned entries (non-zero zone) pass after this change. If helpful, I can add a focused test for delete-in-zone.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
conntrack_linux.go(2 hunks)conntrack_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- conntrack_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
conntrack_linux.go (3)
conntrack_unspecified.go (3)
ConntrackTableType(7-7)InetFamily(10-10)ConntrackFlow(13-13)handle_linux.go (1)
Handle(24-27)nl/conntrack_linux.go (1)
IPCTNL_MSG_CT_DELETE(68-68)
🔇 Additional comments (1)
conntrack_linux.go (1)
74-79: Wrapper looks goodDelegates to pkgHandle consistently with Create/Update; naming and CLI comment align with existing style.
|
@aojea Please remove "feat: " from the commit message |
Add a new ConntrackDelete() function that operates directly on flows, same as the ConntrackCreate() and ConntrackUpdate() functions. We already have ConntrackDeleteFilters() that is very useful to batch operations and to express the intent based on filter matches, but having the function that operate on flows allow to create much more complex filtering. Signed-off-by: Antonio Ojea <[email protected]>
aojea
force-pushed
the
conntrack_delete
branch
from
95ea400 to
448b3ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
conntrack_linux.go (1)
74-79: Doc nit: drop “using the handle” for pkg-level API.Align wording with Create/Update comments.
-// ConntrackDelete deletes an existing conntrack flow in the desired table using the handle +// ConntrackDelete deletes an existing conntrack flow in the desired table // conntrack -D [table] Delete conntrack flow func ConntrackDelete(table ConntrackTableType, family InetFamily, flow *ConntrackFlow) error { return pkgHandle.ConntrackDelete(table, family, flow) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
conntrack_linux.go(2 hunks)conntrack_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- conntrack_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
conntrack_linux.go (3)
conntrack_unspecified.go (3)
ConntrackTableType(7-7)InetFamily(10-10)ConntrackFlow(13-13)handle_linux.go (1)
Handle(24-27)nl/conntrack_linux.go (1)
IPCTNL_MSG_CT_DELETE(68-68)
| // ConntrackDelete deletes an existing conntrack flow in the desired table using the handle | ||
| // conntrack -D [table] Delete a conntrack | ||
| func (h *Handle) ConntrackDelete(table ConntrackTableType, family InetFamily, flow *ConntrackFlow) error { | ||
| req := h.newConntrackRequest(table, family, nl.IPCTNL_MSG_CT_DELETE, unix.NLM_F_ACK) | ||
| attr, err := flow.toNlData() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, a := range attr { | ||
| req.AddData(a) | ||
| } | ||
|
|
||
| _, err = req.Execute(unix.NETLINK_NETFILTER, 0) | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Build a minimal delete key (CTA_TUPLE_ORIG [+ CTA_ZONE]); avoid extra attrs.
Using flow.toNlData() adds CTA_TUPLE_REPLY, CTA_TIMEOUT, MARK, PROTOINFO, which can cause EINVAL or no-match; zone is also currently omitted, so deletes in non-zero zones may fail. Construct the minimal key and include zone when set; use the method parameter ‘family’ to encode the tuple.
func (h *Handle) ConntrackDelete(table ConntrackTableType, family InetFamily, flow *ConntrackFlow) error {
- req := h.newConntrackRequest(table, family, nl.IPCTNL_MSG_CT_DELETE, unix.NLM_F_ACK)
- attr, err := flow.toNlData()
- if err != nil {
- return err
- }
-
- for _, a := range attr {
- req.AddData(a)
- }
-
- _, err = req.Execute(unix.NETLINK_NETFILTER, 0)
- return err
+ if flow == nil {
+ return fmt.Errorf("nil flow")
+ }
+ req := h.newConntrackRequest(table, family, nl.IPCTNL_MSG_CT_DELETE, unix.NLM_F_ACK)
+ // Minimal delete key: CTA_TUPLE_ORIG (+ CTA_ZONE if set). Avoid timeout/mark/protoinfo/reply tuple.
+ forward, err := flow.Forward.toNlData(uint8(family))
+ if err != nil {
+ return err
+ }
+ ctTupleOrig := nl.NewRtAttr(unix.NLA_F_NESTED|nl.CTA_TUPLE_ORIG, nil)
+ for _, a := range forward {
+ ctTupleOrig.AddChild(a)
+ }
+ req.AddData(ctTupleOrig)
+ if flow.Zone != 0 {
+ req.AddData(nl.NewRtAttr(nl.CTA_ZONE, nl.BEUint16Attr(flow.Zone)))
+ }
+ _, err = req.Execute(unix.NETLINK_NETFILTER, 0)
+ return err
}Please add/delete tests for non-zero zones and for flows where only orig tuple is provided (no reply tuple).
I'll verify this review comment by examining the codebase to check the claims about the ConntrackDelete function and the proposed fix.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine the ConntrackFlow struct and related methods
rg -n "type ConntrackFlow struct" -A 20
Length of output: 2673
🏁 Script executed:
#!/bin/bash
# Check for toNlData methods and their signatures
rg -n "func.*toNlData" -B 2 -A 5Length of output: 1669
🏁 Script executed:
#!/bin/bash
# Search for CTA_TUPLE_ORIG, CTA_ZONE, and related netlink constants
rg -n "CTA_TUPLE_ORIG|CTA_TUPLE_REPLY|CTA_ZONE" -B 2 -A 2Length of output: 4845
🏁 Script executed:
#!/bin/bash
# Examine the current ConntrackDelete implementation
rg -n "func.*ConntrackDelete" -A 20Length of output: 10642
🏁 Script executed:
#!/bin/bash
# Search for existing tests for ConntrackDelete
rg -n "ConntrackDelete|conntrackDelete" --type go -g "*test*"Length of output: 895
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full implementation of ConntrackFlow.toNlData() to see what attributes it adds
rg -n "func (s \*ConntrackFlow) toNlData" -A 60
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check what TestConntrackDeleteV4 looks like to understand test coverage and zone handling
sed -n '1517,1625p' conntrack_test.goLength of output: 3237
🏁 Script executed:
#!/bin/bash
# Check nl.NewRtAttr signature and see if BEUint16Attr exists
rg -n "BEUint16Attr|NewRtAttr" nl/ -B 2 -A 2Length of output: 1522
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full ConntrackFlow.toNlData() implementation - try simpler pattern
sed -n '357,450p' conntrack_linux.go
Length of output: 3050
🏁 Script executed:
#!/bin/bash
# Search for what gets added to payload in toNlData
rg -n "payload.*append|payload.*=" conntrack_linux.go | head -20Length of output: 161
🏁 Script executed:
#!/bin/bash
# Check if zone is handled in the current toNlData method
sed -n '357,500p' conntrack_linux.go | grep -i "zone\|timeout\|mark\|protoinfo\|reply"Length of output: 1123
---
🏁 Script executed:
```shell
#!/bin/bash
# Check ConntrackDeleteFilters to see if it handles delete differently
sed -n '184,215p' conntrack_linux.go
Length of output: 1147
🏁 Script executed:
#!/bin/bash
# Search for any kernel documentation or comments about minimal delete keys
rg -n "Delete|minimal|key" conntrack_linux.go | head -20Length of output: 1754
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if fmt is imported in conntrack_linux.go for the proposed nil check
head -50 conntrack_linux.go | grep -E "^import|fmt"
Length of output: 15
🏁 Script executed:
#!/bin/bash
# Look for any integration tests or CI that might test zone-specific deletes
rg -n "Zone.*!= 0|Zone.*> 0|non.?zero.*zone" --type goLength of output: 40
ConntrackDelete must use minimal tuple key and include zone to support non-zero zone operations.
The current implementation calls flow.toNlData() which unnecessarily adds CTA_TUPLE_REPLY, CTA_MARK, CTA_TIMEOUT, and CTA_PROTOINFO. More critically, the zone field is never serialized—deletes in non-zero zones will fail. Use flow.Forward.toNlData() to build only CTA_TUPLE_ORIG and conditionally add CTA_ZONE when set.
func (h *Handle) ConntrackDelete(table ConntrackTableType, family InetFamily, flow *ConntrackFlow) error {
+ if flow == nil {
+ return fmt.Errorf("nil flow")
+ }
req := h.newConntrackRequest(table, family, nl.IPCTNL_MSG_CT_DELETE, unix.NLM_F_ACK)
- attr, err := flow.toNlData()
- if err != nil {
- return err
- }
-
- for _, a := range attr {
- req.AddData(a)
- }
-
+ // Minimal delete key: CTA_TUPLE_ORIG (+ CTA_ZONE if set). Avoid timeout/mark/protoinfo/reply tuple.
+ forward, err := flow.Forward.toNlData(uint8(family))
+ if err != nil {
+ return err
+ }
+ ctTupleOrig := nl.NewRtAttr(unix.NLA_F_NESTED|nl.CTA_TUPLE_ORIG, nil)
+ for _, a := range forward {
+ ctTupleOrig.AddChild(a)
+ }
+ req.AddData(ctTupleOrig)
+ if flow.Zone != 0 {
+ req.AddData(nl.NewRtAttr(nl.CTA_ZONE, nl.BEUint16Attr(flow.Zone)))
+ }
_, err = req.Execute(unix.NETLINK_NETFILTER, 0)
return err
}Add tests for non-zero zones and for flows where only the forward tuple is provided.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ConntrackDelete deletes an existing conntrack flow in the desired table using the handle | |
| // conntrack -D [table] Delete a conntrack | |
| func (h *Handle) ConntrackDelete(table ConntrackTableType, family InetFamily, flow *ConntrackFlow) error { | |
| req := h.newConntrackRequest(table, family, nl.IPCTNL_MSG_CT_DELETE, unix.NLM_F_ACK) | |
| attr, err := flow.toNlData() | |
| if err != nil { | |
| return err | |
| } | |
| for _, a := range attr { | |
| req.AddData(a) | |
| } | |
| _, err = req.Execute(unix.NETLINK_NETFILTER, 0) | |
| return err | |
| } | |
| // ConntrackDelete deletes an existing conntrack flow in the desired table using the handle | |
| // conntrack -D [table] Delete a conntrack | |
| func (h *Handle) ConntrackDelete(table ConntrackTableType, family InetFamily, flow *ConntrackFlow) error { | |
| if flow == nil { | |
| return fmt.Errorf("nil flow") | |
| } | |
| req := h.newConntrackRequest(table, family, nl.IPCTNL_MSG_CT_DELETE, unix.NLM_F_ACK) | |
| // Minimal delete key: CTA_TUPLE_ORIG (+ CTA_ZONE if set). Avoid timeout/mark/protoinfo/reply tuple. | |
| forward, err := flow.Forward.toNlData(uint8(family)) | |
| if err != nil { | |
| return err | |
| } | |
| ctTupleOrig := nl.NewRtAttr(unix.NLA_F_NESTED|nl.CTA_TUPLE_ORIG, nil) | |
| for _, a := range forward { | |
| ctTupleOrig.AddChild(a) | |
| } | |
| req.AddData(ctTupleOrig) | |
| if flow.Zone != 0 { | |
| req.AddData(nl.NewRtAttr(nl.CTA_ZONE, nl.BEUint16Attr(flow.Zone))) | |
| } | |
| _, err = req.Execute(unix.NETLINK_NETFILTER, 0) | |
| return err | |
| } |
🤖 Prompt for AI Agents
In conntrack_linux.go around lines 157 to 173, ConntrackDelete currently uses
flow.toNlData() which serializes full tuple reply, mark, timeout and protinfo
and omits the zone, causing deletes to fail for non-zero zones; change it to
serialize only the minimal delete key by calling flow.Forward.toNlData() (so
only CTA_TUPLE_ORIG is added) and if flow.Zone != 0 add a CTA_ZONE attribute to
the request before Execute; also remove adding extraneous attributes and add
unit tests covering deletes with a non-zero zone and deletes where only the
forward tuple is present.
2 participants
Add a new ConntrackDelete() function that operates directly on flows, same as the ConntrackCreate() and ConntrackUpdate() functions.
We already have ConntrackDeleteFilters() that is very useful to batch operations and to express the intent based on filter matches, but having the function that operate on flows allow to create much more complex filtering.
Summary by CodeRabbit
New Features
Tests