Open
Conversation
|
[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 |
msherif1234
force-pushed
the
quic_parsing
branch
14 times, most recently
from
9916554 to
b3d3cf5
Compare
msherif1234
changed the title
msherif1234
force-pushed
the
quic_parsing
branch
4 times, most recently
from
06b1324 to
8e53fae
Compare
Contributor
Author
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
New images: These will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=34c8c71 make set-agent-image |
msherif1234
force-pushed
the
quic_parsing
branch
from
8e53fae to
dab410d
Compare
github-actions
bot
removed
the
ok-to-test
msherif1234
force-pushed
the
quic_parsing
branch
from
6a5aa55 to
5eb22fa
Compare
|
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:
📝 WalkthroughWalkthroughAdds in-kernel QUIC (HTTP/3) parsing and per-CPU QUIC flow metrics, new eBPF map/types and a config flag, wires metrics through Go bindings, model, protobuf and exporter, adds tests and an end-to-end HTTP/3 example, and updates Makefile test targets for containerized execution. Changes
Sequence DiagramsequenceDiagram
participant Packet as Incoming Packet
participant eBPF as eBPF::flow_monitor()
participant Parser as quic_tracker::parse_quic_header()
participant MapStore as quic_flows (per-CPU map)
participant Tracer as Tracer/LookupAndDeleteMap()
participant Model as BpfFlowContent
participant PB as Protobuf/FlowToPB
participant Exporter as Exporter
Packet->>eBPF: ingress (UDP)
eBPF->>eBPF: check enable_quic_tracking
alt enabled
eBPF->>Parser: track_quic_packet(skb, pkt, eth_proto, dir, len)
Parser->>Parser: validate QUIC fixed bit, header type, CIDs, version
Parser->>MapStore: lookup/insert quic_metrics by flow_id
Parser->>MapStore: update timestamps, bytes, packets, version, flags
end
Tracer->>MapStore: read per-CPU quic_flows entries
MapStore-->>Tracer: aggregated quic_metrics
Tracer->>Model: AccumulateQuic(metrics)
Model->>PB: FlowToPB (includes Quic if present)
PB->>Exporter: serialized flow with QUIC metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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. 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: 3
🧹 Nitpick comments (4)
pkg/config/config.go (1)
290-295: ValidateQUICTrackingModerange at config load time.Consider rejecting or warning on values outside
0..2to avoid silent misconfiguration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/config.go` around lines 290 - 295, Add explicit validation for QUICTrackingMode after the config is populated from environment (e.g., in the function that constructs/loads the config such as LoadConfig/NewConfigFromEnv/ParseEnv) to ensure the integer is within 0..2; if it's out of range, return a clear error (or fail startup) identifying QUICTrackingMode and the invalid value (alternatively log a hard warning and fallback to 0 if you prefer non-fatal behavior). Modify the config loading path to perform this check on the QUICTrackingMode field and update/add tests to cover invalid values.pkg/model/flow_content.go (1)
180-192: Optional: return immediately after firstQuicMetricsinitialization.A fast return after
p.QuicMetrics = othermakes intent clearer and removes redundant comparisons on the same value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/model/flow_content.go` around lines 180 - 192, After initializing p.QuicMetrics with other (inside the block checking if p.QuicMetrics == nil), return immediately to avoid running the subsequent field-by-field comparisons; update the merge function that contains the p.QuicMetrics / other logic so that right after p.QuicMetrics = other you add an early return, preventing redundant checks of p.QuicMetrics.Version, SeenLongHdr and SeenShortHdr against other.examples/test-quic.sh (1)
666-698: Test assertion depends on agent debug log format.The grep patterns (
QUIC flow metrics sample,quicFlowsLogged=,p=17) are tightly coupled to the log format intracer.go. If the log format changes, this test will silently pass (no QUIC found) or fail unexpectedly.Consider adding a comment noting this dependency, or verifying the agent has debug logging enabled before running assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/test-quic.sh` around lines 666 - 698, The test relies on exact agent log messages ("QUIC flow metrics sample", "quicFlowsLogged=", "p=17") which couple it to tracer.go's log format; update examples/test-quic.sh to either (a) add a clear comment near the assertion block noting this dependency on tracer.go log format and expected debug logging, or (b) programmatically verify the agent is running with debug logging before asserting—e.g., inspect agent pods/config (using NETOBSERV_NAMESPACE and AGENT_LABEL) for a debug log-level flag or a recent log line indicating debug level, and fail early with a descriptive message if debug logging isn’t enabled so the subsequent greps (on agent_logs_since, expected_port, START_TS) are valid.bpf/quic_tracker.h (1)
260-268: Nit: Shadowed variableflow.Line 261 redeclares
flow(originally declared at line 232). Works correctly but could be renamed toexistingfor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bpf/quic_tracker.h` around lines 260 - 268, The inner declaration of variable "flow" inside the ret == -EEXIST block shadows the earlier "flow" (declared around line 232) when looking up quic_metrics from the bpf map; rename the inner variable to a non-shadowing name like "existing" (or "existing_flow") and update its uses (existing->end_mono_time_ts, existing->packets, existing->bytes) so the lookup from bpf_map_lookup_elem(&quic_flows, id) no longer hides the outer "flow" variable but preserves the same behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 182-198: The test-container Makefile target runs ./pkg/maps twice
because it appends "&& go test -v ./pkg/maps" after already running ./pkg/...,
causing divergence from the host test target; remove the trailing "&& go test -v
./pkg/maps" from the test-container recipe so the container run matches the test
target (or alternatively add the maps invocation to the test target if you
intend to run it separately), and rely on the existing verify-maps target for
any explicit maps-only checks.
In `@pkg/pbflow/proto.go`:
- Around line 205-211: The QUIC seen flags are being narrowed from uint32 to
uint8 directly in the pbflow conversion (when pb.Quic != nil) which can wrap on
malformed protobufs; change the assignments to
out.Metrics.QuicMetrics.SeenLongHdr and SeenShortHdr so you clamp the incoming
pb.Quic.SeenLongHdr and pb.Quic.SeenShortHdr to 0 or 1 before casting to uint8
(e.g., convert any non‑zero to 1 and zero to 0 or use a min(max(...,0),1) check)
when constructing the ebpf.BpfQuicMetrics values to ensure no overflow/wrap
occurs.
In `@pkg/tracer/tracer.go`:
- Around line 350-404: The code references nonexistent config fields
(EnableDNSTracker, EnablePktTranslation, EnableIPsecTracker); update those
checks to the actual fields on FlowFetcherConfig/config.Agent: replace
cfg.EnableDNSTracker with cfg.EnableDNSTracking, cfg.EnablePktTranslation with
cfg.EnablePktTranslationTracking, and cfg.EnableIPsecTracker with
cfg.EnableIPsecTracking so the conditional blocks around loadPinnedMapInto calls
(e.g., the DNS, pkt translation, and IPsec blocks) use the correct config flags.
---
Nitpick comments:
In `@bpf/quic_tracker.h`:
- Around line 260-268: The inner declaration of variable "flow" inside the ret
== -EEXIST block shadows the earlier "flow" (declared around line 232) when
looking up quic_metrics from the bpf map; rename the inner variable to a
non-shadowing name like "existing" (or "existing_flow") and update its uses
(existing->end_mono_time_ts, existing->packets, existing->bytes) so the lookup
from bpf_map_lookup_elem(&quic_flows, id) no longer hides the outer "flow"
variable but preserves the same behavior.
In `@examples/test-quic.sh`:
- Around line 666-698: The test relies on exact agent log messages ("QUIC flow
metrics sample", "quicFlowsLogged=", "p=17") which couple it to tracer.go's log
format; update examples/test-quic.sh to either (a) add a clear comment near the
assertion block noting this dependency on tracer.go log format and expected
debug logging, or (b) programmatically verify the agent is running with debug
logging before asserting—e.g., inspect agent pods/config (using
NETOBSERV_NAMESPACE and AGENT_LABEL) for a debug log-level flag or a recent log
line indicating debug level, and fail early with a descriptive message if debug
logging isn’t enabled so the subsequent greps (on agent_logs_since,
expected_port, START_TS) are valid.
In `@pkg/config/config.go`:
- Around line 290-295: Add explicit validation for QUICTrackingMode after the
config is populated from environment (e.g., in the function that
constructs/loads the config such as LoadConfig/NewConfigFromEnv/ParseEnv) to
ensure the integer is within 0..2; if it's out of range, return a clear error
(or fail startup) identifying QUICTrackingMode and the invalid value
(alternatively log a hard warning and fallback to 0 if you prefer non-fatal
behavior). Modify the config loading path to perform this check on the
QUICTrackingMode field and update/add tests to cover invalid values.
In `@pkg/model/flow_content.go`:
- Around line 180-192: After initializing p.QuicMetrics with other (inside the
block checking if p.QuicMetrics == nil), return immediately to avoid running the
subsequent field-by-field comparisons; update the merge function that contains
the p.QuicMetrics / other logic so that right after p.QuicMetrics = other you
add an early return, preventing redundant checks of p.QuicMetrics.Version,
SeenLongHdr and SeenShortHdr against other.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4df44998-855d-4305-870f-1e8cbd45dd9b
⛔ Files ignored due to path filters (5)
pkg/ebpf/bpf_arm64_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_powerpc_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_s390_bpfeb.ois excluded by!**/*.opkg/ebpf/bpf_x86_bpfel.ois excluded by!**/*.opkg/pbflow/flow.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (25)
.mk/bc.mkMakefilebpf/configs.hbpf/flows.cbpf/maps_definition.hbpf/quic_tracker.hbpf/types.hdocs/config.mdexamples/test-quic.shpkg/config/config.gopkg/decode/decode_protobuf.gopkg/decode/decode_protobuf_test.gopkg/ebpf/bpf_arm64_bpfel.gopkg/ebpf/bpf_powerpc_bpfel.gopkg/ebpf/bpf_s390_bpfeb.gopkg/ebpf/bpf_x86_bpfel.gopkg/ebpf/gen.gopkg/exporter/converters_test.gopkg/maps/maps.gopkg/model/flow_content.gopkg/model/flow_content_test.gopkg/pbflow/proto.gopkg/tracer/tracer.goproto/flow.protoscripts/agent.yml
Comment on lines
+205
to
+211
| if pb.Quic != nil { | ||
| out.Metrics.QuicMetrics = &ebpf.BpfQuicMetrics{ | ||
| Version: pb.Quic.Version, | ||
| SeenLongHdr: uint8(pb.Quic.SeenLongHdr), | ||
| SeenShortHdr: uint8(pb.Quic.SeenShortHdr), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify proto field types and current narrowing conversions
rg -nP 'message\s+Quic|seen_(long|short)_hdr|uint32\s+seen_(long|short)_hdr' proto/flow.proto
rg -nP 'Seen(Long|Short)Hdr:\s*uint8\(pb\.Quic\.Seen(Long|Short)Hdr\)' pkg/pbflow/proto.goRepository: netobserv/netobserv-ebpf-agent
Length of output: 256
Clamp QUIC seen flags before narrowing to uint8.
Direct uint32 → uint8 casts can wrap on malformed protobuf payloads. Clamp to {0,1} before assignment.
Proposed fix
if pb.Quic != nil {
+ seenLong := pb.Quic.SeenLongHdr
+ if seenLong > 1 {
+ seenLong = 1
+ }
+ seenShort := pb.Quic.SeenShortHdr
+ if seenShort > 1 {
+ seenShort = 1
+ }
out.Metrics.QuicMetrics = &ebpf.BpfQuicMetrics{
Version: pb.Quic.Version,
- SeenLongHdr: uint8(pb.Quic.SeenLongHdr),
- SeenShortHdr: uint8(pb.Quic.SeenShortHdr),
+ SeenLongHdr: uint8(seenLong),
+ SeenShortHdr: uint8(seenShort),
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/pbflow/proto.go` around lines 205 - 211, The QUIC seen flags are being
narrowed from uint32 to uint8 directly in the pbflow conversion (when pb.Quic !=
nil) which can wrap on malformed protobufs; change the assignments to
out.Metrics.QuicMetrics.SeenLongHdr and SeenShortHdr so you clamp the incoming
pb.Quic.SeenLongHdr and pb.Quic.SeenShortHdr to 0 or 1 before casting to uint8
(e.g., convert any non‑zero to 1 and zero to 0 or use a min(max(...,0),1) check)
when constructing the ebpf.BpfQuicMetrics values to ensure no overflow/wrap
occurs.
Contributor
Author
ur TLS tracker is based of TCP with ACK flags while QUIC uses UDP |
msherif1234
force-pushed
the
quic_parsing
branch
from
5eb22fa to
1f664fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bpf/quic_tracker.h`:
- Around line 260-267: The EEXIST fallback only increments packets/bytes and
end_mono_time_ts but must also mirror the main-path header/version updates; in
the ret == -EEXIST handling for quic_metrics from
bpf_map_lookup_elem(&quic_flows, id) set the same fields updated in the primary
lookup path—seen_long_hdr, seen_short_hdr and version—using the local parsed
variables (e.g., seen_long_hdr, seen_short_hdr, version) so the flow's
header-type flags and version are kept consistent with the main path.
In `@pkg/model/flow_content.go`:
- Around line 175-193: In BpfFlowContent.AccumulateQuic, after calling
p.buildBaseFromAdditional(...) and assigning p.QuicMetrics = other when
p.QuicMetrics is nil, add an immediate return to avoid redundant self-comparison
and extra work; locate the nil-check and assignment in AccumulateQuic and mirror
the early-return pattern used in AccumulateDNS so the method exits right after
setting p.QuicMetrics to other.
In `@pkg/tracer/tracer.go`:
- Around line 2250-2254: The merge conflict left Git markers and only one of the
tracking flags in the slice; remove the conflict markers and include both
tracking entries so both constEnableTLSUsageTracking with
uint8(enableTLSTracking) and constEnableQUICTracking with
uint8(enableQUICTracking) are present (or explicitly choose one if intended),
ensuring the slice/array where these are added in tracer.go contains both
entries and compiles cleanly.
- Around line 945-950: The QuicFlows map may be nil when cfg.QUICTrackingMode ==
0, so add a nil check before calling m.objects.QuicFlows.Unpin() and
m.objects.QuicFlows.Close(): only call Unpin() and Close() if
m.objects.QuicFlows != nil (consistent with how other optional maps are
guarded). Ensure any errors returned are still appended to errs as before.
In `@proto/flow.proto`:
- Around line 73-81: Resolve the git merge conflict by keeping the QUIC branch
change and removing the conflict markers and the old SSL fields so that the
proto field reads only "Quic quic = 33;" (remove "uint32 ssl_version", "bool
ssl_mismatch", "uint32 tls_types", "uint32 tls_cipher_suite", and "uint32
tls_key_share" and the <<<<<<<, =======, >>>>>>> markers); then regenerate the
generated protobuf sources (flow.pb.go) so they reflect the new Quic field.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66281f2e-1d80-4390-bfae-676f739c592e
⛔ Files ignored due to path filters (5)
pkg/ebpf/bpf_arm64_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_powerpc_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_s390_bpfeb.ois excluded by!**/*.opkg/ebpf/bpf_x86_bpfel.ois excluded by!**/*.opkg/pbflow/flow.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (25)
.mk/bc.mkMakefilebpf/configs.hbpf/flows.cbpf/maps_definition.hbpf/quic_tracker.hbpf/types.hdocs/config.mdexamples/test-quic.shpkg/config/config.gopkg/decode/decode_protobuf.gopkg/decode/decode_protobuf_test.gopkg/ebpf/bpf_arm64_bpfel.gopkg/ebpf/bpf_powerpc_bpfel.gopkg/ebpf/bpf_s390_bpfeb.gopkg/ebpf/bpf_x86_bpfel.gopkg/ebpf/gen.gopkg/exporter/converters_test.gopkg/maps/maps.gopkg/model/flow_content.gopkg/model/flow_content_test.gopkg/pbflow/proto.gopkg/tracer/tracer.goproto/flow.protoscripts/agent.yml
✅ Files skipped from review due to trivial changes (5)
- .mk/bc.mk
- scripts/agent.yml
- pkg/decode/decode_protobuf.go
- pkg/maps/maps.go
- pkg/ebpf/gen.go
🚧 Files skipped from review as they are similar to previous changes (12)
- docs/config.md
- bpf/flows.c
- pkg/exporter/converters_test.go
- pkg/config/config.go
- bpf/maps_definition.h
- pkg/pbflow/proto.go
- bpf/types.h
- Makefile
- pkg/ebpf/bpf_arm64_bpfel.go
- examples/test-quic.sh
- pkg/ebpf/bpf_x86_bpfel.go
- pkg/ebpf/bpf_s390_bpfeb.go
Comment on lines
+945
to
+950
| if err := m.objects.QuicFlows.Unpin(); err != nil { | ||
| errs = append(errs, err) | ||
| } | ||
| if err := m.objects.QuicFlows.Close(); err != nil { | ||
| errs = append(errs, err) | ||
| } |
There was a problem hiding this comment.
Missing nil check for QuicFlows before Unpin/Close.
Unlike other maps that are always loaded, QuicFlows is only loaded when cfg.QUICTrackingMode != 0. Calling Unpin()/Close() on a nil map will panic.
Proposed fix
- if err := m.objects.QuicFlows.Unpin(); err != nil {
- errs = append(errs, err)
- }
- if err := m.objects.QuicFlows.Close(); err != nil {
- errs = append(errs, err)
+ if m.objects.QuicFlows != nil {
+ if err := m.objects.QuicFlows.Unpin(); err != nil {
+ errs = append(errs, err)
+ }
+ if err := m.objects.QuicFlows.Close(); err != nil {
+ errs = append(errs, err)
+ }
}📝 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
| if err := m.objects.QuicFlows.Unpin(); err != nil { | |
| errs = append(errs, err) | |
| } | |
| if err := m.objects.QuicFlows.Close(); err != nil { | |
| errs = append(errs, err) | |
| } | |
| if m.objects.QuicFlows != nil { | |
| if err := m.objects.QuicFlows.Unpin(); err != nil { | |
| errs = append(errs, err) | |
| } | |
| if err := m.objects.QuicFlows.Close(); err != nil { | |
| errs = append(errs, err) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/tracer/tracer.go` around lines 945 - 950, The QuicFlows map may be nil
when cfg.QUICTrackingMode == 0, so add a nil check before calling
m.objects.QuicFlows.Unpin() and m.objects.QuicFlows.Close(): only call Unpin()
and Close() if m.objects.QuicFlows != nil (consistent with how other optional
maps are guarded). Ensure any errors returned are still appended to errs as
before.
Member
There was a problem hiding this comment.
@msherif1234 I don't see a difference with how other maps are handled, at least wrt map creation & deletion, so maybe it is an hallucination? Have you tried running with QUIC disabled?
However I see one difference with other maps: this one isn't resized according to CacheMaxFlows+feature flag, is it intentional? I guess a 1 << 16 size makes it ok to keep always on (and it's no prealloc).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/tracer/tracer.go (1)
1169-1178: Usefmt.Fprintfinstead ofWriteString(fmt.Sprintf(...)).Static analysis suggests using
fmt.Fprintf(&b, ...)directly for cleaner code and slightly better performance.Proposed fix
- b.WriteString(fmt.Sprintf( - "%s:%d>%s:%d p=%d v=%d lh=%d sh=%d", + fmt.Fprintf(&b, + "%s:%d>%s:%d p=%d v=%d lh=%d sh=%d", model.IP(model.IPAddr(id.SrcIp)).String(), id.SrcPort, model.IP(model.IPAddr(id.DstIp)).String(), id.DstPort, id.TransportProtocol, qm.Version, qm.SeenLongHdr, qm.SeenShortHdr, - )) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tracer/tracer.go` around lines 1169 - 1178, The code uses b.WriteString(fmt.Sprintf(...)) when formatting the flow line; replace that pattern with fmt.Fprintf(&b, ...) to write directly into the bytes.Buffer for cleaner, slightly more efficient formatting. Locate the block that currently calls b.WriteString(fmt.Sprintf(...)) (the variables include b, id.SrcIp/id.SrcPort, id.DstIp/id.DstPort, id.TransportProtocol, qm.Version, qm.SeenLongHdr, qm.SeenShortHdr and references model.IP) and change it to a single fmt.Fprintf(&b, "...", ... ) call preserving the same format string and argument order; leave the surrounding logic (logged++, comment) 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 `@pkg/tracer/tracer.go`:
- Around line 1169-1178: The code uses b.WriteString(fmt.Sprintf(...)) when
formatting the flow line; replace that pattern with fmt.Fprintf(&b, ...) to
write directly into the bytes.Buffer for cleaner, slightly more efficient
formatting. Locate the block that currently calls
b.WriteString(fmt.Sprintf(...)) (the variables include b, id.SrcIp/id.SrcPort,
id.DstIp/id.DstPort, id.TransportProtocol, qm.Version, qm.SeenLongHdr,
qm.SeenShortHdr and references model.IP) and change it to a single
fmt.Fprintf(&b, "...", ... ) call preserving the same format string and argument
order; leave the surrounding logic (logged++, comment) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43b03b35-6464-4b62-9c0e-10195514e9a1
⛔ Files ignored due to path filters (1)
pkg/pbflow/flow.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
pkg/tracer/tracer.goproto/flow.proto
🚧 Files skipped from review as they are similar to previous changes (1)
- proto/flow.proto
msherif1234
force-pushed
the
quic_parsing
branch
from
7244688 to
d870895
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Makefile (1)
198-198:⚠️ Potential issue | 🟡 MinorRemove redundant
./pkg/mapstest execution in container target.
go test -mod vendor ./pkg/... ./cmd/...already includes./pkg/maps; the extra call runs it twice and diverges fromtest.Proposed fix
- sh -lc 'mkdir -p "$$GOPATH" "$$GOCACHE"; export PATH="/usr/local/go/bin:/go/bin:$$PATH"; command -v go >/dev/null || (echo "ERROR: go not found; PATH=$$PATH" && exit 127); go version && go test -mod vendor ./pkg/... ./cmd/... -coverpkg=./... -coverprofile cover.all.out && go test -v ./pkg/maps' + sh -lc 'mkdir -p "$$GOPATH" "$$GOCACHE"; export PATH="/usr/local/go/bin:/go/bin:$$PATH"; command -v go >/dev/null || (echo "ERROR: go not found; PATH=$$PATH" && exit 127); go version && go test -mod vendor ./pkg/... ./cmd/... -coverpkg=./... -coverprofile cover.all.out'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 198, Remove the redundant second test invocation that runs ./pkg/maps twice: in the container target remove the trailing "&& go test -v ./pkg/maps" so only the unified "go test -mod vendor ./pkg/... ./cmd/... -coverpkg=./... -coverprofile cover.all.out" invocation remains; ensure the command still checks go presence and prints go version as before and that no other changes alter PATH/GOPATH handling.pkg/tracer/tracer.go (1)
945-950:⚠️ Potential issue | 🔴 CriticalMissing nil check for
QuicFlowsbefore Unpin/Close.
QuicFlowsis only loaded whencfg.QUICTrackingMode != 0. CallingUnpin()/Close()on a nil map will panic.Proposed fix
- if err := m.objects.QuicFlows.Unpin(); err != nil { - errs = append(errs, err) - } - if err := m.objects.QuicFlows.Close(); err != nil { - errs = append(errs, err) - } + if m.objects.QuicFlows != nil { + if err := m.objects.QuicFlows.Unpin(); err != nil { + errs = append(errs, err) + } + if err := m.objects.QuicFlows.Close(); err != nil { + errs = append(errs, err) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tracer/tracer.go` around lines 945 - 950, The code calls m.objects.QuicFlows.Unpin() and .Close() without checking if m.objects.QuicFlows is nil, which can panic because QuicFlows is only initialized when cfg.QUICTrackingMode != 0; update the cleanup to first test if m.objects.QuicFlows != nil before calling Unpin() and Close(), preserving the existing error aggregation (append to errs) and behavior of both calls (i.e., call Unpin then Close only when QuicFlows is non-nil) so the function safely no-ops when QUIC tracking was never enabled.
🧹 Nitpick comments (2)
pkg/tracer/tracer.go (1)
1171-1177: Usefmt.Fprintfinstead ofWriteString(fmt.Sprintf(...)).Minor efficiency improvement flagged by static analysis.
Proposed fix
- b.WriteString(fmt.Sprintf( - "%s:%d>%s:%d p=%d v=%d lh=%d sh=%d", + fmt.Fprintf(&b, + "%s:%d>%s:%d p=%d v=%d lh=%d sh=%d", model.IP(model.IPAddr(id.SrcIp)).String(), id.SrcPort, model.IP(model.IPAddr(id.DstIp)).String(), id.DstPort, id.TransportProtocol, - qm.Version, qm.SeenLongHdr, qm.SeenShortHdr, - )) + qm.Version, qm.SeenLongHdr, qm.SeenShortHdr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tracer/tracer.go` around lines 1171 - 1177, Replace the allocation via b.WriteString(fmt.Sprintf(...)) with a direct formatted write using fmt.Fprintf to avoid the intermediate Sprintf; specifically, call fmt.Fprintf with the buffer/writer variable b and the format string and the same arguments (model.IP(model.IPAddr(id.SrcIp)).String(), id.SrcPort, model.IP(model.IPAddr(id.DstIp)).String(), id.DstPort, id.TransportProtocol, qm.Version, qm.SeenLongHdr, qm.SeenShortHdr) so the formatted output is written directly to b.examples/test-quic.sh (1)
362-362: Variables used withoutlocaldeclaration.
client_logs(lines 362, 534) andexit_code(line 564) are assigned withoutlocal, making them global. This can cause subtle bugs if functions are called recursively or if variable names collide.Add local declarations
# In run_quic_negative_no_udp: wait_pod_done "${QUIC_NAMESPACE}" "${pod}" 180 || true - client_logs="$(kubectl logs -n "${QUIC_NAMESPACE}" "${pod}" 2>/dev/null || true)" + local client_logs="" + client_logs="$(kubectl logs -n "${QUIC_NAMESPACE}" "${pod}" 2>/dev/null || true)" # In validate_quic_client_pods: - client_logs="$(kubectl logs -n "${ns}" "${pod}" 2>/dev/null || true)" + local client_logs="" + client_logs="$(kubectl logs -n "${ns}" "${pod}" 2>/dev/null || true)" - exit_code="$(kubectl get pod ...)" + local exit_code="" + exit_code="$(kubectl get pod ...)"Also applies to: 534-534, 564-564
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/test-quic.sh` at line 362, The variables client_logs and exit_code are being assigned without local, making them global; update the functions that capture kubectl output and return codes to declare these as local (e.g., local client_logs; local exit_code) before assignment so they don't leak into the global scope, and ensure any other occurrences of client_logs or exit_code in the script are similarly converted to local within their respective functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/test-quic.sh`:
- Around line 585-593: The debug block in validate_quic_client_pods currently
always references the global QUIC_SERVER_NAME, causing wrong server logs when
testing alternate ports; change validate_quic_client_pods to accept a
server_name parameter and use that variable in the kubectl logs command instead
of QUIC_SERVER_NAME, then update its callers—specifically have run_quic_client
pass QUIC_ALT_SERVER_NAME (and any other call sites such as
run_quic_non_443_port that call run_quic_client) so the correct server pod logs
are shown during debugging.
---
Duplicate comments:
In `@Makefile`:
- Line 198: Remove the redundant second test invocation that runs ./pkg/maps
twice: in the container target remove the trailing "&& go test -v ./pkg/maps" so
only the unified "go test -mod vendor ./pkg/... ./cmd/... -coverpkg=./...
-coverprofile cover.all.out" invocation remains; ensure the command still checks
go presence and prints go version as before and that no other changes alter
PATH/GOPATH handling.
In `@pkg/tracer/tracer.go`:
- Around line 945-950: The code calls m.objects.QuicFlows.Unpin() and .Close()
without checking if m.objects.QuicFlows is nil, which can panic because
QuicFlows is only initialized when cfg.QUICTrackingMode != 0; update the cleanup
to first test if m.objects.QuicFlows != nil before calling Unpin() and Close(),
preserving the existing error aggregation (append to errs) and behavior of both
calls (i.e., call Unpin then Close only when QuicFlows is non-nil) so the
function safely no-ops when QUIC tracking was never enabled.
---
Nitpick comments:
In `@examples/test-quic.sh`:
- Line 362: The variables client_logs and exit_code are being assigned without
local, making them global; update the functions that capture kubectl output and
return codes to declare these as local (e.g., local client_logs; local
exit_code) before assignment so they don't leak into the global scope, and
ensure any other occurrences of client_logs or exit_code in the script are
similarly converted to local within their respective functions.
In `@pkg/tracer/tracer.go`:
- Around line 1171-1177: Replace the allocation via
b.WriteString(fmt.Sprintf(...)) with a direct formatted write using fmt.Fprintf
to avoid the intermediate Sprintf; specifically, call fmt.Fprintf with the
buffer/writer variable b and the format string and the same arguments
(model.IP(model.IPAddr(id.SrcIp)).String(), id.SrcPort,
model.IP(model.IPAddr(id.DstIp)).String(), id.DstPort, id.TransportProtocol,
qm.Version, qm.SeenLongHdr, qm.SeenShortHdr) so the formatted output is written
directly to b.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9830284-66d3-43b3-b6a8-c8a738b0a7ac
⛔ Files ignored due to path filters (5)
pkg/ebpf/bpf_arm64_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_powerpc_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_s390_bpfeb.ois excluded by!**/*.opkg/ebpf/bpf_x86_bpfel.ois excluded by!**/*.opkg/pbflow/flow.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (25)
.mk/bc.mkMakefilebpf/configs.hbpf/flows.cbpf/maps_definition.hbpf/quic_tracker.hbpf/types.hdocs/config.mdexamples/test-quic.shpkg/config/config.gopkg/decode/decode_protobuf.gopkg/decode/decode_protobuf_test.gopkg/ebpf/bpf_arm64_bpfel.gopkg/ebpf/bpf_powerpc_bpfel.gopkg/ebpf/bpf_s390_bpfeb.gopkg/ebpf/bpf_x86_bpfel.gopkg/ebpf/gen.gopkg/exporter/converters_test.gopkg/maps/maps.gopkg/model/flow_content.gopkg/model/flow_content_test.gopkg/pbflow/proto.gopkg/tracer/tracer.goproto/flow.protoscripts/agent.yml
✅ Files skipped from review due to trivial changes (6)
- pkg/maps/maps.go
- docs/config.md
- bpf/flows.c
- pkg/ebpf/gen.go
- pkg/exporter/converters_test.go
- bpf/maps_definition.h
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/decode/decode_protobuf.go
- .mk/bc.mk
- pkg/pbflow/proto.go
- pkg/config/config.go
- proto/flow.proto
- bpf/types.h
- bpf/quic_tracker.h
examples/test-quic.sh
Outdated
Comment on lines
+585
to
+593
| if (( failures > 0 )); then | ||
| log "" | ||
| log "ERROR: ${failures} QUIC client pod(s) failed." | ||
| log "" | ||
| log "==> Debug: QUIC server logs (tail 200)" | ||
| kubectl logs -n "${ns}" -l app="${QUIC_SERVER_NAME}" --tail=200 2>/dev/null || true | ||
| log "" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
Debug log references wrong server when testing alternate port.
Line 590 uses the hardcoded QUIC_SERVER_NAME global, but validate_quic_client_pods is called from run_quic_non_443_port via run_quic_client with QUIC_ALT_SERVER_NAME. The debug output will show the wrong server's logs.
Proposed fix: pass server_name as parameter
validate_quic_client_pods() {
local ns="$1"
local pod_base="$2"
local parallel="$3"
+ local server_name="${4:-${QUIC_SERVER_NAME}}"
local failures=0
...
log "==> Debug: QUIC server logs (tail 200)"
- kubectl logs -n "${ns}" -l app="${QUIC_SERVER_NAME}" --tail=200 2>/dev/null || true
+ kubectl logs -n "${ns}" -l app="${server_name}" --tail=200 2>/dev/null || trueThen update the call site in run_quic_client (line 645):
- validate_quic_client_pods "${QUIC_NAMESPACE}" "${pod_base}" "${parallel}"
+ validate_quic_client_pods "${QUIC_NAMESPACE}" "${pod_base}" "${parallel}" "${server_name}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/test-quic.sh` around lines 585 - 593, The debug block in
validate_quic_client_pods currently always references the global
QUIC_SERVER_NAME, causing wrong server logs when testing alternate ports; change
validate_quic_client_pods to accept a server_name parameter and use that
variable in the kubectl logs command instead of QUIC_SERVER_NAME, then update
its callers—specifically have run_quic_client pass QUIC_ALT_SERVER_NAME (and any
other call sites such as run_quic_non_443_port that call run_quic_client) so the
correct server pod logs are shown during debugging.
Member
I know, but regardless of the L3 protocol, QUIC is always TLS 1.3, no? So every flow identified as QUIC could set the "TLS Version" field to 1.3? |
Contributor
Author
yes, https://www.rfc-editor.org/rfc/rfc9001.pdf its only TLS handshake message and embedded into crypto frame. |
….8.0-crc0 (#590) Signed-off-by: red-hat-konflux <126015336+red-hat-konflux[bot]@users.noreply.github.com> Co-authored-by: red-hat-konflux[bot] <126015336+red-hat-konflux[bot]@users.noreply.github.com>
Signed-off-by: red-hat-konflux <126015336+red-hat-konflux[bot]@users.noreply.github.com> Co-authored-by: red-hat-konflux[bot] <126015336+red-hat-konflux[bot]@users.noreply.github.com>
Signed-off-by: Mohamed S. Mahmoud <mmahmoud2201@gmail.com>
msherif1234
force-pushed
the
quic_parsing
branch
from
d870895 to
893c65c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/tracer/tracer.go (1)
945-950:⚠️ Potential issue | 🔴 CriticalMissing nil check for QuicFlows before Unpin/Close.
QuicFlowsis only loaded whencfg.QUICTrackingMode != 0. CallingUnpin()/Close()on a nil map will panic.Proposed fix
- if err := m.objects.QuicFlows.Unpin(); err != nil { - errs = append(errs, err) - } - if err := m.objects.QuicFlows.Close(); err != nil { - errs = append(errs, err) - } + if m.objects.QuicFlows != nil { + if err := m.objects.QuicFlows.Unpin(); err != nil { + errs = append(errs, err) + } + if err := m.objects.QuicFlows.Close(); err != nil { + errs = append(errs, err) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tracer/tracer.go` around lines 945 - 950, m.objects.QuicFlows may be nil when cfg.QUICTrackingMode == 0, so before calling m.objects.QuicFlows.Unpin() and m.objects.QuicFlows.Close() check that m.objects.QuicFlows != nil; only call Unpin() and Close() (and append errors) when the QuicFlows field is non-nil to avoid a panic in the shutdown/cleanup path where QUIC tracking was not initialized.examples/test-quic.sh (1)
585-593:⚠️ Potential issue | 🟡 MinorDebug log references wrong server when testing alternate port.
Line 590 uses global
QUIC_SERVER_NAME, but this function is called fromrun_quic_clientwhich may be invoked withQUIC_ALT_SERVER_NAMEfor the non-443 port test. Debug output will show wrong server logs on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/test-quic.sh` around lines 585 - 593, The debug log is always referencing the global QUIC_SERVER_NAME even when run_quic_client was invoked for the alternate port using QUIC_ALT_SERVER_NAME; update the failure-handling block in run_quic_client to reference the actual server identifier used by the invocation (e.g., the function's server parameter or a local variable that was set to QUIC_ALT_SERVER_NAME for the non-443 test) instead of the global QUIC_SERVER_NAME so the kubectl logs call targets the correct pod (keep the kubectl invocation and tail logic but substitute the correct server variable).
🧹 Nitpick comments (1)
pkg/tracer/tracer.go (1)
1171-1171: Minor: Use fmt.Fprintf for efficiency.Static analysis suggests
fmt.Fprintf(&b, ...)instead ofb.WriteString(fmt.Sprintf(...))to avoid intermediate string allocation.Proposed fix
- b.WriteString(fmt.Sprintf( + fmt.Fprintf(&b, "%s:%d>%s:%d p=%d v=%d lh=%d sh=%d", model.IP(model.IPAddr(id.SrcIp)).String(), id.SrcPort, model.IP(model.IPAddr(id.DstIp)).String(), id.DstPort, id.TransportProtocol, qm.Version, qm.SeenLongHdr, qm.SeenShortHdr, - )) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tracer/tracer.go` at line 1171, Replace the b.WriteString(fmt.Sprintf(...)) pattern with a direct fmt.Fprintf(&b, ...) call to avoid creating the intermediate formatted string; locate the occurrence where the buffer variable b is used (in pkg/tracer/tracer.go) and change it so fmt.Fprintf takes &b and the format + args directly, preserving the original format string and arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/test-quic.sh`:
- Around line 416-433: The function run_quic_non_443_port sets agent QUIC
tracking mode to 2 but may never restore it if run_quic_client or
check_agent_logs_port fails; wrap the temporary mode change with a trap to
always call set_agent_quic_tracking_mode 1 on function exit or error (use trap
'set_agent_quic_tracking_mode 1' EXIT or ERR before calling run_quic_client and
clear/reset the trap after successful completion) so run_quic_client and
check_agent_logs_port cannot leave QUIC_TRACKING_MODE=2; reference the functions
run_quic_non_443_port, set_agent_quic_tracking_mode, run_quic_client, and
check_agent_logs_port when applying the change.
- Around line 680-686: The regex in examples/test-quic.sh incorrectly tries to
match the dst port with ">:${expected_port}" but the log format is
src_ip:src_port>dst_ip:dst_port; update the grep in the block that inspects
$agent_logs_since (the line using grep -Eq
"(:${expected_port}>|>:${expected_port}[[:space:]])") to instead look for either
a src port marker ":${expected_port}>" or a port at the end of a field
":${expected_port}[[:space:]]" (for example change the pattern to
"(:${expected_port}>|:${expected_port}[[:space:]])") so dst ports like
"172.16.0.1:443" are matched correctly.
---
Duplicate comments:
In `@examples/test-quic.sh`:
- Around line 585-593: The debug log is always referencing the global
QUIC_SERVER_NAME even when run_quic_client was invoked for the alternate port
using QUIC_ALT_SERVER_NAME; update the failure-handling block in run_quic_client
to reference the actual server identifier used by the invocation (e.g., the
function's server parameter or a local variable that was set to
QUIC_ALT_SERVER_NAME for the non-443 test) instead of the global
QUIC_SERVER_NAME so the kubectl logs call targets the correct pod (keep the
kubectl invocation and tail logic but substitute the correct server variable).
In `@pkg/tracer/tracer.go`:
- Around line 945-950: m.objects.QuicFlows may be nil when cfg.QUICTrackingMode
== 0, so before calling m.objects.QuicFlows.Unpin() and
m.objects.QuicFlows.Close() check that m.objects.QuicFlows != nil; only call
Unpin() and Close() (and append errors) when the QuicFlows field is non-nil to
avoid a panic in the shutdown/cleanup path where QUIC tracking was not
initialized.
---
Nitpick comments:
In `@pkg/tracer/tracer.go`:
- Line 1171: Replace the b.WriteString(fmt.Sprintf(...)) pattern with a direct
fmt.Fprintf(&b, ...) call to avoid creating the intermediate formatted string;
locate the occurrence where the buffer variable b is used (in
pkg/tracer/tracer.go) and change it so fmt.Fprintf takes &b and the format +
args directly, preserving the original format string and arguments.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 283ec148-f052-4c48-b14c-509b3db6d1af
⛔ Files ignored due to path filters (5)
pkg/ebpf/bpf_arm64_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_powerpc_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_s390_bpfeb.ois excluded by!**/*.opkg/ebpf/bpf_x86_bpfel.ois excluded by!**/*.opkg/pbflow/flow.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (25)
.mk/bc.mkMakefilebpf/configs.hbpf/flows.cbpf/maps_definition.hbpf/quic_tracker.hbpf/types.hdocs/config.mdexamples/test-quic.shpkg/config/config.gopkg/decode/decode_protobuf.gopkg/decode/decode_protobuf_test.gopkg/ebpf/bpf_arm64_bpfel.gopkg/ebpf/bpf_powerpc_bpfel.gopkg/ebpf/bpf_s390_bpfeb.gopkg/ebpf/bpf_x86_bpfel.gopkg/ebpf/gen.gopkg/exporter/converters_test.gopkg/maps/maps.gopkg/model/flow_content.gopkg/model/flow_content_test.gopkg/pbflow/proto.gopkg/tracer/tracer.goproto/flow.protoscripts/agent.yml
✅ Files skipped from review due to trivial changes (5)
- docs/config.md
- bpf/flows.c
- pkg/pbflow/proto.go
- bpf/maps_definition.h
- scripts/agent.yml
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/decode/decode_protobuf.go
- pkg/ebpf/gen.go
- pkg/config/config.go
- .mk/bc.mk
- pkg/exporter/converters_test.go
- proto/flow.proto
- pkg/model/flow_content_test.go
- pkg/model/flow_content.go
- pkg/maps/maps.go
- pkg/ebpf/bpf_powerpc_bpfel.go
examples/test-quic.sh
Outdated
Comment on lines
+416
to
+433
| run_quic_non_443_port() { | ||
| if ! is_pos_int "${QUIC_ALT_PORT}"; then | ||
| log "ERROR: QUIC_ALT_PORT must be a positive integer (got: ${QUIC_ALT_PORT})" | ||
| return 2 | ||
| fi | ||
| if [[ "${QUIC_ALT_PORT}" == "443" ]]; then | ||
| log "ERROR: QUIC_ALT_PORT must be != 443 for the non-443 port case." | ||
| return 2 | ||
| fi | ||
|
|
||
| # Enable any-port detection for this case, then restore to false to keep the default suite behavior. | ||
| set_agent_quic_tracking_mode 2 | ||
|
|
||
| run_quic_client "${QUIC_REQUESTS}" "${QUIC_PARALLEL_CLIENTS}" "${QUIC_ALT_SERVER_NAME}" "${QUIC_ALT_PORT}" | ||
| check_agent_logs_port "${QUIC_ALT_PORT}" | ||
|
|
||
| set_agent_quic_tracking_mode 1 | ||
| } |
There was a problem hiding this comment.
Agent tracking mode not restored on test failure.
If run_quic_client or check_agent_logs_port fails, set -e exits before line 432, leaving the agent with QUIC_TRACKING_MODE=2. Subsequent runs or other tests will have unexpected behavior.
Proposed fix using trap for cleanup
run_quic_non_443_port() {
if ! is_pos_int "${QUIC_ALT_PORT}"; then
log "ERROR: QUIC_ALT_PORT must be a positive integer (got: ${QUIC_ALT_PORT})"
return 2
fi
if [[ "${QUIC_ALT_PORT}" == "443" ]]; then
log "ERROR: QUIC_ALT_PORT must be != 443 for the non-443 port case."
return 2
fi
# Enable any-port detection for this case, then restore to false to keep the default suite behavior.
set_agent_quic_tracking_mode 2
+ trap 'set_agent_quic_tracking_mode 1' RETURN
run_quic_client "${QUIC_REQUESTS}" "${QUIC_PARALLEL_CLIENTS}" "${QUIC_ALT_SERVER_NAME}" "${QUIC_ALT_PORT}"
check_agent_logs_port "${QUIC_ALT_PORT}"
- set_agent_quic_tracking_mode 1
}📝 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
| run_quic_non_443_port() { | |
| if ! is_pos_int "${QUIC_ALT_PORT}"; then | |
| log "ERROR: QUIC_ALT_PORT must be a positive integer (got: ${QUIC_ALT_PORT})" | |
| return 2 | |
| fi | |
| if [[ "${QUIC_ALT_PORT}" == "443" ]]; then | |
| log "ERROR: QUIC_ALT_PORT must be != 443 for the non-443 port case." | |
| return 2 | |
| fi | |
| # Enable any-port detection for this case, then restore to false to keep the default suite behavior. | |
| set_agent_quic_tracking_mode 2 | |
| run_quic_client "${QUIC_REQUESTS}" "${QUIC_PARALLEL_CLIENTS}" "${QUIC_ALT_SERVER_NAME}" "${QUIC_ALT_PORT}" | |
| check_agent_logs_port "${QUIC_ALT_PORT}" | |
| set_agent_quic_tracking_mode 1 | |
| } | |
| run_quic_non_443_port() { | |
| if ! is_pos_int "${QUIC_ALT_PORT}"; then | |
| log "ERROR: QUIC_ALT_PORT must be a positive integer (got: ${QUIC_ALT_PORT})" | |
| return 2 | |
| fi | |
| if [[ "${QUIC_ALT_PORT}" == "443" ]]; then | |
| log "ERROR: QUIC_ALT_PORT must be != 443 for the non-443 port case." | |
| return 2 | |
| fi | |
| # Enable any-port detection for this case, then restore to false to keep the default suite behavior. | |
| set_agent_quic_tracking_mode 2 | |
| trap 'set_agent_quic_tracking_mode 1' RETURN | |
| run_quic_client "${QUIC_REQUESTS}" "${QUIC_PARALLEL_CLIENTS}" "${QUIC_ALT_SERVER_NAME}" "${QUIC_ALT_PORT}" | |
| check_agent_logs_port "${QUIC_ALT_PORT}" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/test-quic.sh` around lines 416 - 433, The function
run_quic_non_443_port sets agent QUIC tracking mode to 2 but may never restore
it if run_quic_client or check_agent_logs_port fails; wrap the temporary mode
change with a trap to always call set_agent_quic_tracking_mode 1 on function
exit or error (use trap 'set_agent_quic_tracking_mode 1' EXIT or ERR before
calling run_quic_client and clear/reset the trap after successful completion) so
run_quic_client and check_agent_logs_port cannot leave QUIC_TRACKING_MODE=2;
reference the functions run_quic_non_443_port, set_agent_quic_tracking_mode,
run_quic_client, and check_agent_logs_port when applying the change.
examples/test-quic.sh
Outdated
Comment on lines
+680
to
+686
| if ! echo "$agent_logs_since" | grep -E 'QUIC flow metrics sample' | grep -Eq "(:${expected_port}>|>:${expected_port}[[:space:]])"; then | ||
| log "FAIL: QUIC metrics sample did not include a flow where src or dst port is ${expected_port}." | ||
| log "" | ||
| log "==> Debug: QUIC flow metrics sample (tail 20)" | ||
| echo "$agent_logs_since" | grep -E 'QUIC flow metrics sample' | tail -n 20 || true | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
Regex doesn't match dst port correctly.
The pattern >:${expected_port} expects literal >:443 but the log format (from tracer.go) is src_ip:src_port>dst_ip:dst_port (e.g., 10.0.0.1:12345>172.16.0.1:443). The dst port won't match because there's an IP between > and :port.
Proposed fix
- if ! echo "$agent_logs_since" | grep -E 'QUIC flow metrics sample' | grep -Eq "(:${expected_port}>|>:${expected_port}[[:space:]])"; then
+ if ! echo "$agent_logs_since" | grep -E 'QUIC flow metrics sample' | grep -Eq ":${expected_port}[[:space:]>]"; thenThis matches :443> (src port) or :443 (dst port at end before space).
📝 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
| if ! echo "$agent_logs_since" | grep -E 'QUIC flow metrics sample' | grep -Eq "(:${expected_port}>|>:${expected_port}[[:space:]])"; then | |
| log "FAIL: QUIC metrics sample did not include a flow where src or dst port is ${expected_port}." | |
| log "" | |
| log "==> Debug: QUIC flow metrics sample (tail 20)" | |
| echo "$agent_logs_since" | grep -E 'QUIC flow metrics sample' | tail -n 20 || true | |
| return 1 | |
| fi | |
| if ! echo "$agent_logs_since" | grep -E 'QUIC flow metrics sample' | grep -Eq ":${expected_port}[[:space:]>]"; then | |
| log "FAIL: QUIC metrics sample did not include a flow where src or dst port is ${expected_port}." | |
| log "" | |
| log "==> Debug: QUIC flow metrics sample (tail 20)" | |
| echo "$agent_logs_since" | grep -E 'QUIC flow metrics sample' | tail -n 20 || true | |
| return 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/test-quic.sh` around lines 680 - 686, The regex in
examples/test-quic.sh incorrectly tries to match the dst port with
">:${expected_port}" but the log format is src_ip:src_port>dst_ip:dst_port;
update the grep in the block that inspects $agent_logs_since (the line using
grep -Eq "(:${expected_port}>|>:${expected_port}[[:space:]])") to instead look
for either a src port marker ":${expected_port}>" or a port at the end of a
field ":${expected_port}[[:space:]]" (for example change the pattern to
"(:${expected_port}>|:${expected_port}[[:space:]])") so dst ports like
"172.16.0.1:443" are matched correctly.
jotak
reviewed
Apr 9, 2026
pkg/tracer/tracer.go
Outdated
| }) | ||
| met.FlowBufferSizeGauge.WithBufferName("quicmap").Set(float64(countQuic)) | ||
|
|
||
| if m.config.Debug { |
Member
There was a problem hiding this comment.
this is temporary, or do you intend to leave it as is?
Contributor
Author
There was a problem hiding this comment.
this was added for the test script to check if quic paring was done or not, now we removing the script this is no longer needed
jotak
reviewed
Apr 9, 2026
jotak
reviewed
Apr 9, 2026
jotak
reviewed
Apr 9, 2026
jotak
reviewed
Apr 9, 2026
| if fr.Metrics.QuicMetrics != nil { | ||
| out["QuicVersion"] = fr.Metrics.QuicMetrics.Version | ||
| out["QuicSeenLongHdr"] = fr.Metrics.QuicMetrics.SeenLongHdr | ||
| out["QuicSeenShortHdr"] = fr.Metrics.QuicMetrics.SeenShortHdr |
Member
There was a problem hiding this comment.
so, my previous question was: wdyt if hree we add:
out["TLSVersion"] = "TLS 1.3"
?
It would ensure TLS consistency directly within the flows, without having the queriers to worry about the special cases where that TLS field is set or not
Contributor
Author
There was a problem hiding this comment.
we can hardcode the version here but the tls tracker populate more fields that we won't be able to do here so not sure if that really add much value ?
jotak
reviewed
Apr 9, 2026
jotak
reviewed
Apr 9, 2026
examples/test-quic.sh
Outdated
| @@ -0,0 +1,714 @@ | |||
| #!/usr/bin/env bash | |||
Member
There was a problem hiding this comment.
I must say I'm not super comfortable with a 700 lines bash script, even if it's only for testing...
wondering if it could be done in go? maybe ai can help to quickly translate that?
Contributor
Author
There was a problem hiding this comment.
if we migrate to go I think will be better part of e2e run as this script require having cluster up
Contributor
Author
There was a problem hiding this comment.
for now I removed the script maybe overall e2e will be better place for this, this was good enough for me to test on kind cluster w/o operator
Signed-off-by: Mohamed S. Mahmoud <mmahmoud2201@gmail.com>
msherif1234
force-pushed
the
quic_parsing
branch
from
49a44ca to
3ace014
Compare
|
@msherif1234: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add RFC9312-friendly QUIC detection on any UDP port (opt-in via QUIC_TRACK_ANY_PORT; default remains UDP/443 only).
Harden eBPF QUIC long-header parsing with CID length sanity checks to reduce false positives in any-port mode.
Update docs/manifest and add the QUIC validation test and extent unit-test coverage.
Fixes netobserv/netobserv-operator#2493
Testing
./examples/test-quic.sh
Dependencies
n/a
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.
To run a perfscale test, comment with:
/test ebpf-node-density-heavy-25nodesSummary by CodeRabbit
New Features
Documentation
Tests / Examples
Chores