upgrade to IG v0.48.0 #695
Conversation
Signed-off-by: Matthias Bertschy <[email protected]>
📝 WalkthroughWalkthroughUpdates gadget names and image registry/tags to v0.48.0, upgrades many Go module dependencies, removes per-event pooling, and changes tracers to pass direct deep copies via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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)
pkg/containerwatcher/v2/tracers/syscall.go (1)
130-137: Data sharing is protected from Release() but remains vulnerable to mutation by downstream handlers.The shared
event.Datais actually safe from being released during loop iteration becauseSyscallEventTypeevents are explicitly excluded from Release in the worker pool (see container_watcher.go line 162-163). Additionally, the initial data is already deep-copied when the event is created (line 112), so the loop iterations operate on a stable copy.However, the design remains fragile: if any downstream handler mutates (rather than releases) the shared data during callback processing, subsequent loop iterations will see corrupted data. The current safeguard depends on the Release() exclusion logic, which is more of a workaround than a definitive fix.
Consider either:
- Deep-copying the data for each syscall iteration, or
- Documenting that downstream handlers must not mutate the shared data and cannot rely on Release() semantics
🤖 Fix all issues with AI agents
In `@go.mod`:
- Line 447: Add a clear comment above the replace directive for
github.com/inspektor-gadget/inspektor-gadget =>
github.com/matthyx/inspektor-gadget v0.0.0-20260126124411-303be8df37f7 in go.mod
that states which specific bugfixes or features from the matthyx fork are
required (cite commit hashes or PR numbers if possible), whether this replace is
temporary pending an upstream merge or intended long-term, and include a short
tracking plan: the upstream PR/issue to watch, expected timeline or criteria for
switching back to the official module, and who owns monitoring/updating (e.g.,
team or person).
In `@pkg/utils/datasource_event.go`:
- Around line 881-883: The Release method on DatasourceEvent should guard
against a nil Datasource to avoid panics; update the DatasourceEvent.Release
function to check if e.Datasource is nil and return early if so (consistent with
patterns like getFieldAccessor), otherwise call e.Datasource.Release(e.Data);
ensure the nil-check uses the same style/error-handling conventions as other
methods in this file.
🧹 Nitpick comments (4)
pkg/containerwatcher/v2/tracers/ptrace.go (1)
21-21: Inconsistent image versioning: using:latestinstead of pinned version.Same issue as
hardlink.go- using:latesttag instead of a pinned version likedns.gouses.♻️ Suggested fix for version consistency
- ptraceImageName = "ghcr.io/inspektor-gadget/gadget/ptrace:latest" + ptraceImageName = "ghcr.io/inspektor-gadget/gadget/ptrace:v0.48.0"pkg/containerwatcher/v2/tracers/kmod.go (1)
22-22: Consider pinning to a specific version tag for reproducibility.Other tracers in this PR (e.g.,
open.go,exec.go,syscall.go) use the explicitv0.48.0tag, while this uses:latest. Using:latestcan lead to unexpected behavior changes when the image is updated upstream.Suggested fix
- kmodImageName = "ghcr.io/inspektor-gadget/gadget/kmod:latest" + kmodImageName = "ghcr.io/inspektor-gadget/gadget/kmod:v0.48.0"pkg/containerwatcher/v2/tracers/fork.go (1)
21-21: Consider pinning to a specific version tag for reproducibility.Similar to the kmod tracer, this uses
:latestwhile other tracers in this PR use:v0.48.0. Pinning to an explicit version ensures consistent behavior across deployments.Suggested fix
- forkImageName = "ghcr.io/inspektor-gadget/gadget/fork:latest" + forkImageName = "ghcr.io/inspektor-gadget/gadget/fork:v0.48.0"pkg/containerwatcher/v2/tracers/exit.go (1)
21-21: Consider pinning to a specific version tag for reproducibility.Similar to kmod and fork tracers, this uses
:latestwhile other tracers in this PR use:v0.48.0. Pinning to an explicit version ensures consistent behavior across deployments.Suggested fix
- exitImageName = "ghcr.io/inspektor-gadget/gadget/exit:latest" + exitImageName = "ghcr.io/inspektor-gadget/gadget/exit:v0.48.0"
| ) | ||
|
|
||
| replace github.com/inspektor-gadget/inspektor-gadget => github.com/matthyx/inspektor-gadget v0.0.0-20251216142151-fe5be153f203 | ||
| replace github.com/inspektor-gadget/inspektor-gadget => github.com/matthyx/inspektor-gadget v0.0.0-20260126124411-303be8df37f7 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check for any references to the fork or explanation in the codebase
rg -i "matthyx" . --type=md --type=yaml --type=toml 2>/dev/null | head -20
# Check if there are comments near the replace directive in go.mod
rg -B 3 -A 3 "matthyx/inspektor-gadget" go.mod
# Look for any explanations in code comments or documentation
rg -i "fork" . --type=md --type=go -B 2 -A 2 2>/dev/null | head -40Repository: kubescape/node-agent
Length of output: 3137
🏁 Script executed:
# Read the go.mod file around line 447 to check for comments
sed -n '440,455p' go.mod
# Check for any changelog or documentation about this
find . -name "CHANGELOG*" -o -name "RELEASE*" -o -name "HISTORY*" | head -5
# Look for any comments in go.mod that might explain the fork
rg "//.*fork|//.*inspektor" go.mod
# Check recent git history if available
git log --oneline go.mod 2>/dev/null | head -10Repository: kubescape/node-agent
Length of output: 461
🏁 Script executed:
# Get the full commit message
git show 17f9530 --no-patch --format=fuller 2>/dev/null || echo "Git show failed"
# Get the diff for go.mod changes in that commit
git show 17f9530 -- go.mod 2>/dev/null | head -80
# Check if there are any comments or documentation about why the fork is needed
rg "inspektor-gadget|IG|v0.48" docs/ --type=md 2>/dev/null | head -20
# Look for any PR or issue references
rg "#[0-9]{3,}" . --type=md --type=go -B 2 -A 2 2>/dev/null | grep -i "inspektor\|fork\|IG" | head -10Repository: kubescape/node-agent
Length of output: 4169
Document the fork's purpose and add a tracking plan for official release adoption.
The inspektor-gadget dependency is replaced with a fork (matthyx/inspektor-gadget) via a replace directive at line 447. While the fork appears intentional (authored by core contributor matthyx), there is no documentation explaining why the fork is necessary or when it can be replaced with an official release. Add a comment in go.mod clarifying:
- What specific features or fixes the fork provides that aren't available in the official release
- Whether this is temporary (pending upstream merge) or long-term
- Expected timeline for adopting the official release if applicable
🤖 Prompt for AI Agents
In `@go.mod` at line 447, Add a clear comment above the replace directive for
github.com/inspektor-gadget/inspektor-gadget =>
github.com/matthyx/inspektor-gadget v0.0.0-20260126124411-303be8df37f7 in go.mod
that states which specific bugfixes or features from the matthyx fork are
required (cite commit hashes or PR numbers if possible), whether this replace is
temporary pending an upstream merge or intended long-term, and include a short
tracking plan: the upstream PR/issue to watch, expected timeline or criteria for
switching back to the official module, and who owns monitoring/updating (e.g.,
team or person).
| func (e *DatasourceEvent) Release() { | ||
| pool, loaded := dataPools.Load(e.EventType) | ||
| if loaded { | ||
| pool.(*sync.Pool).Put(e.Data) | ||
| } | ||
| e.Datasource.Release(e.Data) | ||
| } |
There was a problem hiding this comment.
Missing nil check on Datasource before calling Release.
If e.Datasource is nil, this will panic. Other methods in this file (e.g., getFieldAccessor at line 168) check for nil Datasource. Consider adding a guard here for consistency and safety.
🛡️ Suggested fix
func (e *DatasourceEvent) Release() {
+ if e.Datasource == nil {
+ return
+ }
e.Datasource.Release(e.Data)
}📝 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.
| func (e *DatasourceEvent) Release() { | |
| pool, loaded := dataPools.Load(e.EventType) | |
| if loaded { | |
| pool.(*sync.Pool).Put(e.Data) | |
| } | |
| e.Datasource.Release(e.Data) | |
| } | |
| func (e *DatasourceEvent) Release() { | |
| if e.Datasource == nil { | |
| return | |
| } | |
| e.Datasource.Release(e.Data) | |
| } |
🤖 Prompt for AI Agents
In `@pkg/utils/datasource_event.go` around lines 881 - 883, The Release method on
DatasourceEvent should guard against a nil Datasource to avoid panics; update
the DatasourceEvent.Release function to check if e.Datasource is nil and return
early if so (consistent with patterns like getFieldAccessor), otherwise call
e.Datasource.Release(e.Data); ensure the nil-check uses the same
style/error-handling conventions as other methods in this file.
Signed-off-by: Matthias Bertschy <[email protected]>
Signed-off-by: Matthias Bertschy <[email protected]>
… to EnrichEvent interface and related types
Feature/ecs alerts
Summary by CodeRabbit
Updates
Performance
Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.