profile-compaction: CollapseConfig CRD + projection overlay + user-maged lifecycle#325
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds CollapseConfiguration API support, runtime collapse-settings translation and wiring, wildcard-aware IP/DNS matching with admission validation, server startup logging and WatchList handling updates, idle watch behavior changes, and annotation-based cleanup skipping. ChangesDynamic Path Collapse Configuration and Network Matching
Server Startup and Watch Behavior
User-Managed Cleanup Handling
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
🧩 Analysis chain
🏁 Script executed: Length of output: 193 🏁 Script executed: Length of output: 208
Atomicity Assessment —
|
| Component | Justification |
|---|---|
| CRD types + v1beta1 conversion | Core — inseparable |
| Generated deepcopy / protobuf / openapi | Codegen artifacts — must travel with the type definition |
| Generated clientset / informers / listers | Codegen artifacts — must travel with the type definition |
| etcd registration | Required to make the resource storable — inseparable from CRD introduction |
applicationprofile_processor + containerprofile_processor |
First consumer in this repo — reasonable to bundle here |
pkg/registry/file/cleanup.go |
Logically cohesive (user-managed lifecycle gate) but is a separable concern |
artifacts/collapseconfiguration-default-sample.yaml |
Low-risk, conventional to ship with the CRD |
The only split worth flagging is that cleanup.go is a behavioral policy change independent of the CRD API surface. If you ever need to bisect a regression, that separation could help — but it is not a blocking concern.
Cross-PR Coupling Flags
🔴 Hard compile-time dependency: #325 → #323
applicationprofile_processor and containerprofile_processor in #325 import and call the CollapseConfig provider, whose Go types are introduced in #323. #325 cannot compile without #323's types in the target branch. This is your expected and correctly modeled Tier 0 → Tier 2 dependency.
Merge gate: #323 must be merged (or squashed per your advisory) before #325 can target main.
🟡 Same-tier ordering: #325 and #322
Both are Tier 2. If the processors in #325 reference exec-related types added by #322 (e.g., CompareExecArgs), you have an intra-tier ordering constraint. If the processors only depend on CollapseConfig (#323) and existing types, #322 and #325 are independently mergeable in any order after #323.
Recommend verifying: Do applicationprofile_processor or containerprofile_processor in #325 import anything from #322's CompareExecArgs surface? If yes, #322 must precede #325.
🟡 Cross-repo dependency: #325 → node-agent#808
The projection overlay in node-agent#808 presumably consumes the CollapseConfig CRD registered by #325. This is your expected Tier 2 → Tier 4 dependency. No action needed in #325 itself, but node-agent#808 cannot target a release that doesn't include #325.
✅ No coupling detected: #325 ↔ #324 (networkmatch)
The networkmatch package (Tier 3) operates on a distinct resource surface and shares no apparent file-level overlap with the CollapseConfig CRD or the exec processors.
Summary Table
`#323` (Tier 0) ──► `#325` (Tier 2) ──► node-agent#808 (Tier 4)
│
possible order dep
│
`#322` (Tier 2)
The tier structure is correct. The one item to double-check before merging #325 is whether its processors import any symbol from #322 — if so, serialize #322 before #325 within Tier 2.
✅ Actions performed
Full review triggered.
| *out = new(metav1.LabelSelector) | ||
| (*in).DeepCopyInto(*out) | ||
| } | ||
| if in.IPAddresses != nil { |
There was a problem hiding this comment.
Blocking: this generated deepcopy now expects NetworkNeighbor.IPAddresses, but both internal and v1beta1 NetworkNeighbor still expose IPAddress (singular). go test ./pkg/apis/softwarecomposition fails immediately here. Unless the field rename is included in this PR/rebased prerequisite, this codegen drift leaves the branch unbuildable.
| func deflateApplicationProfileContainer(container softwarecomposition.ApplicationProfileContainer, sbomSet mapset.Set[string]) softwarecomposition.ApplicationProfileContainer { | ||
| opens, err := dynamicpathdetector.AnalyzeOpens(container.Opens, dynamicpathdetector.NewPathAnalyzer(OpenDynamicThreshold), sbomSet) | ||
| func deflateApplicationProfileContainer(container softwarecomposition.ApplicationProfileContainer, sbomSet mapset.Set[string], settings dynamicpathdetector.CollapseSettings) softwarecomposition.ApplicationProfileContainer { | ||
| opens, err := dynamicpathdetector.AnalyzeOpens(container.Opens, dynamicpathdetector.NewPathAnalyzerWithConfigs(settings.OpenDynamicThreshold, settings.CollapseConfigs), sbomSet) |
There was a problem hiding this comment.
Blocking: this PR now calls dynamicpathdetector.NewPathAnalyzerWithConfigs(...), but that symbol (along with CollapseConfig, DefaultCollapseConfigs, and the threshold constants used by collapse_config_from_crd.go) does not exist on current main. As-is, the branch still depends on an unmerged prerequisite and cannot compile until those path-wildcards changes are rebased/cherry-picked in.
| ) | ||
| apiGroupInfo.VersionedResourcesStorageMap["v1beta1"] = map[string]rest.Storage{ | ||
| "applicationprofiles": ep(applicationprofile.NewREST, applicationProfileStorageImpl), | ||
| "collapseconfigurations": ep(collapseconfiguration.NewREST), |
There was a problem hiding this comment.
Blocking: registering the CRD endpoint here does not wire it into the live deflate path. applicationProfileStorageImpl and containerProfileStorageImpl are still constructed with the default collapse-settings providers, and I cannot find any non-test call to SetCollapseSettings(...) / assignment to CollapseSettings. So CollapseConfiguration/default is stored, but never consulted when profiles are compacted; applying the sample manifest is a no-op.
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
I found three merge blockers:
- the generated deepcopy drift now references
NetworkNeighbor.IPAddresseswhile the handwritten API types still exposeIPAddress, so the branch does not build; - this branch calls dynamicpathdetector APIs that are not on current
main(NewPathAnalyzerWithConfigs,CollapseConfig,DefaultCollapseConfigs, etc.), so it still depends on an unmerged prerequisite; - the new
CollapseConfigurationresource is registered, but the application/container profile processors still use compiled-in defaults, so the CRD is never read in production and the sample manifest is a no-op.
Please fix those before merge.
entlein
force-pushed
the
upstream-pr/sbob-crd-compaction
branch
2 times, most recently
from
7cba45a to
69c895e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/registry/file/applicationprofile_processor.go (1)
85-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSnapshot collapse settings once per
PreSaveexecution.Line 105 resolves settings inside the per-container loop. With a CRD-backed provider, this can trigger repeated storage lookups and can apply mixed thresholds within the same profile if config changes mid-iteration. Resolve once and reuse for all containers in this save pass.
Proposed fix
// size is the sum of all fields in all containers var size int + settings := a.effectiveCollapseSettings() // Define a function to process a slice of containers processContainers := func(containers []softwarecomposition.ApplicationProfileContainer) []softwarecomposition.ApplicationProfileContainer { for i, container := range containers { @@ - containers[i] = deflateApplicationProfileContainer(container, sbomSet, a.effectiveCollapseSettings()) + containers[i] = deflateApplicationProfileContainer(container, sbomSet, settings) size += len(containers[i].Execs)🤖 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/registry/file/applicationprofile_processor.go` around lines 85 - 106, The code calls a.effectiveCollapseSettings() inside the per-container loop (in processContainers) causing repeated CRD/storage lookups and potential mixed thresholds; fix by resolving the collapse settings once at the start of the PreSave/save pass (e.g., capture into a local variable like collapseSettings before creating/entering processContainers) and then pass that single collapseSettings value into deflateApplicationProfileContainer instead of calling a.effectiveCollapseSettings() for each container so all containers use the same resolved settings for the entire save operation.
🧹 Nitpick comments (1)
pkg/registry/file/applicationprofile_processor_test.go (1)
21-25: ⚡ Quick winDerive
/usr/libthreshold from active collapse configs instead of the global constant.These tests currently assume
/usr/libusesOpenDynamicThreshold. If a prefix-specific override is added, assertions will fail for the wrong reason. Match the/etcpattern and read threshold viaFindConfigForPath("/usr/lib/...").Proposed refactor
-func openThreshold() int { - return dynamicpathdetector.OpenDynamicThreshold +func openThresholdForPath(path string) int { + analyzer := dynamicpathdetector.NewPathAnalyzerWithConfigs( + dynamicpathdetector.OpenDynamicThreshold, + dynamicpathdetector.DefaultCollapseConfigs(), + ) + return analyzer.FindConfigForPath(path).Threshold } @@ - numOpens := openThreshold() + 1 + numOpens := openThresholdForPath("/usr/lib/x86_64-linux-gnu/libX.so.1") + 1 @@ - numOpens := openThreshold() + 1 + numOpens := openThresholdForPath("/usr/lib/x86_64-linux-gnu/libX.so.1") + 1 @@ - usrLibThreshold := openThreshold() + usrLibThreshold := openThresholdForPath("/usr/lib/libX.so") @@ - numOpens := openThreshold() + 1 + numOpens := openThresholdForPath("/usr/lib/x86_64-linux-gnu/libX.so.1") + 1Also applies to: 273-275, 303-304, 338-340, 420-421
🤖 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/registry/file/applicationprofile_processor_test.go` around lines 21 - 25, Tests currently hardcode dynamicpathdetector.OpenDynamicThreshold for /usr/lib paths; update them to derive the threshold from active collapse configs like the /etc pattern by calling dynamicpathdetector.FindConfigForPath("/usr/lib/...") (or the package's exported FindConfigForPath) and reading the config's collapse threshold instead of using OpenDynamicThreshold; change the openThreshold helper (and other occurrences referencing dynamicpathdetector.OpenDynamicThreshold) to fetch the config for "/usr/lib/..." and return its threshold, and apply the same replacement for the other test spots that currently assume the global constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/registry/file/collapse_config_provider.go`:
- Around line 71-73: Replace the unbounded context used in s.Get with a bounded
context so storage reads can't hang indefinitely: create a short timeout context
(e.g., via context.WithTimeout) before calling s.Get, defer the cancel, and pass
that ctx into s.Get; keep the existing error/empty-name check and return
dynamicpathdetector.DefaultCollapseSettings() on error or deadline. Ensure you
update the call site where s.Get is invoked (the s.Get(...) call that populates
crd) and leave the rest of the function (including use of crd and
dynamicpathdetector.DefaultCollapseSettings) unchanged.
---
Outside diff comments:
In `@pkg/registry/file/applicationprofile_processor.go`:
- Around line 85-106: The code calls a.effectiveCollapseSettings() inside the
per-container loop (in processContainers) causing repeated CRD/storage lookups
and potential mixed thresholds; fix by resolving the collapse settings once at
the start of the PreSave/save pass (e.g., capture into a local variable like
collapseSettings before creating/entering processContainers) and then pass that
single collapseSettings value into deflateApplicationProfileContainer instead of
calling a.effectiveCollapseSettings() for each container so all containers use
the same resolved settings for the entire save operation.
---
Nitpick comments:
In `@pkg/registry/file/applicationprofile_processor_test.go`:
- Around line 21-25: Tests currently hardcode
dynamicpathdetector.OpenDynamicThreshold for /usr/lib paths; update them to
derive the threshold from active collapse configs like the /etc pattern by
calling dynamicpathdetector.FindConfigForPath("/usr/lib/...") (or the package's
exported FindConfigForPath) and reading the config's collapse threshold instead
of using OpenDynamicThreshold; change the openThreshold helper (and other
occurrences referencing dynamicpathdetector.OpenDynamicThreshold) to fetch the
config for "/usr/lib/..." and return its threshold, and apply the same
replacement for the other test spots that currently assume the global constant.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b453484d-6b0b-4912-91f5-2638c966dfd3
⛔ Files ignored due to path filters (17)
pkg/apis/softwarecomposition/v1beta1/generated.pb.gois excluded by!**/*.pb.gopkg/apis/softwarecomposition/v1beta1/generated.protomessage.pb.gois excluded by!**/*.pb.gopkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigentry.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigurationspec.gois excluded by!**/generated/**pkg/generated/applyconfiguration/utils.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_collapseconfiguration.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_softwarecomposition_client.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/generated_expansion.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/softwarecomposition_client.gois excluded by!**/generated/**pkg/generated/informers/externalversions/generic.gois excluded by!**/generated/**pkg/generated/informers/externalversions/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/informers/externalversions/softwarecomposition/v1beta1/interface.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/expansion_generated.gois excluded by!**/generated/**pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (39)
artifacts/collapseconfiguration-default-sample.yamlpkg/apis/softwarecomposition/collapse_types.gopkg/apis/softwarecomposition/network_types.gopkg/apis/softwarecomposition/register.gopkg/apis/softwarecomposition/v1beta1/collapse_types.gopkg/apis/softwarecomposition/v1beta1/generated.protopkg/apis/softwarecomposition/v1beta1/network_types.gopkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.gopkg/apis/softwarecomposition/v1beta1/register.gopkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.gopkg/apis/softwarecomposition/v1beta1/zz_generated.deepcopy.gopkg/apis/softwarecomposition/v1beta1/zz_generated.model_name.gopkg/apis/softwarecomposition/zz_generated.deepcopy.gopkg/apiserver/apiserver.gopkg/registry/file/applicationprofile_processor.gopkg/registry/file/applicationprofile_processor_collapse_provider_test.gopkg/registry/file/applicationprofile_processor_test.gopkg/registry/file/cleanup.gopkg/registry/file/cleanup_test.gopkg/registry/file/collapse_config_provider.gopkg/registry/file/collapse_config_provider_test.gopkg/registry/file/containerprofile_processor.gopkg/registry/file/containerprofile_processor_collapse_provider_test.gopkg/registry/file/dynamicpathdetector/collapse_config_from_crd.gopkg/registry/file/dynamicpathdetector/tests/collapse_config_crd_test.gopkg/registry/file/networkmatch/README.mdpkg/registry/file/networkmatch/bench_test.gopkg/registry/file/networkmatch/doc.gopkg/registry/file/networkmatch/match_dns.gopkg/registry/file/networkmatch/match_dns_test.gopkg/registry/file/networkmatch/match_ip.gopkg/registry/file/networkmatch/match_ip_test.gopkg/registry/file/networkmatch/validate.gopkg/registry/file/networkmatch/validate_test.gopkg/registry/softwarecomposition/collapseconfiguration/etcd.gopkg/registry/softwarecomposition/collapseconfiguration/strategy.gopkg/registry/softwarecomposition/collapseconfiguration/strategy_test.gopkg/registry/softwarecomposition/networkneighborhood/strategy.gopkg/registry/softwarecomposition/networkneighborhood/strategy_test.go
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Rechecked after the latest push: the three blockers from my previous review are fixed, but I found two new blockers.
pkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy.go:340and:419still generate peers/policy refs only from the deprecated singularneighbor.IPAddress/neighbor.DNSfields. The newipAddressespayload added in this PR is never consumed here, so a profile that uses onlyipAddressesproduces no correspondingIPBlockentries in the generated NetworkPolicy.pkg/registry/file/networkneighborhood_processor.go:91merges duplicate neighbors byIdentifier, but on merge it only appendsDNSNamesandPorts. The newIPAddressesslice is not appended/deduped, so when multiple observations for the same identifier contribute different IPs, every IP except the first neighbor's slice is dropped before the profile is stored.
|
One more blocker after checking the generated artifacts: the API type now exposes
Please regenerate the applyconfiguration/OpenAPI artifacts for |
entlein
force-pushed
the
upstream-pr/sbob-crd-compaction
branch
from
69c895e to
a3dd980
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/apis/softwarecomposition/network_types.go`:
- Around line 69-71: The IPAddresses field comment uses the IPv4 CIDR format
example `a.b.c.d/n`, which incorrectly suggests only IPv4 is supported, while
the code actually accepts both IPv4 and IPv6 CIDR entries. Update the comment
for the IPAddresses field to be protocol-agnostic by clarifying that each entry
can be a literal IP (supporting both IPv4 and IPv6), a CIDR in either IPv4 or
IPv6 format, or the "*" sentinel. This will accurately reflect what downstream
code and tests already support.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 806fddb0-5a24-4267-af4a-6f624b6890be
⛔ Files ignored due to path filters (17)
pkg/apis/softwarecomposition/v1beta1/generated.pb.gois excluded by!**/*.pb.gopkg/apis/softwarecomposition/v1beta1/generated.protomessage.pb.gois excluded by!**/*.pb.gopkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigentry.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigurationspec.gois excluded by!**/generated/**pkg/generated/applyconfiguration/utils.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_collapseconfiguration.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_softwarecomposition_client.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/generated_expansion.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/softwarecomposition_client.gois excluded by!**/generated/**pkg/generated/informers/externalversions/generic.gois excluded by!**/generated/**pkg/generated/informers/externalversions/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/informers/externalversions/softwarecomposition/v1beta1/interface.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/expansion_generated.gois excluded by!**/generated/**pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (39)
artifacts/collapseconfiguration-default-sample.yamlpkg/apis/softwarecomposition/collapse_types.gopkg/apis/softwarecomposition/network_types.gopkg/apis/softwarecomposition/register.gopkg/apis/softwarecomposition/v1beta1/collapse_types.gopkg/apis/softwarecomposition/v1beta1/generated.protopkg/apis/softwarecomposition/v1beta1/network_types.gopkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.gopkg/apis/softwarecomposition/v1beta1/register.gopkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.gopkg/apis/softwarecomposition/v1beta1/zz_generated.deepcopy.gopkg/apis/softwarecomposition/v1beta1/zz_generated.model_name.gopkg/apis/softwarecomposition/zz_generated.deepcopy.gopkg/apiserver/apiserver.gopkg/registry/file/applicationprofile_processor.gopkg/registry/file/applicationprofile_processor_collapse_provider_test.gopkg/registry/file/applicationprofile_processor_test.gopkg/registry/file/cleanup.gopkg/registry/file/cleanup_test.gopkg/registry/file/collapse_config_provider.gopkg/registry/file/collapse_config_provider_test.gopkg/registry/file/containerprofile_processor.gopkg/registry/file/containerprofile_processor_collapse_provider_test.gopkg/registry/file/dynamicpathdetector/collapse_config_from_crd.gopkg/registry/file/dynamicpathdetector/tests/collapse_config_crd_test.gopkg/registry/file/networkmatch/README.mdpkg/registry/file/networkmatch/bench_test.gopkg/registry/file/networkmatch/doc.gopkg/registry/file/networkmatch/match_dns.gopkg/registry/file/networkmatch/match_dns_test.gopkg/registry/file/networkmatch/match_ip.gopkg/registry/file/networkmatch/match_ip_test.gopkg/registry/file/networkmatch/validate.gopkg/registry/file/networkmatch/validate_test.gopkg/registry/softwarecomposition/collapseconfiguration/etcd.gopkg/registry/softwarecomposition/collapseconfiguration/strategy.gopkg/registry/softwarecomposition/collapseconfiguration/strategy_test.gopkg/registry/softwarecomposition/networkneighborhood/strategy.gopkg/registry/softwarecomposition/networkneighborhood/strategy_test.go
✅ Files skipped from review due to trivial changes (9)
- artifacts/collapseconfiguration-default-sample.yaml
- pkg/apis/softwarecomposition/v1beta1/register.go
- pkg/registry/file/networkmatch/doc.go
- pkg/registry/file/networkmatch/validate_test.go
- pkg/registry/file/networkmatch/README.md
- pkg/apis/softwarecomposition/v1beta1/zz_generated.model_name.go
- pkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.go
- pkg/apis/softwarecomposition/v1beta1/zz_generated.deepcopy.go
- pkg/apis/softwarecomposition/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (28)
- pkg/apis/softwarecomposition/v1beta1/collapse_types.go
- pkg/apis/softwarecomposition/collapse_types.go
- pkg/registry/softwarecomposition/collapseconfiguration/etcd.go
- pkg/apis/softwarecomposition/register.go
- pkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.go
- pkg/apis/softwarecomposition/v1beta1/network_types.go
- pkg/registry/file/networkmatch/match_ip_test.go
- pkg/registry/softwarecomposition/collapseconfiguration/strategy_test.go
- pkg/registry/file/dynamicpathdetector/collapse_config_from_crd.go
- pkg/registry/file/dynamicpathdetector/tests/collapse_config_crd_test.go
- pkg/registry/file/collapse_config_provider.go
- pkg/registry/file/collapse_config_provider_test.go
- pkg/registry/file/containerprofile_processor_collapse_provider_test.go
- pkg/registry/file/networkmatch/match_ip.go
- pkg/registry/file/cleanup.go
- pkg/registry/softwarecomposition/collapseconfiguration/strategy.go
- pkg/registry/file/networkmatch/match_dns.go
- pkg/registry/file/applicationprofile_processor_collapse_provider_test.go
- pkg/registry/file/cleanup_test.go
- pkg/registry/file/networkmatch/bench_test.go
- pkg/apiserver/apiserver.go
- pkg/registry/file/networkmatch/match_dns_test.go
- pkg/registry/file/applicationprofile_processor.go
- pkg/registry/file/networkmatch/validate.go
- pkg/registry/softwarecomposition/networkneighborhood/strategy_test.go
- pkg/registry/softwarecomposition/networkneighborhood/strategy.go
- pkg/registry/file/containerprofile_processor.go
- pkg/registry/file/applicationprofile_processor_test.go
entlein
force-pushed
the
upstream-pr/sbob-crd-compaction
branch
from
a3dd980 to
d0b21b2
Compare
|
@CodeRabbit, please review all PRs 328 324 and 325 together now |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh api repos/kubescape/storage/pulls/328 2>/dev/null | jq '{number: .number, title: .title, state: .state, base: .base.ref, head: .head.ref, body: .body[0:500]}' 2>/dev/null || echo "failed"
echo "=== PR `#328` files ==="
gh api repos/kubescape/storage/pulls/328/files 2>/dev/null | jq '.[].filename' 2>/dev/null || echo "failed"Length of output: 2414
Combined Analysis —
|
|
Pushed a fix for the request-path stall flagged in review (commit Root cause: the Fix: bound each read with a short timeout and move it off the request path into an async refresher that publishes an atomic snapshot, falling back to Validated end-to-end against the component-test harness: with this fix the storage apiserver no longer stalls and the previously-timing-out profile tests pass. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Inspect the fix commit
gh api repos/kubescape/storage/commits/a6fce873 2>/dev/null | jq '{sha: .sha, message: .commit.message, files: [.files[].filename]}' 2>/dev/null || echo "commit lookup failed"
echo ""
echo "=== Changed file diffs ==="
gh api repos/kubescape/storage/commits/a6fce873 2>/dev/null | jq -r '.files[] | "### \(.filename)\n```diff\n\(.patch // "(binary or no patch)")\n```"' 2>/dev/null || echo "diff fetch failed"Length of output: 7299 Good find and fix on the root cause, 🔴 Issue: transient error overwrites the last known-good snapshotIn if err != nil || crd.Name == "" {
settings = dynamicpathdetector.DefaultCollapseSettings()
} else {
settings = dynamicpathdetector.CollapseSettingsFromCRD(crd)
}
current.Store(&settings) // ← always writes, even on errorIf a tick fires during a transient storage blip (e.g., etcd leader election, brief network partition), the provider reverts to compiled defaults, dropping the operator-applied Suggested fix — only update refresh := func() {
ctx, cancel := context.WithTimeout(context.Background(), collapseConfigGetTimeout)
defer cancel()
crd := &softwarecomposition.CollapseConfiguration{}
err := s.Get(ctx, key, storage.GetOptions{IgnoreNotFound: true}, crd)
if err != nil {
return // keep last known-good snapshot; next tick will retry
}
settings := dynamicpathdetector.CollapseSettingsFromCRD(crd) // handles crd.Name=="" → defaults internally
current.Store(&settings)
}This keeps the last good snapshot on transient errors and only reverts to defaults when the CR is genuinely absent (which 🟡 Goroutine has no stop mechanismThe background ticker goroutine runs for the lifetime of the process with no go func() {
t := time.NewTicker(collapseConfigRefreshInterval)
defer t.Stop()
for range t.C {
refresh()
}
}()For a server-lifetime provider this is probably acceptable, but it makes the goroutine impossible to cleanly shut down in tests or graceful-shutdown sequences. Consider accepting a 🟡 Initial
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/registry/file/collapse_config_provider.go (1)
40-42: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse the API group constant when building the storage key.
pkg/apiserver/apiserver.goregisters this API group throughsoftwarecomposition.GroupName; duplicating the literal here creates a silent fallback-to-default failure mode if the group string ever changes.Proposed refactor
func collapseConfigurationKey(name string) string { - return K8sKeysToPath("", "spdx.softwarecomposition.kubescape.io", "collapseconfigurations", "", "", name) + return K8sKeysToPath("", softwarecomposition.GroupName, "collapseconfigurations", "", "", name) }🤖 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/registry/file/collapse_config_provider.go` around lines 40 - 42, The storage key builder in collapseConfigurationKey is hardcoding the API group string, which can drift from the registered group. Update collapseConfigurationKey to use the shared API group constant from softwarecomposition.GroupName instead of the literal, keeping the K8sKeysToPath call otherwise unchanged so the storage key stays aligned with the apiserver registration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/registry/file/collapse_config_provider.go`:
- Around line 27-31: Update the comment near collapseConfigGetTimeout and the
collapse config provider logic/comments to match the real fallback behavior: on
Get error or missing CR, the provider uses DefaultCollapseSettings() rather than
keeping the last known settings. Remove the CodeRabbit/review-tool reference
from the source comment, and ensure the description around the functions in
collapse_config_provider.go accurately reflects the behavior implemented in the
provider’s read path.
---
Nitpick comments:
In `@pkg/registry/file/collapse_config_provider.go`:
- Around line 40-42: The storage key builder in collapseConfigurationKey is
hardcoding the API group string, which can drift from the registered group.
Update collapseConfigurationKey to use the shared API group constant from
softwarecomposition.GroupName instead of the literal, keeping the K8sKeysToPath
call otherwise unchanged so the storage key stays aligned with the apiserver
registration.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b74958e3-aef2-42fe-8602-13a68945e3f7
⛔ Files ignored due to path filters (17)
pkg/apis/softwarecomposition/v1beta1/generated.pb.gois excluded by!**/*.pb.gopkg/apis/softwarecomposition/v1beta1/generated.protomessage.pb.gois excluded by!**/*.pb.gopkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigentry.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigurationspec.gois excluded by!**/generated/**pkg/generated/applyconfiguration/utils.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_collapseconfiguration.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_softwarecomposition_client.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/generated_expansion.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/softwarecomposition_client.gois excluded by!**/generated/**pkg/generated/informers/externalversions/generic.gois excluded by!**/generated/**pkg/generated/informers/externalversions/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/informers/externalversions/softwarecomposition/v1beta1/interface.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/expansion_generated.gois excluded by!**/generated/**pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (26)
artifacts/collapseconfiguration-default-sample.yamlpkg/apis/softwarecomposition/collapse_types.gopkg/apis/softwarecomposition/register.gopkg/apis/softwarecomposition/v1beta1/collapse_types.gopkg/apis/softwarecomposition/v1beta1/generated.protopkg/apis/softwarecomposition/v1beta1/register.gopkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.gopkg/apis/softwarecomposition/v1beta1/zz_generated.deepcopy.gopkg/apis/softwarecomposition/v1beta1/zz_generated.model_name.gopkg/apis/softwarecomposition/zz_generated.deepcopy.gopkg/apiserver/apiserver.gopkg/registry/file/applicationprofile_processor.gopkg/registry/file/applicationprofile_processor_collapse_provider_test.gopkg/registry/file/applicationprofile_processor_test.gopkg/registry/file/cleanup.gopkg/registry/file/cleanup_test.gopkg/registry/file/collapse_config_provider.gopkg/registry/file/collapse_config_provider_test.gopkg/registry/file/containerprofile_processor.gopkg/registry/file/containerprofile_processor_collapse_provider_test.gopkg/registry/file/dynamicpathdetector/collapse_config_from_crd.gopkg/registry/file/dynamicpathdetector/tests/collapse_config_crd_test.gopkg/registry/file/networkmatch/match_differential_test.gopkg/registry/softwarecomposition/collapseconfiguration/etcd.gopkg/registry/softwarecomposition/collapseconfiguration/strategy.gopkg/registry/softwarecomposition/collapseconfiguration/strategy_test.go
✅ Files skipped from review due to trivial changes (5)
- pkg/apis/softwarecomposition/v1beta1/zz_generated.model_name.go
- artifacts/collapseconfiguration-default-sample.yaml
- pkg/apis/softwarecomposition/zz_generated.deepcopy.go
- pkg/apis/softwarecomposition/v1beta1/zz_generated.deepcopy.go
- pkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.go
🚧 Files skipped from review as they are similar to previous changes (18)
- pkg/apis/softwarecomposition/v1beta1/register.go
- pkg/apis/softwarecomposition/register.go
- pkg/registry/softwarecomposition/collapseconfiguration/etcd.go
- pkg/registry/file/cleanup_test.go
- pkg/registry/file/dynamicpathdetector/collapse_config_from_crd.go
- pkg/registry/file/collapse_config_provider_test.go
- pkg/apis/softwarecomposition/v1beta1/collapse_types.go
- pkg/apiserver/apiserver.go
- pkg/apis/softwarecomposition/collapse_types.go
- pkg/registry/softwarecomposition/collapseconfiguration/strategy_test.go
- pkg/registry/softwarecomposition/collapseconfiguration/strategy.go
- pkg/registry/file/applicationprofile_processor.go
- pkg/registry/file/containerprofile_processor_collapse_provider_test.go
- pkg/registry/file/containerprofile_processor.go
- pkg/registry/file/dynamicpathdetector/tests/collapse_config_crd_test.go
- pkg/registry/file/cleanup.go
- pkg/registry/file/applicationprofile_processor_test.go
- pkg/registry/file/applicationprofile_processor_collapse_provider_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/registry/file/collapse_config_provider.go`:
- Around line 86-95: The cached snapshot in collapse_config_provider.go is
exposing shared slice state through a shallow copy of CollapseSettings, so
callers can mutate CollapseConfigs and affect later reads. Update the
snapshot-return path around the current.Store / retrieval logic in the
CollapseSettings provider to return a defensive copy of CollapseConfigs (for
both DefaultCollapseSettings() and CollapseSettingsFromCRD(crd) cases) before
handing settings back to callers, preserving isolation of the atomic cache.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a820b325-9bb9-4dbd-8f3d-13a38bf32f46
📒 Files selected for processing (2)
pkg/registry/file/collapse_config_provider.gopkg/registry/file/collapse_config_provider_test.go
|
@coderabbitai — addressed in Good catch on the fallback regression — fixed. The refresher no longer overwrites the snapshot on a transient read error: on One adjustment to the suggested diff: I kept the explicit Also from the review:
Provider tests pass under |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Triggering the full review now. ✅ Action performedReview finished.
|
entlein
force-pushed
the
upstream-pr/sbob-crd-compaction
branch
from
e034f99 to
b359654
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/registry/file/collapse_config_provider.go (1)
54-57: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove storage reads off the request path and bound the read context.
Line 71 performs
s.Getwithcontext.Background()on each call, so storage slowness can stall admission/deflate paths. This also regresses the stated async-snapshot design in the PR objective. Please switch to a background refresher with an atomic last-known-good snapshot (or at minimum usecontext.WithTimeoutfor the read path).Suggested direction
- err := s.Get(context.Background(), key, storage.GetOptions{IgnoreNotFound: true}, crd) + ctx, cancel := context.WithTimeout(context.Background(), collapseConfigGetTimeout) + defer cancel() + err := s.Get(ctx, key, storage.GetOptions{IgnoreNotFound: true}, crd)Also applies to: 71-73
🤖 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/registry/file/collapse_config_provider.go` around lines 54 - 57, Move the storage fetch in collapse_config_provider’s deflate path off the request path and avoid blocking with context.Background(). Update the closure around the current s.Get call to read from an atomic last-known-good snapshot, refreshed asynchronously in the background, or at minimum wrap the storage read in a bounded context.WithTimeout. Keep the logic centered on the existing collapse config provider behavior so edits still pick up without restart while preventing admission/deflate stalls.
🧹 Nitpick comments (3)
pkg/cmd/server/start_test.go (1)
69-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSource-order test dependency is fragile.
TestPersistentPreRunERejectsUnknownGaterelies on being declared last because it pollutes the process-global kube feature gate. This holds today (Go executes top-level tests in source order), but the contract silently breaks if a test is added below it or if these are ever parallelized. Consider isolating the stale-gate scenario in its own package/binary or resetting the registry in a cleanup hook so ordering is no longer load-bearing.🤖 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/cmd/server/start_test.go` around lines 69 - 82, The test `TestPersistentPreRunERejectsUnknownGate` depends on source order because it leaves stale state in the process-global kube feature gate, making later tests fragile. Update this test setup so the `NewCommandStartWardleServer`/`PersistentPreRunE` stale-gate scenario is isolated from the shared registry, either by moving it into a separate package/binary-style test or by adding cleanup that resets the feature-gate registry after the assertion. Remove the comment/assumption that it must remain last once the test no longer relies on declaration order.main.go (1)
44-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLogic is correct; minor naming nuance.
zap.IncreaseLevelcan only raise severity, so the defaultIncreaseLevel(zap.FatalLevel)mutes klog, whileKS_LOGGER_LEVEL=debugresolves tozap.InfoLevel(already theNewProductionfloor) and surfaces Info-and-above. Note that higher klog V-level "debug" lines (which zapr maps to sub-Info levels) will still be filtered, so thedebugflag effectively exposes Info diagnostics rather than verbose debug output. Functionally fine for the stated goal of unmuting startup/readyz diagnostics.🤖 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 `@main.go` around lines 44 - 54, The klog level toggle in main.go is functionally correct, but the KS_LOGGER_LEVEL=debug setting is misleading because zap.IncreaseLevel(zap.InfoLevel) only exposes Info-and-above logs, not full debug/V-level output. Update the naming or inline comment around the zap.IncreaseLevel and klog.SetLogger setup so it accurately states that “debug” un-mutes startup/readyz diagnostics to Info level rather than enabling verbose debug logging.pkg/cmd/server/start.go (1)
150-158: 📐 Maintainability & Code Quality | 🔵 TrivialCorrect inaccurate version reference in the feature gate comment.
The comment suggests ServerSideApply reached GA in "Kubernetes 1.35", but it actually reached General Availability in v1.22 (and the feature gate was removed entirely in v1.32). The behavioral note regarding the token validation remains valid.
🤖 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/cmd/server/start.go` around lines 150 - 158, The feature-gate comment near the startup flag setup has an inaccurate Kubernetes version reference for ServerSideApply. Update the comment text in the block that sets the feature gates and calls defaults.ComponentGlobalsRegistry.Set() so it refers to ServerSideApply reaching General Availability in v1.22 and note that the gate was removed in v1.32, while keeping the token validation note unchanged.
🤖 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.
Duplicate comments:
In `@pkg/registry/file/collapse_config_provider.go`:
- Around line 54-57: Move the storage fetch in collapse_config_provider’s
deflate path off the request path and avoid blocking with context.Background().
Update the closure around the current s.Get call to read from an atomic
last-known-good snapshot, refreshed asynchronously in the background, or at
minimum wrap the storage read in a bounded context.WithTimeout. Keep the logic
centered on the existing collapse config provider behavior so edits still pick
up without restart while preventing admission/deflate stalls.
---
Nitpick comments:
In `@main.go`:
- Around line 44-54: The klog level toggle in main.go is functionally correct,
but the KS_LOGGER_LEVEL=debug setting is misleading because
zap.IncreaseLevel(zap.InfoLevel) only exposes Info-and-above logs, not full
debug/V-level output. Update the naming or inline comment around the
zap.IncreaseLevel and klog.SetLogger setup so it accurately states that “debug”
un-mutes startup/readyz diagnostics to Info level rather than enabling verbose
debug logging.
In `@pkg/cmd/server/start_test.go`:
- Around line 69-82: The test `TestPersistentPreRunERejectsUnknownGate` depends
on source order because it leaves stale state in the process-global kube feature
gate, making later tests fragile. Update this test setup so the
`NewCommandStartWardleServer`/`PersistentPreRunE` stale-gate scenario is
isolated from the shared registry, either by moving it into a separate
package/binary-style test or by adding cleanup that resets the feature-gate
registry after the assertion. Remove the comment/assumption that it must remain
last once the test no longer relies on declaration order.
In `@pkg/cmd/server/start.go`:
- Around line 150-158: The feature-gate comment near the startup flag setup has
an inaccurate Kubernetes version reference for ServerSideApply. Update the
comment text in the block that sets the feature gates and calls
defaults.ComponentGlobalsRegistry.Set() so it refers to ServerSideApply reaching
General Availability in v1.22 and note that the gate was removed in v1.32, while
keeping the token validation note unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49af8c06-cf17-4b41-9cb8-9940e89214be
⛔ Files ignored due to path filters (17)
pkg/apis/softwarecomposition/v1beta1/generated.pb.gois excluded by!**/*.pb.gopkg/apis/softwarecomposition/v1beta1/generated.protomessage.pb.gois excluded by!**/*.pb.gopkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigentry.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigurationspec.gois excluded by!**/generated/**pkg/generated/applyconfiguration/utils.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_collapseconfiguration.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_softwarecomposition_client.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/generated_expansion.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/softwarecomposition_client.gois excluded by!**/generated/**pkg/generated/informers/externalversions/generic.gois excluded by!**/generated/**pkg/generated/informers/externalversions/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/informers/externalversions/softwarecomposition/v1beta1/interface.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/collapseconfiguration.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/expansion_generated.gois excluded by!**/generated/**pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (50)
MakefileREADME.mdartifacts/collapseconfiguration-default-sample.yamldocs/features/watchlist-disabled.mdgo.modmain.gopkg/apis/softwarecomposition/collapse_types.gopkg/apis/softwarecomposition/network_types.gopkg/apis/softwarecomposition/register.gopkg/apis/softwarecomposition/v1beta1/collapse_types.gopkg/apis/softwarecomposition/v1beta1/generated.protopkg/apis/softwarecomposition/v1beta1/network_types.gopkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.gopkg/apis/softwarecomposition/v1beta1/register.gopkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.gopkg/apis/softwarecomposition/v1beta1/zz_generated.deepcopy.gopkg/apis/softwarecomposition/v1beta1/zz_generated.model_name.gopkg/apis/softwarecomposition/zz_generated.deepcopy.gopkg/apiserver/apiserver.gopkg/cmd/server/start.gopkg/cmd/server/start_test.gopkg/registry/file/applicationprofile_processor.gopkg/registry/file/applicationprofile_processor_collapse_provider_test.gopkg/registry/file/applicationprofile_processor_test.gopkg/registry/file/cleanup.gopkg/registry/file/cleanup_test.gopkg/registry/file/collapse_config_provider.gopkg/registry/file/collapse_config_provider_test.gopkg/registry/file/containerprofile_processor.gopkg/registry/file/containerprofile_processor_collapse_provider_test.gopkg/registry/file/dynamicpathdetector/collapse_config_from_crd.gopkg/registry/file/dynamicpathdetector/tests/collapse_config_crd_test.gopkg/registry/file/idlewatch_test.gopkg/registry/file/networkmatch/README.mdpkg/registry/file/networkmatch/bench_test.gopkg/registry/file/networkmatch/doc.gopkg/registry/file/networkmatch/match_differential_test.gopkg/registry/file/networkmatch/match_dns.gopkg/registry/file/networkmatch/match_dns_test.gopkg/registry/file/networkmatch/match_ip.gopkg/registry/file/networkmatch/match_ip_test.gopkg/registry/file/networkmatch/validate.gopkg/registry/file/networkmatch/validate_test.gopkg/registry/file/storage.gopkg/registry/file/watch.gopkg/registry/softwarecomposition/collapseconfiguration/etcd.gopkg/registry/softwarecomposition/collapseconfiguration/strategy.gopkg/registry/softwarecomposition/collapseconfiguration/strategy_test.gopkg/registry/softwarecomposition/networkneighborhood/strategy.gopkg/registry/softwarecomposition/networkneighborhood/strategy_test.go
✅ Files skipped from review due to trivial changes (9)
- artifacts/collapseconfiguration-default-sample.yaml
- go.mod
- pkg/registry/file/networkmatch/doc.go
- docs/features/watchlist-disabled.md
- README.md
- pkg/registry/file/networkmatch/README.md
- pkg/registry/file/networkmatch/bench_test.go
- pkg/apis/softwarecomposition/zz_generated.deepcopy.go
- pkg/apis/softwarecomposition/v1beta1/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (31)
- pkg/registry/file/networkmatch/validate_test.go
- pkg/apis/softwarecomposition/v1beta1/register.go
- pkg/registry/file/dynamicpathdetector/collapse_config_from_crd.go
- pkg/registry/softwarecomposition/collapseconfiguration/etcd.go
- pkg/apis/softwarecomposition/network_types.go
- pkg/apis/softwarecomposition/register.go
- pkg/apis/softwarecomposition/v1beta1/collapse_types.go
- pkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.go
- pkg/apis/softwarecomposition/v1beta1/zz_generated.model_name.go
- pkg/registry/softwarecomposition/networkneighborhood/strategy_test.go
- pkg/registry/file/networkmatch/validate.go
- pkg/apiserver/apiserver.go
- pkg/registry/softwarecomposition/collapseconfiguration/strategy_test.go
- pkg/apis/softwarecomposition/v1beta1/network_types.go
- pkg/apis/softwarecomposition/collapse_types.go
- pkg/registry/file/dynamicpathdetector/tests/collapse_config_crd_test.go
- pkg/registry/file/containerprofile_processor.go
- pkg/registry/file/containerprofile_processor_collapse_provider_test.go
- pkg/registry/softwarecomposition/collapseconfiguration/strategy.go
- pkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.go
- pkg/registry/file/networkmatch/match_dns.go
- pkg/registry/file/cleanup.go
- pkg/registry/file/networkmatch/match_differential_test.go
- pkg/registry/file/networkmatch/match_ip.go
- pkg/registry/file/networkmatch/match_dns_test.go
- pkg/registry/file/applicationprofile_processor.go
- pkg/registry/softwarecomposition/networkneighborhood/strategy.go
- pkg/registry/file/applicationprofile_processor_collapse_provider_test.go
- pkg/registry/file/networkmatch/match_ip_test.go
- pkg/registry/file/cleanup_test.go
- pkg/registry/file/applicationprofile_processor_test.go
…naged lifecycle Introduces a CollapseConfiguration CRD with a projection overlay and a user-managed lifecycle, and wires it into the profile deflate path so operator-supplied compaction settings actually take effect. Adds NewCRDCollapseSettingsProvider(storage.Interface) — a closure that reads CollapseConfiguration/default on every deflate call, projects through dynamicpathdetector.CollapseSettingsFromCRD, and falls back to DefaultCollapseSettings when the CR is absent or storage is nil. apiserver.go refactors processor construction so both the application and container profile processors are reachable after the storage backend is built, then wires the same provider into both via SetCollapseSettings (ApplicationProfileProcessor) and direct field assignment (ContainerProfileProcessor). One shared closure keeps the two compaction paths consistent on every CR edit. The no-cache design lets bobctl autotune-style write->read cycles take effect immediately without restart or invalidation. Deflate frequency is low compared to disk Get latency; if benchmarks ever surface this as hot, wrap with a watched cache. Tests pin five contract points: fall-back on absent CR, faithful projection of an applied CR, nil-storage defence, transient-error fall-back, and live-update without invalidation. Resolves matthyx review on apiserver.go:164 (2026-05-27). Signed-off-by: entlein <einentlein@gmail.com> Docs-exempt: rebase/squash of contributor PR kubescape#325; CRD feature docs to be handled in PR review
matthyx
force-pushed
the
upstream-pr/sbob-crd-compaction
branch
from
b359654 to
e234650
Compare
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Automated review of the current head (e2346504).
Scope of this head is the CollapseConfiguration CRD + projection overlay, the cleanup user-managed skip, and the networkmatch differential test. Good news first:
- The three blockers from the 2026-05-27 review are resolved on this head: the
dynamicpathdetectorsymbols (NewPathAnalyzerWithConfigs,CollapseConfig,DefaultCollapseConfigs,FindConfigForPath) exist on the branch, and the CRD is now wired into the live deflate path viaNewCRDCollapseSettingsProvider→ both processors. - The two 2026-05-28 blockers (NetworkNeighbor
IPAddressesnot consumed innetworkpolicy.go/networkneighborhood_processor.go) reference code that is not in this PR's diff anymore —main...HEADtouches onlynetworkmatch/match_differential_test.goundernetwork*. They appear resolved-by-rescope; worth confirming theipAddressesfeature landed (or is tracked) elsewhere.
One blocker remains, plus a few non-blocking notes.
🔴 Blocker — a partial / zero-valued CollapseConfiguration silently over-collapses every profile
See the inline comments on strategy.go and collapse_config_from_crd.go. In short: updateNodeStats collapses once node.Count > threshold, so an effective threshold of 0 turns every path into ⋯. Validation allows the two global thresholds to be 0 (only < 0 is rejected) while each per-prefix entry must be >= 1, and the v1beta1 JSON fields are not omitempty — so a CR that omits openDynamicThreshold (or only sets collapseConfigs) decodes to 0 and is passed through verbatim, silently nuking all opens/endpoints in production. Fix: require >= 1 for the global thresholds too, or treat 0 as "use compiled default" in CollapseSettingsFromCRD.
🟡 Notes (non-blocking)
- Storage-instance asymmetry (apiserver.go). The
collapseconfigurationsREST endpoint is registered asep(collapseconfiguration.NewREST)→ backed by the plainstorageImpl, but the provider reads the CR fromapplicationProfileStorageBackend(the with-collector backend). It works today only because both point at the sameDefaultStorageRooton the sameOsFs. Reading the CR through the samestorageImplthe endpoint writes to would remove a latent footgun. - Singleton name not enforced. Only
CollapseConfiguration/defaultis ever consulted; creating it under any other name is a silent no-op. Consider aWarningsOnCreate/WarningsOnUpdatewhenmetadata.name != "default". - Breaking signature on an exported func.
DeflateContainerProfileSpecgained asettingsparameter. If anything outside this repo imports it (e.g. node-agent), that's a compile break — please confirm there are no external callers or provide a wrapper. - Truncated license header in
pkg/registry/file/collapse_config_provider.go(only the first 3 lines of the Apache header; the warranty paragraph other files carry is missing) — license-header linters may flag it. - DCO check is currently failing — independent merge gate; needs a
Signed-off-byon all commits.
Nice work on the test coverage — the defensive-copy / nil-provider / live-update pins and the networkmatch differential oracle are thorough.
| // longest-prefix-wins outcome at runtime). | ||
| func validateCollapseConfigurationSpec(spec *softwarecomposition.CollapseConfigurationSpec, fp *field.Path) field.ErrorList { | ||
| var errs field.ErrorList | ||
| if spec.OpenDynamicThreshold < 0 { |
There was a problem hiding this comment.
Blocker: the global thresholds accept 0, which collapses everything.
updateNodeStats collapses a node once node.Count > threshold (analyzer.go:334), so an effective threshold of 0 collapses every node that has any children — i.e. every open/endpoint path becomes ⋯.
This validator rejects only < 0 for OpenDynamicThreshold / EndpointDynamicThreshold, while it requires each per-prefix Threshold >= 1 (line 146). The inconsistency is dangerous: the v1beta1 JSON fields are not omitempty, so a CR that simply omits openDynamicThreshold decodes to 0, and CollapseSettingsFromCRD passes it through verbatim (collapse_config_from_crd.go:81). The result is that applying a CR which sets only collapseConfigs silently zeroes the global thresholds and over-collapses all profiles in production.
Suggest requiring >= 1 here too (consistent with the per-entry rule), or treating 0 as “use the compiled default” in CollapseSettingsFromCRD.
| } | ||
| } | ||
| return CollapseSettings{ | ||
| OpenDynamicThreshold: int(crd.Spec.OpenDynamicThreshold), |
There was a problem hiding this comment.
Related to the threshold-validation blocker. This projection copies Spec values verbatim, with two consequences worth deciding explicitly:
- A
0global threshold is taken literally (see thestrategy.govalidation thread) → collapses everything. CollapseConfigsis replaced wholesale — a CR with a singlecollapseConfigsentry drops all compiled-in per-prefix defaults (/etc=100,/opt=50,/var/run=50, …) rather than overlaying on top of them.
If wholesale-replace is intended, please document it on the CRD type and in artifacts/collapseconfiguration-default-sample.yaml so operators know a partial CR is not a partial override. If overlay was intended, merge with DefaultCollapseSettings() here and guard zero-valued thresholds.
|
Re-checking the current
Both endpoint deflate paths call Please pass |
|
I found another blocker in the current diff:
But Result: the provider reads the wrong path, misses the applied CR, and falls back to defaults every time. That makes applying Please build the cluster-scoped key without the empty namespace segment (or use the same key-generation path the REST store uses) and add an integration-style regression test that writes the CR through real storage and verifies the provider can read it back. |
…r-scoped CR key Two blockers in the CollapseConfiguration wiring found in PR kubescape#325 review: 1. Endpoint compaction ignored CollapseConfiguration.collapseConfigs. deflateApplicationProfileContainer and DeflateContainerProfileSpec built the endpoint analyzer with NewPathAnalyzerWithConfigs(EndpointDynamicThreshold, nil), so per-prefix overrides reached opens but never endpoints. Pass settings.CollapseConfigs into both endpoint analyzers so the CRD drives endpoint compaction too. 2. collapseConfigurationKey() built /spdx.../collapseconfigurations//default (a stray empty namespace segment) via K8sKeysToPath, while the cluster-scoped REST storage keys the CR as /spdx.../collapseconfigurations/default (genericregistry NoNamespaceKeyFunc). The provider therefore missed the applied CR and silently fell back to defaults. Add a K8sClusterScopedKeysToPath helper next to K8sKeysToPath (whose namespace segment is unconditional and cannot express a cluster-scoped key) and build the key through it, keeping the storage-key format centralized. Adds regression tests pinning the exact storage key string and endpoint collapse behavior. The existing provider tests stored and read through the same helper, so they were self-consistent and could not catch the key bug. Docs-exempt: bug fixes to unreleased CollapseConfiguration feature; no behavioral doc exists yet Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…naged lifecycle Introduces a CollapseConfiguration CRD with a projection overlay and a user-managed lifecycle, and wires it into the profile deflate path so operator-supplied compaction settings actually take effect. Adds NewCRDCollapseSettingsProvider(storage.Interface) — a closure that reads CollapseConfiguration/default on every deflate call, projects through dynamicpathdetector.CollapseSettingsFromCRD, and falls back to DefaultCollapseSettings when the CR is absent or storage is nil. apiserver.go refactors processor construction so both the application and container profile processors are reachable after the storage backend is built, then wires the same provider into both via SetCollapseSettings (ApplicationProfileProcessor) and direct field assignment (ContainerProfileProcessor). One shared closure keeps the two compaction paths consistent on every CR edit. The no-cache design lets bobctl autotune-style write->read cycles take effect immediately without restart or invalidation. Deflate frequency is low compared to disk Get latency; if benchmarks ever surface this as hot, wrap with a watched cache. Tests pin five contract points: fall-back on absent CR, faithful projection of an applied CR, nil-storage defence, transient-error fall-back, and live-update without invalidation. Resolves matthyx review on apiserver.go:164 (2026-05-27). Signed-off-by: entlein <einentlein@gmail.com> Docs-exempt: rebase/squash of contributor PR kubescape#325; CRD feature docs to be handled in PR review Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
…r-scoped CR key Two blockers in the CollapseConfiguration wiring found in PR kubescape#325 review: 1. Endpoint compaction ignored CollapseConfiguration.collapseConfigs. deflateApplicationProfileContainer and DeflateContainerProfileSpec built the endpoint analyzer with NewPathAnalyzerWithConfigs(EndpointDynamicThreshold, nil), so per-prefix overrides reached opens but never endpoints. Pass settings.CollapseConfigs into both endpoint analyzers so the CRD drives endpoint compaction too. 2. collapseConfigurationKey() built /spdx.../collapseconfigurations//default (a stray empty namespace segment) via K8sKeysToPath, while the cluster-scoped REST storage keys the CR as /spdx.../collapseconfigurations/default (genericregistry NoNamespaceKeyFunc). The provider therefore missed the applied CR and silently fell back to defaults. Add a K8sClusterScopedKeysToPath helper next to K8sKeysToPath (whose namespace segment is unconditional and cannot express a cluster-scoped key) and build the key through it, keeping the storage-key format centralized. Adds regression tests pinning the exact storage key string and endpoint collapse behavior. The existing provider tests stored and read through the same helper, so they were self-consistent and could not catch the key bug. Docs-exempt: bug fixes to unreleased CollapseConfiguration feature; no behavioral doc exists yet Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
…nt replace semantics Addresses two PR kubescape#325 review blockers on the CollapseConfiguration CRD. 1. Zero-valued global thresholds silently over-collapsed every profile. A partial CR that omits openDynamicThreshold / endpointDynamicThreshold (or sets only collapseConfigs) decodes the field to 0, and updateNodeStats collapses any node with Count > threshold — so 0 flattens every open AND endpoint to a single dynamic identifier. The earlier endpoints fix widened the blast radius to endpoints too. CollapseSettingsFromCRD now treats a non-positive global threshold as 'use the compiled-in default' rather than copying 0 verbatim. This is the load-bearing guard: it holds even for direct storage writes that bypass admission. The two global thresholds are also made optional in the v1beta1 type (omitempty + +optional + protobuf opt) so partial CRs are first-class; per-prefix entries keep their stricter >= 1 admission check (an explicit override has no default to fall back to). Regenerated openapi/proto/ applyconfiguration accordingly. 2. Wholesale-replace of CollapseConfigs was undocumented. A CR with one entry drops the compiled-in /etc, /opt, /var/run defaults rather than overlaying them. Documented the replace-not-merge contract on both the internal and v1beta1 CRD types and in the sample manifest (which already lists the full default set), plus the zero-means-default semantics. Adds regression tests for the zero-guard (omitted globals fall back to defaults; only-collapseConfigs keeps global defaults). Docs-exempt: documents CRD behavior inline on the API type + sample; no docs/ feature page exists for this unreleased CRD Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
upstream-pr/sbob-crd-compaction
branch
from
a1aef11 to
b90b528
Compare
2 participants
CollapseConfigCRD (v1beta1 with conversion, deepcopy, protobuf, openapi, generated clientset/informers/listers, etcd registration)lets cluster operators declare path-prefix policies that drive profile-compaction without code changes
applicationprofile_processor+containerprofile_processorconsume the CollapseConfig provider to choose per-profile collapse strategies at admission timepkg/registry/file/cleanup.go: cleanup loop skips user-managed resources (annotation-based marker, not labels —labels would silently miss every user-managed profile)
artifacts/collapseconfiguration-default-sample.yamlships a sample CRSummary by CodeRabbit
New Features
CollapseConfigurationCRD (including a default sample) to control dynamic path-collapse thresholds, with per-prefix longest-prefix-wins overrides.IPAddresses(list form) for wildcard-aware IP matching, alongside expanded NetworkNeighborhood admission validation.Bug Fixes
Documentation
Tests