Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces cloud metadata integration to the container profile manager, updates two Go module dependencies, adjusts configuration defaults for timing parameters, refines container data lifecycle logic, and updates the GetLabels function to include cloud-derived label augmentation. Changes
Sequence DiagramsequenceDiagram
participant Init as Initialization
participant IMDS as Cloud Metadata<br/>(IMDS)
participant CPM as ContainerProfileManager
participant Monitor as Monitoring
participant Labels as Label Generator
participant Storage as Container Storage
Init->>IMDS: getCloudMetadataFromNode(ctx, k8sClient, nodeName)
IMDS-->>Init: CloudMetadata{HostType, Cluster, AccountID, Region}
Init->>CPM: store cloudMetadata field
rect rgba(100, 200, 150, 0.5)
Note over Monitor,Labels: Periodic Monitoring Cycle
Monitor->>Monitor: monitorContainer (if UpdateDataPeriod > 0)
Monitor->>Monitor: Ticker Reset
Monitor->>Labels: saveContainerProfile(cloudMetadata, watchedContainer)
Labels->>Labels: GetLabels(cloudMetadata, watchedContainer, stripContainer)
Labels->>Labels: Augment with Cloud Labels<br/>(HostType, Cluster, AWSAccountID, Region)
Labels-->>Storage: labels{...existing + cloud-derived}
end
Note over CPM: On Shutdown
CPM->>CPM: Close() → graceful queue shutdown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (3)
pkg/containerprofilemanager/v1/containerprofile_manager.go (2)
197-202: Redundant fallback ingetHostType.Since
hostTypeis always initialized to"k8s"in the constructor (line 124), the empty string check and fallback will never be triggered.♻️ Simplify the getter
func (cpm *ContainerProfileManager) getHostType() string { - if cpm.hostType != "" { - return cpm.hostType - } - return "k8s" + return cpm.hostType }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerprofilemanager/v1/containerprofile_manager.go` around lines 197 - 202, The getHostType method contains an unnecessary empty-string fallback because ContainerProfileManager.hostType is initialized to "k8s" in the constructor (e.g., NewContainerProfileManager); simplify getHostType by returning cpm.hostType directly (remove the if-check and hardcoded "k8s" return) so the method just returns the hostType field.
180-195: Unused parameters in helper functions.Both
getClusterNameFromConfigandgetCloudMetadataFromNodeaccept parameters that are not used:
getClusterNameFromConfig:k8sClientandnodeNameare unused (onlyCLUSTER_NAMEenv var is checked)getCloudMetadataFromNode:k8sClientandnodeNameare unused (only IMDS is queried)If these parameters are intended for future enhancements (e.g., fetching cluster name from k8s API or node labels), consider adding a TODO comment. Otherwise, remove them to avoid confusion.
💡 Option 1: Remove unused parameters if not needed
-func getClusterNameFromConfig(k8sClient k8sclient.K8sClientInterface, nodeName string) string { +func getClusterNameFromConfig() string { clusterName := os.Getenv("CLUSTER_NAME") if clusterName != "" { return clusterName } return "unknown" } -func getCloudMetadataFromNode(ctx context.Context, k8sClient k8sclient.K8sClientInterface, nodeName string) *armotypes.CloudMetadata { +func getCloudMetadataFromNode(ctx context.Context) *armotypes.CloudMetadata { cloudMeta, err := cloudmetadata.GetCloudMetadataWithIMDS(ctx) if err != nil { logger.L().Debug("failed to get cloud metadata from IMDS", helpers.Error(err)) return nil } return cloudMeta }And update the constructor calls accordingly:
- hostType: "k8s", - clusterName: getClusterNameFromConfig(k8sClient, cfg.NodeName), - cloudMetadata: getCloudMetadataFromNode(ctx, k8sClient, cfg.NodeName), + hostType: "k8s", + clusterName: getClusterNameFromConfig(), + cloudMetadata: getCloudMetadataFromNode(ctx),💡 Option 2: Add TODO comments if parameters are for future use
+// getClusterNameFromConfig retrieves the cluster name from environment variable. +// TODO: k8sClient and nodeName are reserved for future enhancement to fetch cluster name from k8s API. func getClusterNameFromConfig(k8sClient k8sclient.K8sClientInterface, nodeName string) string {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerprofilemanager/v1/containerprofile_manager.go` around lines 180 - 195, The functions getClusterNameFromConfig and getCloudMetadataFromNode declare unused parameters (k8sClient, nodeName); if those params are not needed, remove them from the signatures of getClusterNameFromConfig and getCloudMetadataFromNode and update all callers to the new zero-arg signatures, ensuring any call sites in containerprofile_manager.go or elsewhere are updated; if you intend to keep them for future work instead, add a brief TODO comment inside each function and either reference the params (e.g., _ = k8sClient; _ = nodeName) or rename them to _k8sClient/_nodeName to avoid unused-parameter compile errors.pkg/containerprofilemanager/v1/monitoring.go (1)
170-171: Empty annotations when cloud metadata is unavailable.
getAWSAccountID()andgetRegion()return empty strings whencloudMetadatais nil or the values are not set. This results in annotations like:kubescape.io/aws-account-id: "" kubescape.io/region: ""If empty annotations are undesirable, consider conditionally adding these annotations only when values are present.
♻️ Conditionally add cloud metadata annotations
containerProfile := &v1beta1.ContainerProfile{ ObjectMeta: metav1.ObjectMeta{ Name: slug, Namespace: container.K8s.Namespace, - Annotations: map[string]string{ + Annotations: cpm.buildAnnotations(watchedContainer), - helpersv1.InstanceIDMetadataKey: watchedContainer.InstanceID.GetStringFormatted(), - helpersv1.WlidMetadataKey: watchedContainer.Wlid, - // ... rest of annotations },With a helper method:
func (cpm *ContainerProfileManager) buildAnnotations(watchedContainer *objectcache.WatchedContainerData) map[string]string { annotations := map[string]string{ helpersv1.InstanceIDMetadataKey: watchedContainer.InstanceID.GetStringFormatted(), helpersv1.WlidMetadataKey: watchedContainer.Wlid, helpersv1.CompletionMetadataKey: string(watchedContainer.GetCompletionStatus()), helpersv1.StatusMetadataKey: string(watchedContainer.GetStatus()), helpersv1.ContainerTypeMetadataKey: watchedContainer.ContainerType.String(), helpersv1.ReportSeriesIdMetadataKey: watchedContainer.SeriesID, helpersv1.PreviousReportTimestampMetadataKey: watchedContainer.PreviousReportTimestamp.String(), helpersv1.ReportTimestampMetadataKey: watchedContainer.CurrentReportTimestamp.String(), helpersv1.HostTypeMetadataKey: cpm.getHostType(), helpersv1.ClusterMetadataKey: cpm.getClusterName(), helpersv1.HostIDMetadataKey: watchedContainer.InstanceID.GetStringFormatted(), } if awsAccountID := cpm.getAWSAccountID(); awsAccountID != "" { annotations[helpersv1.AWSAccountIDMetadataKey] = awsAccountID } if region := cpm.getRegion(); region != "" { annotations[helpersv1.RegionMetadataKey] = region } return annotations }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerprofilemanager/v1/monitoring.go` around lines 170 - 171, The annotations map currently inserts helpersv1.AWSAccountIDMetadataKey and helpersv1.RegionMetadataKey with empty strings when cpm.getAWSAccountID() or cpm.getRegion() return empty; change this to only add those keys when the returned value is non-empty. Implement a helper on ContainerProfileManager (e.g., buildAnnotations(watchedContainer *objectcache.WatchedContainerData)) that builds the base annotations and conditionally sets annotations[helpersv1.AWSAccountIDMetadataKey] = awsAccountID only if awsAccountID != "" and annotations[helpersv1.RegionMetadataKey] = region only if region != "", then use that helper where the annotations map is created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/containerprofilemanager/v1/containerprofile_manager.go`:
- Around line 170-175: Stop() currently only closes queueData and leaves timers
running for entries in the ContainerProfileManager.containers map; update Stop()
to iterate over cpm.containers, stop each container's timer (or invoke
deleteContainer(containerID) for proper cleanup if that safely encapsulates
timer teardown), and remove entries from the map so no timers remain active.
Ensure you perform this iteration under the same synchronization used elsewhere
for containers (the manager's mutex used by other container methods) to avoid
races, then close queueData as before.
In `@pkg/containerprofilemanager/v1/monitoring.go`:
- Around line 168-172: The HostIDMetadataKey is being set to
watchedContainer.InstanceID.GetStringFormatted(), duplicating
InstanceIDMetadataKey; update the metadata assignment to use cpm.hostID instead
of watchedContainer.InstanceID.GetStringFormatted() (change the value for
HostIDMetadataKey in the metadata map) and then either initialize cpm.hostID
where ContainerProfileManager is constructed/created (ensure the constructor or
NewContainerProfileManager sets cpm.hostID) or remove the unused hostID field
from the ContainerProfileManager type if host profile support is not
implemented; touch the symbols HostIDMetadataKey, InstanceIDMetadataKey,
watchedContainer.InstanceID.GetStringFormatted(), cpm.hostID, and the
ContainerProfileManager constructor/type to make the change consistent.
---
Nitpick comments:
In `@pkg/containerprofilemanager/v1/containerprofile_manager.go`:
- Around line 197-202: The getHostType method contains an unnecessary
empty-string fallback because ContainerProfileManager.hostType is initialized to
"k8s" in the constructor (e.g., NewContainerProfileManager); simplify
getHostType by returning cpm.hostType directly (remove the if-check and
hardcoded "k8s" return) so the method just returns the hostType field.
- Around line 180-195: The functions getClusterNameFromConfig and
getCloudMetadataFromNode declare unused parameters (k8sClient, nodeName); if
those params are not needed, remove them from the signatures of
getClusterNameFromConfig and getCloudMetadataFromNode and update all callers to
the new zero-arg signatures, ensuring any call sites in
containerprofile_manager.go or elsewhere are updated; if you intend to keep them
for future work instead, add a brief TODO comment inside each function and
either reference the params (e.g., _ = k8sClient; _ = nodeName) or rename them
to _k8sClient/_nodeName to avoid unused-parameter compile errors.
In `@pkg/containerprofilemanager/v1/monitoring.go`:
- Around line 170-171: The annotations map currently inserts
helpersv1.AWSAccountIDMetadataKey and helpersv1.RegionMetadataKey with empty
strings when cpm.getAWSAccountID() or cpm.getRegion() return empty; change this
to only add those keys when the returned value is non-empty. Implement a helper
on ContainerProfileManager (e.g., buildAnnotations(watchedContainer
*objectcache.WatchedContainerData)) that builds the base annotations and
conditionally sets annotations[helpersv1.AWSAccountIDMetadataKey] = awsAccountID
only if awsAccountID != "" and annotations[helpersv1.RegionMetadataKey] = region
only if region != "", then use that helper where the annotations map is created.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bc9bcfd-e9d3-44f1-813d-5e94376a33a9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.modpkg/containerprofilemanager/containerprofile_manager_interface.gopkg/containerprofilemanager/containerprofile_manager_mock.gopkg/containerprofilemanager/v1/containerprofile_manager.gopkg/containerprofilemanager/v1/monitoring.go
There was a problem hiding this comment.
🧹 Nitpick comments (4)
pkg/containerprofilemanager/v1/containerprofile_manager.go (4)
180-186: Unused parameters in function signature.Parameters
k8sClientandnodeNameare declared but never used. This suggests either incomplete implementation or parameters passed for future use. Consider removing them if not needed, or add a TODO comment explaining the intended future use.♻️ If parameters are not needed
-func getClusterNameFromConfig(k8sClient k8sclient.K8sClientInterface, nodeName string) string { +func getClusterNameFromConfig() string { clusterName := os.Getenv("CLUSTER_NAME") if clusterName != "" { return clusterName } return "unknown" }Then update the call site at line 125:
- clusterName: getClusterNameFromConfig(k8sClient, cfg.NodeName), + clusterName: getClusterNameFromConfig(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerprofilemanager/v1/containerprofile_manager.go` around lines 180 - 186, The function getClusterNameFromConfig has unused parameters k8sClient and nodeName; either remove these parameters from the signature and all call sites (update the call at the site mentioned) if they are not needed, or keep them and add a clear TODO comment in getClusterNameFromConfig explaining the intended future use and reference why k8sClient and nodeName are present; ensure any changes are mirrored where getClusterNameFromConfig is invoked so compilation stays correct and the function name remains unchanged.
188-195: Unused parameters in function signature.Same issue as
getClusterNameFromConfig: parametersk8sClientandnodeNameare declared but never used. The function only callscloudmetadata.GetCloudMetadataWithIMDS(ctx).♻️ If parameters are not needed
-func getCloudMetadataFromNode(ctx context.Context, k8sClient k8sclient.K8sClientInterface, nodeName string) *armotypes.CloudMetadata { +func getCloudMetadataFromNode(ctx context.Context) *armotypes.CloudMetadata { cloudMeta, err := cloudmetadata.GetCloudMetadataWithIMDS(ctx) if err != nil { logger.L().Debug("failed to get cloud metadata from IMDS", helpers.Error(err)) return nil } return cloudMeta }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerprofilemanager/v1/containerprofile_manager.go` around lines 188 - 195, The function getCloudMetadataFromNode declares unused parameters k8sClient and nodeName; remove these unused parameters from its signature and from all call sites so the function becomes getCloudMetadataFromNode(ctx context.Context) and only takes ctx, or if you must keep the signature for compatibility, explicitly mark them as used (e.g., _ = k8sClient; _ = nodeName) to avoid lint errors—update occurrences that call getCloudMetadataFromNode accordingly to match the new signature or leave the placeholders if you choose the compatibility route.
94-97: Unused fields declared.The fields
hostProfile,hostProfileMu, andhostIDare declared but not initialized or used in this PR. If these are placeholders for future functionality, consider deferring their addition until they're needed to keep the code clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerprofilemanager/v1/containerprofile_manager.go` around lines 94 - 97, Remove the unused fields hostProfile, hostProfileMu, and hostID from the struct where they are declared (the ContainerProfileManager struct in containerprofile_manager.go) to avoid dead code; if they are intended as future placeholders instead, add a clear TODO comment explaining intended use and suppress unused warnings (or initialize/use them), otherwise delete the declarations to keep the code clean.
197-209: Redundant fallback logic.The fallback defaults in
getHostType()andgetClusterName()duplicate the initialization logic. SincehostTypeis always initialized to"k8s"(line 124) andclusterNamealways has a value fromgetClusterNameFromConfig(), the empty-string checks are unnecessary.While defensive, this creates maintenance burden if defaults need to change. Consider either:
- Removing the redundant checks and returning the field directly, or
- Removing the initialization defaults and relying solely on the accessors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerprofilemanager/v1/containerprofile_manager.go` around lines 197 - 209, The accessor methods getHostType() and getClusterName() contain redundant empty-string fallbacks; replace the conditional logic in getHostType() and getClusterName() with direct returns of cpm.hostType and cpm.clusterName respectively, relying on the existing initialization code that sets hostType to "k8s" and clusterName via getClusterNameFromConfig(); ensure no other callers expect the old fallback behavior before removing the checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/containerprofilemanager/v1/containerprofile_manager.go`:
- Around line 180-186: The function getClusterNameFromConfig has unused
parameters k8sClient and nodeName; either remove these parameters from the
signature and all call sites (update the call at the site mentioned) if they are
not needed, or keep them and add a clear TODO comment in
getClusterNameFromConfig explaining the intended future use and reference why
k8sClient and nodeName are present; ensure any changes are mirrored where
getClusterNameFromConfig is invoked so compilation stays correct and the
function name remains unchanged.
- Around line 188-195: The function getCloudMetadataFromNode declares unused
parameters k8sClient and nodeName; remove these unused parameters from its
signature and from all call sites so the function becomes
getCloudMetadataFromNode(ctx context.Context) and only takes ctx, or if you must
keep the signature for compatibility, explicitly mark them as used (e.g., _ =
k8sClient; _ = nodeName) to avoid lint errors—update occurrences that call
getCloudMetadataFromNode accordingly to match the new signature or leave the
placeholders if you choose the compatibility route.
- Around line 94-97: Remove the unused fields hostProfile, hostProfileMu, and
hostID from the struct where they are declared (the ContainerProfileManager
struct in containerprofile_manager.go) to avoid dead code; if they are intended
as future placeholders instead, add a clear TODO comment explaining intended use
and suppress unused warnings (or initialize/use them), otherwise delete the
declarations to keep the code clean.
- Around line 197-209: The accessor methods getHostType() and getClusterName()
contain redundant empty-string fallbacks; replace the conditional logic in
getHostType() and getClusterName() with direct returns of cpm.hostType and
cpm.clusterName respectively, relying on the existing initialization code that
sets hostType to "k8s" and clusterName via getClusterNameFromConfig(); ensure no
other callers expect the old fallback behavior before removing the checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad173c3b-78d4-4491-b950-24b21532f5a8
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modpkg/containerprofilemanager/v1/containerprofile_manager.gopkg/containerprofilemanager/v1/monitoring.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/containerprofilemanager/v1/monitoring.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/containerprofilemanager/v1/container_data.go`:
- Around line 36-44: The isEmpty() method in container_data.go currently omits
the syscalls field, so containers with only syscall events may be treated as
empty and skipped; update isEmpty() to include a nil/empty check for cd.syscalls
(similar to the existing checks for cd.execs, cd.opens, etc.) so it returns
false when syscalls contains data, and ensure this aligns with emptyEvents()
which clears cd.syscalls; modify the isEmpty() implementation (function isEmpty)
to reference cd.syscalls accordingly so forceSend behavior is correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 735424f6-5b34-45dd-91d5-d8dbfc028537
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
go.modpkg/config/config.gopkg/containerprofilemanager/containerprofile_manager_interface.gopkg/containerprofilemanager/containerprofile_manager_mock.gopkg/containerprofilemanager/v1/container_data.gopkg/containerprofilemanager/v1/containerprofile_manager.gopkg/containerprofilemanager/v1/lifecycle.gopkg/containerprofilemanager/v1/monitoring.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- pkg/containerprofilemanager/v1/containerprofile_manager.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/containerprofilemanager/v1/containerprofile_manager.go (1)
170-175:⚠️ Potential issue | 🟠 Major
Close()still leaves live container timers behind.This only closes
queueData. Ifcpm.containersstill has entries during shutdown, theircontainerData.timerinstances are left running. Please tear down the remaining containers undercontainersMubefore closing the queue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerprofilemanager/v1/containerprofile_manager.go` around lines 170 - 175, The Close() method on ContainerProfileManager currently only closes queueData and leaves running timers in cpm.containers; update ContainerProfileManager.Close() to acquire the containersMu mutex, iterate over cpm.containers, stop each containerData.timer (and any other per-container resources), remove entries from the map (or set to nil), release the mutex, then call cpm.queueData.Close() so no container timers remain running during shutdown. Ensure you reference the ContainerProfileManager.Close method, containersMu, containers, and containerData.timer when making the change.pkg/containerprofilemanager/v1/container_data.go (1)
36-51:⚠️ Potential issue | 🟠 MajorInclude
syscallsin the non-empty fast path.
emptyEvents()clearscd.syscalls, butisEmpty()still ignores it. A container that only accumulated syscall data can still be treated as empty here, sosaveContainerProfile()will skip persisting that update.Suggested fix
func (cd *containerData) isEmpty() bool { if cd.capabilites != nil || + cd.syscalls != nil || cd.endpoints != nil || cd.execs != nil || cd.opens != nil || cd.rulePolicies != nil || cd.callStacks != nil || cd.networks != nil { return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerprofilemanager/v1/container_data.go` around lines 36 - 51, isEmpty() currently ignores cd.syscalls so a container that only has syscall data can be treated as empty and skipped by saveContainerProfile(); update the non-empty fast path in isEmpty() to include cd.syscalls (check cd.syscalls != nil or appropriate non-empty check consistent with cd.capabilites/cd.endpoints style) so that syscall activity prevents the empty fast path from returning true, leaving the remaining watchedContainerData/completion/status checks unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/containerprofilemanager/v1/containerprofile_manager.go`:
- Around line 197-206: There's a data race on cpm.hostType between SetHostType
and getHostType; protect reads/writes by adding a sync.RWMutex (e.g.,
hostTypeMu) to the ContainerProfileManager and use hostTypeMu.Lock()/Unlock() in
SetHostType and hostTypeMu.RLock()/RUnlock() in getHostType (or alternatively
store hostType in an atomic.Value and read/write via Load/Store). Update the
methods to reference the new mutex when accessing cpm.hostType (keep the same
method names: getHostType and SetHostType).
- Around line 188-195: getCloudMetadataFromNode currently forwards the caller
ctx into cloudmetadata.GetCloudMetadataWithIMDS, which can block startup; wrap
the call in a short deadlineed context using context.WithTimeout (e.g., 2–5s)
inside getCloudMetadataFromNode, defer the cancel(), call
GetCloudMetadataWithIMDS with the timeout ctx, and on context deadline exceeded
or error log at Debug/Info and return nil so NewContainerProfileManager can
continue without metadata.
---
Duplicate comments:
In `@pkg/containerprofilemanager/v1/container_data.go`:
- Around line 36-51: isEmpty() currently ignores cd.syscalls so a container that
only has syscall data can be treated as empty and skipped by
saveContainerProfile(); update the non-empty fast path in isEmpty() to include
cd.syscalls (check cd.syscalls != nil or appropriate non-empty check consistent
with cd.capabilites/cd.endpoints style) so that syscall activity prevents the
empty fast path from returning true, leaving the remaining
watchedContainerData/completion/status checks unchanged.
In `@pkg/containerprofilemanager/v1/containerprofile_manager.go`:
- Around line 170-175: The Close() method on ContainerProfileManager currently
only closes queueData and leaves running timers in cpm.containers; update
ContainerProfileManager.Close() to acquire the containersMu mutex, iterate over
cpm.containers, stop each containerData.timer (and any other per-container
resources), remove entries from the map (or set to nil), release the mutex, then
call cpm.queueData.Close() so no container timers remain running during
shutdown. Ensure you reference the ContainerProfileManager.Close method,
containersMu, containers, and containerData.timer when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d7e940f-0ed2-4850-860d-190535f305fd
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
configuration/config.jsondocs/CONFIGURATION.mdgo.modpkg/config/config.gopkg/containerprofilemanager/containerprofile_manager_interface.gopkg/containerprofilemanager/containerprofile_manager_mock.gopkg/containerprofilemanager/v1/container_data.gopkg/containerprofilemanager/v1/containerprofile_manager.gopkg/containerprofilemanager/v1/lifecycle.gopkg/containerprofilemanager/v1/monitoring.go
✅ Files skipped from review due to trivial changes (2)
- docs/CONFIGURATION.md
- go.mod
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/containerprofilemanager/v1/monitoring.go
- pkg/containerprofilemanager/containerprofile_manager_mock.go
- pkg/containerprofilemanager/v1/lifecycle.go
- pkg/containerprofilemanager/containerprofile_manager_interface.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/containerprofilemanager/v1/container_data.go (1)
36-44:⚠️ Potential issue | 🟡 MinorMissing
syscallscheck inisEmpty()still present.The refactored
isEmpty()method checkscapabilites,endpoints,execs,opens,rulePolicies,callStacks, andnetworks, but still omitssyscalls. SinceemptyEvents()clearssyscalls(line 21), containers with only new syscall data will be incorrectly treated as empty whenforceSendis false.🐛 Proposed fix to add syscalls check
func (cd *containerData) isEmpty() bool { if cd.capabilites != nil || cd.endpoints != nil || cd.execs != nil || cd.opens != nil || cd.rulePolicies != nil || cd.callStacks != nil || + cd.syscalls != nil || cd.networks != nil { return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerprofilemanager/v1/container_data.go` around lines 36 - 44, The isEmpty() method in container_data.go currently omits checking the syscalls field, so containers with only syscall data are treated as empty; update isEmpty() to include a nil/empty check for cd.syscalls similar to the other fields (the same field cleared by emptyEvents()), ensuring the function returns false when cd.syscalls is non-nil/non-empty; locate the isEmpty() function and add the syscalls check alongside capabilites, endpoints, execs, opens, rulePolicies, callStacks, and networks.
🧹 Nitpick comments (1)
pkg/objectcache/shared_container_data_test.go (1)
77-77: Test updated for new GetLabels signature, but missing coverage for cloud metadata.The call is correctly updated to pass
nilfor cloudMetadata. However, there's no test case verifying the cloud metadata label augmentation when a non-nilcloudMetadatais provided. Consider adding a test case to verifyHostType,Cluster,AWSAccountID, andRegionlabels are correctly populated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/shared_container_data_test.go` at line 77, Add a new unit test case in shared_container_data_test.go that calls GetLabels with a non-nil cloudMetadata argument (instead of nil) to verify cloud metadata augmentation; construct a cloudMetadata value with fields HostType, Cluster, AWSAccountID, and Region set to distinct test values, call got := GetLabels(cloudMetadata, tt.args.watchedContainer, tt.args.stripContainer) and assert the returned labels include the expected "HostType", "Cluster", "AWSAccountID", and "Region" keys with the corresponding values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/containerprofilemanager/v1/container_data.go`:
- Around line 36-44: The isEmpty() method in container_data.go currently omits
checking the syscalls field, so containers with only syscall data are treated as
empty; update isEmpty() to include a nil/empty check for cd.syscalls similar to
the other fields (the same field cleared by emptyEvents()), ensuring the
function returns false when cd.syscalls is non-nil/non-empty; locate the
isEmpty() function and add the syscalls check alongside capabilites, endpoints,
execs, opens, rulePolicies, callStacks, and networks.
---
Nitpick comments:
In `@pkg/objectcache/shared_container_data_test.go`:
- Line 77: Add a new unit test case in shared_container_data_test.go that calls
GetLabels with a non-nil cloudMetadata argument (instead of nil) to verify cloud
metadata augmentation; construct a cloudMetadata value with fields HostType,
Cluster, AWSAccountID, and Region set to distinct test values, call got :=
GetLabels(cloudMetadata, tt.args.watchedContainer, tt.args.stripContainer) and
assert the returned labels include the expected "HostType", "Cluster",
"AWSAccountID", and "Region" keys with the corresponding values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c9eca4d-8f1e-4913-b284-92bec6408c39
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
configuration/config.jsondocs/CONFIGURATION.mdgo.modpkg/config/config.gopkg/containerprofilemanager/v1/container_data.gopkg/containerprofilemanager/v1/containerprofile_manager.gopkg/containerprofilemanager/v1/lifecycle.gopkg/containerprofilemanager/v1/monitoring.gopkg/objectcache/shared_container_data.gopkg/objectcache/shared_container_data_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/CONFIGURATION.md
- configuration/config.json
- pkg/containerprofilemanager/v1/containerprofile_manager.go
- go.mod
matthyx
force-pushed
the
cp-metadata
branch
5 times, most recently
from
9184323 to
f027144
Compare
Signed-off-by: Matthias Bertschy <matthias.bertschy@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.
Summary by CodeRabbit
New Features
Configuration Changes
Dependencies
Improvements