OCPBUGS-31495: sanitize pin filenames to enable pinning subinterfaces #686
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@bverschueren: This pull request references Jira Issue OCPBUGS-31495, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@bverschueren: This pull request references Jira Issue OCPBUGS-31495, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@bverschueren: This pull request references Jira Issue OCPBUGS-31495, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds a pin-directory dot-placeholder constant ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (2)
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: 1
🧹 Nitpick comments (1)
pkg/ebpf/ingress_node_firewall_loader_test.go (1)
132-158: Consider adding edge case tests.The test covers the basic functionality well with the bidirectional conversion. Consider adding test cases for edge scenarios to ensure robustness:
- Multiple dots:
"vlan.100.200"→"vlan_dot_100_dot_200"- Empty string:
""- Only dots:
"."- Interface with multiple components:
"bond0.100.200"Example additional test cases:
{ in: "vlan.100.200", expected: "vlan_dot_100_dot_200", }, { in: "vlan_dot_100_dot_200", expected: "vlan.100.200", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/ebpf/ingress_node_firewall_loader.go(4 hunks)pkg/ebpf/ingress_node_firewall_loader_test.go(1 hunks)
🔇 Additional comments (3)
pkg/ebpf/ingress_node_firewall_loader.go (3)
41-41: LGTM! Clear constant for bpffs filename compatibility.The
pinDirDotPlaceholderconstant is well-chosen and serves its purpose of replacing dots in filenames for bpffs compatibility.
323-323: LGTM! Correct sanitization before pinning to bpffs.The sanitization is correctly applied to the pin path before writing to the BPF filesystem, enabling subinterfaces with dots (e.g., VLAN interfaces) to be pinned successfully.
481-481: LGTM! Correct desanitization when loading pinned links.The sanitization is correctly applied to convert the filename back to the original interface name with dots, ensuring the in-memory links map uses the correct interface name as the key.
bverschueren
force-pushed
the
support_subinterface_filename
branch
from
d2d17e1 to
4be6b05
Compare
bverschueren
force-pushed
the
support_subinterface_filename
branch
from
4be6b05 to
f0aac00
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/ebpf/ingress_node_firewall_loader_test.go (1)
132-158: Consider additional edge case coverage.The test covers the basic scenarios well, but consider adding test cases for:
- Multiple dots:
"bond0.100.200"→"bond0_dot_100_dot_200"- Empty string:
""→""- Edge cases with the placeholder at boundaries
These additional cases would ensure the bidirectional behavior is robust across all realistic interface naming scenarios.
Apply this diff to add edge case coverage:
func TestSanitizePinDir(t *testing.T) { tests := []struct { + name string in string expected string }{ { + name: "no dots or placeholder", in: "eth0", expected: "eth0", }, { + name: "dot to placeholder", in: "bond0.100", expected: "bond0_dot_100", }, { + name: "placeholder to dot", in: "eth0_dot_100", expected: "eth0.100", }, + { + name: "multiple dots", + in: "bond0.100.200", + expected: "bond0_dot_100_dot_200", + }, + { + name: "empty string", + in: "", + expected: "", + }, } for _, tc := range tests { - result := sanitizePinDir(tc.in) - if result != tc.expected { - t.Fatalf("Failed to sanitize, expected %s, got %s", - tc.expected, result) - } + t.Run(tc.name, func(t *testing.T) { + result := sanitizePinDir(tc.in) + if result != tc.expected { + t.Fatalf("Failed to sanitize, expected %s, got %s", + tc.expected, result) + } + }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
pkg/ebpf/constants.go(1 hunks)pkg/ebpf/ingress_node_firewall_loader.go(3 hunks)pkg/ebpf/ingress_node_firewall_loader_test.go(1 hunks)pkg/webhook/webhook.go(2 hunks)pkg/webhook/webhook_suite_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/webhook/webhook.go
- pkg/webhook/webhook_suite_test.go
- pkg/ebpf/ingress_node_firewall_loader.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/ebpf/ingress_node_firewall_loader_test.gopkg/ebpf/constants.go
🔇 Additional comments (1)
pkg/ebpf/constants.go (1)
17-19: LGTM! Clean constant definition.The constant is well-named and the value
"_dot_"is a reasonable choice for the placeholder, as it's unlikely to naturally occur in interface names while remaining readable.
bverschueren
force-pushed
the
support_subinterface_filename
branch
from
f0aac00 to
95397af
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/constants/constants.go (1)
17-19: Consider adding a documentation comment for the exported constant.Following Go conventions, exported identifiers should have documentation comments explaining their purpose and usage. This helps maintainers understand why this placeholder exists and how it relates to BPF filesystem constraints.
Example:
const ( + // PinDirDotPlaceholder is used to replace dots in interface names when creating + // BPF pin directory paths, since the BPF filesystem does not allow dots in filenames. PinDirDotPlaceholder = "_dot_" )pkg/ebpf/ingress_node_firewall_loader_test.go (1)
132-158: LGTM! Test coverage is adequate for the core functionality.The test correctly validates the bidirectional conversion behavior of
sanitizePinDir. The table-driven approach is clean and the three test cases cover the essential scenarios: no change needed, dot-to-placeholder conversion, and placeholder-to-dot conversion.Optionally, consider adding test cases for edge scenarios:
{ in: "bond0.100.200", expected: "bond0_dot_100_dot_200", }, { in: "", expected: "", },This would ensure robustness for interfaces with multiple dots or empty strings, though the current coverage is sufficient for the main use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
pkg/constants/constants.go(1 hunks)pkg/ebpf/ingress_node_firewall_loader.go(4 hunks)pkg/ebpf/ingress_node_firewall_loader_test.go(1 hunks)pkg/webhook/webhook.go(2 hunks)pkg/webhook/webhook_suite_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/ebpf/ingress_node_firewall_loader.go
- pkg/webhook/webhook_suite_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/constants/constants.gopkg/ebpf/ingress_node_firewall_loader_test.gopkg/webhook/webhook.go
🔇 Additional comments (2)
pkg/webhook/webhook.go (2)
11-11: LGTM!The import is correctly added to support the new validation logic.
108-112: LGTM! Essential validation to prevent naming conflicts.This validation correctly prevents users from specifying interface names containing the reserved placeholder string, which would otherwise create ambiguity in the bidirectional sanitization logic. The error message is clear and the check is placed appropriately alongside other interface name validations.
|
/LGTM |
|
@msherif1234: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
msherif1234
left a comment
msherif1234
left a comment
There was a problem hiding this comment.
Thanks for the fix!!
|
@msherif1234: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
bverschueren
force-pushed
the
support_subinterface_filename
branch
from
95397af to
adba87e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/webhook/webhook_suite_test.go (1)
34-34: Import appears unused in new test cases.The
admissionv1beta1import is marked as a new addition but doesn't appear to be used by the new test cases at lines 183-193. It's already used at line 104, so this may be a reorganization rather than a new requirement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
pkg/constants/constants.go(1 hunks)pkg/ebpf/ingress_node_firewall_loader.go(4 hunks)pkg/ebpf/ingress_node_firewall_loader_test.go(1 hunks)pkg/webhook/webhook.go(2 hunks)pkg/webhook/webhook_suite_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/ebpf/ingress_node_firewall_loader_test.go
- pkg/constants/constants.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/webhook/webhook.gopkg/ebpf/ingress_node_firewall_loader.gopkg/webhook/webhook_suite_test.go
🧬 Code graph analysis (1)
pkg/webhook/webhook.go (1)
pkg/constants/constants.go (1)
PinDirDotPlaceholder(18-18)
🔇 Additional comments (3)
pkg/webhook/webhook.go (1)
108-112: LGTM! Validation correctly prevents edge case.The validation logic properly rejects interface names containing the placeholder, preventing the bidirectional conversion issue in
sanitizePinDirwhere an interface naturally named with "dot" could be misinterpreted.pkg/ebpf/ingress_node_firewall_loader.go (1)
734-744: Bidirectional sanitization works correctly with webhook validation.The bidirectional conversion (dots ↔ placeholder) correctly handles the common case: interface names with dots are sanitized for pinning and restored when loading. The webhook validation prevents the edge case where an interface naturally named with the placeholder substring could be misinterpreted.
For future consideration: explicit
encodePinPath()anddecodePinPath()functions would be more explicit than the heuristic, but the current approach is acceptable given the webhook safeguard.pkg/webhook/webhook_suite_test.go (1)
183-193: LGTM! Test coverage validates both dot handling and placeholder rejection.The two tests correctly verify:
- Interface names with dots (e.g., "eth0.100") are accepted, enabling subinterface support
- Interface names containing the placeholder string are rejected, preventing the edge case in sanitization logic
The BPF fs does not allow dots (".") in filenames [1], which prevents
subinterfaces to be pinned correctly. By translating these dots before
writing to bpffs this allows pinning subinterfaces (e.g. vlans).
[1] https://github.com/torvalds/linux/blob/6146a0f1dfae5d37442a9ddcba012add260bceb0/kernel/bpf/inode.c#L371-L381
bverschueren
force-pushed
the
support_subinterface_filename
branch
from
adba87e to
0136fa7
Compare
|
/retest |
|
@bverschueren: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bverschueren, jcaamano, msherif1234 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
4 participants
The BPF fs does not allow dots (".") in filenames, which prevents subinterfaces to be pinned correctly. By translating these dots before writing to bpffs this allows pinning subinterfaces (e.g. vlans).
- What this PR does and why is it needed
Without the change, using a subinterface (with a dot in its generated pinDir filename), an error is logged:
- How to verify it
Create an
ingressnodefirewalltargeting a vlan interface:Delete the rule again and see the link being upinned:
- Description for the changelog
Allow subinterfaces to be pinned by converting filename dots with a placeholder for writing to the
BPFfilesystem.