Skip to content

profile-compaction: CollapseConfig CRD + projection overlay + user-maged lifecycle#325

Open
entlein wants to merge 7 commits into
kubescape:mainfrom
k8sstormcenter:upstream-pr/sbob-crd-compaction
Open

profile-compaction: CollapseConfig CRD + projection overlay + user-maged lifecycle#325
entlein wants to merge 7 commits into
kubescape:mainfrom
k8sstormcenter:upstream-pr/sbob-crd-compaction

Conversation

@entlein

@entlein entlein commented May 15, 2026

Copy link
Copy Markdown
Contributor
  • CollapseConfig CRD (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_processor consume the CollapseConfig provider to choose per-profile collapse strategies at admission time
  • pkg/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.yaml ships a sample CR

Summary by CodeRabbit

  • New Features

    • Added CollapseConfiguration resource for cluster administrators to dynamically configure path collapse thresholds and per-prefix overrides.
    • Enhanced network neighbor profiles to support multiple IP addresses and improved wildcard-aware validation for IP and DNS entries.
    • Added validation for network profile entries with support for literal IPs, CIDR blocks, and DNS patterns.
  • Bug Fixes

    • Improved resource cleanup to respect user-managed resource markers.
  • Tests

    • Added comprehensive test coverage for collapse configuration management and network profile validation.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 15, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@entlein, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 47 minutes and 40 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 75010cfc-09e1-4a62-97ef-f0dd769323ed

📥 Commits

Reviewing files that changed from the base of the PR and between 7cba45a and 69c895e.

⛔ Files ignored due to path filters (17)
  • pkg/apis/softwarecomposition/v1beta1/generated.pb.go is excluded by !**/*.pb.go
  • pkg/apis/softwarecomposition/v1beta1/generated.protomessage.pb.go is excluded by !**/*.pb.go
  • pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigentry.go is excluded by !**/generated/**
  • pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfiguration.go is excluded by !**/generated/**
  • pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigurationspec.go is excluded by !**/generated/**
  • pkg/generated/applyconfiguration/utils.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/collapseconfiguration.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_collapseconfiguration.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_softwarecomposition_client.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/generated_expansion.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/softwarecomposition_client.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/generic.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/softwarecomposition/v1beta1/collapseconfiguration.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/softwarecomposition/v1beta1/interface.go is excluded by !**/generated/**
  • pkg/generated/listers/softwarecomposition/v1beta1/collapseconfiguration.go is excluded by !**/generated/**
  • pkg/generated/listers/softwarecomposition/v1beta1/expansion_generated.go is excluded by !**/generated/**
  • pkg/generated/openapi/zz_generated.openapi.go is excluded by !**/generated/**
📒 Files selected for processing (39)
  • artifacts/collapseconfiguration-default-sample.yaml
  • pkg/apis/softwarecomposition/collapse_types.go
  • pkg/apis/softwarecomposition/network_types.go
  • pkg/apis/softwarecomposition/register.go
  • pkg/apis/softwarecomposition/v1beta1/collapse_types.go
  • pkg/apis/softwarecomposition/v1beta1/generated.proto
  • pkg/apis/softwarecomposition/v1beta1/network_types.go
  • pkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.go
  • pkg/apis/softwarecomposition/v1beta1/register.go
  • pkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.go
  • pkg/apis/softwarecomposition/v1beta1/zz_generated.deepcopy.go
  • pkg/apis/softwarecomposition/v1beta1/zz_generated.model_name.go
  • pkg/apis/softwarecomposition/zz_generated.deepcopy.go
  • pkg/apiserver/apiserver.go
  • pkg/registry/file/applicationprofile_processor.go
  • pkg/registry/file/applicationprofile_processor_collapse_provider_test.go
  • pkg/registry/file/applicationprofile_processor_test.go
  • pkg/registry/file/cleanup.go
  • pkg/registry/file/cleanup_test.go
  • pkg/registry/file/collapse_config_provider.go
  • pkg/registry/file/collapse_config_provider_test.go
  • pkg/registry/file/containerprofile_processor.go
  • pkg/registry/file/containerprofile_processor_collapse_provider_test.go
  • pkg/registry/file/dynamicpathdetector/collapse_config_from_crd.go
  • pkg/registry/file/dynamicpathdetector/tests/collapse_config_crd_test.go
  • pkg/registry/file/networkmatch/README.md
  • pkg/registry/file/networkmatch/bench_test.go
  • pkg/registry/file/networkmatch/doc.go
  • pkg/registry/file/networkmatch/match_dns.go
  • pkg/registry/file/networkmatch/match_dns_test.go
  • pkg/registry/file/networkmatch/match_ip.go
  • pkg/registry/file/networkmatch/match_ip_test.go
  • pkg/registry/file/networkmatch/validate.go
  • pkg/registry/file/networkmatch/validate_test.go
  • pkg/registry/softwarecomposition/collapseconfiguration/etcd.go
  • pkg/registry/softwarecomposition/collapseconfiguration/strategy.go
  • pkg/registry/softwarecomposition/collapseconfiguration/strategy_test.go
  • pkg/registry/softwarecomposition/networkneighborhood/strategy.go
  • pkg/registry/softwarecomposition/networkneighborhood/strategy_test.go
📝 Walkthrough

Walkthrough

This PR implements configurable dynamic path collapse for software composition profiling via a new CollapseConfiguration cluster-scoped CRD, extends NetworkNeighbor to support flexible IP/DNS matching, and adds wildcard-aware profile matching validators. Processors are wired to read thresholds from the CRD at runtime with fallback to defaults.

Changes

Dynamic Path Collapse Configuration and Network Matching

Layer / File(s) Summary
CollapseConfiguration API types and NetworkNeighbor extension
pkg/apis/softwarecomposition/collapse_types.go, pkg/apis/softwarecomposition/register.go, pkg/apis/softwarecomposition/v1beta1/collapse_types.go, pkg/apis/softwarecomposition/network_types.go, pkg/apis/softwarecomposition/v1beta1/network_types.go
CollapseConfiguration CRD defines global open/endpoint thresholds and per-prefix overrides with longest-prefix-wins semantics; NetworkNeighbor gains IPAddresses []string alongside deprecated singular IPAddress.
Protobuf definitions and generated Kubernetes code
pkg/apis/softwarecomposition/v1beta1/generated.proto, pkg/apis/softwarecomposition/v1beta1/zz_generated.*.go, pkg/apis/softwarecomposition/zz_generated.deepcopy.go, pkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.go
Protobuf message types for CollapseConfiguration, conversion and deepcopy implementations across v1beta1/internal APIs, OpenAPI model names, and round-trip serialization tests for IPAddresses.
IP and DNS wildcard matching implementation
pkg/registry/file/networkmatch/doc.go, pkg/registry/file/networkmatch/match_ip.go, pkg/registry/file/networkmatch/match_dns.go, pkg/registry/file/networkmatch/validate.go, pkg/registry/file/networkmatch/*_test.go, pkg/registry/file/networkmatch/README.md, pkg/registry/file/networkmatch/bench_test.go
MatchIP supports literal IPs, CIDR ranges, and "*" sentinel; MatchDNS supports RFC4592 leading wildcards, dynamic labels (), and trailing-star semantics; validators enforce entry format rules for admission control.
Runtime collapse settings translation from CRD
pkg/registry/file/dynamicpathdetector/collapse_config_from_crd.go, pkg/registry/file/dynamicpathdetector/tests/collapse_config_crd_test.go
CollapseSettings struct and conversion functions between CRD and runtime representations, defensive copying of threshold/config slices, round-trip preservation, and CollapseSettingsProvider callback type.
CollapseConfiguration CRD storage and admission strategy
pkg/registry/softwarecomposition/collapseconfiguration/etcd.go, pkg/registry/softwarecomposition/collapseconfiguration/strategy.go, pkg/registry/softwarecomposition/collapseconfiguration/strategy_test.go
Kubernetes REST storage, cluster-scoped strategy, validation of non-negative thresholds and per-prefix entries (non-empty prefix with leading /, threshold ≥ 1), duplicate prefix rejection, and complete test coverage.
CRD-backed collapse settings provider
pkg/registry/file/collapse_config_provider.go, pkg/registry/file/collapse_config_provider_test.go
NewCRDCollapseSettingsProvider reads CollapseConfiguration/default CR from storage, falls back to compiled defaults when CR absent/unavailable, supports live updates without caching.
Application and container profile processor collapse settings wiring
pkg/registry/file/applicationprofile_processor.go, pkg/registry/file/containerprofile_processor.go, pkg/apiserver/apiserver.go
Processors gain CollapseSettings provider field, initialize to defaults, accept override via setter, resolve effective settings during PreSave, and pass into deflation logic; apiserver wires shared provider to both processors and registers REST endpoint.
NetworkNeighborhood network profile entry validation
pkg/registry/softwarecomposition/networkneighborhood/strategy.go, pkg/registry/softwarecomposition/networkneighborhood/strategy_test.go
Validates IPAddresses[]/DNSNames[] entries and deprecated singular fields using networkmatch validators during create/update, with precise field path error reporting.
Cleanup and user-managed resource detection
pkg/registry/file/cleanup.go, pkg/registry/file/cleanup_test.go
isUserManaged helper checks ManagedByMetadataKey annotation to skip deleting user-managed resources during namespace cleanup.
ApplicationProfileProcessor and ContainerProfileProcessor collapse behavior tests
pkg/registry/file/applicationprofile_processor_test.go, pkg/registry/file/applicationprofile_processor_collapse_provider_test.go, pkg/registry/file/containerprofile_processor_collapse_provider_test.go
Tests for default initialization, nil fallback, custom provider integration, settings propagation to deflate logic, and end-to-end path collapse with dynamic thresholds.
Sample configuration and documentation
artifacts/collapseconfiguration-default-sample.yaml, pkg/registry/file/networkmatch/README.md
Sample CollapseConfiguration manifest with default thresholds and per-prefix overrides; README documenting wildcard semantics, normalization rules, and performance expectations.

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested labels

release

Suggested reviewers

  • matthyx

🐰 A rabbit's verse on collapse and match,
Dynamic thresholds in a CRD patch,
Networks now dance with wildcard flair,
IP and DNS rules floating in air!

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

@entlein

entlein commented May 16, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Collective re-review request after applying your atomic-split advisory + rabbit-feedback fixes across the full SBOB upstream PR set. Each PR is now scoped to a single tier per your dependency map:

Tier 0 (opens):

Tier 2 (execs):

Tier 3 (network):

Tier 4 (tamper/sign):

Umbrellas:

Please confirm atomicity per your earlier advisory and flag any remaining cross-PR coupling we missed.

@coderabbitai

coderabbitai Bot commented May 16, 2026

Copy link
Copy Markdown
🧩 Analysis chain

@entlein: Triggering a fresh full review of #325 now. I'll also pull lightweight metadata from the related PRs in your tier map to cross-check coupling.


🏁 Script executed:

#!/bin/bash
# Gather PR details for this PR and related storage PRs to check cross-PR coupling
echo "=== PR `#325` ==="
gh pr view 325 --repo kubescape/storage --json title,body,files,baseRefName,headRefName 2>/dev/null | jq '{title,baseRefName,headRefName,files: [.files[].path]}'

echo ""
echo "=== PR `#323` (Tier 0 - path-wildcards + CollapseConfig API) ==="
gh pr view 323 --repo kubescape/storage --json title,body,files,baseRefName,headRefName 2>/dev/null | jq '{title,baseRefName,headRefName,files: [.files[].path]}'

echo ""
echo "=== PR `#322` (Tier 2 - CompareExecArgs) ==="
gh pr view 322 --repo kubescape/storage --json title,body,files,baseRefName,headRefName 2>/dev/null | jq '{title,baseRefName,headRefName,files: [.files[].path]}'

echo ""
echo "=== PR `#324` (Tier 3 - networkmatch) ==="
gh pr view 324 --repo kubescape/storage --json title,body,files,baseRefName,headRefName 2>/dev/null | jq '{title,baseRefName,headRefName,files: [.files[].path]}'

echo ""
echo "=== PR `#326` (Umbrella) ==="
gh pr view 326 --repo kubescape/storage --json title,body,files,baseRefName,headRefName 2>/dev/null | jq '{title,baseRefName,headRefName,files: [.files[].path]}'

Length of output: 193


🏁 Script executed:

#!/bin/bash
# Try alternate approach to fetch PR data
gh auth status 2>&1

echo "=== PR `#325` files ==="
gh api repos/kubescape/storage/pulls/325/files 2>/dev/null | jq '.[].filename' 2>/dev/null || echo "failed"

echo "=== PR `#323` files ==="
gh api repos/kubescape/storage/pulls/323/files 2>/dev/null | jq '.[].filename' 2>/dev/null || echo "failed"

echo "=== PR `#322` files ==="
gh api repos/kubescape/storage/pulls/322/files 2>/dev/null | jq '.[].filename' 2>/dev/null || echo "failed"

echo "=== PR `#324` files ==="
gh api repos/kubescape/storage/pulls/324/files 2>/dev/null | jq '.[].filename' 2>/dev/null || echo "failed"

echo "=== PR `#326` files ==="
gh api repos/kubescape/storage/pulls/326/files 2>/dev/null | jq '.[].filename' 2>/dev/null || echo "failed"

echo "=== PR `#325` base/head ==="
gh api repos/kubescape/storage/pulls/325 2>/dev/null | jq '{title: .title, base: .base.ref, head: .head.ref, state: .state}' 2>/dev/null || echo "failed"

Length of output: 208


@entlein: A full re-review of #325 has been kicked off above. Here is my atomicity confirmation and cross-PR coupling analysis based on the PR objectives and your tier map:


Atomicity Assessment — #325 (CollapseConfig CRD + Processors)

✅ Internally Atomic

The contents of #325 are well-scoped for a CRD introduction:

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.

@matthyx matthyx moved this to WIP in KS PRs tracking May 26, 2026
*out = new(metav1.LabelSelector)
(*in).DeepCopyInto(*out)
}
if in.IPAddresses != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 matthyx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found three merge blockers:

  • the generated deepcopy drift now references NetworkNeighbor.IPAddresses while the handwritten API types still expose IPAddress, 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 CollapseConfiguration resource 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 entlein force-pushed the upstream-pr/sbob-crd-compaction branch from cd83527 to 7cba45a Compare May 27, 2026 17:33
entlein and others added 7 commits May 27, 2026 19:41
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
…nal state

Signed-off-by: entlein <einentlein@gmail.com>
NetworkNeighbor.IPAddresses (added on this branch) was missing
hand-edited entries in the Marshal/Size/String/Unmarshal stanzas of
generated.pb.go, the proto declaration, both conversion functions, and
the internal+v1beta1 deepcopy. Result: TestNetworkNeighbor_IPAddresses_
ProtobufRoundtrip failed and the field was silently dropped on every
real storage write.

This patch adds the missing codec for field 9 (repeated string, wire
tag 0x4a), the .proto declaration, the conversion stanzas in both
directions, and the deepcopy slice copy in both type-system layers.

Codegen pipeline is not run as part of build on this fork (the protoc
image is x86_64-only); hand-edit follows the proven recipe from commit
0d83e2b / ad60a5b.

Resolves matthyx review on network_types.go:68 (2026-05-27).

Signed-off-by: entlein <einentlein@gmail.com>
…naged lifecycle

Signed-off-by: entlein <einentlein@gmail.com>
… final state

Signed-off-by: entlein <einentlein@gmail.com>
The CRD endpoint registered at /apis/.../collapseconfigurations stored
operator-supplied CollapseConfiguration manifests but never consulted
them — applicationProfileStorageImpl and containerProfileStorageImpl
were constructed with the compiled-in DefaultCollapseSettings
provider, and no non-test caller invoked SetCollapseSettings / assigned
CollapseSettings. Applying artifacts/collapseconfiguration-default-
sample.yaml was a no-op.

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 the processor construction so both 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>
@entlein entlein force-pushed the upstream-pr/sbob-crd-compaction branch from 7cba45a to 69c895e Compare May 27, 2026 17:41

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Snapshot collapse settings once per PreSave execution.

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 win

Derive /usr/lib threshold from active collapse configs instead of the global constant.

These tests currently assume /usr/lib uses OpenDynamicThreshold. If a prefix-specific override is added, assertions will fail for the wrong reason. Match the /etc pattern and read threshold via FindConfigForPath("/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") + 1

Also 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

📥 Commits

Reviewing files that changed from the base of the PR and between 98649ae and 7cba45a.

⛔ Files ignored due to path filters (17)
  • pkg/apis/softwarecomposition/v1beta1/generated.pb.go is excluded by !**/*.pb.go
  • pkg/apis/softwarecomposition/v1beta1/generated.protomessage.pb.go is excluded by !**/*.pb.go
  • pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigentry.go is excluded by !**/generated/**
  • pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfiguration.go is excluded by !**/generated/**
  • pkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigurationspec.go is excluded by !**/generated/**
  • pkg/generated/applyconfiguration/utils.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/collapseconfiguration.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_collapseconfiguration.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_softwarecomposition_client.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/generated_expansion.go is excluded by !**/generated/**
  • pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/softwarecomposition_client.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/generic.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/softwarecomposition/v1beta1/collapseconfiguration.go is excluded by !**/generated/**
  • pkg/generated/informers/externalversions/softwarecomposition/v1beta1/interface.go is excluded by !**/generated/**
  • pkg/generated/listers/softwarecomposition/v1beta1/collapseconfiguration.go is excluded by !**/generated/**
  • pkg/generated/listers/softwarecomposition/v1beta1/expansion_generated.go is excluded by !**/generated/**
  • pkg/generated/openapi/zz_generated.openapi.go is excluded by !**/generated/**
📒 Files selected for processing (39)
  • artifacts/collapseconfiguration-default-sample.yaml
  • pkg/apis/softwarecomposition/collapse_types.go
  • pkg/apis/softwarecomposition/network_types.go
  • pkg/apis/softwarecomposition/register.go
  • pkg/apis/softwarecomposition/v1beta1/collapse_types.go
  • pkg/apis/softwarecomposition/v1beta1/generated.proto
  • pkg/apis/softwarecomposition/v1beta1/network_types.go
  • pkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.go
  • pkg/apis/softwarecomposition/v1beta1/register.go
  • pkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.go
  • pkg/apis/softwarecomposition/v1beta1/zz_generated.deepcopy.go
  • pkg/apis/softwarecomposition/v1beta1/zz_generated.model_name.go
  • pkg/apis/softwarecomposition/zz_generated.deepcopy.go
  • pkg/apiserver/apiserver.go
  • pkg/registry/file/applicationprofile_processor.go
  • pkg/registry/file/applicationprofile_processor_collapse_provider_test.go
  • pkg/registry/file/applicationprofile_processor_test.go
  • pkg/registry/file/cleanup.go
  • pkg/registry/file/cleanup_test.go
  • pkg/registry/file/collapse_config_provider.go
  • pkg/registry/file/collapse_config_provider_test.go
  • pkg/registry/file/containerprofile_processor.go
  • pkg/registry/file/containerprofile_processor_collapse_provider_test.go
  • pkg/registry/file/dynamicpathdetector/collapse_config_from_crd.go
  • pkg/registry/file/dynamicpathdetector/tests/collapse_config_crd_test.go
  • pkg/registry/file/networkmatch/README.md
  • pkg/registry/file/networkmatch/bench_test.go
  • pkg/registry/file/networkmatch/doc.go
  • pkg/registry/file/networkmatch/match_dns.go
  • pkg/registry/file/networkmatch/match_dns_test.go
  • pkg/registry/file/networkmatch/match_ip.go
  • pkg/registry/file/networkmatch/match_ip_test.go
  • pkg/registry/file/networkmatch/validate.go
  • pkg/registry/file/networkmatch/validate_test.go
  • pkg/registry/softwarecomposition/collapseconfiguration/etcd.go
  • pkg/registry/softwarecomposition/collapseconfiguration/strategy.go
  • pkg/registry/softwarecomposition/collapseconfiguration/strategy_test.go
  • pkg/registry/softwarecomposition/networkneighborhood/strategy.go
  • pkg/registry/softwarecomposition/networkneighborhood/strategy_test.go

Comment on lines +71 to +73
err := s.Get(context.Background(), key, storage.GetOptions{IgnoreNotFound: true}, crd)
if err != nil || crd.Name == "" {
return dynamicpathdetector.DefaultCollapseSettings()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Bound storage reads with a timeout to prevent request-path stalls.

Line 71 uses context.Background() for s.Get, which can block indefinitely during storage degradation. Use a short timeout and keep the same fallback-to-default behavior on error/deadline.

Proposed fix
 import (
 	"context"
+	"time"
@@
 const DefaultCollapseConfigurationName = "default"
+const collapseConfigurationReadTimeout = 2 * time.Second
@@
-		err := s.Get(context.Background(), key, storage.GetOptions{IgnoreNotFound: true}, crd)
+		ctx, cancel := context.WithTimeout(context.Background(), collapseConfigurationReadTimeout)
+		defer cancel()
+		err := s.Get(ctx, key, storage.GetOptions{IgnoreNotFound: true}, crd)
 		if err != nil || crd.Name == "" {
 			return dynamicpathdetector.DefaultCollapseSettings()
 		}
📝 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
err := s.Get(context.Background(), key, storage.GetOptions{IgnoreNotFound: true}, crd)
if err != nil || crd.Name == "" {
return dynamicpathdetector.DefaultCollapseSettings()
import (
"context"
"time"
)
const DefaultCollapseConfigurationName = "default"
const collapseConfigurationReadTimeout = 2 * time.Second
ctx, cancel := context.WithTimeout(context.Background(), collapseConfigurationReadTimeout)
defer cancel()
err := s.Get(ctx, key, storage.GetOptions{IgnoreNotFound: true}, crd)
if err != nil || crd.Name == "" {
return dynamicpathdetector.DefaultCollapseSettings()
}
🤖 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 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.

@matthyx matthyx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:340 and :419 still generate peers/policy refs only from the deprecated singular neighbor.IPAddress / neighbor.DNS fields. The new ipAddresses payload added in this PR is never consumed here, so a profile that uses only ipAddresses produces no corresponding IPBlock entries in the generated NetworkPolicy.
  • pkg/registry/file/networkneighborhood_processor.go:91 merges duplicate neighbors by Identifier, but on merge it only appends DNSNames and Ports. The new IPAddresses slice 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.

@matthyx

matthyx commented May 28, 2026

Copy link
Copy Markdown
Contributor

One more blocker after checking the generated artifacts: the API type now exposes ipAddresses, but the generated surfaces are still stale.

  • pkg/generated/applyconfiguration/softwarecomposition/v1beta1/networkneighbor.go still only has IPAddress / WithIPAddress, so server-side-apply builders cannot express the new field.
  • pkg/generated/openapi/zz_generated.openapi.go still serves only ipAddress and even keeps it in the required list, so the published schema is out of sync with the updated Go type.

Please regenerate the applyconfiguration/OpenAPI artifacts for NetworkNeighbor as part of this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: WIP

Development

Successfully merging this pull request may close these issues.

2 participants