fixed addRunningContainers

dwertent commented 7 months ago

User description

Overview

Type

enhancement, bug_fix

Description

Refactored container management logic in container_watcher_private.go for better clarity and efficiency.
Introduced comprehensive unit tests for new container management methods.
Implemented a mock Kubernetes client for testing container management logic.
Enhanced file information retrieval in helpers.go with better path construction and error handling.
Removed unnecessary error logging in r0002_unexpected_file_access.go.

Changes walkthrough

Relevant files

Enhancement

container_watcher_private.go

Refactor and Enhance Container Management Logic

pkg/containerwatcher/v1/container_watcher_private.go

Refactored addRunningContainers to startRunningContainers and added a
new method addRunningContainers.

Improved error handling by using logger.L().Fatal instead of returning
an error when creating Kubernetes client fails.

Enhanced container management logic to handle adding and removing
containers more effectively.

+34/-33

helpers.go

Enhance File Information Retrieval and Error Handling

pkg/ruleengine/v1/helpers.go

Improved file path construction for getting path from PID.

Enhanced file hash calculation and file size retrieval with better
error handling.

Added checks to ensure file exists before attempting to calculate
hashes or size.

+45/-39

Tests

container_watcher_private_test.go `Add Unit Tests for Container Management` pkg/containerwatcher/v1/container_watcher_private_test.go Introduced unit tests for `addRunningContainers` and `unregisterContainer` methods. Tests cover scenarios like adding, removing containers, and ignoring pods.	+270/-0
ig_k8sclient.go `Implement Kubernetes Client Mock for Testing` pkg/containerwatcher/v1/ig_k8sclient.go Implemented `IGK8sClientMock` for mocking Kubernetes client operations. Added methods for getting running and non-running containers, and listing containers.	+89/-0

Bug_fix

r0002_unexpected_file_access.go

Remove Unnecessary Error Logging

pkg/ruleengine/v1/r0002_unexpected_file_access.go - Removed an error log statement that was deemed unnecessary.

+0/-1

✨ PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent-free[bot] commented 7 months ago

PR Description updated to latest commit (https://github.com/kubescape/node-agent/commit/52c3748fd678943812397acf675041a44f1a33ad)

codiumai-pr-agent-free[bot] commented 7 months ago

PR Review

⏱️ Estimated effort to review [1-5]	4, because the PR involves significant changes across multiple files, including the introduction of new functionalities, refactoring of existing code, and the addition of comprehensive tests. The complexity of the changes and the need to understand the context of container management logic in Kubernetes environments increase the review effort.
🧪 Relevant tests	Yes
🔍 Possible issues	Fatal Logging Strategy: The change from returning an error to using `logger.L().Fatal` in `startRunningContainers` method could terminate the program unexpectedly. Consider handling the error more gracefully or documenting the rationale for this approach.
	Error Handling Omission: In `helpers.go`, the new error handling for file path construction and file info retrieval introduces a warning log but does not address the potential absence of file hash calculations and file size retrieval when the file does not exist or another error occurs. This could lead to incomplete alert enrichments.
	Test Coverage for Error Paths: While comprehensive unit tests have been added, it's unclear if there are tests covering error paths, especially for the new error handling logic in `helpers.go` and the fatal logging in `container_watcher_private.go`.
🔒 Security concerns	No

✨ Review tool usage guide:

**Overview:** The `review` tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be [added](https://pr-agent-docs.codium.ai/tools/review/#general-configurations) by configuring the tool. The tool can be triggered [automatically](https://pr-agent-docs.codium.ai/usage-guide/automations_and_usage/#github-app-automatic-tools-when-a-new-pr-is-opened) every time a new PR is opened, or can be invoked manually by commenting on any PR. - When commenting, to edit [configurations](https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/settings/configuration.toml#L23) related to the review tool (`pr_reviewer` section), use the following template: ``` /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=... ``` - With a [configuration file](https://pr-agent-docs.codium.ai/usage-guide/configuration_options/), use the following template: ``` [pr_reviewer] some_config1=... some_config2=... ``` See the review [usage page](https://pr-agent-docs.codium.ai/tools/review/) for a comprehensive guide on using this tool.

codiumai-pr-agent-free[bot] commented 7 months ago

PR Code Suggestions

Category	Suggestions
Enhancement	Replace direct fatal logging with error return for better error handling. ___ Instead of calling `logger.L().Fatal` directly when failing to create a Kubernetes client, consider returning the error to the caller. This allows the caller to handle the error appropriately, which is especially important in a library or shared code where the caller might want to manage error handling differently. [pkg/containerwatcher/v1/container_watcher_private.go [97]](https://github.com/kubescape/node-agent/pull/249/files#diff-6f95b4caa6090a17da5aed1923600fd049392d228e0fba99cc212f48111f3ffeR97-R97) ```diff -logger.L().Fatal("creating IG Kubernetes client", helpers.Error(err)) +return fmt.Errorf("failed to create IG Kubernetes client: %w", err) ```
Best practice	Rename variable to avoid shadowing and improve code clarity. ___ Avoid using the same variable name `container` for both the range variable and a new variable inside the loop. This can lead to confusion and bugs. Consider renaming the inner variable to something more descriptive, like `newContainerCopy`. [pkg/containerwatcher/v1/container_watcher_private.go [132-133]](https://github.com/kubescape/node-agent/pull/249/files#diff-6f95b4caa6090a17da5aed1923600fd049392d228e0fba99cc212f48111f3ffeR132-R133) ```diff -newContainer := containercollection.Container{} -newContainer = container +newContainerCopy := containercollection.Container{} +newContainerCopy = container ```
Maintainability	Refactor conditional logic to reduce nesting and improve readability. ___ When checking if a container is already being monitored, it's more efficient to continue the loop immediately after finding a match instead of nesting the main logic inside an `if` block. This reduces nesting and makes the code easier to read. [pkg/containerwatcher/v1/container_watcher_private.go [125-135]](https://github.com/kubescape/node-agent/pull/249/files#diff-6f95b4caa6090a17da5aed1923600fd049392d228e0fba99cc212f48111f3ffeR125-R135) ```diff -if ch.timeBasedContainers.Contains(container.Runtime.ContainerID) { - // the container is already being monitored - continue +if !ch.timeBasedContainers.Contains(container.Runtime.ContainerID) { + // Make a copy instead of passing the same pointer at + // each iteration of the loop + newContainer := containercollection.Container{} + newContainer = container + ch.preRunningContainersIDs.Add(container.Runtime.ContainerID) + ch.containerCollection.AddContainer(&newContainer) } -// Make a copy instead of passing the same pointer at -// each iteration of the loop -newContainer := containercollection.Container{} -newContainer = container ```
Possible issue	Modify method to return actual container IDs instead of names. ___ The `GetRunningContainers` method currently returns container names instead of actual container IDs for running containers. This might not be the intended behavior since container IDs are usually required for container management operations. Consider modifying the method to return actual container IDs. [pkg/containerwatcher/v1/ig_k8sclient.go [63-68]](https://github.com/kubescape/node-agent/pull/249/files#diff-d2efb84a804461237aa6fba85fa216394cfed7a09daa41e2aa5ee6d40ec050d5R63-R68) ```diff containers = append(containers, containercollection.Container{ Runtime: containercollection.RuntimeMetadata{ BasicRuntimeMetadata: types.BasicRuntimeMetadata{ - ContainerID: s.Name, + ContainerID: extractContainerID(s), }, }, }) ```
Performance	Refactor code to avoid unnecessary operations on non-existent files. ___ The current implementation of `enrichRuleFailure` performs file existence checks and hash calculations even if the file does not exist. Refactor the code to first check if the file exists and then proceed with hash calculations and size retrieval to avoid unnecessary operations on non-existent files. [pkg/ruleengine/v1/helpers.go [129-133]](https://github.com/kubescape/node-agent/pull/249/files#diff-f3329a44e0082b4930f884bc54a29b221995eb51217668bf1dc11152b2022fd7R129-R133) ```diff if _, err := os.Stat(hostPath); err == nil { - // file does not exist if ruleFailure.BaseRuntimeAlert.MD5Hash == "" { md5hash, err := utils.CalculateMD5FileHash(hostPath) ```

✨ Improve tool usage guide:

**Overview:** The `improve` tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered [automatically](https://pr-agent-docs.codium.ai/usage-guide/automations_and_usage/#github-app-automatic-tools-when-a-new-pr-is-opened) every time a new PR is opened, or can be invoked manually by commenting on a PR. - When commenting, to edit [configurations](https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/settings/configuration.toml#L78) related to the improve tool (`pr_code_suggestions` section), use the following template: ``` /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=... ``` - With a [configuration file](https://pr-agent-docs.codium.ai/usage-guide/configuration_options/), use the following template: ``` [pr_code_suggestions] some_config1=... some_config2=... ``` See the improve [usage page](https://pr-agent-docs.codium.ai/tools/improve/) for a comprehensive guide on using this tool.

github-actions[bot] commented 7 months ago

Summary:

License scan: failure
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: failure

github-actions[bot] commented 7 months ago

Summary:

License scan: failure
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: failure

kubescape / node-agent