Migrate user-defined AP/NN to containerprofile - #58
Closed
entlein wants to merge 11 commits into
Closed
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (38)
📝 WalkthroughWalkthroughChangesThe cache now prefers valid user-authored ContainerProfiles for labeled pods, falls back to legacy ApplicationProfile and NetworkNeighborhood overlays when needed, refreshes and tracks the new source, converts legacy profiles for compatibility validation, and routes wildcard paths into projection patterns. User-authored ContainerProfile overlay migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Pod
participant ContainerProfileCache
participant ProfileStorage
participant Projection
Pod->>ContainerProfileCache: provide user-defined profile label
ContainerProfileCache->>ProfileStorage: fetch user-authored ContainerProfile
ProfileStorage-->>ContainerProfileCache: return ContainerProfile
ContainerProfileCache->>Projection: apply profile as authoritative base
Projection-->>ContainerProfileCache: return projected cached profile
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/component_test.go (1)
1651-1654: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "creates a user-defined ApplicationProfile/AP" docstrings after the ContainerProfile migration. Both helper docstrings were left describing the old AP/NN-based flow even though the code beneath each was migrated to build and poll a
ContainerProfile.
tests/component_test.go#L1651-L1654: update the comment to describe creating/polling a user-definedContainerProfileinstead of anApplicationProfile.tests/component_test.go#L2029-L2032: update the comment to describe creating/polling a user-definedContainerProfileinstead of an "AP".🤖 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 `@tests/component_test.go` around lines 1651 - 1654, Update the helper docstrings at tests/component_test.go:1651-1654 and tests/component_test.go:2029-2032 to describe creating and polling a user-defined ContainerProfile, replacing the stale ApplicationProfile/AP terminology; leave the underlying helper behavior unchanged.pkg/objectcache/containerprofilecache/containerprofilecache.go (1)
451-508: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
entry.RVgets clobbered with the user-defined CP's ResourceVersion, freezing refresh forever.
cpis reused for two different concepts: the learned/consolidated CP fetched bycpName, and (once overwritten at Line 455-457) the authoritative "effective base" for projection.buildEntry(Line 569, shown in context) setsentry.RV = cp.ResourceVersionfrom whatever was passed in — so whenuserDefinedCP != nil,entry.RVends up equal to the user-defined CP's resource version instead of the learned CP's (which should be""since learning is suppressed for these containers).Consequence: in
refreshOneEntry(reconciler.go), the learned-CP fetch ate.CPNamewill 404 every tick (as designed — learning is suppressed), but becausee.RV != ""the code treats the 404 as transient and returns immediately, before ever reaching theUserCPRefrefresh block. The user-defined ContainerProfile is therefore never re-fetched after initial population — edits to it are permanently invisible.Contrast this with
reconciler.go'srebuildEntryFromSources, which correctly keepscp(learned CP) andeffectiveCP(=userDefinedCPwhen present) as separate variables and computesRV: rvOfCP(cp)— the pattern this file should mirror.🐛 Proposed fix — keep the learned-CP RV separate from the effective base
+ // Keep the learned/consolidated CP's identity separate from the + // "effective base" used for projection below — entry.RV must reflect + // the learned CP (or "" if absent), not whichever object becomes the + // authoritative base. + learnedCP := cp if userDefinedCP != nil { cp = userDefinedCP } @@ entry := c.buildEntry(cp, userAP, userNN, pod, container, sharedData) entry.CPName = cpName + entry.RV = rvOfCP(learnedCP)🤖 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/objectcache/containerprofilecache/containerprofilecache.go` around lines 451 - 508, Keep the learned/consolidated CP separate from the effective projection base in the entry-building flow: preserve the original learned CP ResourceVersion before applying userDefinedCP, use the effective base for projection and buildEntry, and set entry.RV from the preserved learned CP value. Mirror rebuildEntryFromSources and retain CPName behavior so suppressed learned-profile 404s still allow the UserCPRef refresh path.
🤖 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/objectcache/containerprofilecache/containerprofilecache_test.go`:
- Around line 215-250: Extend TestOverlayPath_UserDefinedCP_NewWay to assert
that entry.RV is empty after applying the user-defined ContainerProfile overlay.
Add a refresh-cycle test or follow-up tick through the reconciler refresh path,
then verify the user-defined CP is re-fetched using entry.UserCPRef rather than
being skipped due to the CP ResourceVersion.
In
`@pkg/objectcache/containerprofilecache/projection_wildcard_classification_test.go`:
- Around line 21-28: Extend the projection wildcard classification test to cover
an explicit opens filter with All=false, using a configuration that has no
exact, prefix, or suffix matcher for /etc/ssl/*. Assert that /etc/ssl/* remains
in pcp.Opens.Patterns and is absent from pcp.Opens.Values, while preserving the
existing nil-spec pass-through assertions.
---
Outside diff comments:
In `@pkg/objectcache/containerprofilecache/containerprofilecache.go`:
- Around line 451-508: Keep the learned/consolidated CP separate from the
effective projection base in the entry-building flow: preserve the original
learned CP ResourceVersion before applying userDefinedCP, use the effective base
for projection and buildEntry, and set entry.RV from the preserved learned CP
value. Mirror rebuildEntryFromSources and retain CPName behavior so suppressed
learned-profile 404s still allow the UserCPRef refresh path.
In `@tests/component_test.go`:
- Around line 1651-1654: Update the helper docstrings at
tests/component_test.go:1651-1654 and tests/component_test.go:2029-2032 to
describe creating and polling a user-defined ContainerProfile, replacing the
stale ApplicationProfile/AP terminology; leave the underlying helper behavior
unchanged.
🪄 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: 254f76f5-8ba2-46d6-a4bb-984c361caaf7
📒 Files selected for processing (9)
pkg/objectcache/containerprofilecache/containerprofilecache.gopkg/objectcache/containerprofilecache/containerprofilecache_test.gopkg/objectcache/containerprofilecache/projection_apply.gopkg/objectcache/containerprofilecache/projection_wildcard_classification_test.gopkg/objectcache/containerprofilecache/reconciler.gopkg/objectcache/containerprofilecache/usercp.gopkg/objectcache/containerprofilecache/usercp_bench_test.gopkg/objectcache/containerprofilecache/usercp_diff_oracle_test.gotests/component_test.go
Comment on lines
+215
to
+250
| // TestOverlayPath_UserDefinedCP_NewWay verifies the migrated path: when the | ||
| // user-defined-profile label names a user-authored ContainerProfile | ||
| // (managed-by: User), it becomes the authoritative base — UserCPRef is set, the | ||
| // legacy UserAPRef/UserNNRef are NOT, and the projection reflects the CP. | ||
| func TestOverlayPath_UserDefinedCP_NewWay(t *testing.T) { | ||
| userCP := &v1beta1.ContainerProfile{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "override", Namespace: "default", ResourceVersion: "uc1", | ||
| Annotations: map[string]string{ | ||
| helpersv1.ManagedByMetadataKey: helpersv1.ManagedByUserValue, | ||
| helpersv1.StatusMetadataKey: helpersv1.Completed, | ||
| helpersv1.CompletionMetadataKey: helpersv1.Full, | ||
| }, | ||
| }, | ||
| Spec: v1beta1.ContainerProfileSpec{Capabilities: []string{"NET_BIND_SERVICE"}}, | ||
| } | ||
| // cp: nil (learning suppressed for user-defined); userCP served at "override". | ||
| client := &fakeProfileClient{cp: nil, cpErr: apierrors.NewNotFound(schema.GroupResource{}, "x"), userCP: userCP} | ||
| c, k8s := newTestCache(t, client) | ||
|
|
||
| id := "container-udcp" | ||
| primeSharedData(t, k8s, id, "wlid://cluster-a/namespace-default/deployment-nginx") | ||
|
|
||
| ev := eventContainer(id) | ||
| ev.K8s.PodLabels = map[string]string{helpersv1.UserDefinedProfileMetadataKey: "override"} | ||
| require.NoError(t, c.addContainer(ev, context.Background())) | ||
|
|
||
| entry, ok := c.entries.Load(id) | ||
| require.True(t, ok) | ||
| assert.NotNil(t, entry.Projected, "user-defined CP path must produce a projected profile") | ||
| require.NotNil(t, entry.UserCPRef, "UserCPRef must be recorded for refresh") | ||
| assert.Equal(t, "override", entry.UserCPRef.Name) | ||
| assert.Equal(t, "uc1", entry.UserCPRV) | ||
| assert.Nil(t, entry.UserAPRef, "legacy AP ref must not be set on the new path") | ||
| assert.Nil(t, entry.UserNNRef, "legacy NN ref must not be set on the new path") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Consider asserting entry.RV here, and/or adding a refresh-cycle test.
This test would have caught the entry.RV bug flagged in containerprofilecache.go (Line 451-508: entry.RV ends up equal to the user-defined CP's ResourceVersion instead of ""). Adding assert.Equal(t, "", entry.RV, ...) here, plus a follow-up test that calls the reconciler's refresh path a second tick and confirms the user-defined CP overlay is re-fetched, would close that gap.
🤖 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/objectcache/containerprofilecache/containerprofilecache_test.go` around
lines 215 - 250, Extend TestOverlayPath_UserDefinedCP_NewWay to assert that
entry.RV is empty after applying the user-defined ContainerProfile overlay. Add
a refresh-cycle test or follow-up tick through the reconciler refresh path, then
verify the user-defined CP is re-fetched using entry.UserCPRef rather than being
skipped due to the CP ResourceVersion.
Comment on lines
+21
to
+28
| pcp := Apply(nil, cp, nil) // nil spec => pass-through (All=true) | ||
|
|
||
| require.Contains(t, pcp.Opens.Patterns, "/etc/ssl/*", | ||
| "a '*'-bearing path entry must be a Pattern") | ||
| _, inValues := pcp.Opens.Values["/etc/ssl/*"] | ||
| assert.False(t, inValues, "'*'-bearing path entry must NOT be a literal Value") | ||
| _, cacheInValues := pcp.Opens.Values["/etc/ld.so.cache"] | ||
| assert.True(t, cacheInValues, "a literal path entry stays a Value") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Cover explicit filtering mode too.
This test only exercises nil-spec pass-through (All=true). Add a case with the opens field in use and All=false, verifying that /etc/ssl/* remains in Patterns even without an exact, prefix, or suffix matcher. Otherwise, a regression in the explicitly filtered path can pass this test.
🤖 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/objectcache/containerprofilecache/projection_wildcard_classification_test.go`
around lines 21 - 28, Extend the projection wildcard classification test to
cover an explicit opens filter with All=false, using a configuration that has no
exact, prefix, or suffix matcher for /etc/ssl/*. Assert that /etc/ssl/* remains
in pcp.Opens.Patterns and is absent from pcp.Opens.Values, while preserving the
existing nil-spec pass-through assertions.
Author
Benchmark: user-defined CP vs legacy AP+NN overlayPer-container projection cost (the reconciler runs it on every changed tick).
~35% faster, ~27% less memory — the new path uses the authored CP as the base and skips the two-object merge. Behaviour is proven identical by the differential oracle ( |
… -> ContainerProfile ConvertUserProfilesToContainerProfile produces the single user-defined ContainerProfile equivalent to a legacy user-authored ApplicationProfile + NetworkNeighborhood pair, reusing the existing projectUserProfiles merge onto an empty base. The differential oracle pins the migration contract at the enforcement level: for representative user-defined shapes (opens/exec argv wildcards, HTTP endpoints, egress/ingress + LabelSelector) the ProjectedContainerProfile from the legacy AP+NN overlay path equals the one from using the converted ContainerProfile as the base with no overlay. Behaviour-preserving by construction. Signed-off-by: entlein <einentlein@gmail.com>
…e authoritative base When the user-defined-profile pod label names a ContainerProfile carrying managed-by: User, the cache now uses it directly as the container's base profile (the migrated "new way"), instead of overlaying a legacy ApplicationProfile + NetworkNeighborhood pair. It falls back to the legacy AP+NN pair when no such CP exists, which still fires the existing deprecation signal. - add-time (tryPopulateEntry) and refresh (reconciler) both prefer the user CP, gated on the managed-by: User annotation so a learned CP at the same name is never mistaken for a user-defined one - UserCPRef/UserCPRV bookkeeping mirrors the legacy UserAPRV/UserNNRV RV tracking so the reconciler re-fetches and rebuilds only when the user CP changes Signed-off-by: entlein <einentlein@gmail.com>
Test_28 now creates one ContainerProfile (managed-by: User) carrying the merged exec/syscall + egress/selector surfaces, instead of a separate user-authored ApplicationProfile + NetworkNeighborhood pair — exercising the migrated read-path end to end. Assertions unchanged. Signed-off-by: entlein <einentlein@gmail.com>
…verlay The new path (converted CP as base, no overlay) is ~35% faster and allocates ~27% less than the legacy AP+NN overlay per projection, since it skips the two-object merge. Signed-off-by: entlein <einentlein@gmail.com>
Test_27 (opens R0002, both the regex and the curl-wildcard profile sites), Test_32 (R0040 argv wildcards), and Test_33 (opens wildcard anchoring — the previously-fixed one) now each create a single user-defined ContainerProfile (managed-by: User) carrying the merged exec/open/syscall (+ egress/selector for 32) surfaces, replacing the legacy ApplicationProfile + NetworkNeighborhood pair. Assertions unchanged. Test_28 was ported in an earlier commit. Signed-off-by: entlein <einentlein@gmail.com>
containsDynamicSegment recognised only the one-segment DynamicIdentifier
('⋯'), so a path-surface opens entry bearing the zero-or-more
WildcardIdentifier ('*') — e.g. '/etc/ssl/*' — was routed to Values as if it
were a literal. was_path_opened tolerates this (Values and Patterns are both
matched via CompareDynamic), but it is wrong for any consumer that treats
Values as exact membership, and it drops '*'-only entries a rule needs when
spec.All is false and no prefix/suffix matcher retains them. Recognise both
wildcard markers. Regression test pins '/etc/ssl/*' -> Patterns.
Pre-existing in upstream (identical containsDynamicSegment).
Signed-off-by: Entlein <eineintlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Test_33's anchoring subtests assert R0002 alerts, but the rule's file-access monitoring is opt-in (monitored prefixes incl. /etc/). Test_33 never applied r0002-files-access-enabled.yaml (Test_27 does via enableR0002ForTest), so R0002 never evaluated the opens and every 'expect alert' case silently passed as a no-alert — invisible because Test_33 had never run in CI. Enable it like Test_27. Verified on a live cluster: /etc/ssl/* correctly alerts on the bare parent /etc/ssl and stays silent on the child /etc/ssl/openssl.cnf. Signed-off-by: entlein <einentlein@gmail.com>
…tations A user-authored profile is authoritative and complete by definition — it should not carry the learning-lifecycle status/completion markers, nor a managed-by annotation. The pod's user-defined-profile label is what declares it user-authored (a signature, added by the signing tooling, is the integrity marker). - read-path/reconciler no longer gate on managed-by: the label-referenced CP is used as authoritative, and the entry State is forced to Completed+Full so the rule engine enforces it despite the absent completion annotation - the converter emits clean CPs (name + namespace + spec only) - fake client models an absent overlay-name fetch (drives the legacy fallback) Signed-off-by: entlein <einentlein@gmail.com>
…mple - Test_28 now loads its user-defined ContainerProfile from a real yaml (resources/containerprofile-user-defined-network.yaml) via a loader helper, so the fixture doubles as the copy-pasteable "how to author a user-defined profile" example. - The example — and all the test CPs — carry only name + spec: the nonsensical learning-lifecycle annotations (status/completion) and the managed-by marker are dropped (node-agent now treats a label-referenced CP as authoritative and forces the enforce-state itself). Signed-off-by: entlein <einentlein@gmail.com>
Migrate the network-wildcards NetworkNeighborhood fixtures into their user-authored ContainerProfile form so users can copy-paste them to author user-defined-profile allow-lists: - per-container spec.egress/ingress (NN's spec.containers[] collapses, one CP document per container; fixture 20 splits into two) - no lifecycle annotations — name only (namespace injected by tooling), matching the clean user-managed CP contract - teaching comments preserved verbatim from the NN fixtures Adds 00-fusioncore-homoglyph-attack.yaml: a pinned single-vendor allow-list and the look-alike (homoglyph) domains its exact-match dnsNames compare rejects (each fires R0005). All 21 documents strict-parse against v1beta1 ContainerProfile and carry zero annotations (verified). Signed-off-by: entlein <einentlein@gmail.com>
The component tests build their user-defined profiles inline as ContainerProfiles (Test_27/32/33) or load a CP yaml (Test_28); the legacy ApplicationProfile / NetworkNeighborhood fixtures they were derived from are no longer referenced by any Go test. Migrate the two CT-relevant ones to their user-authored CP form and delete the orphan: - exec-arg-wildcards-profile.yaml (AP curl-32-overlay) -> containerprofile-exec-arg-wildcards.yaml. CP form mirrors Test_32's inline CP exactly (same execs incl. busybox-symlink + literal-* entries, same syscalls, matchLabels app: curl-32). Demonstrates the exec-arg wildcard grammar for authoring. - known-network-neighborhood.yaml (NN fusioncore-network) -> containerprofile-fusioncore-network.yaml. Clean user-managed CP: name only, no managed-by / status / completion annotations. - user-profile.yaml: deleted. Zero references anywhere; its nginx/server exec surface matches no current test or deployment. Both new documents strict-parse against v1beta1 ContainerProfile and carry zero annotations (verified). CT compiles unchanged (go vet -tags component); no Go test referenced the deleted files. Signed-off-by: entlein <einentlein@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview