PR #43 follow-up: rabbit nits + nil guards (batch 1)#46
Open
entlein wants to merge 1 commit into
Open
Conversation
Five small fixes from CodeRabbit's review of PR #43: - profilehelper.go: nil-guard objectCache before dereference. The cpc return value was already guarded but the outer objectCache wasn't, which can panic if a caller passes a nil ObjectCache. - function_cache.go: skip nil extraKeyFn callbacks before invocation. WithCache's variadic param accepted nil entries silently and called fn(values) on them, panicking on dereference. - projection_trie.go: align doc comment with implementation. The trie is rune-keyed (map[rune]*trie) so multi-byte UTF-8 codepoints like the `⋯` token are treated as a single step. Comment claimed byte-level. - README.md: refresh wasExecutedWithArgs behaviour table entry. The prior text described the path-only fallback before PR #43 added the ExecsByPath composite-key surface. Argument matching is now active and wildcard-aware. - tests/resources/network-wildcards/README.md: fix two doc nits in the network-wildcards fixture index — the description of fixture 14 said `dnsNames: ["**"]` but the actual fixture uses `["**.example.com."]`. Also normalise the singular `SBoBs` spelling to `SBOBs` for consistency with the rest of the suite. These are the trivial / minor entries in the review. Critical and major items (argv[0] spoofing in parse.go, ProjectedField.All unconditional match in network.go, blocking sends in rulebinding cache, HTTP method-aware semantics, was_address_port_protocol ignoring port/protocol, ...) need separate dedicated PRs with pinning tests per `feedback_regression_test_harness.md`. They are tracked but intentionally NOT bundled here so this batch can land cleanly without security-sensitive review.
📝 WalkthroughWalkthroughThis PR introduces defensive nil-checks in cache composition and profile initialization to prevent panics, updates documentation to clarify argument matching behavior and internal data structure design, and corrects test fixture descriptions and migration guidance. ChangesDefensive validations and documentation clarifications
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/rulemanager/profilehelper/profilehelper.go (2)
27-40: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding nil-check for consistency.
GetContainerNamedereferencesobjectCacheon line 28 without checking for nil. For consistency with the nil-guard added toGetProjectedContainerProfile(lines 13-15), consider adding the same defensive check here to prevent potential panics.🛡️ Proposed nil-guard
func GetContainerName(objectCache objectcache.ObjectCache, containerID string) string { + if objectCache == nil { + return "" + } sharedData := objectCache.K8sObjectCache().GetSharedContainerData(containerID)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/rulemanager/profilehelper/profilehelper.go` around lines 27 - 40, GetContainerName currently dereferences objectCache without guarding for nil; add the same defensive nil-check used in GetProjectedContainerProfile by returning "" early if objectCache == nil (before calling objectCache.K8sObjectCache().GetSharedContainerData), so that calls to GetContainerName, and subsequent calls to K8sObjectCache() and GetSharedContainerData(containerID), cannot panic when objectCache is nil.
42-54: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding nil-check for consistency.
GetPodSpecdereferencesobjectCacheon line 43 without checking for nil. For consistency with the nil-guard added toGetProjectedContainerProfile(lines 13-15), consider adding the same defensive check here to prevent potential panics.🛡️ Proposed nil-guard
func GetPodSpec(objectCache objectcache.ObjectCache, containerID string) error { + if objectCache == nil { + return nil, errors.New("no object cache available") + } sharedData := objectCache.K8sObjectCache().GetSharedContainerData(containerID)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/rulemanager/profilehelper/profilehelper.go` around lines 42 - 54, Add a nil-guard at the start of GetPodSpec: check if the objectCache parameter is nil and return an appropriate error (consistent with GetProjectedContainerProfile's nil-check) before calling objectCache.K8sObjectCache(), so you avoid panics when dereferencing objectCache; update the error text to something like "object cache is nil" and keep the existing checks for sharedData and podSpec (referencing GetPodSpec, objectCache.K8sObjectCache, and GetProjectedContainerProfile for the pattern).
🤖 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.
Outside diff comments:
In `@pkg/rulemanager/profilehelper/profilehelper.go`:
- Around line 27-40: GetContainerName currently dereferences objectCache without
guarding for nil; add the same defensive nil-check used in
GetProjectedContainerProfile by returning "" early if objectCache == nil (before
calling objectCache.K8sObjectCache().GetSharedContainerData), so that calls to
GetContainerName, and subsequent calls to K8sObjectCache() and
GetSharedContainerData(containerID), cannot panic when objectCache is nil.
- Around line 42-54: Add a nil-guard at the start of GetPodSpec: check if the
objectCache parameter is nil and return an appropriate error (consistent with
GetProjectedContainerProfile's nil-check) before calling
objectCache.K8sObjectCache(), so you avoid panics when dereferencing
objectCache; update the error text to something like "object cache is nil" and
keep the existing checks for sharedData and podSpec (referencing GetPodSpec,
objectCache.K8sObjectCache, and GetProjectedContainerProfile for the pattern).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 07a74fa0-ec35-48b8-bd11-97b410d5f5de
📒 Files selected for processing (5)
README.mdpkg/objectcache/containerprofilecache/projection_trie.gopkg/rulemanager/cel/libraries/cache/function_cache.gopkg/rulemanager/profilehelper/profilehelper.gotests/resources/network-wildcards/README.md
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
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
1 participant
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.
Summary
First batch of CodeRabbit feedback from the merged PR #43 review (
merge: upstream/main + recover wildcards/signing on projection-v1). Scope is the trivial/minor entries only — five small fixes, no behavioural change to security-sensitive paths.Critical and major items from the same review need dedicated PRs (with pinning tests per the project's regression-harness rule) and are tracked separately — see the "Deferred to follow-up PRs" section below.
Fixes in this PR
pkg/rulemanager/profilehelper/profilehelper.goobjectCachebefore dereferencingContainerProfileCache()pkg/rulemanager/cel/libraries/cache/function_cache.goextraKeyFncallbacks inWithCachevariadic to avoid panicpkg/objectcache/containerprofilecache/projection_trie.go⋯)README.mdwasExecutedWithArgstable entry refreshed — argument matching is active post-PR #43 (ExecsByPathcomposite-key surface)tests/resources/network-wildcards/README.md["**.example.com."]);SBoBs→SBOBsValidation
go build ./...cleango test ./pkg/rulemanager/cel/libraries/cache/ ./pkg/objectcache/containerprofilecache/— greenDeferred to follow-up PRs
These need dedicated work + pinning tests; intentionally NOT bundled here so this batch can land cleanly without holding up the trivial fixes.
🔴 Critical
pkg/rulemanager/cel/libraries/parse/parse.go—getExecPathWithExePathtrusts absoluteargv[0]overevent.exepath.argv[0]is process-controlled (caller ofexecvecan set it to anything), so an attacker can present an allowlisted absolute path while executing a different binary. The existing comment defends this but is wrong about the kernel guaranteeingargv[0]faithfulness.pkg/rulemanager/cel/libraries/networkneighborhood/network.go—ProjectedField.Alltreated as unconditionaltruematch; should mean "retain all entries", not "match anything".pkg/objectcache/containerprofilecache/projection.go—projectUserProfilesmay be called on already-projected/stamped ContainerProfiles; need a guard or call-graph audit.🟠 Major
pkg/rulemanager/cel/libraries/applicationprofile/exec.go— Exact-path-match falls through to pattern matching when args don't match, defeating the strict-match semantics.pkg/rulemanager/cel/libraries/applicationprofile/open.go— Usesstrings.HasPrefix/HasSuffixon wildcardisedPatterns; produces false negatives for concrete paths.pkg/rulemanager/cel/libraries/applicationprofile/http.go—was_endpoint_accessed_with_method[s]ignores HTTP method semantics (path-match only).pkg/rulemanager/cel/libraries/networkneighborhood/network.go(Line 181) —was_address_port_protocol_in_*only callsmatchIPField, ignoring port and protocol.pkg/rulebindingmanager/cache/cache.go— Blocking sends in refresh-notifier fan-out + watch handlers; context not propagated through rule-binding branches.cmd/main.go—LoadServiceURLsfailure / empty SBOM URL receiver fails silently.pkg/objectcache/containerprofilecache/projection_apply.go(PolicyByRuleId site, separate from the Args-clone fix already in PR merge: upstream/main + recover wildcards/signing on projection-v1 #43) — Shallow-copy hazard for nested maps.tests/component_test.go—R0001-silence subtests need an explicit "ExecIntoPod succeeded" pre-check before treating silence as evidence the matcher was exercised.go.mod—k8s.io/apiv0.35.x mixed withk8s.io/kubectlv0.34.1; verify compatibility guidance.🟡 Minor / further nits
pkg/objectcache/containerprofilecache/tamper_alert_test.go— Add operational-error coverage path.pkg/objectcache/containerprofilecache/test32_projection_test.go— Pin overlay-removal transition (currently tests add/update only).pkg/rulemanager/cel/libraries/applicationprofile/integration_test.go— Restore flag-aware assertion coverage that the swap to path-only reduced.pkg/rulemanager/cel/libraries/applicationprofile/open_test.go—was_path_opened_with_flagsexpectation flipped to expecttrueon flag mismatch; bakes in path-only semantics under a flag-aware function name.pkg/objectcache/v1/mock.go—Args == nilshould mean "no map entry", not[]string{}; mirror selective endpoint projection instead of forcingEndpoints.All=true.tests/resources/network-wildcards/19-port-protocol-with-cidr.yaml— The fixture documents strict(IP, port, protocol)enforcement which the matcher currently doesn't honour (see the network.go major above). The fixture is correct; the matcher needs to catch up.pkg/objectcache/containerprofilecache/containerprofilecache.go— Document thenudgechannel's buffered-cap-1 coalescing semantics..github/workflows/component-tests.yaml—actions/setup-go@v4→@v5.Test plan
go build ./...passesgo test ./pkg/rulemanager/cel/libraries/cache/ ./pkg/objectcache/containerprofilecache/green