Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 74 additions & 29 deletions pkg/objectcache/containerprofilecache/containerprofilecache.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ type CachedContainerProfile struct {
UserAPRef *namespacedName
UserNNRef *namespacedName

// UserCPRef is set when the user-defined-profile label names a single
// user-authored ContainerProfile (the migrated "new way"), which is used
// as the authoritative base for the container. Mutually exclusive with the
// legacy UserAPRef/UserNNRef overlay. Used by the reconciler to re-fetch.
UserCPRef *namespacedName

// CPName is the storage name of the ContainerProfile. Populated at
// addContainer time so the reconciler can re-fetch without re-querying
// shared data (which may have been evicted from K8sObjectCache by then).
Expand All @@ -83,6 +89,7 @@ type CachedContainerProfile struct {
UserManagedNNRV string // user-managed NN (ug-<workload>) RV at last projection, "" if absent
UserAPRV string // user-AP (label-referenced) resourceVersion at last projection, "" if no overlay
UserNNRV string // user-NN (label-referenced) resourceVersion at last projection, "" if no overlay
UserCPRV string // user-defined ContainerProfile (label-referenced) RV at last load, "" if not used
}

// pendingContainer captures the minimum state needed to retry the initial
Expand Down Expand Up @@ -392,41 +399,63 @@ func (c *ContainerProfileCacheImpl) tryPopulateEntry(
// transient failures are recovered.
var userAP *v1beta1.ApplicationProfile
var userNN *v1beta1.NetworkNeighborhood
var userDefinedCP *v1beta1.ContainerProfile
overlayName, hasOverlay := container.K8s.PodLabels[helpersv1.UserDefinedProfileMetadataKey]
if hasOverlay && overlayName != "" {
var userAPErr error
_ = c.refreshRPC(ctx, func(rctx context.Context) error {
userAP, userAPErr = c.storageClient.GetApplicationProfile(rctx, ns, overlayName)
return userAPErr
})
if userAPErr != nil {
logger.L().Debug("user-defined ApplicationProfile not available",
helpers.String("containerID", containerID),
helpers.String("namespace", ns),
helpers.String("name", overlayName),
helpers.Error(userAPErr))
userAP = nil
}
var userNNErr error
// Migration (#862): the user-defined-profile label now names a single
// user-authored ContainerProfile ("new way") — the unified replacement
// for the legacy AP+NN pair. Prefer it: it is authoritative and needs no
// overlay merge. Fall back to the legacy AP+NN pair only when no such CP
// exists, in which case emitOverlayMetrics fires the deprecation signal.
var userCPErr error
_ = c.refreshRPC(ctx, func(rctx context.Context) error {
userNN, userNNErr = c.storageClient.GetNetworkNeighborhood(rctx, ns, overlayName)
return userNNErr
userDefinedCP, userCPErr = c.storageClient.GetContainerProfile(rctx, ns, overlayName)
return userCPErr
})
if userNNErr != nil {
logger.L().Debug("user-defined NetworkNeighborhood not available",
helpers.String("containerID", containerID),
helpers.String("namespace", ns),
helpers.String("name", overlayName),
helpers.Error(userNNErr))
userNN = nil
if userCPErr != nil {
userDefinedCP = nil
var userAPErr error
_ = c.refreshRPC(ctx, func(rctx context.Context) error {
userAP, userAPErr = c.storageClient.GetApplicationProfile(rctx, ns, overlayName)
return userAPErr
})
if userAPErr != nil {
logger.L().Debug("user-defined ApplicationProfile not available",
helpers.String("containerID", containerID),
helpers.String("namespace", ns),
helpers.String("name", overlayName),
helpers.Error(userAPErr))
userAP = nil
}
var userNNErr error
_ = c.refreshRPC(ctx, func(rctx context.Context) error {
userNN, userNNErr = c.storageClient.GetNetworkNeighborhood(rctx, ns, overlayName)
return userNNErr
})
if userNNErr != nil {
logger.L().Debug("user-defined NetworkNeighborhood not available",
helpers.String("containerID", containerID),
helpers.String("namespace", ns),
helpers.String("name", overlayName),
helpers.Error(userNNErr))
userNN = nil
}
}
}

// Need SOMETHING to cache. If we have nothing, stay pending and retry.
if cp == nil && userManagedAP == nil && userManagedNN == nil && userAP == nil && userNN == nil {
if cp == nil && userDefinedCP == nil && userManagedAP == nil && userManagedNN == nil && userAP == nil && userNN == nil {
return false
}

// A user-defined ContainerProfile is authoritative for this container: it is
// the migrated replacement for the AP+NN overlay, so it becomes the base
// (the ug- user-managed pass may still union on top). Learning is suppressed
// for user-defined containers, so no consolidated CP competes with it.
if userDefinedCP != nil {
cp = userDefinedCP
}

// When no consolidated CP is available, synthesize an empty CP named
// after the workload so downstream state display is sensible. Projection
// below merges user-managed + user-defined overlay onto this base.
Expand Down Expand Up @@ -492,11 +521,27 @@ func (c *ContainerProfileCacheImpl) tryPopulateEntry(
// these refs to re-fetch on every tick; without them, a transient 404
// at add time would permanently lose the overlay.
if hasOverlay && overlayName != "" {
if entry.UserAPRef == nil {
entry.UserAPRef = &namespacedName{Namespace: ns, Name: overlayName}
}
if entry.UserNNRef == nil {
entry.UserNNRef = &namespacedName{Namespace: ns, Name: overlayName}
if userDefinedCP != nil {
// New way: track the user-defined CP for re-fetch; no legacy refs.
entry.UserCPRef = &namespacedName{Namespace: ns, Name: overlayName}
entry.UserCPRV = userDefinedCP.ResourceVersion
// A user-authored profile is authoritative and complete by
// definition — it carries no learning-lifecycle status/completion
// annotations (those are meaningless on an authored profile). Force
// the terminal state so the rule engine enforces it (rule_manager
// gates on Completed+Full).
entry.State = &objectcache.ProfileState{
Status: helpersv1.Completed,
Completion: helpersv1.Full,
Name: userDefinedCP.Name,
}
} else {
if entry.UserAPRef == nil {
entry.UserAPRef = &namespacedName{Namespace: ns, Name: overlayName}
}
if entry.UserNNRef == nil {
entry.UserNNRef = &namespacedName{Namespace: ns, Name: overlayName}
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ import (
// pointer equality).
type fakeProfileClient struct {
cp *v1beta1.ContainerProfile
// userCP, when non-nil, is returned by GetContainerProfile for a name
// matching userCP.Name (the migrated user-defined ContainerProfile). Other
// names fall through to cp. Lets tests exercise the new-way overlay path.
userCP *v1beta1.ContainerProfile
ap *v1beta1.ApplicationProfile // returned for Get by ap.Name match (or any if overlayOnly is empty)
nn *v1beta1.NetworkNeighborhood
cpErr error
Expand Down Expand Up @@ -78,8 +82,17 @@ func (f *fakeProfileClient) GetNetworkNeighborhood(_ context.Context, _, name st
}
return f.nn, f.nnErr
}
func (f *fakeProfileClient) GetContainerProfile(_ context.Context, _, _ string) (*v1beta1.ContainerProfile, error) {
func (f *fakeProfileClient) GetContainerProfile(_ context.Context, _, name string) (*v1beta1.ContainerProfile, error) {
f.getCPCalls++
if f.userCP != nil && name == f.userCP.Name {
return f.userCP, nil
}
// The overlay label points at overlayOnly; with no user CP published at that
// name it is absent, which drives the legacy AP/NN fallback path. (The base
// CP fetch uses the derived slug, a different name, and still gets f.cp.)
if f.overlayOnly != "" && name == f.overlayOnly {
return nil, apierrors.NewNotFound(schema.GroupResource{Resource: "containerprofiles"}, name)
}
return f.cp, f.cpErr
}
func (f *fakeProfileClient) ListApplicationProfiles(_ context.Context, _ string, _ int64, _ string) (*v1beta1.ApplicationProfileList, error) {
Expand Down Expand Up @@ -205,6 +218,43 @@ func TestOverlayPath_DeepCopies(t *testing.T) {
assert.Equal(t, "u1", entry.UserAPRV)
}

// 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")
}
Comment on lines +221 to +256

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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.


// TestDeleteContainer_LockAndCleanup verifies that deleteContainer removes
// the entry and releases the per-container lock so a later Add re-uses a
// fresh mutex.
Expand Down
15 changes: 12 additions & 3 deletions pkg/objectcache/containerprofilecache/projection_apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,10 +143,19 @@ func projectField(spec objectcache.FieldSpec, rawEntries []string, isPathSurface
return pf
}

// containsDynamicSegment reports whether e contains the dynamic-path marker.
// Always references the constant from the storage package; never hardcodes the glyph.
// containsDynamicSegment reports whether e contains a wildcard-path marker —
// either the one-segment DynamicIdentifier ("⋯") OR the zero-or-more
// WildcardIdentifier ("*"). On path surfaces both are dynamic and must be
// routed to Patterns, never treated as literal Values. Omitting "*" here
// silently misclassifies entries like "/etc/ssl/*" as literals; that happens
// to be harmless for was_path_opened (both Values and Patterns are 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. Always reference the storage
// constants; never hardcode the glyphs.
func containsDynamicSegment(e string) bool {
return strings.Contains(e, dynamicpathdetector.DynamicIdentifier)
return strings.Contains(e, dynamicpathdetector.DynamicIdentifier) ||
strings.Contains(e, dynamicpathdetector.WildcardIdentifier)
}

// --- Field extractors ---
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package containerprofilecache

import (
"testing"

"github.com/kubescape/storage/pkg/apis/softwarecomposition/v1beta1"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// TestProjectField_StarPathRoutesToPatterns pins that a path-surface opens
// entry containing the "*" WildcardIdentifier is classified as a Pattern,
// not a literal Value. Regression guard: containsDynamicSegment previously
// recognised only "⋯", silently routing "/etc/ssl/*" into Values.
func TestProjectField_StarPathRoutesToPatterns(t *testing.T) {
cp := &v1beta1.ContainerProfile{
Spec: v1beta1.ContainerProfileSpec{
Opens: []v1beta1.OpenCalls{{Path: "/etc/ssl/*"}, {Path: "/etc/ld.so.cache"}},
},
}
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")
Comment on lines +21 to +28

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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.

}
52 changes: 48 additions & 4 deletions pkg/objectcache/containerprofilecache/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,28 @@ func (c *ContainerProfileCacheImpl) refreshOneEntry(ctx context.Context, id stri
}
}

// Re-fetch the user-defined ContainerProfile (migrated "new way") when the
// entry was built from one. It is the authoritative base; a transient fetch
// error keeps the entry as-is.
var userDefinedCP *v1beta1.ContainerProfile
if e.UserCPRef != nil {
var userCPErr error
_ = c.refreshRPC(ctx, func(rctx context.Context) error {
userDefinedCP, userCPErr = c.storageClient.GetContainerProfile(rctx, e.UserCPRef.Namespace, e.UserCPRef.Name)
return userCPErr
})
if userCPErr != nil && e.UserCPRV != "" {
logger.L().Debug("refreshOneEntry: user-defined CP fetch failed; keeping cached entry",
helpers.String("containerID", id),
helpers.String("name", e.UserCPRef.Name),
helpers.Error(userCPErr))
return
}
if userCPErr != nil {
userDefinedCP = nil
}
}

// Fast-skip when nothing changed. We match "absent" (nil) with empty RV:
// this avoids spurious rebuilds when an optional source is still missing,
// as long as it was also missing at the last build. Also skip when the
Expand All @@ -413,6 +435,7 @@ func (c *ContainerProfileCacheImpl) refreshOneEntry(ctx context.Context, id stri
currentSpecHash = spec.Hash
}
if rvsMatchCP(cp, e.RV) &&
rvsMatchCP(userDefinedCP, e.UserCPRV) &&
rvsMatchAP(userManagedAP, e.UserManagedAPRV) &&
rvsMatchNN(userManagedNN, e.UserManagedNNRV) &&
rvsMatchAP(userAP, e.UserAPRV) &&
Expand All @@ -421,7 +444,7 @@ func (c *ContainerProfileCacheImpl) refreshOneEntry(ctx context.Context, id stri
return
}

c.rebuildEntryFromSources(id, e, cp, userManagedAP, userManagedNN, userAP, userNN)
c.rebuildEntryFromSources(id, e, cp, userDefinedCP, userManagedAP, userManagedNN, userAP, userNN)
}

// rvsMatchCP, rvsMatchAP, rvsMatchNN return true when either (a) the object is
Expand Down Expand Up @@ -456,6 +479,7 @@ func (c *ContainerProfileCacheImpl) rebuildEntryFromSources(
id string,
prev *CachedContainerProfile,
cp *v1beta1.ContainerProfile,
userDefinedCP *v1beta1.ContainerProfile,
userManagedAP *v1beta1.ApplicationProfile,
userManagedNN *v1beta1.NetworkNeighborhood,
userAP *v1beta1.ApplicationProfile,
Expand All @@ -474,10 +498,17 @@ func (c *ContainerProfileCacheImpl) rebuildEntryFromSources(
podUID = string(pod.UID)
}

// When the consolidated CP is absent but we still have user-managed /
// user-defined overlays to project, synthesize an empty base so
// downstream state display is sensible.
// A user-defined ContainerProfile ("new way") is the authoritative base,
// replacing the learned CP for this container. cp (the learned CP) stays
// separate so RV bookkeeping tracks each source independently.
effectiveCP := cp
if userDefinedCP != nil {
effectiveCP = userDefinedCP
}

// When neither a learned nor a user-defined CP is available but we still
// have user-managed overlays to project, synthesize an empty base so
// downstream state display is sensible.
if effectiveCP == nil {
syntheticName := prev.WorkloadName
if syntheticName == "" {
Expand Down Expand Up @@ -543,6 +574,19 @@ func (c *ContainerProfileCacheImpl) rebuildEntryFromSources(
UserManagedNNRV: rvOfNN(userManagedNN),
UserAPRV: rvOfAP(userAP),
UserNNRV: rvOfNN(userNN),
UserCPRV: rvOfCP(userDefinedCP),
}
if userDefinedCP != nil {
newEntry.UserCPRef = &namespacedName{Namespace: userDefinedCP.Namespace, Name: userDefinedCP.Name}
// A user-authored profile is complete by definition (no learning-lifecycle
// annotations); force the terminal state so the rule engine enforces it.
newEntry.State = &objectcache.ProfileState{
Status: helpersv1.Completed,
Completion: helpersv1.Full,
Name: userDefinedCP.Name,
}
} else if prev.UserCPRef != nil {
newEntry.UserCPRef = prev.UserCPRef
}
if userAP != nil {
newEntry.UserAPRef = &namespacedName{Namespace: userAP.Namespace, Name: userAP.Name}
Expand Down
2 changes: 1 addition & 1 deletion pkg/objectcache/containerprofilecache/reconciler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -995,7 +995,7 @@ func TestOverlayLabel_TransientFetchFailure_RefsRetained(t *testing.T) {
},
}
// Overlay fetch returns an error; the base CP is fine.
client := &fakeProfileClient{cp: cp, apErr: assertErrNotFound("override"), nnErr: assertErrNotFound("override")}
client := &fakeProfileClient{cp: cp, overlayOnly: "override", apErr: assertErrNotFound("override"), nnErr: assertErrNotFound("override")}
c, k8s := newTestCache(t, client)

id := "container-transient-overlay"
Expand Down
Loading
Loading