NETOBSERV-2273: support multiple DNS tracking ports via comma-separated list#994
NETOBSERV-2273: support multiple DNS tracking ports via comma-separated list#994OlivierCazade wants to merge 1 commit into
Conversation
|
@OlivierCazade: This pull request references NETOBSERV-2273 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDNS tracking now accepts up to 8 configured ports instead of one. The BPF contract stores a port array plus count, the tracer parses comma-separated config into those values, and tests plus docs cover the new format. ChangesDNS Multi-Port Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 2 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bpf/configs.h (1)
1-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the required GPLv2 header for BPF code.
This changed
bpf/header is missing the required GPL v2 license header.
As per coding guidelines, "eBPF code in bpf/ directory must use GPL v2 license headers".Suggested patch
+// SPDX-License-Identifier: GPL-2.0 + `#ifndef` __CONFIGS_H__ `#define` __CONFIGS_H__🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bpf/configs.h` around lines 1 - 5, The bpf/configs.h file is missing the required GPL v2 license header that must appear at the top of all eBPF code files. Add the GPL v2 license header comment block at the very beginning of the file, before the `#ifndef __CONFIGS_H__` preprocessor guard. This header should include the standard GPL v2 license text and copyright information to comply with the coding guidelines for BPF code.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/tracer/tracer.go`:
- Around line 2147-2151: The condition checking `i >= maxDNSPorts` uses the raw
loop index which counts all ports including invalid or empty entries, causing
the loop to stop before accepting the maximum of 8 valid ports. Instead of
checking the loop index, introduce a separate counter variable to track only the
number of accepted/valid ports and increment it only when a port is successfully
processed and added. Replace the condition in the loop with a check against this
accepted port counter instead of the loop index `i`.
- Around line 2141-2166: Create unit tests for the DNS multi-port parsing logic
in the configureFlowSpecVariables function to verify correct behavior when
handling valid ports, invalid ports, empty entries, and the maxDNSPorts
truncation limit. Write the tests using Go's standard testing package with test
functions accepting *testing.T parameter to ensure complete coverage of the port
parsing and validation code paths that split and parse the DNS tracking ports
configuration.
---
Outside diff comments:
In `@bpf/configs.h`:
- Around line 1-5: The bpf/configs.h file is missing the required GPL v2 license
header that must appear at the top of all eBPF code files. Add the GPL v2
license header comment block at the very beginning of the file, before the
`#ifndef __CONFIGS_H__` preprocessor guard. This header should include the
standard GPL v2 license text and copyright information to comply with the coding
guidelines for BPF code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2abf4a0c-a76e-4782-a9f7-ffb923b1c42f
📒 Files selected for processing (5)
bpf/configs.hbpf/dns_tracker.hdocs/config.mdpkg/config/config.gopkg/tracer/tracer.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bpf/configs.h (1)
1-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the required GPL v2 license header to this eBPF header.
This file is under
bpf/and currently has no GPL v2 header, which violates repo policy forbpf/*.{c,h}files.As per coding guidelines, "eBPF code in bpf/ directory must use GPL v2 license headers".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bpf/configs.h` around lines 1 - 3, Add a GPL v2 license header comment block at the very beginning of the configs.h file, before the existing header guards (__CONFIGS_H__). The license header should be formatted as a multi-line C comment containing the GPL v2 license text that complies with the repository's eBPF coding guidelines for files in the bpf/ directory.Source: Coding guidelines
🧹 Nitpick comments (1)
pkg/tracer/dns_ports_test.go (1)
155-175: 🏗️ Heavy liftAvoid mirroring production parsing logic inside the test body.
The test reimplements
configureFlowSpecVariablesparsing path, so future logic regressions can be duplicated in test and still pass. Prefer testing the production path directly (or extracting a shared parser helper used by both prod and tests).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tracer/dns_ports_test.go` around lines 155 - 175, The test is reimplementing the DNS port parsing logic using splitAndTrimPorts, parsePort, and manual array population instead of calling the actual production function configureFlowSpecVariables. This duplication means regressions in the production parsing logic won't be caught by the test. Replace the inline parsing logic with a direct call to the production function configureFlowSpecVariables, or extract the shared parsing logic into a helper function that both the production code and test can reuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/tracer/dns_ports_test.go`:
- Line 1: The file dns_ports_test.go is missing the mandatory Apache v2 license
header at the top. Add the Apache v2 license header comments before the package
tracer declaration to comply with the coding guidelines that require all Go
files to include this license preamble. The license header should appear as
comments at the very beginning of the file, before any other content including
the package declaration.
- Around line 17-22: The parsePort function is returning raw errors without
context, which loses information about which input failed and violates Go
error-wrapping guidelines. Modify the error return statement on line 20 to wrap
the error returned from strconv.ParseUint with contextual information about the
input that failed, using Go's error wrapping pattern with fmt.Errorf and the %w
verb to include both a descriptive message and the original error.
---
Outside diff comments:
In `@bpf/configs.h`:
- Around line 1-3: Add a GPL v2 license header comment block at the very
beginning of the configs.h file, before the existing header guards
(__CONFIGS_H__). The license header should be formatted as a multi-line C
comment containing the GPL v2 license text that complies with the repository's
eBPF coding guidelines for files in the bpf/ directory.
---
Nitpick comments:
In `@pkg/tracer/dns_ports_test.go`:
- Around line 155-175: The test is reimplementing the DNS port parsing logic
using splitAndTrimPorts, parsePort, and manual array population instead of
calling the actual production function configureFlowSpecVariables. This
duplication means regressions in the production parsing logic won't be caught by
the test. Replace the inline parsing logic with a direct call to the production
function configureFlowSpecVariables, or extract the shared parsing logic into a
helper function that both the production code and test can reuse.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27b06ca6-e301-4d14-bc0b-a847e23e53f7
📒 Files selected for processing (6)
bpf/configs.hbpf/dns_tracker.hdocs/config.mdpkg/config/config.gopkg/tracer/dns_ports_test.gopkg/tracer/tracer.go
✅ Files skipped from review due to trivial changes (1)
- docs/config.md
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/config/config.go
- bpf/dns_tracker.h
- pkg/tracer/tracer.go
|
|
||
| static __always_inline bool is_dns_port(u16 port) { | ||
| // Bounded loop for eBPF verifier | ||
| #pragma unroll |
There was a problem hiding this comment.
no need to use those pragma the compliers take care of that we no longer need to use them
| if (i >= dns_ports_count) { | ||
| break; | ||
| } | ||
| if (port == dns_ports[i]) { |
There was a problem hiding this comment.
have u checked out using global array map table to pass in this configuration instead of using global variables array ?
There was a problem hiding this comment.
To be honest I am not sure to fully understand the differences between both. Could you elaborate ?
As far as I can tell, the project is using global variables when needed.
There was a problem hiding this comment.
we use global as config knobs but I think for passing an array which operator can change runtime its better to store the list as an array map from user space and let the kernel read it
There was a problem hiding this comment.
This config comes from env, the operator can not change it without recreating the pod.
There was a problem hiding this comment.
how many ports this can be ? as global u will always burn that space regardless if DNS is enabled or not, but if its few ports then that might not be a concern ?
There was a problem hiding this comment.
For now, there is a hard limit of 8 different ports.
OlivierCazade
force-pushed
the
dns-ports
branch
2 times, most recently
from
fbccda9 to
ebbd76c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/tracer/dns_ports_test.go (1)
116-121: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win“DNS tracking disabled” case is testing the test branch, not production behavior.
Line 143 conditionally skips parsing inside the test itself, so the case at Line 116 only validates zero-value locals. It does not verify the real disabled-path wiring in
configureFlowSpecVariables(pkg/tracer/tracer.goLines 2168-2181). Consider moving this assertion to a test that exercises that function directly (or drop this case from the parser table).Also applies to: 143-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tracer/dns_ports_test.go` around lines 116 - 121, The test case "DNS tracking disabled - no parsing" at line 116-121 does not actually validate production behavior because the parsing is conditionally skipped inside the test at line 143-145, meaning it only checks zero-value defaults rather than the real disabled-path wiring in the configureFlowSpecVariables function in pkg/tracer/tracer.go. Either move this test assertion to directly test the configureFlowSpecVariables function's disabled-path implementation at lines 2168-2181, or remove this test case from the parser table since it does not exercise actual production code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bpf/configs.h`:
- Around line 1-5: The file bpf/configs.h is missing the required GPL v2 license
header. Add the GPL v2 license header (including SPDX identifier and license
text) at the very top of the file, before the include guard that starts with
`#ifndef __CONFIGS_H__`. This ensures compliance with eBPF coding guidelines
that mandate GPL v2 license headers for all files in the bpf/ directory.
---
Nitpick comments:
In `@pkg/tracer/dns_ports_test.go`:
- Around line 116-121: The test case "DNS tracking disabled - no parsing" at
line 116-121 does not actually validate production behavior because the parsing
is conditionally skipped inside the test at line 143-145, meaning it only checks
zero-value defaults rather than the real disabled-path wiring in the
configureFlowSpecVariables function in pkg/tracer/tracer.go. Either move this
test assertion to directly test the configureFlowSpecVariables function's
disabled-path implementation at lines 2168-2181, or remove this test case from
the parser table since it does not exercise actual production code paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca635ed2-f3cd-4e0a-9289-4a9cab071d94
📒 Files selected for processing (6)
bpf/configs.hbpf/dns_tracker.hdocs/config.mdpkg/config/config.gopkg/tracer/dns_ports_test.gopkg/tracer/tracer.go
✅ Files skipped from review due to trivial changes (1)
- docs/config.md
🚧 Files skipped from review as they are similar to previous changes (3)
- bpf/dns_tracker.h
- pkg/config/config.go
- pkg/tracer/tracer.go
| DNSTrackingPort uint16 `env:"DNS_TRACKING_PORT" envDefault:"53"` | ||
| // DNSTrackingPorts used to define which ports the DNS service is mapped to at the pod level, | ||
| // so we can track DNS at the pod level. Comma-separated list of ports (e.g., "53,5353,8053") | ||
| DNSTrackingPorts string `env:"DNS_TRACKING_PORT" envDefault:"53"` |
There was a problem hiding this comment.
wouldn't it be cleaner if we define it as
DNSTrackingPorts []uint16
Update DNS_TRACKING_PORT to accept comma-separated port values while maintaining backward compatibility with single port configuration. Changes: - Add support for up to 8 DNS ports (MAX_DNS_PORTS) - Replace single dns_port with dns_ports array in eBPF config - Remove hardcoded port 53 check in track_dns_packet() - Add is_dns_port() helper with bounded loop for eBPF verifier - Update config to parse DNS_TRACKING_PORT as comma-separated string - Update documentation with multi-port usage examples Examples: - DNS_TRACKING_PORT=53 (single port, backward compatible) - DNS_TRACKING_PORT=53,5353,8053 (multiple ports) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@OlivierCazade: 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. |
|
/LGTM |
3 participants
Description
Update DNS_TRACKING_PORT to accept comma-separated port values while maintaining backward compatibility with single port configuration.
Changes:
Examples:
Dependencies
n/a
Checklist
To run a perfscale test, comment with:
/test ebpf-node-density-heavy-25nodesSummary by CodeRabbit
DNS_TRACKING_PORTas a comma-separated list (default:53, e.g.,53,5053).DNS_TRACKING_PORTdocs with multi-port format and max supported count.