OCPBUGS-76699: ebpf: consolidate packet event logs into single line#706
OCPBUGS-76699: ebpf: consolidate packet event logs into single line#706rameshsahoo11 wants to merge 2 commits intoopenshift:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughConsolidated ingress node firewall event logging: multiple per-field syslog calls replaced by constructing a single composite log message (ruleId, action, pktLen, interface, and one mutually-exclusive protocol detail branch for IPv4/IPv6 + transport/ICMP) and emitting it once. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @rameshsahoo111. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
Internal Jira - https://redhat.atlassian.net/browse/OCPBUGS-76699 |
|
/cc |
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
|
@Meina-rh thank you for your review. With the current code (using else if): From a networking perspective, a single packet can ONLY be either IPv4 OR IPv6, never both. This is fundamental to how IP works: So the current code with else if correctly handles both: In a dual-stack cluster: I confirmed that IPV6 logging is also correctly logged; Requesting you to further review and my original change and update. |
|
@rameshsahoo111 thanks for your explaining, I forgot it's from packet. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
Unit test is failing because ./testbin/setup-envtest.sh is failing to download kubebuilder-tools. Requesting someone to fix CI failure. |
|
/retest-required |
|
Verified with all test cases and the below are the result. Requesting someone to fix the CI failure. IngressNodeFirewall RulesapiVersion: ingressnodefirewall.openshift.io/v1alpha1
kind: IngressNodeFirewall
metadata:
name: ingressnodefirewall
spec:
interfaces:
- breth0
nodeSelector:
matchLabels:
kubernetes.io/hostname: ovn-worker
ingress:
- sourceCIDRs:
- 0.0.0.0/0
- ::/0
rules:
- order: 10
protocolConfig:
protocol: TCP
tcp:
ports: "5000-5001"
action: Deny
- order: 11
protocolConfig:
protocol: UDP
udp:
ports: "5000-5001"
action: Deny
- order: 12
protocolConfig:
protocol: ICMP
icmp:
icmpType: 8 #ICMP Echo request
action: Deny
- order: 13
protocolConfig:
protocol: ICMPv6
icmpv6:
icmpType: 128 #ICMPV6 Echo request
action: Deny
- order: 14
protocolConfig:
protocol: SCTP
sctp:
ports: "6000-6001"
action: Deny
Test Result[SCTP TEST]
Server Listen;
socat -v SCTP6-LISTEN:6000,fork,ipv6only=0 -
SCTP Client connect;
echo "Hello Ramesh"|socat - SCTP:10.89.0.16:6000
echo "Hello Ramesh"|socat - SCTP:[fc00:f853:ccd:e793::10]:6000
[TCP/UDP TEST]
nc -l 5000
curl --retry 0 --fail --max-time 1 -v telnet://10.89.0.16:5000 --connect-timeout 1
curl --retry 0 --fail --max-time 1 -v telnet://[fc00:f853:ccd:e793::10]:5000 --connect-timeout 1
nc -lu 5000
nc -zu 10.89.0.16 5000
nc -6 -zu fc00:f853:ccd:e793::10 5000
[ICMP Test]
ping -c1 10.89.0.16
ping6 -c1 fc00:f853:ccd:e793::10
# oc logs -c events ingress-node-firewall-daemon-h44gk
[TCP]
2026-04-03 10:53:58 +0000 UTC ovn-worker ruleId 10 action Drop len 74 if breth0 | ipv4 src 10.89.0.1 dst 10.89.0.16 | tcp srcPort 58444 dstPort 5000
2026-04-03 10:55:17 +0000 UTC ovn-worker ruleId 10 action Drop len 94 if breth0 | ipv6 src fc00:f853:ccd:e793::1 dst fc00:f853:ccd:e793::10 | tcp srcPort 40772 dstPort 5000
[UDP]
2026-04-03 10:56:18 +0000 UTC ovn-worker ruleId 11 action Drop len 43 if breth0 | ipv4 src 10.89.0.1 dst 10.89.0.16 | udp srcPort 52237 dstPort 5000
2026-04-03 10:56:44 +0000 UTC ovn-worker ruleId 11 action Drop len 63 if breth0 | ipv6 src fc00:f853:ccd:e793::1 dst fc00:f853:ccd:e793::10 | udp srcPort 50747 dstPort 5000
[SCTP]
2026-04-03 10:59:27 +0000 UTC ovn-worker ruleId 14 action Drop len 98 if breth0 | ipv4 src 10.89.0.1 dst 10.89.0.16 | sctp srcPort 58063 dstPort 6000
2026-04-03 10:59:30 +0000 UTC ovn-worker ruleId 14 action Drop len 98 if breth0 | ipv4 src 10.89.0.1 dst 10.89.0.16 | sctp srcPort 58063 dstPort 6000
2026-04-03 10:59:36 +0000 UTC ovn-worker ruleId 14 action Drop len 98 if breth0 | ipv4 src 10.89.0.1 dst 10.89.0.16 | sctp srcPort 58063 dstPort 6000
2026-04-03 11:00:20 +0000 UTC ovn-worker ruleId 14 action Drop len 102 if breth0 | ipv6 src fc00:f853:ccd:e793::1 dst fc00:f853:ccd:e793::10 | sctp srcPort 36912 dstPort 6000
2026-04-03 11:00:23 +0000 UTC ovn-worker ruleId 14 action Drop len 102 if breth0 | ipv6 src fc00:f853:ccd:e793::1 dst fc00:f853:ccd:e793::10 | sctp srcPort 36912 dstPort 6000
2026-04-03 11:00:30 +0000 UTC ovn-worker ruleId 14 action Drop len 102 if breth0 | ipv6 src fc00:f853:ccd:e793::1 dst fc00:f853:ccd:e793::10 | sctp srcPort 36912 dstPort 6000
[ICMP]
2026-04-03 11:00:58 +0000 UTC ovn-worker ruleId 12 action Drop len 98 if breth0 | ipv4 src 10.89.0.1 dst 10.89.0.16 | icmpv4 type 8 code 0
2026-04-03 11:01:20 +0000 UTC ovn-worker ruleId 13 action Drop len 118 if breth0 | ipv6 src fc00:f853:ccd:e793::1 dst fc00:f853:ccd:e793::10 | icmpv6 type 128 code 0 |
rameshsahoo11
changed the title
rameshsahoo11
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@rameshsahoo111: This pull request references Jira Issue OCPBUGS-76699, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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 |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@rameshsahoo111: This pull request references Jira Issue OCPBUGS-76699, 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: DetailsIn 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. |
|
/retest-required |
|
/testwith openshift/ingress-node-firewall/master/unit-test #705 |
|
/testwith openshift/ingress-node-firewall/master/ingress-node-firewall-e2e-metal-ipi #705 |
|
@rameshsahoo11 we need to wait #705 PR merged which will resolve unit-test CI failures.
|
|
@Meina-rh thanks , I will fix e2e |
openshift-ci-robot
added
jira/invalid-bug
|
@rameshsahoo11: This pull request references Jira Issue OCPBUGS-76699, which is invalid:
Comment DetailsIn 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/events/events.go (1)
62-97: Optional: switch to raw string literals for readability.The double-escaped patterns (
\\s,\\|,\\w,\\d) are hard to eyeball against the producer format. Go raw string literals remove one level of escaping and make the pipe delimiters explicit.♻️ Proposed refactor
- transportEventRE := "ruleId\\s([0-9]+)\\saction\\s(?P<action>\\w+).*if\\s(?P<inf>\\w+)\\s\\|\\s(?:ipv4|ipv6)\\ssrc\\s(?P<srcaddr>[0-9.:a-z]+)\\sdst\\s(?P<dstaddr>[0-9.:a-z]+)\\s\\|\\s(?P<proto>tcp|udp|sctp)\\ssrcPort\\s\\d+\\sdstPort\\s(?P<dstport>\\d+)" + transportEventRE := `ruleId\s([0-9]+)\saction\s(?P<action>\w+).*if\s(?P<inf>\w+)\s\|\s(?:ipv4|ipv6)\ssrc\s(?P<srcaddr>[0-9.:a-z]+)\sdst\s(?P<dstaddr>[0-9.:a-z]+)\s\|\s(?P<proto>tcp|udp|sctp)\ssrcPort\s\d+\sdstPort\s(?P<dstport>\d+)` ... - icmpEventRE := "ruleId\\s([0-9]+)\\saction\\s(?P<action>\\w+).*if\\s(?P<inf>\\w+)\\s\\|\\s(?:ipv4|ipv6)\\ssrc\\s(?P<srcaddr>[0-9.:a-z]+)\\sdst\\s(?P<dstaddr>[0-9.:a-z]+)\\s\\|\\s(?P<proto>icmpv4|icmpv6)\\stype\\s(?P<type>\\d+)\\scode\\s(?P<code>\\d+)" + icmpEventRE := `ruleId\s([0-9]+)\saction\s(?P<action>\w+).*if\s(?P<inf>\w+)\s\|\s(?:ipv4|ipv6)\ssrc\s(?P<srcaddr>[0-9.:a-z]+)\sdst\s(?P<dstaddr>[0-9.:a-z]+)\s\|\s(?P<proto>icmpv4|icmpv6)\stype\s(?P<type>\d+)\scode\s(?P<code>\d+)`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/events/events.go` around lines 62 - 97, The transportEventRE and icmpEventRE string literals are hard to read due to double-escaping; update both (the variables transportEventRE and icmpEventRE) to use Go raw string literals (back-quoted) so `\s`, `\|`, `\w`, `\d` appear as single-escaped sequences and the pipe delimiters are explicit, then ensure regexp.MustCompile calls (transportEventPattern) still compile the same patterns and run tests to confirm behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/events/events.go`:
- Around line 62-97: The transportEventRE and icmpEventRE string literals are
hard to read due to double-escaping; update both (the variables transportEventRE
and icmpEventRE) to use Go raw string literals (back-quoted) so `\s`, `\|`,
`\w`, `\d` appear as single-escaped sequences and the pipe delimiters are
explicit, then ensure regexp.MustCompile calls (transportEventPattern) still
compile the same patterns and run tests to confirm behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 90ba4621-725a-4dad-9f46-b20bf996eee7
📒 Files selected for processing (1)
test/e2e/events/events.go
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rameshsahoo11 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/jira-refresh |
danwinship
left a comment
danwinship
left a comment
There was a problem hiding this comment.
Basically looks good, but let's wait for the unit tests to get fixed. (Someone is going to redo #705.)
Do we know whether the change in logging format is going to affect any other components?
| log.Printf("syslog event logging failed %q", err) | ||
| } | ||
| // Build a complete log message with all packet details | ||
| var logMsg string |
There was a problem hiding this comment.
use strings.Builder and fmt.Fprintf
| // Match for each drop. | ||
| // If any change to the event output in pkg/ebpf/ingress_node_firewall_events.go, an update is required here. | ||
| transportEventRE := "ruleId\\s([0-9]+)\\saction\\s(?P<action>\\w+).*if\\s(?P<inf>\\w+)\n.*\\ssrc\\saddr\\s(?P<srcaddr>[0-9.:a-z]+)\\sdst\\saddr\\s(?P<dstaddr>[0-9.:a-z]+)\n.*(?P<proto>tcp|udp|sctp)\\ssrcPort\\s\\d+\\sdstPort\\s(?P<dstport>\\d+)" | ||
| transportEventRE := "ruleId\\s([0-9]+)\\saction\\s(?P<action>\\w+).*if\\s(?P<inf>\\w+)\\s\\|\\s(?:ipv4|ipv6)\\ssrc\\s(?P<srcaddr>[0-9.:a-z]+)\\sdst\\s(?P<dstaddr>[0-9.:a-z]+)\\s\\|\\s(?P<proto>tcp|udp|sctp)\\ssrcPort\\s\\d+\\sdstPort\\s(?P<dstport>\\d+)" |
There was a problem hiding this comment.
These ought to use backtick strings so you don't need double backslashes...
There was a problem hiding this comment.
Thank you for the feedback. I’ve incorporated your suggestions into the latest commit. Please let me know if you have any further thoughts.
rameshsahoo11
left a comment
rameshsahoo11
left a comment
There was a problem hiding this comment.
Thank you for the feedback. I’ve incorporated your suggestions into the latest commit. Please let me know if you have any further thoughts.
|
please squash the fixes in with the original commits. Put the strings.Builder fix in the first commit and the backticks fix in the second commit |
Previously, firewall packet events were logged as three separate syslog
messages (rule info, IP addresses, protocol details), making log parsing
and analysis more difficult. This change consolidates all packet event
information into a single log line for better observability.
Changes:
- Merge three separate eventsLogger.Info() calls into one
- Use strings.Builder and fmt.Fprintf for efficient log construction
- Use pipe separators (|) to delimit log sections visually
Example output format:
Before (3 lines):
ruleId 10 action Drop len 74 if br1
ipv4 src addr 192.xx.xx.149 dst addr 192.xx.xx.56
tcp srcPort 56354 dstPort 22
After (1 line):
ruleId 10 action Drop len 74 if br1 | ipv4 src 192.xx.xx.149 dst 192.xx.xx.56 | tcp srcPort 56354 dstPort 22
This improves log grep-ability, parsing, and integration with log
aggregation tools where single-line entries are preferred.
Signed-off-by: Ramesh Sahoo <rsahoo@redhat.com>
Update event parsing regex patterns in test/e2e/events/events.go to
match the new single-line log format with pipe delimiters.
Changes:
- Replace newline separators (\n) with pipe delimiters (\s\|\s)
- Remove 'addr' keyword from IP address fields
- Add IP version prefix matching (ipv4|ipv6)
- Use raw string literals (backticks) to avoid escaping backslashes
Old format (multi-line):
ruleId 10 action Drop len 98 if eth0
ipv4 src addr 172.20.0.1 dst addr 172.20.0.4
tcp srcPort 12345 dstPort 8080
New format (single line):
ruleId 10 action Drop len 98 if eth0 | ipv4 src 172.20.0.1 dst 172.20.0.4 | tcp srcPort 12345 dstPort 8080
rameshsahoo11
force-pushed
the
consolidate-event-logs
branch
from
afa2cce to
89ac802
Compare
|
@danwinship thank you for your helpful suggestions. I’ve incorporated them into the latest commit. Please take a look and let me know your feedback. |
|
@rameshsahoo11: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
4 participants
Previously, firewall packet events were logged as three separate syslog messages (rule info, IP addresses, protocol details), making log parsing and analysis more difficult. This change consolidates all packet event information into a single log line for better observability.
Changes:
Example output format:
Before (3 lines):
After (1 line):
2026-02-06 09:04:59 +0000 UTC ruleId 10 action Drop len 74 if br1 | ipv4 src 192.xx.xx.149 dst 192.xx.xx.56 | tcp srcPort 56354 dstPort 22This improves log grep-ability, parsing, and integration with log aggregation tools where single-line entries are preferred.
Test result from ovnkind cluster after appling the fix
Summary by CodeRabbit