conntrack: encode conntrack labels if set #1135
Conversation
WalkthroughAdds a Labels field to ConntrackFlow and emits a CTA_LABELS netlink attribute from ConntrackFlow.toNlData when Labels is non-nil; requires Labels length == 16 (error otherwise). Tests and kernel hook setup were extended to set and verify per-conntrack labels. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test code
participant CF as ConntrackFlow
participant ToNL as toNlData()
participant NL as Netlink payload
Test->>CF: create ConntrackFlow (Labels maybe set)
CF->>ToNL: toNlData()
ToNL->>ToNL: emit CTA_* fields (mark, timeout, ...)
alt Labels non-nil
ToNL->>ToNL: validate len(Labels) == 16
opt len==16
ToNL->>NL: append CTA_LABELS (raw 16 bytes)
end
opt len!=16
ToNL-->>Test: return error
end
else Labels nil
ToNL->>NL: no CTA_LABELS appended
end
ToNL-->>Test: return payload or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)conntrack_test.go (5)
🔇 Additional comments (6)
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 |
|
ping @aboch , appreciate if you can prioritize this review , it solves a gap in network policies in kubernetes kubernetes-sigs/kube-network-policies#268 |
![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 (2)
conntrack_test.go (2)
76-79: Add cleanup for connlabel rules and make rule insertion more resilient.
- Clean up the inserted connlabel rules to keep the test netns tidy.
- Optionally, fall back to nft “ct label set 1” if xt_connlabel isn’t available.
Add to the cleanup closure:
return func() { if addedInput { ipt(false, "-D", "INPUT", "-m", "conntrack", "--ctstate", "NEW,ESTABLISHED", "-j", "ACCEPT") } if addedOutput { ipt(false, "-D", "OUTPUT", "-m", "conntrack", "--ctstate", "ESTABLISHED", "-j", "ACCEPT") } + // Best-effort removal of label rules. + ipt(false, "-D", "INPUT", "-m", "connlabel", "--set", "--label", "1") + ipt(false, "-D", "OUTPUT", "-m", "connlabel", "--set", "--label", "1") }Also applies to: 83-86
1607-1615: Clarify clearing semantics in comment.You send a 16-byte all-zero slice to clear, not an empty slice.
Apply this diff:
- // Change the conntrack and update the kernel entry. + // Change the conntrack and update the kernel entry. flow.Mark = 10 - flow.Labels = make([]byte, 16) // zero labels + flow.Labels = make([]byte, 16) // 16-byte all-zero slice clears labels @@ - // To clear the labels we send an empty slice, but when reading back - // from the kernel we get a nil slice. + // After clearing, reading back yields a nil slice from the kernel.Also applies to: 1636-1639
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
conntrack_linux.go(2 hunks)conntrack_test.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
conntrack_test.go (3)
netlink_test.go (1)
KernelVersion(290-308)conntrack_linux.go (9)
ConntrackFlow(298-309)IPTuple(256-264)ProtoInfo(219-221)ConntrackUpdate(70-72)ConntrackTable(22-22)ConntrackCreate(64-66)ConntrackTableList(51-53)ConntrackFilter(750-756)ConntrackFilterType(727-727)netlink.go (1)
NewIPNet(35-40)
conntrack_linux.go (2)
nl/nl_linux.go (1)
NewRtAttr(402-410)nl/conntrack_linux.go (1)
CTA_LABELS(126-126)
🔇 Additional comments (5)
conntrack_linux.go (1)
369-371: Doc addition looks good.CTA_LABELS placement documented clearly.
conntrack_test.go (4)
7-7: Import is correct.bytes is needed for bytes.Equal in label comparisons.
1512-1590: End-to-end label create/read assertions look solid.Good coverage of label propagation through create/list paths.
1665-1666: Round-trip NL data now includes Labels – LGTM.The added Labels fixtures match parser expectations.
Also applies to: 1688-1689
1746-1749: Label comparison via bytes.Equal – LGTM.Handles nil vs empty equivalence correctly for test purposes.
aojea
force-pushed
the
conntrack_labels
branch
2 times, most recently
from
7804471 to
5dad30c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
conntrack_linux.go (1)
397-404: CTA_LABELS emission semantics are correct.Nil skips sending; exact 16 bytes enforced; zeroed 16-byte slice clears. This matches kernel expectations. Good change.
conntrack_test.go (1)
108-112: Fix nft rule: remove stray brace and add input-chain rule.Current command is invalid due to trailing "}" and only sets label on output; add input for parity with iptables.
- // Add a rule to set conntrack label to allocate the label space - // https://lore.kernel.org/netfilter-devel/[email protected]/ - _ = exec.Command("nft", "add", "rule", "inet", "ct_test", "output", - "ct", "label", "set", "1", "}").Run() + // Add rules to set a conntrack label and allocate label space + // https://lore.kernel.org/netfilter-devel/[email protected]/ + _ = exec.Command("nft", "add", "rule", "inet", "ct_test", "output", + "ct", "label", "set", "1").Run() + _ = exec.Command("nft", "add", "rule", "inet", "ct_test", "input", + "ct", "label", "set", "1").Run()
🧹 Nitpick comments (2)
conntrack_linux.go (1)
334-420: Add a negative test for invalid label length.To lock the contract, add a test that sets
flow.Labelsto a non-16 length (e.g., 8 or 17) and assertstoNlData()returns an error, and thatConntrackCreate/Updatepropagate it. Keeps behavior from regressing.conntrack_test.go (1)
71-87: Also delete iptables connlabel rules in cleanup for symmetry.We insert connlabel rules but never remove them. Track and delete them to keep the test namespace tidy.
- var addedInput, addedOutput bool + var addedInput, addedOutput bool + var addedLabelInput, addedLabelOutput bool if ipt(false, "-C", "INPUT", "-m", "conntrack", "--ctstate", "NEW,ESTABLISHED", "-j", "ACCEPT") != nil { ipt(true, "-I", "INPUT", "-m", "conntrack", "--ctstate", "NEW,ESTABLISHED", "-j", "ACCEPT") // Add a rule to set conntrack label to allocate the label space // https://lore.kernel.org/netfilter-devel/[email protected]/ - ipt(true, "-I", "INPUT", "-m", "connlabel", "--set", "--label", "1") + if ipt(true, "-I", "INPUT", "-m", "connlabel", "--set", "--label", "1") == nil { + addedLabelInput = true + } addedInput = true } if ipt(false, "-C", "OUTPUT", "-m", "conntrack", "--ctstate", "ESTABLISHED", "-j", "ACCEPT") != nil { ipt(true, "-I", "OUTPUT", "-m", "conntrack", "--ctstate", "ESTABLISHED", "-j", "ACCEPT") // Add a rule to set conntrack label to allocate the label space // https://lore.kernel.org/netfilter-devel/[email protected]/ - ipt(true, "-I", "OUTPUT", "-m", "connlabel", "--set", "--label", "1") + if ipt(true, "-I", "OUTPUT", "-m", "connlabel", "--set", "--label", "1") == nil { + addedLabelOutput = true + } addedOutput = true } return func() { if addedInput { ipt(false, "-D", "INPUT", "-m", "conntrack", "--ctstate", "NEW,ESTABLISHED", "-j", "ACCEPT") + if addedLabelInput { + ipt(false, "-D", "INPUT", "-m", "connlabel", "--set", "--label", "1") + } } if addedOutput { ipt(false, "-D", "OUTPUT", "-m", "conntrack", "--ctstate", "ESTABLISHED", "-j", "ACCEPT") + if addedLabelOutput { + ipt(false, "-D", "OUTPUT", "-m", "connlabel", "--set", "--label", "1") + } } }Also applies to: 88-96
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
conntrack_linux.go(2 hunks)conntrack_test.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
conntrack_test.go (5)
netlink_test.go (1)
KernelVersion(290-308)conntrack_linux.go (15)
ConntrackFlow(298-309)IPTuple(256-264)ProtoInfo(219-221)ConntrackUpdate(70-72)ConntrackTable(22-22)ConntrackCreate(64-66)ConntrackTableList(51-53)ConntrackFilter(754-760)ConntrackFilterType(731-731)ConntrackOrigSrcIP(734-734)ConntrackOrigDstIP(735-735)ConntrackReplySrcIP(736-736)ConntrackReplyDstIP(737-737)ConntrackOrigSrcPort(739-739)ConntrackOrigDstPort(740-740)nl/nl_linux.go (1)
FAMILY_V4(25-25)nl/conntrack_linux.go (2)
TCP_CONNTRACK_SYN_SENT2(45-45)TCP_CONNTRACK_ESTABLISHED(38-38)netlink.go (1)
NewIPNet(35-40)
conntrack_linux.go (2)
nl/nl_linux.go (1)
NewRtAttr(402-410)nl/conntrack_linux.go (1)
CTA_LABELS(126-126)
🔇 Additional comments (3)
conntrack_test.go (3)
1746-1749: Good choice using bytes.Equal for Labels comparison.Handles nil vs empty slices correctly; avoids false negatives when clearing labels.
1648-1693: Including Labels in FlowToNlData round-trip tests is valuable.Ensures CTA_LABELS is serialized and parsed consistently for both v4 and v6.
1512-1643: TestConntrackLabels exercises set/clear semantics well.Create with labels, update with zeroed 16-byte slice to clear, then compare against nil on readback. Matches kernel behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
conntrack_test.go (1)
76-95: Cleanup function doesn't remove connlabel rules.The iptables connlabel rules added on lines 78 and 85 are not removed in the cleanup function (lines 88-94). While test namespaces are typically destroyed, explicitly cleaning up all added rules would be more consistent.
Consider updating the cleanup function:
return func() { if addedInput { ipt(false, "-D", "INPUT", "-m", "conntrack", "--ctstate", "NEW,ESTABLISHED", "-j", "ACCEPT") + ipt(false, "-D", "INPUT", "-m", "connlabel", "--set", "--label", "1") } if addedOutput { ipt(false, "-D", "OUTPUT", "-m", "conntrack", "--ctstate", "ESTABLISHED", "-j", "ACCEPT") + ipt(false, "-D", "OUTPUT", "-m", "connlabel", "--set", "--label", "1") } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
conntrack_linux.go(2 hunks)conntrack_test.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- conntrack_linux.go
🧰 Additional context used
🧬 Code graph analysis (1)
conntrack_test.go (5)
netlink_test.go (1)
KernelVersion(290-308)conntrack_linux.go (11)
ConntrackFlow(298-309)IPTuple(256-264)ProtoInfo(219-221)ProtoInfoTCP(225-227)ProtoInfoTCP(230-230)ConntrackFilter(754-760)ConntrackFilterType(731-731)ConntrackOrigSrcIP(734-734)ConntrackOrigDstIP(735-735)ConntrackReplySrcIP(736-736)ConntrackReplyDstIP(737-737)nl/nl_linux.go (1)
FAMILY_V4(25-25)nl/conntrack_linux.go (2)
TCP_CONNTRACK_SYN_SENT2(45-45)TCP_CONNTRACK_ESTABLISHED(38-38)netlink.go (1)
NewIPNet(35-40)
🔇 Additional comments (4)
conntrack_test.go (4)
7-7: LGTM: Import added for label comparison.The
bytesimport is correctly added to support thebytes.Equalcomparison for conntrack labels incheckFlowsEqual.
108-113: LGTM: nft rules correctly set labels for both chains.The nft rules now correctly set conntrack labels for both input and output chains without syntax errors. The cleanup properly removes these rules by deleting the entire table.
1537-1645: LGTM: Test correctly exercises label lifecycle.The test appropriately verifies creating conntrack entries with labels, updating labels to clear them (using an all-zero slice), and confirming the cleared state (which reads back as nil). The documented semantic difference between sending an empty slice and receiving nil is intentional per the PR objectives.
1747-1751: LGTM: Labels comparison correctly implemented.Using
bytes.Equalis the appropriate way to compare byte slices and handles nil values correctly for the label clearing semantics.
conntrack_test.go
Outdated
| // TestConntrackLabels test the conntrack table labels | ||
| // Creates some flows and then checks the labels associated | ||
| func TestConntrackLabels(t *testing.T) { | ||
| skipUnlessRoot(t) | ||
| t.Cleanup(setUpNetlinkTestWithKModule(t, "nf_conntrack")) | ||
| t.Cleanup(setUpNetlinkTestWithKModule(t, "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 { | ||
| t.Cleanup(setUpNetlinkTestWithKModule(t, "nf_conntrack_ipv4")) | ||
| } | ||
| t.Cleanup(setUpNetlinkTestWithKModule(t, "nf_conntrack")) |
There was a problem hiding this comment.
Remove duplicate module setup.
Line 1529 duplicates the nf_conntrack module setup from line 1518, which is unnecessary.
Apply this diff:
skipUnlessRoot(t)
t.Cleanup(setUpNetlinkTestWithKModule(t, "nf_conntrack"))
t.Cleanup(setUpNetlinkTestWithKModule(t, "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 {
t.Cleanup(setUpNetlinkTestWithKModule(t, "nf_conntrack_ipv4"))
}
- t.Cleanup(setUpNetlinkTestWithKModule(t, "nf_conntrack"))
// Creates a new namespace and bring up the loopback interface📝 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.
| // TestConntrackLabels test the conntrack table labels | |
| // Creates some flows and then checks the labels associated | |
| func TestConntrackLabels(t *testing.T) { | |
| skipUnlessRoot(t) | |
| t.Cleanup(setUpNetlinkTestWithKModule(t, "nf_conntrack")) | |
| t.Cleanup(setUpNetlinkTestWithKModule(t, "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 { | |
| t.Cleanup(setUpNetlinkTestWithKModule(t, "nf_conntrack_ipv4")) | |
| } | |
| t.Cleanup(setUpNetlinkTestWithKModule(t, "nf_conntrack")) | |
| // TestConntrackLabels test the conntrack table labels | |
| // Creates some flows and then checks the labels associated | |
| func TestConntrackLabels(t *testing.T) { | |
| skipUnlessRoot(t) | |
| t.Cleanup(setUpNetlinkTestWithKModule(t, "nf_conntrack")) | |
| t.Cleanup(setUpNetlinkTestWithKModule(t, "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 { | |
| t.Cleanup(setUpNetlinkTestWithKModule(t, "nf_conntrack_ipv4")) | |
| } |
🤖 Prompt for AI Agents
In conntrack_test.go around lines 1514 to 1529, the final
t.Cleanup(setUpNetlinkTestWithKModule(t, "nf_conntrack")) is a duplicate of the
earlier module setup; remove that duplicate line (the one at line 1529) so each
kernel module is registered for cleanup only once.
The logic for updating conntrack labels was missing. The conntrack labels is an slice of bytes, if is not nil we send its current value. The all zeros slice has a special meaning , that wipes out the existing labels. There is also some unexpected behavior, the conntrack table does not reserve space for the labels if there is no label set in any rule, causing the netlink calls to fail with ENOSPC Signed-off-by: Antonio Ojea <[email protected]>
aojea
force-pushed
the
conntrack_labels
branch
from
5dad30c to
628845b
Compare
|
LGTM |
2 participants
conntrack: update conntrack labels
The logic for updating conntrack labels was missing.
The conntrack labels is an slice of bytes, if is not nil we send
its current value.
The all zeros slice has a special meaning , that wipes out the existing
labels.
There is also some unexpected behavior, the conntrack table does not
reserve space for the labels if there is no label set in any rule,
causing the netlink calls to fail with ENOSPC
Signed-off-by: Antonio Ojea [email protected]
Summary by CodeRabbit
New Features
Tests