feat(docs): improve docs throughout collector#3604
Draft
Stringy wants to merge 1 commit into
Draft
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3604 +/- ##
=======================================
Coverage 27.34% 27.34%
=======================================
Files 95 95
Lines 5420 5420
Branches 2545 2545
=======================================
Hits 1482 1482
Misses 3211 3211
Partials 727 727
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change expands documentation across build files, collector runtime components, container packaging, and integration tests. It also adds explicit ChangesCollector documentation and build configuration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 15
🧹 Nitpick comments (1)
integration-tests/pkg/collector/collector.go (1)
7-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
Mountssemantics or narrow the claim.The comment says
Mounts,Env, andConfigfollow different code paths, but only explainsEnvandConfig. Add the host/container path semantics forMounts, or remove it from that claim.As per path instructions: focus on major issues impacting performance, readability, maintainability and security; avoid nitpicks and avoid verbosity.
🤖 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 `@integration-tests/pkg/collector/collector.go` around lines 7 - 17, Update the StartupOptions documentation to explain that Mounts maps host paths to container paths, or remove Mounts from the statement about settings following different code paths. Keep the existing Env and Config semantics documented.Source: Path instructions
🤖 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 `@collector/CMakeLists.txt`:
- Around line 117-122: Reword the comments around BUILD_SHARED_LIBS and
LIBELF_LIB_SUFFIX to state that project libraries default to static linkage
while required shared runtime dependencies remain supported, avoiding any
implication that the binary is fully static. Update the BUILD_SHARED_LIBS cache
help text to describe static/shared linkage rather than position-independent
code.
In `@collector/container/Dockerfile`:
- Around line 48-50: Update the Dockerfile comment above the COPY command to
describe self-checks only as a separate startup-validation executable, removing
the claim that it is independent from the full collector dependency tree. Do not
change the executable or copy behavior unless the link graph is intentionally
updated elsewhere.
In `@collector/lib/ConfigLoader.h`:
- Around line 180-185: Update the documentation comment above WatchFile to
describe the watchers as up to three categories rather than stating that all
three are always maintained. Clarify that LOADER_CONFIG_FILE is created only
when the file exists and LOADER_CONFIG_REALPATH only applies to symlink targets,
while retaining the existing category descriptions.
In `@collector/lib/HostHeuristics.cpp`:
- Around line 76-77: Update the comment near the Power heuristic condition to
describe the actual predicate: ppc64le systems running kernel 4.18 builds below
477. Do not imply RHEL 8.6-specific validation unless the implementation is also
changed to verify the distribution.
In `@collector/lib/NetworkSignalHandler.cpp`:
- Around line 15-18: Update the comment describing Modifier::ADD near
HandleSignal to state that events add or refresh tracked connections, while the
removal case handles closed sockets. Keep the explanation that this enum drives
HandleSignal’s decision without implying ADD only represents newly established
connections.
In `@collector/lib/NetworkStatusNotifier.h`:
- Around line 26-27: Update the class documentation comment in
NetworkStatusNotifier to state that arena allocation occurs only when
USE_PROTO_ARENAS is enabled, and that HeapProtoAllocator is used otherwise. Keep
the documentation aligned with the existing ProtoAllocator alias behavior.
- Around line 22-24: Update NetworkStatusNotifier::OnRecvControlMessage() so the
branch calling ReceivePublicIPs() checks the public-IP field’s presence, not
has_ip_networks(). Keep the ReceiveIPNetworks() branch guarded by the network
field and preserve the existing handling for messages containing either or both
fields.
In `@collector/lib/RateLimit.h`:
- Around line 9-14: Clarify the `TokenBucket::last_time` member comment to
consistently describe its actual timestamp semantics, matching how refill
calculations use it. Remove the conflicting elapsed-time wording while
preserving the existing `TokenBucket` fields and units.
In `@collector/lib/SelfCheckHandler.h`:
- Around line 19-25: The self-check documentation overstates verification and
startup failure behavior. In collector/lib/SelfCheckHandler.h lines 19-25,
describe the guarantee as matching events reaching the collector rather than
proving the full event pipeline; in collector/self-checks.cpp lines 12-20,
remove or qualify the statement that startup aborts so it reflects the timeout
path logging a warning and returning FINISHED.
In `@collector/self-checks.cpp`:
- Around line 57-60: Update the select() call in the listener self-check to pass
listener + 1 instead of 1, ensuring the listener descriptor is monitored.
Preserve the existing FD_ISSET(listener, &sockets) validation before calling
accept().
In `@integration-tests/pkg/collector/collector.go`:
- Around line 21-23: Update documentation at
integration-tests/pkg/collector/collector.go:21-23 to describe Kubernetes
support as future unless an implementation exists; revise the New documentation
at integration-tests/pkg/collector/collector.go:35-37 to state that it returns
the default manager and delegates execution to the supplied executor; align
integration-tests/suites/base.go:40-48 with current Docker/Podman/CRI support,
explicitly marking Kubernetes as future if mentioned.
In `@integration-tests/pkg/executor/executor.go`:
- Around line 75-77: Update New to explicitly accept only the supported runtime
commands "docker", "podman", and the intended CRI values; return an error for
any unknown or misspelled command instead of routing it to newCriExecutor.
Preserve the existing executor selection for valid runtimes.
In `@integration-tests/pkg/mock_sensor/ring.go`:
- Around line 3-7: The ring buffer implementation around RingChan must make both
oldest-value eviction and new-value enqueue non-blocking, including the gap
between detecting a full output channel and removing an item; update its
send/receive operations accordingly. In
integration-tests/pkg/mock_sensor/server.go lines 62-64, revise the channel
comments to claim blocking prevention only under the ring’s corrected
non-blocking behavior.
In `@integration-tests/pkg/mock_sensor/server.go`:
- Around line 28-31: Update the comment above gDefaultRingSize to describe 32
slots as buffering typical event bursts between scrape intervals, and explicitly
state that older values may be dropped when the ring reaches capacity; do not
claim the buffer prevents drops.
In `@integration-tests/suites/async_connections.go`:
- Around line 74-76: Correct the comment and test setup around the unreachable
address in the async connection test: do not describe 10.255.255.1 as RFC 5737
documentation space. Replace it with a reliably blackholed route/firewall-based
simulation, or explicitly document the environment-specific assumption if
retaining the address.
---
Nitpick comments:
In `@integration-tests/pkg/collector/collector.go`:
- Around line 7-17: Update the StartupOptions documentation to explain that
Mounts maps host paths to container paths, or remove Mounts from the statement
about settings following different code paths. Keep the existing Env and Config
semantics documented.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: dcddd241-e848-4423-a697-74504ba61116
📒 Files selected for processing (48)
CMakeLists.txtMakefilecollector/CMakeLists.txtcollector/collector.cppcollector/container/Dockerfilecollector/container/status-check.shcollector/lib/CMakeLists.txtcollector/lib/CollectorService.cppcollector/lib/CollectorService.hcollector/lib/ConfigLoader.hcollector/lib/ConnTracker.cppcollector/lib/ConnTracker.hcollector/lib/DuplexGRPC.hcollector/lib/ExternalIPsConfig.hcollector/lib/HostHeuristics.cppcollector/lib/HostHeuristics.hcollector/lib/HostInfo.hcollector/lib/Inotify.hcollector/lib/NRadix.hcollector/lib/NetworkConnection.hcollector/lib/NetworkSignalHandler.cppcollector/lib/NetworkSignalHandler.hcollector/lib/NetworkStatusNotifier.cppcollector/lib/NetworkStatusNotifier.hcollector/lib/ProcessSignalHandler.cppcollector/lib/ProcfsScraper.hcollector/lib/ProtoAllocator.hcollector/lib/RateLimit.hcollector/lib/SelfCheckHandler.cppcollector/lib/SelfCheckHandler.hcollector/lib/SignalHandler.hcollector/lib/StoppableThread.hcollector/lib/system-inspector/Service.cppcollector/lib/system-inspector/Service.hcollector/self-checks.cppintegration-tests/pkg/collector/collector.gointegration-tests/pkg/executor/executor.gointegration-tests/pkg/mock_sensor/ring.gointegration-tests/pkg/mock_sensor/server.gointegration-tests/suites/async_connections.gointegration-tests/suites/base.gointegration-tests/suites/connections_and_endpoints.gointegration-tests/suites/listening_ports.gointegration-tests/suites/processes_and_endpoints.gointegration-tests/suites/procfs_scraper.gointegration-tests/suites/repeated_network_flow.gointegration-tests/suites/runtime_config_file.gointegration-tests/suites/threads.go
Comment on lines
+117
to
122
| # Collector ships as a statically-linked binary in a minimal UBI-micro | ||
| # container — dynamic linking would require shipping .so files. | ||
| set(BUILD_SHARED_LIBS OFF CACHE BOOL "Build position independent libraries and executables" FORCE) | ||
| # Exception to static linking: libbpf dlopen()s libelf at runtime to parse | ||
| # BPF ELF objects, so libelf must remain a shared library. | ||
| set(LIBELF_LIB_SUFFIX ".so" CACHE STRING "Use libelf.so" FORCE) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
rg -n 'BUILD_SHARED_LIBS|LIBELF_LIB_SUFFIX|target_link_libraries|dnf install|libstdc\+\+|openssl|tbb' \
collector/CMakeLists.txt collector/lib/CMakeLists.txt collector/container/DockerfileRepository: stackrox/collector
Length of output: 1860
🏁 Script executed:
#!/bin/sh
set -eu
sed -n '108,126p' collector/CMakeLists.txt
printf '\n---\n'
sed -n '1,40p' collector/container/DockerfileRepository: stackrox/collector
Length of output: 3098
Reword this to avoid implying a fully static binary. BUILD_SHARED_LIBS only changes the default for project libraries; the image still ships shared runtime deps like libstdc++, openssl, tbb, and libelf remains shared. The cache help text should also describe static/shared linkage, not position-independent code.
🤖 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 `@collector/CMakeLists.txt` around lines 117 - 122, Reword the comments around
BUILD_SHARED_LIBS and LIBELF_LIB_SUFFIX to state that project libraries default
to static linkage while required shared runtime dependencies remain supported,
avoiding any implication that the binary is fully static. Update the
BUILD_SHARED_LIBS cache help text to describe static/shared linkage rather than
position-independent code.
Source: Path instructions
Comment on lines
+48
to
50
| # self-checks is a separate binary for BPF driver validation at startup, | ||
| # kept independent from the full collector dependency tree. | ||
| COPY container/bin/collector /usr/local/bin/ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the self-checks dependency claim.
collector/lib/CMakeLists.txt states that collector_lib is linked by all three executables, so copying self-checks separately does not make it independent from the collector dependency tree. Describe it as a separate startup-validation executable unless its link graph is also changed.
As per path instructions, this is a cross-layer maintainability issue with release-time dependency implications, not a wording nit.
🤖 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 `@collector/container/Dockerfile` around lines 48 - 50, Update the Dockerfile
comment above the COPY command to describe self-checks only as a separate
startup-validation executable, removing the claim that it is independent from
the full collector dependency tree. Do not change the executable or copy
behavior unless the link graph is intentionally updated elsewhere.
Source: Path instructions
Comment on lines
+180
to
+185
| /// Handles both direct files and symlinks (Kubernetes ConfigMaps are mounted | ||
| /// as symlink chains that get atomically swapped, which generates different | ||
| /// inotify events than a simple file write). Three watchers are maintained: | ||
| /// - LOADER_PARENT_PATH: directory-level events (create/delete of the file) | ||
| /// - LOADER_CONFIG_FILE: events on the config file/symlink itself | ||
| /// - LOADER_CONFIG_REALPATH: events on the symlink target (for ConfigMap swaps) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document these as up to three watcher categories.
WatchFile() only creates the realpath watcher for symlinks, and it does not create the config-file watcher until the file exists. Saying “Three watchers are maintained” is therefore inaccurate for direct-file and missing-file configurations.
Proposed wording
-/// as symlink chains that get atomically swapped, which generates different
-/// inotify events than a simple file write). Three watchers are maintained:
+/// as symlink chains that get atomically swapped, which generates different
+/// inotify events than a simple file write). Up to three watcher categories
+/// are used:
...
-/// - LOADER_CONFIG_REALPATH: events on the symlink target (for ConfigMap swaps)
+/// - LOADER_CONFIG_REALPATH: events on the symlink target, when the config
+/// path is a symlink (for ConfigMap swaps)📝 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
| /// Handles both direct files and symlinks (Kubernetes ConfigMaps are mounted | |
| /// as symlink chains that get atomically swapped, which generates different | |
| /// inotify events than a simple file write). Three watchers are maintained: | |
| /// - LOADER_PARENT_PATH: directory-level events (create/delete of the file) | |
| /// - LOADER_CONFIG_FILE: events on the config file/symlink itself | |
| /// - LOADER_CONFIG_REALPATH: events on the symlink target (for ConfigMap swaps) | |
| /// Handles both direct files and symlinks (Kubernetes ConfigMaps are mounted | |
| /// as symlink chains that get atomically swapped, which generates different | |
| /// inotify events than a simple file write). Up to three watcher categories | |
| /// are used: | |
| /// - LOADER_PARENT_PATH: directory-level events (create/delete of the file) | |
| /// - LOADER_CONFIG_FILE: events on the config file/symlink itself | |
| /// - LOADER_CONFIG_REALPATH: events on the symlink target, when the config | |
| /// path is a symlink (for ConfigMap swaps) |
🤖 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 `@collector/lib/ConfigLoader.h` around lines 180 - 185, Update the
documentation comment above WatchFile to describe the watchers as up to three
categories rather than stating that all three are always maintained. Clarify
that LOADER_CONFIG_FILE is created only when the file exists and
LOADER_CONFIG_REALPATH only applies to symlink targets, while retaining the
existing category descriptions.
Comment on lines
+76
to
+77
| // Early RHEL 8.6 kernels on ppc64le have BPF verifier bugs. | ||
| // Block those builds rather than letting users hit cryptic errors. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the Power heuristic comment match its actual condition.
The implementation only checks ppc64le and kernel 4.18 builds below 477 (Lines [83]-[88]); it does not verify RHEL 8.6. Either add distro validation or describe the architecture/version predicate instead of implying RHEL-only behavior.
🤖 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 `@collector/lib/HostHeuristics.cpp` around lines 76 - 77, Update the comment
near the Power heuristic condition to describe the actual predicate: ppc64le
systems running kernel 4.18 builds below 477. Do not imply RHEL 8.6-specific
validation unless the implementation is also changed to verify the distribution.
Comment on lines
+15
to
+18
| // Each syscall event either adds a connection to the tracker (the | ||
| // connection was just established) or removes one (the socket was | ||
| // closed). This enum drives that decision so HandleSignal does not | ||
| // need a per-event-type switch. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe Modifier::ADD as a tracking update, not only a new connection.
ADD is also used for getsockopt and send/receive events (Lines [32]-[43]), which may refresh existing state rather than establish a socket. Update the comment to say that events add or refresh tracked connections.
🤖 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 `@collector/lib/NetworkSignalHandler.cpp` around lines 15 - 18, Update the
comment describing Modifier::ADD near HandleSignal to state that events add or
refresh tracked connections, while the removal case handles closed sockets. Keep
the explanation that this enum drives HandleSignal’s decision without implying
ADD only represents newly established connections.
Comment on lines
+21
to
+23
| // Manager abstracts the collector container lifecycle so that the same | ||
| // test suites can run against different execution backends (e.g. Docker, | ||
| // Kubernetes) without changing test logic. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align runtime-abstraction documentation across layers.
The changed comments mix current Docker/Podman/CRI support with future Kubernetes support and describe the current collector factory as Docker-based despite accepting an abstract executor.
integration-tests/pkg/collector/collector.go#L21-L23: describe Kubernetes as future support unless a backend exists.integration-tests/pkg/collector/collector.go#L35-L37: describeNewas returning the default manager and delegating execution to the supplied executor.integration-tests/suites/base.go#L40-L48: use Docker/Podman/CRI, or explicitly label Kubernetes as future support.
📍 Affects 2 files
integration-tests/pkg/collector/collector.go#L21-L23(this comment)integration-tests/pkg/collector/collector.go#L35-L37integration-tests/suites/base.go#L40-L48
🤖 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 `@integration-tests/pkg/collector/collector.go` around lines 21 - 23, Update
documentation at integration-tests/pkg/collector/collector.go:21-23 to describe
Kubernetes support as future unless an implementation exists; revise the New
documentation at integration-tests/pkg/collector/collector.go:35-37 to state
that it returns the default manager and delegates execution to the supplied
executor; align integration-tests/suites/base.go:40-48 with current
Docker/Podman/CRI support, explicitly marking Kubernetes as future if mentioned.
Source: Path instructions
Comment on lines
+75
to
+77
| // New creates the appropriate executor based on the configured container | ||
| // runtime. Docker and Podman use the Docker-compatible API client, while | ||
| // CRI (containerd, crio) uses the CRI gRPC API directly. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
rg -n -C 3 'RuntimeInfo\(\)|Command.*(docker|podman|containerd|crio)|newCriExecutor' .Repository: stackrox/collector
Length of output: 3986
🏁 Script executed:
#!/bin/sh
set -e
printf '\n## integration-tests/pkg/config/config.go\n'
sed -n '1,220p' integration-tests/pkg/config/config.go
printf '\n## integration-tests/pkg/executor/executor.go\n'
sed -n '1,140p' integration-tests/pkg/executor/executor.go
printf '\n## all references to runtime command values\n'
rg -n 'runtimeDefaultCommand|envRuntimeCommand|Command:|RuntimeInfo\(\)\.Command|== "docker"|== "podman"|containerd|crio' integration-tests/pkgRepository: stackrox/collector
Length of output: 8346
Reject unsupported runtime commands. New() sends any value other than "docker" or "podman" to newCriExecutor(), so typos or unexpected RUNTIME_COMMAND values silently fall into the CRI path. Return an error for unknown values, or make the fallback explicit in the docs.
🤖 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 `@integration-tests/pkg/executor/executor.go` around lines 75 - 77, Update New
to explicitly accept only the supported runtime commands "docker", "podman", and
the intended CRI values; return an error for any unknown or misspelled command
instead of routing it to newCriExecutor. Preserve the existing executor
selection for valid runtimes.
Source: Path instructions
Comment on lines
+3
to
+7
| // RingChan is a non-blocking ring buffer channel. When the output channel | ||
| // is full, the oldest unread value is dropped to make room for the new one. | ||
| // This prevents gRPC event handlers from blocking when tests consume events | ||
| // slower than they arrive — critical because blocking the gRPC handler would | ||
| // stall the entire collector→sensor stream. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not promise a non-blocking stream until eviction is non-blocking.
The ring implementation can block between checking a full output channel and removing its oldest value, so both comments currently overstate the availability guarantee.
integration-tests/pkg/mock_sensor/ring.go#L3-L7: make eviction and enqueue use non-blocking operations.integration-tests/pkg/mock_sensor/server.go#L62-L64: describe the channels as preventing blocking only after the ring implementation is fixed.
📍 Affects 2 files
integration-tests/pkg/mock_sensor/ring.go#L3-L7(this comment)integration-tests/pkg/mock_sensor/server.go#L62-L64
🤖 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 `@integration-tests/pkg/mock_sensor/ring.go` around lines 3 - 7, The ring
buffer implementation around RingChan must make both oldest-value eviction and
new-value enqueue non-blocking, including the gap between detecting a full
output channel and removing an item; update its send/receive operations
accordingly. In integration-tests/pkg/mock_sensor/server.go lines 62-64, revise
the channel comments to claim blocking prevention only under the ring’s
corrected non-blocking behavior.
Source: Path instructions
Comment on lines
+28
to
31
| // Ring channel size for live event streaming to test assertions. Small | ||
| // enough to avoid excessive memory use but large enough to buffer events | ||
| // between scrape intervals without dropping. | ||
| gDefaultRingSize = 32 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not document the 32-slot ring as lossless.
RingChan drops the oldest value when full, so “large enough ... without dropping” is not guaranteed. Describe 32 as a typical-burst buffer and explicitly note that values may be dropped when capacity is exceeded.
As per path instructions: focus on major issues impacting performance, readability, maintainability and security; avoid nitpicks and avoid verbosity.
🤖 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 `@integration-tests/pkg/mock_sensor/server.go` around lines 28 - 31, Update the
comment above gDefaultRingSize to describe 32 slots as buffering typical event
bursts between scrape intervals, and explicitly state that older values may be
dropped when the ring reaches capacity; do not claim the buffer prevents drops.
Source: Path instructions
Comment on lines
+74
to
+76
| // RFC 5737 documentation range — guaranteed non-routable, so the | ||
| // kernel's connect() will hang in SYN_SENT until it times out, | ||
| // simulating a firewall-blocked connection. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
10.255.255.1 is not RFC 5737 documentation space. It's RFC 1918 private space, so connect() can fail immediately depending on the container network. Use a blackholed route/firewall rule, or document the environment-specific assumption.
🤖 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 `@integration-tests/suites/async_connections.go` around lines 74 - 76, Correct
the comment and test setup around the unreachable address in the async
connection test: do not describe 10.255.255.1 as RFC 5737 documentation space.
Replace it with a reliably blackholed route/firewall-based simulation, or
explicitly document the environment-specific assumption if retaining the
address.
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
A detailed explanation of the changes in your PR.
Feel free to remove this section if it is overkill for your PR, and the title of your PR is sufficiently descriptive.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.
For more details, ref the Confluence page about this section.