Remove ApplicationProfile and NetworkNeighborhood CRDs (kubescape/node-agent#864) - #39
Remove ApplicationProfile and NetworkNeighborhood CRDs (kubescape/node-agent#864)#39entlein wants to merge 17 commits into
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 change removes the ApplicationProfile and persisted NetworkNeighborhood APIs, storage, processors, and REST endpoints. ContainerProfile becomes the source for consolidation, generated network policies, migration decoding, configuration sizing, and related tests. ChangesContainerProfile transition
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GeneratedNetworkPolicyStorage
participant ContainerProfileStore
participant GenerateNetworkPolicy
participant KnownServersFinder
GeneratedNetworkPolicyStorage->>ContainerProfileStore: Read ContainerProfile
ContainerProfileStore-->>GeneratedNetworkPolicyStorage: Return profile
GeneratedNetworkPolicyStorage->>GenerateNetworkPolicy: Generate policy from profile
GenerateNetworkPolicy->>KnownServersFinder: Resolve known server mappings
KnownServersFinder-->>GenerateNetworkPolicy: Return server data
GenerateNetworkPolicy-->>GeneratedNetworkPolicyStorage: Return GeneratedNetworkPolicy
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/types.go`:
- Line 214: Add the same ContainerProfile supersession deprecation notice to the
v1beta1 ApplicationProfile type comment in
pkg/apis/softwarecomposition/v1beta1/types.go, preserving the existing CRD
documentation so generated clients expose the marker consistently.
In `@pkg/registry/file/containerprofile_user_managed.go`:
- Around line 37-42: Remove the unused legacy provenance constants
mergedSourceUserAPKey, mergedSourceUserNNKey, mergedSourceUserAPRVKey, and
mergedSourceUserNNRVKey, along with their associated compatibility comments in
the ContainerProfile definitions. Leave active provenance constants and behavior
unchanged.
- Around line 119-121: The ContainerProfile key construction around cpID and
BuildContainerProfileKey currently uses an AP-specific helper without
documenting the shared prefix. Add an inline comment clarifying that
helpers.UserApplicationProfilePrefix is intentionally reused for the
user-managed ContainerProfile, or replace it with a reusable shared ug- prefix
constant while preserving the generated key.
- Around line 128-145: Update the user-managed ContainerProfile read in the
profile-building function around storageImpl.GetWithConn so non-NotFound errors
are returned to the caller instead of leaving cpPresent false. Preserve the
current not-found behavior as absence, while successful reads continue merging
the profile and setting hasOverlay; ensure refreshMergedProfile receives the
storage error and does not delete the merged artifact on transient read
failures.
- Around line 126-142: Collapse the redundant cpPresent and hasOverlay flags in
ContainerProfileProcessor.buildMergedProfile into a single boolean, using it for
the existing presence check and overlay result. Update the successful
user-managed profile branch and return path to return merged, true, nil directly
as requested.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 87e3a0ab-105c-4430-a4aa-1313883a0a4f
📒 Files selected for processing (4)
pkg/apis/softwarecomposition/network_types.gopkg/apis/softwarecomposition/types.gopkg/registry/file/containerprofile_user_managed.gopkg/registry/file/containerprofile_user_managed_test.go
| // +genclient | ||
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
|
||
| // Deprecated: superseded by ContainerProfile; retained for backward-compatibility views and slated for removal. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'types.go|network_types.go' pkg/apis | xargs rg -n -B4 '^type (ApplicationProfile|NetworkNeighborhood) struct'Repository: k8sstormcenter/storage
Length of output: 1966
Mirror the Deprecated marker on pkg/apis/softwarecomposition/v1beta1/types.go.
pkg/apis/softwarecomposition/types.go notes ApplicationProfile is superseded by ContainerProfile, but the versioned type still only has the generic CRD struct comment. Add the same user-facing deprecation notice to the v1beta1 type so generated clients show it.
🤖 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/apis/softwarecomposition/types.go` at line 214, Add the same
ContainerProfile supersession deprecation notice to the v1beta1
ApplicationProfile type comment in
pkg/apis/softwarecomposition/v1beta1/types.go, preserving the existing CRD
documentation so generated clients expose the marker consistently.
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/generatednetworkpolicy.go`:
- Around line 96-107: Verify the lookup semantics used by Get and GetList around
replaceKeyForKind and containerProfileToNetworkNeighborhood: resolve
workload-level GeneratedNetworkPolicy/NetworkNeighborhood requests to the
corresponding ContainerProfiles despite per-container names, aggregating all
profiles for the workload where required. Reuse the established
workload-resolution or storage-wiring path if one exists, and ensure Get returns
the workload aggregate while GetList emits one aggregate policy per workload
rather than one per container.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 53ddfbaf-0bb7-4955-9540-cdfeadea33b3
⛔ Files ignored due to path filters (24)
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/applicationprofile.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/applicationprofilecontainer.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/applicationprofilespec.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/networkneighborhood.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/networkneighborhoodcontainer.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/networkneighborhoodspec.gois excluded by!**/generated/**pkg/generated/applyconfiguration/utils.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/applicationprofile.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_applicationprofile.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_networkneighborhood.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/networkneighborhood.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/applicationprofile.gois excluded by!**/generated/**pkg/generated/informers/externalversions/softwarecomposition/v1beta1/interface.gois excluded by!**/generated/**pkg/generated/informers/externalversions/softwarecomposition/v1beta1/networkneighborhood.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/applicationprofile.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/expansion_generated.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/networkneighborhood.gois excluded by!**/generated/**pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (50)
pkg/apis/softwarecomposition/network_types.gopkg/apis/softwarecomposition/register.gopkg/apis/softwarecomposition/types.gopkg/apis/softwarecomposition/types_test.gopkg/apis/softwarecomposition/v1beta1/generated.protopkg/apis/softwarecomposition/v1beta1/network_types.gopkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.gopkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/networkpolicy.gopkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/networkpolicy_test.gopkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/testdata/known-servers.jsonpkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/testdata/nn-operator.jsonpkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/testdata/np-operator.jsonpkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/testdata/np.new.jsonpkg/apis/softwarecomposition/v1beta1/register.gopkg/apis/softwarecomposition/v1beta1/types.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/config/config.gopkg/config/config_test.gopkg/registry/file/applicationprofile_processor.gopkg/registry/file/applicationprofile_processor_collapse_provider_test.gopkg/registry/file/applicationprofile_processor_test.gopkg/registry/file/applicationprofile_storage.gopkg/registry/file/containerprofile_processor.gopkg/registry/file/containerprofile_processor_test.gopkg/registry/file/containerprofile_storage.gopkg/registry/file/containerprofile_storage_interface.gopkg/registry/file/containerprofile_user_managed.gopkg/registry/file/containerprofile_user_managed_test.gopkg/registry/file/dynamicpathdetector/tests/compare_exec_args_test.gopkg/registry/file/dynamicpathdetector/tests/coverage_test.gopkg/registry/file/dynamicpathdetector/tests/execargs_wildcard_ap_test.gopkg/registry/file/generatednetworkpolicy.gopkg/registry/file/generatednetworkpolicy_test.gopkg/registry/file/networkneighborhood_ipcollapse.gopkg/registry/file/networkneighborhood_processor.gopkg/registry/file/networkneighborhood_processor_test.gopkg/registry/file/networkneighborhood_storage.gopkg/registry/file/storage_test.gopkg/registry/file/testdata/expectedFilesToDelete.jsonpkg/registry/softwarecomposition/applicationprofile/etcd.gopkg/registry/softwarecomposition/applicationprofile/strategy.gopkg/registry/softwarecomposition/applicationprofile/strategy_test.gopkg/registry/softwarecomposition/collapseconfiguration/strategy_test.gopkg/registry/softwarecomposition/networkneighborhood/etcd.gopkg/registry/softwarecomposition/networkneighborhood/strategy.gopkg/registry/softwarecomposition/networkneighborhood/strategy_test.go
💤 Files with no reviewable changes (33)
- pkg/apis/softwarecomposition/v1beta1/register.go
- pkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/testdata/np.new.json
- pkg/registry/softwarecomposition/applicationprofile/etcd.go
- pkg/registry/file/networkneighborhood_processor_test.go
- pkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/testdata/known-servers.json
- pkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/testdata/np-operator.json
- pkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/networkpolicy.go
- pkg/registry/softwarecomposition/networkneighborhood/etcd.go
- pkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/testdata/nn-operator.json
- pkg/registry/softwarecomposition/applicationprofile/strategy.go
- pkg/apis/softwarecomposition/register.go
- pkg/registry/file/networkneighborhood_processor.go
- pkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/networkpolicy_test.go
- pkg/registry/file/applicationprofile_processor_test.go
- pkg/registry/softwarecomposition/networkneighborhood/strategy_test.go
- pkg/registry/softwarecomposition/networkneighborhood/strategy.go
- pkg/registry/file/applicationprofile_processor_collapse_provider_test.go
- pkg/registry/softwarecomposition/applicationprofile/strategy_test.go
- pkg/apis/softwarecomposition/v1beta1/network_types.go
- pkg/registry/file/containerprofile_storage_interface.go
- pkg/registry/file/applicationprofile_storage.go
- pkg/registry/file/networkneighborhood_storage.go
- pkg/registry/file/containerprofile_storage.go
- pkg/registry/file/applicationprofile_processor.go
- pkg/registry/file/testdata/expectedFilesToDelete.json
- pkg/apis/softwarecomposition/v1beta1/zz_generated.model_name.go
- pkg/apis/softwarecomposition/v1beta1/generated.proto
- pkg/config/config.go
- pkg/apis/softwarecomposition/types.go
- pkg/apis/softwarecomposition/v1beta1/zz_generated.deepcopy.go
- pkg/apis/softwarecomposition/v1beta1/types.go
- pkg/apis/softwarecomposition/zz_generated.deepcopy.go
- pkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.go
entlein
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/config/config.go`:
- Line 24: Update LoadConfig and the configuration binding around
MaxContainerProfileSize to continue accepting the legacy
maxApplicationProfileSize key, using an alias or explicit migration while
preserving the existing 40000 default and supporting the new
maxContainerProfileSize key.
In `@pkg/registry/file/storage.go`:
- Around line 675-678: Update the type selection in the Get migration path and
the corresponding logic around the second referenced location to use an explicit
type switch for supported ContainerProfile and SeccompProfile objects. Return an
error for any unsupported object type instead of defaulting to the
ContainerProfile schema, preserving the existing schema selection for supported
types.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b8b695af-450c-40ce-9c52-c4475e4e953c
📒 Files selected for processing (11)
cmd/migration/main.gopkg/apis/softwarecomposition/types.gopkg/apis/softwarecomposition/v1beta1/types.gopkg/config/config.gopkg/config/config_test.gopkg/registry/file/containerprofile_aggregator_test.gopkg/registry/file/containerprofile_processor.gopkg/registry/file/containerprofile_processor_collapse_provider_test.gopkg/registry/file/dynamicpathdetector/types.gopkg/registry/file/storage.gopkg/registry/softwarecomposition/collapseconfiguration/etcd.go
💤 Files with no reviewable changes (1)
- pkg/registry/file/containerprofile_aggregator_test.go
| typeName := "ContainerProfile" | ||
| if _, ok := objPtr.(*softwarecomposition.SeccompProfile); ok { | ||
| typeName = "SeccompProfile" | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'migrateObject\(|appendGobObjectFromFile\(' --glob '*.go' .Repository: k8sstormcenter/storage
Length of output: 4430
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== storage.go outline == "
ast-grep outline ./pkg/registry/file/storage.go --match StorageImpl --view expanded || true
echo "== relevant migrateObject section =="
sed -n '642,705p' ./pkg/registry/file/storage.go | nl -ba -v642
echo "== relevant migration object creation calls =="
sed -n '560,642p' ./pkg/registry/file/storage.go | nl -ba -v560
echo "== relevant appendGobSection =="
sed -n '1132,1198p' ./pkg/registry/file/storage.go | nl -ba -v1132
echo "== keyFromPath and key construction =="
rg -n 'func \(s \*StorageImpl\) keyFromPath|keyFromPath\(' --glob '*.go' ./pkg/registry/file/storage.goRepository: k8sstormcenter/storage
Length of output: 4706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant code sections =="
sed -n '528,642p' pkg/registry/file/storage.go
printf '\n---\n'
sed -n '658,705p' pkg/registry/file/storage.go
printf '\n---\n'
sed -n '1118,1198p' pkg/registry/file/storage.go
echo "== object creator usages in storage.go =="
rg -n -C3 'New|Create\(.*runtime\.Object|runtime\.NewObject|scheme\.NewObject|runtime\.Scheme|TypeFor|ObjectKind|SeccompProfile|ContainerProfile' pkg/registry/file/storage.go
echo "== all SeccompProfile/ContainerProfile references =="
rg -n 'SeccompProfile|ContainerProfile|appGobList|appGobObject|appGobPayload|payloadFile\[' pkg/registry/file/storage.goRepository: k8sstormcenter/storage
Length of output: 21283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== storage interface methods and implementation signatures =="
rg -n -C2 'type StorageQuerier interface|type StorageImpl struct|func \(s \*StorageImpl\) (Get|GetList|GetByNamespace|GuaranteedUpdate)\(' pkg/registry/file pkg/registry/storage.go 2>/dev/null || rg -n -C2 'type StorageQuerier interface|type StorageImpl struct|func \(s \*StorageImpl\) (Get|GetList|GetByNamespace|GuaranteedUpdate)\(' .
echo "== imports =="
sed -n '1,120p' pkg/registry/file/storage.go
echo "== appGob path references =="
rg -n -C3 'appGobList|appGobObject|appGobPayload|appGob|payloadFiles|filepath\.WalkDir|filepath\.Walk' pkg/registry/file -g '*.go'
echo "== package files under pkg/registry =="
git ls-files 'pkg/registry/*.go' | sed -n '1,200p'Repository: k8sstormcenter/storage
Length of output: 18080
Fail closed for unsupported migration object types.
Get can invoke this path for any Go object passed through storage.Interface.Get, and GetList walks legacy Gob/G extensions as payload files. If the legacy payload is for a third kind, both paths decode it into the requested ContainerProfile schema instead of returning an error. Use an explicit type switch and return an error for unsupported types.
Also applies to: 1187-1190
🤖 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/storage.go` around lines 675 - 678, Update the type
selection in the Get migration path and the corresponding logic around the
second referenced location to use an explicit type switch for supported
ContainerProfile and SeccompProfile objects. Return an error for any unsupported
object type instead of defaulting to the ContainerProfile schema, preserving the
existing schema selection for supported types.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/registry/file/containerprofile_processor.go (2)
265-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDropping the
applicationprofiles/networkneighborhoods(and merged CP) cleanup entries leaves pre-existing files unreclaimed.Clusters upgrading from a prior release still have
applicationprofiles/,networkneighborhoods/, and mergedug-/effective ContainerProfile files on disk. With the API types and these cleanup handlers gone, nothing ever deletes them, so the storage PVC keeps that data indefinitely. Consider a one-shot startup purge (or a migration step incmd/migration) for those directories/keys.🤖 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/containerprofile_processor.go` around lines 265 - 267, The cleanup map in the container profile processing flow only handles current ContainerProfile files, leaving legacy applicationprofiles, networkneighborhoods, and merged ug-/effective ContainerProfile data unreclaimed. Add a one-time startup purge or migration step that removes those legacy directories and keys from existing storage, while preserving deleteByTemplateHashOrWlid for current containerprofiles.
532-553: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore the aggregated workload annotations before replacing the merged flow.
updateProfilesaves only the observed profile and exits; the removedupdateAggregatedProfiles/refreshMergedProfileflow is gone, butComputeAggregatedDatastill runs only its standalone tests. If the aggregated key still needsstatus,completion, andSyncChecksumfor consumers, add equivalent write paths that persist those annotations.🤖 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/containerprofile_processor.go` around lines 532 - 553, Update updateProfile to restore persistence of aggregated workload annotations after ComputeAggregatedData produces the merged result, including status, completion, and SyncChecksum on the aggregated key. Add equivalent write paths for the removed updateAggregatedProfiles/refreshMergedProfile behavior while preserving the existing observed-profile save and no-new-data handling.pkg/registry/file/containerprofile_storage.go (1)
110-125: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winComment now describes a removed mechanism, and the defect it documents is still unfixed.
The rationale references the merged CP and its
merged-source-observed-rvannotation, which no longer exist. What remains true is the underlying behavior:cachedExistingObjectisnil, soGuaranteedUpdate's "same serialized contents" short-circuit never triggers and the observed CP is rewritten (RV bump + watch event) on every tick carrying new time-series data, even when byte-identical. Either pass the loaded profile ascachedExistingObjector trim the comment to the still-relevant part.🤖 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/containerprofile_storage.go` around lines 110 - 125, Update the GuaranteedUpdateWithConn call in the consolidation flow to pass the loaded profile as cachedExistingObject instead of a new empty ContainerProfile, allowing identical serialized content to skip the write and preventing unnecessary ResourceVersion bumps and watch events. Remove the stale comment references to merged CP behavior and retain only rationale relevant to the cached profile and write short-circuit.pkg/apiserver/apiserver.go (1)
141-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWiring comments still assume two processors (AP + CP). Both blocks were written when an ApplicationProfile processor existed alongside the ContainerProfile one; only
containerProfileProcessorremains.
pkg/apiserver/apiserver.go#L141-L150: drop "application/" and make the processor singular.pkg/apiserver/apiserver.go#L165-L176: replace "both processors" / "wired into both processors" with the single container profile processor.🤖 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/apiserver/apiserver.go` around lines 141 - 150, Update the wiring comments in pkg/apiserver/apiserver.go at lines 141-150 and 165-176 to describe only the singular container profile processor: remove “application/” references, change plural “processors” wording to singular, and replace “both processors”/“wired into both processors” with wording that identifies containerProfileProcessor alone. No code changes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/apiserver/apiserver.go`:
- Around line 141-150: Update the wiring comments in pkg/apiserver/apiserver.go
at lines 141-150 and 165-176 to describe only the singular container profile
processor: remove “application/” references, change plural “processors” wording
to singular, and replace “both processors”/“wired into both processors” with
wording that identifies containerProfileProcessor alone. No code changes are
needed.
In `@pkg/registry/file/containerprofile_processor.go`:
- Around line 265-267: The cleanup map in the container profile processing flow
only handles current ContainerProfile files, leaving legacy applicationprofiles,
networkneighborhoods, and merged ug-/effective ContainerProfile data
unreclaimed. Add a one-time startup purge or migration step that removes those
legacy directories and keys from existing storage, while preserving
deleteByTemplateHashOrWlid for current containerprofiles.
- Around line 532-553: Update updateProfile to restore persistence of aggregated
workload annotations after ComputeAggregatedData produces the merged result,
including status, completion, and SyncChecksum on the aggregated key. Add
equivalent write paths for the removed
updateAggregatedProfiles/refreshMergedProfile behavior while preserving the
existing observed-profile save and no-new-data handling.
In `@pkg/registry/file/containerprofile_storage.go`:
- Around line 110-125: Update the GuaranteedUpdateWithConn call in the
consolidation flow to pass the loaded profile as cachedExistingObject instead of
a new empty ContainerProfile, allowing identical serialized content to skip the
write and preventing unnecessary ResourceVersion bumps and watch events. Remove
the stale comment references to merged CP behavior and retain only rationale
relevant to the cached profile and write short-circuit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: be007b95-3eb0-4e8a-b532-22485adeb42a
📒 Files selected for processing (8)
pkg/apiserver/apiserver.gopkg/registry/file/containerprofile_aggregator_test.gopkg/registry/file/containerprofile_processor.gopkg/registry/file/containerprofile_rest_storage.gopkg/registry/file/containerprofile_storage.gopkg/registry/file/containerprofile_storage_interface.gopkg/registry/file/containerprofile_user_managed.gopkg/registry/file/containerprofile_user_managed_test.go
💤 Files with no reviewable changes (5)
- pkg/registry/file/containerprofile_rest_storage.go
- pkg/registry/file/containerprofile_aggregator_test.go
- pkg/registry/file/containerprofile_user_managed.go
- pkg/registry/file/containerprofile_storage_interface.go
- pkg/registry/file/containerprofile_user_managed_test.go
| [ | ||
| { | ||
| "apiVersion": "spdx.softwarecomposition.kubescape.io/v1beta1", | ||
| "kind": "KnownServer", |
There was a problem hiding this comment.
chicken, I said migrate the tests, not delete them . find where those fixtures go and migrate them there
There was a problem hiding this comment.
chicken — migrated, not dropped. The generation tests moved with the package to pkg/apis/softwarecomposition/networkpolicy/v2/: known-servers.json / np-operator.json / np.new.json carried over (rename-preserved), and the NetworkNeighborhood-input cases were converted to ContainerProfile inputs. TestGenerateNetworkPolicy keeps the same subtests (known-servers, not-ready status, multi-container, dns-neighbors, …) and TestGenerateNetworkPolicyFromFile is the file oracle: cp-operator.json → np-operator.json/np.new.json goldens. All green. The only file not carried is nn-operator.json (a NetworkNeighborhood input); its ContainerProfile equivalent cp-operator.json drives the oracle instead.
| // NetworkNeighborhood is an in-process, non-CRD intermediate used only to feed | ||
| // the GeneratedNetworkPolicy generation. It is projected from ContainerProfile | ||
| // data at request time and is never stored or served as an API object. | ||
| type NetworkNeighborhood struct { |
There was a problem hiding this comment.
chicken — intentionally kept. NetworkNeighbor is the per-connection element the generated NetworkPolicy is built from: the ContainerProfile-based GenerateNetworkPolicy reads ingress/egress neighbor lists off the ContainerProfile and emits rules from them (listIngress/EgressNetworkNeighbors → generateIngress/EgressRule). 26 non-test references. It is NetworkNeighborhood (the aggregate CRD) that's removed, not NetworkNeighbor.
| // MatchExecArgs(profileArgs, true, runtimeArgs); allowed if ANY vector matches. | ||
| func matchAP(ap *types.ApplicationProfile, container string, runtime []string) bool { | ||
| for _, c := range ap.Spec.Containers { | ||
| if c.Name != container { |
There was a problem hiding this comment.
how did we now solve the multi-container naming? make sure this is consistent
There was a problem hiding this comment.
Resolved and made consistent across the read path. ContainerProfiles are per-container (replicaset-named object, e.g. replicaset-<workload>-<hash>-<container>-<hash>), while a NetworkPolicy is workload-level — a policy selects pods, and all containers of a pod share the network namespace. GeneratedNetworkPolicy now groups a namespace's ContainerProfiles by the workload-identity labels (kubescape.io/workload-kind + kubescape.io/workload-name → <lower(kind)>-<name>, the same name GenerateNetworkPolicy derives) and unions their ingress/egress into one policy per workload: Get(<kind>-<name>) aggregates the workload's per-container profiles; GetList emits one policy per workload, not one per container. The internal listing is forced to full-spec (the default list path returns metadata only, which lacks ingress/egress).
Consistency: the group key, the requested name, and the generated policy name are all derived from the same labels, so they always agree — checked against a live cluster where a Deployment's profiles are replicaset-named but carry top-owner labels (Deployment/<name>), and multi-container StatefulSets group into one policy. Pinned by generatednetworkpolicy_multicontainer_test.go (workload-level Get aggregation + one-policy-per-workload GetList, both failing pre-fix); the two prior Get subtests that requested a profile by its object name were corrected to the workload-level name.
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/generatednetworkpolicy.go (1)
57-97:⚠️ Potential issue | 🟠 MajorPer-container ContainerProfile naming vs. workload-level Get/GetList still unresolved.
Still relevant:
Getexpects aContainerProfileunder the exact same name as the requestedGeneratedNetworkPolicykey, andGetListnow emits one entry perContainerProfilerather than per workload. The newTestGenerateNetworkPolicy_PerContainerProfilestest (added in this PR, innetworkpolicy_test.go) confirms the consequence concretely: two per-containerContainerProfiles for the same workload produceNetworkPolicyspecs sharing the identical name (deployment-mc) but with disjoint ingress/egress — i.e., there is still no aggregation step reconciling per-container profiles into one workload-level policy view here.Please confirm whether this is intentionally deferred (e.g., consumers apply the
GeneratedNetworkPolicy.Spec.ObjectMeta.Namecollision knowingly) or still needs workload-level resolution/aggregation.Also applies to: 107-131
🤖 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/generatednetworkpolicy.go` around lines 57 - 97, Resolve GeneratedNetworkPolicyStorage.Get and GetList at the workload level rather than treating each ContainerProfile name as a separate policy key. Aggregate all per-container ContainerProfiles belonging to the requested workload before calling networkpolicy.GenerateNetworkPolicy, and make GetList emit one reconciled policy per workload with consistent metadata naming. Preserve the existing known-server lookup and serialization behavior.
🤖 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/containerprofile_processor.go`:
- Around line 532-537: Update ContainerProfileProcessor.updateProfile so
profiles missing helpers.InstanceIDMetadataKey do not reach the unconditional
Kubernetes slug dispatch after being accepted. Ensure profile initialization
establishes the annotation before dispatch, or return through an explicit path
that skips slug dispatch while still deleting the processed time series and
preserving the intended invalid-profile handling.
---
Outside diff comments:
In `@pkg/registry/file/generatednetworkpolicy.go`:
- Around line 57-97: Resolve GeneratedNetworkPolicyStorage.Get and GetList at
the workload level rather than treating each ContainerProfile name as a separate
policy key. Aggregate all per-container ContainerProfiles belonging to the
requested workload before calling networkpolicy.GenerateNetworkPolicy, and make
GetList emit one reconciled policy per workload with consistent metadata naming.
Preserve the existing known-server lookup and serialization behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 99c404eb-1e66-40ad-8082-b78f881f8861
⛔ Files ignored due to path filters (25)
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/applicationprofile.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/applicationprofilecontainer.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/applicationprofilespec.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/containerprofilespec.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/networkneighborhood.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/networkneighborhoodcontainer.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/networkneighborhoodspec.gois excluded by!**/generated/**pkg/generated/applyconfiguration/utils.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/applicationprofile.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_applicationprofile.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1/fake/fake_networkneighborhood.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/networkneighborhood.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/applicationprofile.gois excluded by!**/generated/**pkg/generated/informers/externalversions/softwarecomposition/v1beta1/interface.gois excluded by!**/generated/**pkg/generated/informers/externalversions/softwarecomposition/v1beta1/networkneighborhood.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/applicationprofile.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/expansion_generated.gois excluded by!**/generated/**pkg/generated/listers/softwarecomposition/v1beta1/networkneighborhood.gois excluded by!**/generated/**pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (62)
cmd/migration/main.gopkg/apis/softwarecomposition/network_types.gopkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy.gopkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy_file_test.gopkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy_test.gopkg/apis/softwarecomposition/networkpolicy/v2/testdata/cp-operator.jsonpkg/apis/softwarecomposition/networkpolicy/v2/testdata/known-servers.jsonpkg/apis/softwarecomposition/networkpolicy/v2/testdata/np-operator.jsonpkg/apis/softwarecomposition/networkpolicy/v2/testdata/np.new.jsonpkg/apis/softwarecomposition/register.gopkg/apis/softwarecomposition/types.gopkg/apis/softwarecomposition/types_test.gopkg/apis/softwarecomposition/v1beta1/generated.protopkg/apis/softwarecomposition/v1beta1/network_types.gopkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.gopkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/networkpolicy.gopkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/networkpolicy_test.gopkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/testdata/nn-operator.jsonpkg/apis/softwarecomposition/v1beta1/register.gopkg/apis/softwarecomposition/v1beta1/types.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/config/config.gopkg/config/config_test.gopkg/registry/file/applicationprofile_processor.gopkg/registry/file/applicationprofile_processor_collapse_provider_test.gopkg/registry/file/applicationprofile_processor_test.gopkg/registry/file/applicationprofile_storage.gopkg/registry/file/cleanup.gopkg/registry/file/containerprofile_aggregator_test.gopkg/registry/file/containerprofile_processor.gopkg/registry/file/containerprofile_processor_collapse_provider_test.gopkg/registry/file/containerprofile_processor_test.gopkg/registry/file/containerprofile_rest_storage.gopkg/registry/file/containerprofile_storage.gopkg/registry/file/containerprofile_storage_interface.gopkg/registry/file/containerprofile_user_managed.gopkg/registry/file/containerprofile_user_managed_test.gopkg/registry/file/dynamicpathdetector/tests/compare_exec_args_test.gopkg/registry/file/dynamicpathdetector/tests/coverage_test.gopkg/registry/file/dynamicpathdetector/tests/execargs_wildcard_ap_test.gopkg/registry/file/dynamicpathdetector/types.gopkg/registry/file/generatednetworkpolicy.gopkg/registry/file/generatednetworkpolicy_test.gopkg/registry/file/networkneighborhood_ipcollapse.gopkg/registry/file/networkneighborhood_processor.gopkg/registry/file/networkneighborhood_processor_test.gopkg/registry/file/networkneighborhood_storage.gopkg/registry/file/storage.gopkg/registry/file/storage_test.gopkg/registry/file/testdata/expectedFilesToDelete.jsonpkg/registry/softwarecomposition/applicationprofile/etcd.gopkg/registry/softwarecomposition/applicationprofile/strategy.gopkg/registry/softwarecomposition/applicationprofile/strategy_test.gopkg/registry/softwarecomposition/collapseconfiguration/etcd.gopkg/registry/softwarecomposition/collapseconfiguration/strategy_test.gopkg/registry/softwarecomposition/networkneighborhood/etcd.gopkg/registry/softwarecomposition/networkneighborhood/strategy.gopkg/registry/softwarecomposition/networkneighborhood/strategy_test.go
💤 Files with no reviewable changes (30)
- pkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/testdata/nn-operator.json
- pkg/registry/softwarecomposition/applicationprofile/etcd.go
- pkg/registry/softwarecomposition/networkneighborhood/etcd.go
- pkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/networkpolicy_test.go
- pkg/apis/softwarecomposition/v1beta1/register.go
- pkg/registry/file/testdata/expectedFilesToDelete.json
- pkg/apis/softwarecomposition/v1beta1/network_types.go
- pkg/registry/file/containerprofile_rest_storage.go
- pkg/apis/softwarecomposition/v1beta1/networkpolicy/v2/networkpolicy.go
- pkg/apis/softwarecomposition/register.go
- pkg/registry/file/containerprofile_aggregator_test.go
- pkg/registry/file/containerprofile_user_managed.go
- pkg/registry/file/applicationprofile_processor.go
- pkg/registry/softwarecomposition/applicationprofile/strategy_test.go
- pkg/registry/softwarecomposition/networkneighborhood/strategy.go
- pkg/registry/file/networkneighborhood_storage.go
- pkg/registry/softwarecomposition/applicationprofile/strategy.go
- pkg/registry/file/networkneighborhood_processor.go
- pkg/registry/softwarecomposition/networkneighborhood/strategy_test.go
- pkg/registry/file/applicationprofile_storage.go
- pkg/registry/file/applicationprofile_processor_collapse_provider_test.go
- pkg/registry/file/networkneighborhood_processor_test.go
- pkg/apis/softwarecomposition/v1beta1/zz_generated.model_name.go
- pkg/apis/softwarecomposition/network_types.go
- pkg/registry/file/applicationprofile_processor_test.go
- pkg/registry/file/containerprofile_storage_interface.go
- pkg/registry/file/containerprofile_user_managed_test.go
- pkg/apis/softwarecomposition/v1beta1/zz_generated.deepcopy.go
- pkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.go
- pkg/apis/softwarecomposition/zz_generated.deepcopy.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/containerprofile_consolidate_golden_test.go`:
- Around line 15-26: Update TestConsolidateContainerProfileGolden so its
documented SBOM-subsumption coverage matches the test data: provide matching
SBOM path fixtures and pass the populated SBOM set to
DeflateContainerProfileSpec, or remove SBOM subsumption from the contract
comments if it is not intended to be exercised.
In `@pkg/registry/file/containerprofile_processor.go`:
- Around line 192-200: Update the ContainerProfileStorage metadata lookup in the
surrounding update flow so create semantics apply only when
GetContainerProfileMetadataNoLock returns a not-found error. Propagate or abort
on transient SQLite, decoding, context, and other unexpected errors, while
preserving the completed-status guard and annotation update for the valid
not-found/create case to prevent completed profiles from regressing to Learning.
In `@pkg/registry/file/containerprofile_storage.go`:
- Around line 82-94: Add a real-storage integration test covering
GetContainerProfileMetadataNoLock during GuaranteedUpdate: arrange for PreSave
to invoke the no-lock metadata method while the same key’s write lock is held,
and assert the update completes successfully without deadlocking. Use the
SQLite-backed storage path and verify the metadata result, ensuring the test
exercises the hasWriteLock behavior rather than the completed-guard mock
implementation.
In `@pkg/registry/file/generatednetworkpolicy.go`:
- Line 172: The ContainerProfile listing truncates source data before workload
aggregation. In pkg/registry/file/generatednetworkpolicy.go:172-172, update the
listNamespaceContainerProfiles flow to paginate through every profile needed for
the requested workload instead of treating containerProfileListLimit as
complete; at pkg/registry/file/generatednetworkpolicy.go:227-227, remove the
GeneratedNetworkPolicy client limit from the source listing. Aggregate complete
workloads first, then apply output pagination to the generated policies.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d9321a5d-18d0-4523-b66f-654a4c54f593
📒 Files selected for processing (14)
pkg/apis/softwarecomposition/networkpolicy/v2/testdata/cp-operator.jsonpkg/registry/file/containerprofile_aggregator_test.gopkg/registry/file/containerprofile_completed_guard_test.gopkg/registry/file/containerprofile_consolidate_golden_test.gopkg/registry/file/containerprofile_processor.gopkg/registry/file/containerprofile_storage.gopkg/registry/file/containerprofile_storage_interface.gopkg/registry/file/generatednetworkpolicy.gopkg/registry/file/generatednetworkpolicy_multicontainer_test.gopkg/registry/file/generatednetworkpolicy_test.gopkg/registry/file/testdata/consolidate_golden/consolidated.golden.jsonpkg/registry/file/testdata/consolidate_golden/window_1.jsonpkg/registry/file/testdata/consolidate_golden/window_2.jsonpkg/registry/file/testdata/consolidate_golden/window_3.json
| // TestConsolidateContainerProfileGolden is a golden-regression oracle for the | ||
| // time-series -> consolidated ContainerProfile transformation. | ||
| // | ||
| // It pins the two-stage consolidation contract that the processor runs on every | ||
| // ConsolidateTimeSeries pass, using the same package-level entry points the | ||
| // production path uses: | ||
| // | ||
| // 1. accumulation: mergeContainerProfileTS folds each successive learning-window | ||
| // ContainerProfile into the running profile (append semantics — no dedup yet); | ||
| // 2. collapse: DeflateContainerProfileSpec deflates the accumulated spec — opens | ||
| // wildcarding + SBOM subsumption, endpoint merge, exec/syscall/capability/arch | ||
| // dedup+sort, rule-policy union, and network-neighbor CIDR collapse. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Exercise the claimed SBOM-subsumption behavior.
The test comments include SBOM subsumption in the contract, but DeflateContainerProfileSpec is called with a nil SBOM set and no fixture provides matching SBOM paths. Add SBOM fixtures/set data, or remove SBOM subsumption from the documented coverage.
Also applies to: 73-74
🤖 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/containerprofile_consolidate_golden_test.go` around lines
15 - 26, Update TestConsolidateContainerProfileGolden so its documented
SBOM-subsumption coverage matches the test data: provide matching SBOM path
fixtures and pass the populated SBOM set to DeflateContainerProfileSpec, or
remove SBOM subsumption from the contract comments if it is not intended to be
exercised.
| if existingProfile, err := a.ContainerProfileStorage.GetContainerProfileMetadataNoLock(ctx, key); err == nil { | ||
| if existingProfile.Annotations[helpers.StatusMetadataKey] == helpers.Completed && | ||
| profile.Annotations[helpers.StatusMetadataKey] == helpers.Learning { | ||
| if profile.Annotations == nil { | ||
| profile.Annotations = make(map[string]string) | ||
| } | ||
| profile.Annotations[helpers.StatusMetadataKey] = helpers.Completed | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not treat every metadata-read error as a create.
This path bypasses the completed-status guard for any error, including transient SQLite, decoding, or context failures. Only a not-found result should allow create semantics; unexpected errors must abort the update, otherwise a completed profile can regress to Learning.
Proposed fix
- if existingProfile, err := a.ContainerProfileStorage.GetContainerProfileMetadataNoLock(ctx, key); err == nil {
+ existingProfile, err := a.ContainerProfileStorage.GetContainerProfileMetadataNoLock(ctx, key)
+ if err != nil {
+ if !storage.IsNotFound(err) {
+ return fmt.Errorf("failed to read existing container profile metadata: %w", err)
+ }
+ } else {
if existingProfile.Annotations[helpers.StatusMetadataKey] == helpers.Completed &&
profile.Annotations[helpers.StatusMetadataKey] == helpers.Learning {
if profile.Annotations == nil {
profile.Annotations = make(map[string]string)
}
profile.Annotations[helpers.StatusMetadataKey] = helpers.Completed
}
}📝 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.
| if existingProfile, err := a.ContainerProfileStorage.GetContainerProfileMetadataNoLock(ctx, key); err == nil { | |
| if existingProfile.Annotations[helpers.StatusMetadataKey] == helpers.Completed && | |
| profile.Annotations[helpers.StatusMetadataKey] == helpers.Learning { | |
| if profile.Annotations == nil { | |
| profile.Annotations = make(map[string]string) | |
| } | |
| profile.Annotations[helpers.StatusMetadataKey] = helpers.Completed | |
| } | |
| } | |
| existingProfile, err := a.ContainerProfileStorage.GetContainerProfileMetadataNoLock(ctx, key) | |
| if err != nil { | |
| if !storage.IsNotFound(err) { | |
| return fmt.Errorf("failed to read existing container profile metadata: %w", err) | |
| } | |
| } else { | |
| if existingProfile.Annotations[helpers.StatusMetadataKey] == helpers.Completed && | |
| profile.Annotations[helpers.StatusMetadataKey] == helpers.Learning { | |
| if profile.Annotations == nil { | |
| profile.Annotations = make(map[string]string) | |
| } | |
| profile.Annotations[helpers.StatusMetadataKey] = helpers.Completed | |
| } | |
| } |
🤖 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/containerprofile_processor.go` around lines 192 - 200,
Update the ContainerProfileStorage metadata lookup in the surrounding update
flow so create semantics apply only when GetContainerProfileMetadataNoLock
returns a not-found error. Propagate or abort on transient SQLite, decoding,
context, and other unexpected errors, while preserving the completed-status
guard and annotation update for the valid not-found/create case to prevent
completed profiles from regressing to Learning.
| // GetContainerProfileMetadataNoLock reads container profile metadata without | ||
| // acquiring the per-key lock. GetContainerProfileMetadata takes a read lock via | ||
| // GetWithConn; when PreSave runs from inside GuaranteedUpdate, the write lock for | ||
| // the same key is already held, so a read lock there would self-deadlock. The | ||
| // metadata branch of get() reads straight from SQLite and takes no lock of its | ||
| // own, so hasWriteLock (caller already holds the lock) is passed to skip any | ||
| // lock management. | ||
| func (c *ContainerProfileStorageImpl) GetContainerProfileMetadataNoLock(ctx context.Context, key string) (softwarecomposition.ContainerProfile, error) { | ||
| conn := ctx.Value(connKey).(*sqlite.Conn) | ||
| profile := softwarecomposition.ContainerProfile{} | ||
| err := c.storageImpl.get(ctx, conn, key, storage.GetOptions{ResourceVersion: softwarecomposition.ResourceVersionMetadata}, &profile, hasWriteLock) | ||
| return profile, err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift
Exercise the no-lock path with real storage.
containerprofile_completed_guard_test.go implements GetContainerProfileMetadataNoLock by calling the locking method, so it cannot detect a self-deadlock or an incorrect hasWriteLock flag in this new SQLite implementation. Add a storage/GuaranteedUpdate integration test that invokes PreSave while the per-key write lock is held.
🤖 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/containerprofile_storage.go` around lines 82 - 94, Add a
real-storage integration test covering GetContainerProfileMetadataNoLock during
GuaranteedUpdate: arrange for PreSave to invoke the no-lock metadata method
while the same key’s write lock is held, and assert the update completes
successfully without deadlocking. Use the SQLite-backed storage path and verify
the metadata result, ensuring the test exercises the hasWriteLock behavior
rather than the completed-guard mock implementation.
rc5k validation — component tests, benchmark, chartComponent tests — fork overlay harness, node-agent + storage both built from Benchmark — run https://github.com/k8sstormcenter/node-agent/actions/runs/30485745861, quality gate passed. Swaps only
21 steady-state samples each. Storage runs chart-default upstream in both arms, so it is not the variable under test. Chart — This rc5k carries the workload-level GeneratedNetworkPolicy aggregation fix ( |
|
Correction to the earlier rc5k validation note above: the multi-container GeneratedNetworkPolicy aggregation did not actually work in rc5k. The unit tests passed, but they seeded the store with the plural Fixed in |
rc5l released — GeneratedNetworkPolicy fix + backend/synchronizer migrationrc5l supersedes rc5k (rc5k's GeneratedNetworkPolicy was broken — see the correction above and storage Component tests (run https://github.com/k8sstormcenter/node-agent/actions/runs/30518037316): 25 pass / 3 fail — the 3 are the known infra flakes (Test_09 false-positive, Test_20/Test_21 partial-then-learn timing); all migration legs green; GeneratedNetworkPolicy fix — confirmed live on rc5l: a two-container Deployment yields exactly one workload-level backend + synchronizer migrated to ContainerProfile as well (AP/NN removed, storage pinned v0.0.290): backend's gRPC proto regenerated CP-only; synchronizer's filter + integration tests moved to ContainerProfile. Benchmark unchanged from rc5k (node-agent binary identical in rc5l): node-agent memory −20.9% vs upstream v0.3.158, CPU flat. |
…NN merge Migrate the server-side user-managed overlay merge (buildMergedProfile) from the legacy ug- ApplicationProfile + NetworkNeighborhood pair to a single ug- ContainerProfile via a new mergeUserCPIntoCP (flat single-container union that mirrors the node-agent client-side merge). Provenance now stamps the ug- CP source key/RV. The AP/NN merge helpers and their provenance keys are marked deprecated. The AP/NN CRD *types* are deliberately left untouched so this change carries no generated-code (openapi/protobuf/deepcopy) impact; the type-level deprecation and the full x86 codegen regeneration belong to the eventual CRD-removal change. Tests migrated to feed a ug- ContainerProfile with equivalent merge assertions. Signed-off-by: entlein <einentlein@gmail.com>
Remove the ApplicationProfile and NetworkNeighborhood custom resources entirely: their API types, registration, aggregated-apiserver storage backends, processors, and REST strategy/etcd wiring are gone, and the generated code (deepcopy, clientset, listers, informers, applyconfiguration, openapi, protobuf) is regenerated without them. ContainerProfile is the sole runtime/user-facing profile resource. - GeneratedNetworkPolicy is reworked to source from ContainerProfile (it projects the CP into the internal NetworkNeighborhood shape used by the policy generator), so that feature is preserved. The NetworkNeighborhood Go struct is retained as an internal, non-served intermediate only. - The user-managed "ug-" overlay merge is ContainerProfile-only. - Deprecated AP/NN merge helpers removed; tests migrated to ContainerProfile or dropped where they covered removed CRD-specific behaviour. Signed-off-by: entlein <einentlein@gmail.com>
The kindQueues default config still spun up applicationprofiles/networkneighborhoods queue workers, and the relevancy cleanup path still targeted applicationprofiles, after the CRDs were removed. Point them at containerprofiles. Signed-off-by: entlein <einentlein@gmail.com>
Rename MaxApplicationProfileSize -> MaxContainerProfileSize; drop the ApplicationProfile decode path from cmd/migration; neutralize AP/NN comments and the dead gob type-name default. The intentional GNP ContainerProfile-> NetworkNeighborhood projection is retained. Signed-off-by: entlein <einentlein@gmail.com>
The ug-<workload> user-managed merge (an additive overlay unioned onto the learned/observed ContainerProfile and republished as a derived merged profile) has no consumers. Remove the merge engine (buildMergedProfile/mergeUserCPIntoCP), the merged-first REST wrapper, the per-tick merged refresh + merged-CP GC, and the Save/Get/Delete Merged storage-interface methods and their wiring. The observed/learned ContainerProfile and the user-defined authoritative ContainerProfile remain the only profile mechanisms. Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
The migrated network-policy fixtures (cp-operator.json + the np-operator/np.new goldens) were moved but not yet consumed by a test. Add TestGenerateNetworkPolicyFromFile: it loads the v1beta1 ContainerProfile fixture, converts it to the internal type as the read path does, generates a policy, and asserts the result reproduces the pre-migration golden output under the generator's deterministic sort -- a diff-oracle that generating directly from a ContainerProfile matches the removed NetworkNeighborhood intermediate path. Add TestGenerateNetworkPolicy_PerContainerProfiles: two per-container profiles for one workload each generate their own policy reflecting only that container's network, with no cross-container bleed -- pinning the <label>-<containerName> per-container binding contract on the storage side. Signed-off-by: entlein <einentlein@gmail.com>
TestConsolidateData ran the fixtures through ConsolidateTimeSeries but asserted only one field, so the consolidated output was never pinned. Add TestConsolidateContainerProfileGolden: three time-series window fixtures folded via mergeContainerProfileTS then DeflateContainerProfileSpec, frozen to a golden (UPDATE_GOLDEN=1 to regenerate). Exercises opens wildcard collapse, arch/cap/ syscall dedup+sort, endpoint merge, rule-policy union, and ingress CIDR collapse. Golden-regression freeze (complements the CP->NetworkPolicy differential oracle). Signed-off-by: entlein <einentlein@gmail.com>
cp-operator.json carried over the deprecated singular dns/ipAddress fields from the NetworkNeighborhood form it was migrated from. Author it in the ContainerProfile-native representation instead: dnsNames/ipAddresses lists, no deprecated backwards-compat fields. The golden GeneratedNetworkPolicy is unchanged, confirming the native list fields generate the identical policy. Signed-off-by: entlein <einentlein@gmail.com>
A direct client patch of a consolidated ContainerProfile could regress its status from completed back to learning, because PreSave only guarded the time-series ingestion path. Add the guard to the consolidated (non-TS) update path: if the stored profile is already Completed and the incoming status is Learning, revert it to Completed (the update still succeeds). Reading existing metadata inside GuaranteedUpdate (which already holds the key's write lock) needed a lock-free reader (GetContainerProfileMetadataNoLock) to avoid self-deadlock. Pins Test_15_CompletedApCannotBecomeReadyAgain; Completed-> Completed amendments (Test_17) are unaffected. Signed-off-by: entlein <einentlein@gmail.com>
…edNetworkPolicy A GeneratedNetworkPolicy is workload-level: a NetworkPolicy selects pods, and all containers of a pod share the network namespace, so a workload's policy must union every container's ingress/egress. Before the ContainerProfile migration this held because the store read one workload-level NetworkNeighborhood per workload. After the migration the store reads per-container ContainerProfiles (replicaset-named, one object per container), but Get still did a single literal-key lookup and GetList emitted one policy per profile. A multi-container workload therefore produced one policy per container, and a workload-level Get (<lower(kind)>-<name>, e.g. deployment-<name>) returned NotFound because no ContainerProfile is named that way. Reproduced on a two-container workload: two per-container profiles with split egress, a workload-level Get returning NotFound, and an empty list. Resolve by grouping a namespace's ContainerProfiles by the workload-kind / workload-name labels (<lower(kind)>-<name>, the same name GenerateNetworkPolicy derives), unioning their neighbors, and generating one policy per workload. Get aggregates the requested workload's profiles; GetList emits one policy per workload. The internal listing is forced to full-spec because the default list path returns metadata only, which lacks the ingress/egress needed to generate. Add generatednetworkpolicy_multicontainer_test.go pinning workload-level Get aggregation and one-policy-per-workload GetList (both fail pre-fix), and correct the two Get subtests that requested a profile by its object name rather than the workload-level name. Signed-off-by: entlein <einentlein@gmail.com>
… storage kind ContainerProfiles are written under the SINGULAR storage kind segment "containerprofile" (ContainerProfileKind) — the processor keys them via BuildContainerProfileKey(id, "containerprofile"). The GeneratedNetworkPolicy aggregation looked them up under the plural "containerprofiles" (only the REST resource name), so the metadata query WHERE kind = 'containerprofiles' matched zero rows: Get returned NotFound and GetList returned empty for every workload. Observed live on a cluster: a two-container workload's per-container ContainerProfiles persisted correctly, but `get generatednetworkpolicy deployment-<name>` returned NotFound and the list was empty in every namespace, including real learned profiles in kube-system. Use ContainerProfileKind (the singular kind) for the internal lookup so the listing matches the stored keys. The unit tests masked this by seeding the fake store with the plural key; seed them with the singular "containerprofile" segment the real storage uses, so a plural lookup now misses and the tests fail on the pre-fix code. Signed-off-by: entlein <einentlein@gmail.com>
…der load Under sustained concurrent writes (many container profiles consolidating at once, on slow emptyDir I/O) a writer could wait past SQLite's default 10s busy timeout for the single-writer lock and fail with 'database is locked'. The insert was dropped, so the container profile never reached Completed and consumers hung — reproduced by component tests Test_09/20/21 timing out at 20m waiting for completion (storage logs: 'insert metadata: sqlite: step: database is locked'). The pool already opens connections in WAL mode (sqlitex defaults Flags=0 to OpenWAL); raise the busy timeout to 60s via PrepareConn so contended writes wait for the lock instead of failing, and re-assert WAL + synchronous=NORMAL explicitly so the config survives a Flags change. Signed-off-by: entlein <einentlein@gmail.com>
… drop under load" This reverts commit 809b840. Signed-off-by: entlein <einentlein@gmail.com>
- cmd/migration: golden gob->JSON test (legacy uint64 fields round-trip verbatim incl. max-uint64; spec shape; unsupported-type/corrupt-stream/missing-file). - containerprofile REST strategy: GetAttrs/SelectableFields expose the CP namespace/name/workload labels (catches the SBOMSyft copy-paste), NamespaceScoped, create/update/validate hooks. - completed-immutability guard driven through the REAL GuaranteedUpdate + sqlite pool (not the aliasing fake): Completed->Learning reverts, TooLarge short-circuits; pointing the guard at the locking read deadlocks (proves the no-lock variant). - ECS/Host key Build/Parse round-trips for all HostTypes + malformed branches. - Is/NormalizeContainerProfileKind + workloadPolicyName table tests. - GeneratedNetworkPolicy excludes non-available (TooLarge) profiles from Get/GetList. Test-only; no production changes. Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com> Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
1 participant
Part of the AP/NN → ContainerProfile migration (kubescape/node-agent#864). Removes the ApplicationProfile and NetworkNeighborhood CRDs (types, register, REST, backends, processors) in favor of the unified ContainerProfile.
ug-user-managed merge feature is decommissioned (no consumers): the merge engine (buildMergedProfile/mergeUserCPIntoCP), the merged-first REST wrapper, and the merged-CP GC are removed. The observed/learned ContainerProfile and the user-defined authoritative ContainerProfile are the only profile mechanisms.GeneratedNetworkPolicyis generated directly from the ContainerProfile (spec.ingress/spec.egress+ the embedded workload selector); the in-processNetworkNeighborhoodintermediate struct (and...Spec/...Container) is removed, with the leafNetworkNeighbor/NetworkPorttypes retained as ContainerProfile spec fields.TestGenerateNetworkPolicyFromFile). A per-container test (TestGenerateNetworkPolicy_PerContainerProfiles) pins the<label>-<containerName>binding on the storage side.Multi-container binding (recorded decision)
A ContainerProfile is per-container, but the user-defined-profile binding is a pod label. Go-forward contract:
<label-value>-<containerName>, where<label-value>is the pod labelkubescape.io/user-defined-profile. The read path binds each container to its own ContainerProfile by that name, falling back to the bare<label-value>only for single-container pods.Example — pod label
kubescape.io/user-defined-profile: mc35on a pod with containersapp+sidecar→ ContainerProfilesmc35-appandmc35-sidecar; a single-container pod → baremc35.Tested: node-agent
Test_35_MultiContainerPerContainerBinding(per-containerR0001isolation, no cross-inheritance) and a storage per-containerGeneratedNetworkPolicytest.Verified with
go build/test -mod=mod ./...(green).