Skip to content

feat: add ConntrackDelete command #1134

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add ConntrackDelete command #1134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions conntrack_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ func ConntrackUpdate(table ConntrackTableType, family InetFamily, flow *Conntrac
return pkgHandle.ConntrackUpdate(table, family, flow)
}

// ConntrackDelete deletes an existing conntrack flow in the desired table using the handle
// conntrack -D [table] Delete conntrack flow
func ConntrackDelete(table ConntrackTableType, family InetFamily, flow *ConntrackFlow) error {
return pkgHandle.ConntrackDelete(table, family, flow)
}

// ConntrackDeleteFilter deletes entries on the specified table on the base of the filter
// conntrack -D [table] parameters Delete conntrack or expectation
//
Expand Down Expand Up @@ -148,6 +154,23 @@ func (h *Handle) ConntrackUpdate(table ConntrackTableType, family InetFamily, fl
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 {
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
}

Comment on lines +157 to +173
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 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 5

Length 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 2

Length of output: 4845

🏁 Script executed:

#!/bin/bash
# Examine the current ConntrackDelete implementation
rg -n "func.*ConntrackDelete" -A 20

Length 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.go

Length of output: 3237

🏁 Script executed:

#!/bin/bash
# Check nl.NewRtAttr signature and see if BEUint16Attr exists
rg -n "BEUint16Attr|NewRtAttr" nl/ -B 2 -A 2

Length 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 -20

Length 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 -20

Length 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 go

Length 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.

Suggested change
// 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.

// ConntrackDeleteFilter deletes entries on the specified table on the base of the filter using the netlink handle passed
// conntrack -D [table] parameters Delete conntrack or expectation
//
Expand Down
220 changes: 220 additions & 0 deletions conntrack_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1511,6 +1511,226 @@ func TestConntrackCreateV6(t *testing.T) {
checkProtoInfosEqual(t, flow.ProtoInfo, match.ProtoInfo)
}

// 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.
func TestConntrackDeleteV4(t *testing.T) {
// Print timestamps in UTC
os.Setenv("TZ", "")

requiredModules := []string{"nf_conntrack", "nf_conntrack_netlink"}
k, m, err := KernelVersion()
if err != nil {
t.Fatal(err)
}
// Conntrack l3proto was unified since 4.19
// https://github.com/torvalds/linux/commit/a0ae2562c6c4b2721d9fddba63b7286c13517d9f
if k < 4 || k == 4 && m < 19 {
requiredModules = append(requiredModules, "nf_conntrack_ipv4")
}
// Implicitly skips test if not root:
nsStr, teardown := setUpNamedNetlinkTestWithKModule(t, requiredModules...)
t.Cleanup(teardown)

ns, err := netns.GetFromName(nsStr)
if err != nil {
t.Fatalf("couldn't get handle to generated namespace: %s", err)
}

h, err := NewHandleAt(ns, nl.FAMILY_V4)
if err != nil {
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,
Forward: IPTuple{
SrcIP: net.IP{234, 234, 234, 234},
DstIP: net.IP{123, 123, 123, 123},
SrcPort: 48385,
DstPort: 53,
Protocol: unix.IPPROTO_TCP,
},
Reverse: IPTuple{
SrcIP: net.IP{123, 123, 123, 123},
DstIP: net.IP{234, 234, 234, 234},
SrcPort: 53,
DstPort: 48385,
Protocol: unix.IPPROTO_TCP,
},
TimeOut: 100,
Mark: 12,
ProtoInfo: &ProtoInfoTCP{
State: nl.TCP_CONNTRACK_ESTABLISHED,
},
}

// Create the entry using the handle
if err := h.ConntrackCreate(ConntrackTable, nl.FAMILY_V4, &flow); err != nil {
t.Fatalf("failed to insert conntrack: %s", err)
}

// Verify it exists
flows, err := h.ConntrackTableList(ConntrackTable, nl.FAMILY_V4)
if err != nil {
t.Fatalf("failed to list conntracks following successful insert: %s", err)
}
filter := ConntrackFilter{
ipNetFilter: map[ConntrackFilterType]*net.IPNet{
ConntrackOrigSrcIP: NewIPNet(flow.Forward.SrcIP),
ConntrackOrigDstIP: NewIPNet(flow.Forward.DstIP),
ConntrackReplySrcIP: NewIPNet(flow.Reverse.SrcIP),
ConntrackReplyDstIP: NewIPNet(flow.Reverse.DstIP),
},
portFilter: map[ConntrackFilterType]uint16{
ConntrackOrigSrcPort: flow.Forward.SrcPort,
ConntrackOrigDstPort: flow.Forward.DstPort,
},
protoFilter: unix.IPPROTO_TCP,
}
var match *ConntrackFlow
for _, f := range flows {
if filter.MatchConntrackFlow(f) {
match = f
break
}
}
if match == nil {
t.Fatalf("didn't find any matching conntrack entries for original flow: %+v\n Filter used: %+v", flow, filter)
}

// Delete using the handler
if err := h.ConntrackDelete(ConntrackTable, InetFamily(nl.FAMILY_V4), &flow); err != nil {
t.Fatalf("failed to delete conntrack via handler: %s", err)
}

// Verify it's gone
flows, err = h.ConntrackTableList(ConntrackTable, nl.FAMILY_V4)
if err != nil {
t.Fatalf("failed to list conntracks following delete: %s", err)
}
for _, f := range flows {
if filter.MatchConntrackFlow(f) {
t.Fatalf("found flow after delete: %+v", f)
}
}
}

// 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.
func TestConntrackDeleteV6(t *testing.T) {
// Print timestamps in UTC
os.Setenv("TZ", "")

requiredModules := []string{"nf_conntrack", "nf_conntrack_netlink"}
k, m, err := KernelVersion()
if err != nil {
t.Fatal(err)
}
// Conntrack l3proto was unified since 4.19
// https://github.com/torvalds/linux/commit/a0ae2562c6c4b2721d9fddba63b7286c13517d9f
if k < 4 || k == 4 && m < 19 {
requiredModules = append(requiredModules, "nf_conntrack_ipv4")
}
// Implicitly skips test if not root:
nsStr, teardown := setUpNamedNetlinkTestWithKModule(t, requiredModules...)
t.Cleanup(teardown)

ns, err := netns.GetFromName(nsStr)
if err != nil {
t.Fatalf("couldn't get handle to generated namespace: %s", err)
}

h, err := NewHandleAt(ns, nl.FAMILY_V6)
if err != nil {
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,
Forward: IPTuple{
SrcIP: net.ParseIP("2001:db8::68"),
DstIP: net.ParseIP("2001:db9::32"),
SrcPort: 48385,
DstPort: 53,
Protocol: unix.IPPROTO_TCP,
},
Reverse: IPTuple{
SrcIP: net.ParseIP("2001:db9::32"),
DstIP: net.ParseIP("2001:db8::68"),
SrcPort: 53,
DstPort: 48385,
Protocol: unix.IPPROTO_TCP,
},
TimeOut: 100,
Mark: 12,
ProtoInfo: &ProtoInfoTCP{
State: nl.TCP_CONNTRACK_ESTABLISHED,
},
}

// Create the entry using the handle
if err := h.ConntrackCreate(ConntrackTable, nl.FAMILY_V6, &flow); err != nil {
t.Fatalf("failed to insert conntrack: %s", err)
}

// Verify it exists
flows, err := h.ConntrackTableList(ConntrackTable, nl.FAMILY_V6)
if err != nil {
t.Fatalf("failed to list conntracks following successful insert: %s", err)
}
filter := ConntrackFilter{
ipNetFilter: map[ConntrackFilterType]*net.IPNet{
ConntrackOrigSrcIP: NewIPNet(flow.Forward.SrcIP),
ConntrackOrigDstIP: NewIPNet(flow.Forward.DstIP),
ConntrackReplySrcIP: NewIPNet(flow.Reverse.SrcIP),
ConntrackReplyDstIP: NewIPNet(flow.Reverse.DstIP),
},
portFilter: map[ConntrackFilterType]uint16{
ConntrackOrigSrcPort: flow.Forward.SrcPort,
ConntrackOrigDstPort: flow.Forward.DstPort,
},
protoFilter: unix.IPPROTO_TCP,
}
var match *ConntrackFlow
for _, f := range flows {
if filter.MatchConntrackFlow(f) {
match = f
break
}
}
if match == nil {
t.Fatalf("didn't find any matching conntrack entries for original flow: %+v\n Filter used: %+v", flow, filter)
}

// Delete using the handler
if err := h.ConntrackDelete(ConntrackTable, InetFamily(nl.FAMILY_V6), &flow); err != nil {
t.Fatalf("failed to delete conntrack via handler: %s", err)
}

// Verify it's gone
flows, err = h.ConntrackTableList(ConntrackTable, nl.FAMILY_V6)
if err != nil {
t.Fatalf("failed to list conntracks following delete: %s", err)
}
for _, f := range flows {
if filter.MatchConntrackFlow(f) {
t.Fatalf("found flow after delete: %+v", f)
}
}
}

// TestConntrackLabels test the conntrack table labels
// Creates some flows and then checks the labels associated
func TestConntrackLabels(t *testing.T) {
Expand Down