Feature/new exporters - Githubissues

User description

Overview

Type

enhancement, bug_fix

Description

Refactored HTTPExporter, SyslogExporter, and CsvExporter to use apitypes.RuntimeAlert.
Implemented AlertManagerExporter for sending alerts to Prometheus Alertmanager.
Enhanced ClamAV malware detection with additional metadata and refactoring.
Added helper methods for rule failure enrichment in rule engine.
Updated various rules to use apitypes.RuntimeAlert and added enrichment with additional metadata.
Added utility methods for file hash calculation and command line retrieval.
Implemented StdoutExporter for logging alerts to stdout.
Updated tests to reflect changes in HTTPExporter, SyslogExporter, CsvExporter, and AlertManagerExporter.

Changes walkthrough

Relevant files

Enhancement

17 files

http_exporter.go `Refactor HTTPExporter to use RuntimeAlert and support cluster details` pkg/exporters/http_exporter.go Added new fields `ClusterName`, `k8sClient` to `HTTPExporter` struct. Replaced `HTTPAlert` struct with `apitypes.RuntimeAlert`. Modified `sendAlertLimitReached`, `SendRuleAlert`, and `SendMalwareAlert` methods to use `apitypes.RuntimeAlert`. Added method `getWorkloadIdentifiers` to retrieve workload details.	+86/-99
clamav.go `Enhance ClamAV malware detection with additional metadata and` `refactoring` pkg/malwaremanager/v1/clamav/clamav.go Added new constants and methods for malware detection. Refactored `handleExecEvent` and `handleOpenEvent` to use `apitypes.RuntimeAlert`. Enhanced malware detection with additional metadata.	+111/-39
alert_manager.go `Implement AlertManagerExporter for sending alerts to Prometheus` `Alertmanager` pkg/exporters/alert_manager.go Added `AlertManagerExporter` struct with methods to send rule and malware alerts. Alerts are now sent to Prometheus Alertmanager.	+155/-0
syslog_exporter.go `Refactor SyslogExporter to use RuntimeAlert` pkg/exporters/syslog_exporter.go Refactored `SyslogExporter` to use `apitypes.RuntimeAlert`. Updated `SendRuleAlert` and `SendMalwareAlert` methods.	+41/-46
helpers.go `Add helper methods for rule failure enrichment in rule engine` pkg/ruleengine/v1/helpers.go Added helper methods for rule failure enrichment. Methods include `getPathFromPid`, `getCommFromPid`, and `enrichRuleFailure`.	+181/-0
r0007_kubernetes_client_executed.go `Update Kubernetes client executed rule to use RuntimeAlert and` `enrichment` pkg/ruleengine/v1/r0007_kubernetes_client_executed.go Updated to use `apitypes.RuntimeAlert` for rule failure. Added enrichment of rule failure with additional metadata.	+50/-17
malwareresult.go `Refactor GenericMalwareResult to use new RuntimeAlert types` pkg/malwaremanager/v1/types/malwareresult.go Refactored `GenericMalwareResult` to use `apitypes.BaseRuntimeAlert` and related types.	+26/-89
utils.go `Add utility methods for file hash calculation and command line` `retrieval` pkg/utils/utils.go Added utility methods for file hash calculation and command line retrieval.	+147/-0
r0003_unexpected_system_call.go `Update unexpected system call rule to use RuntimeAlert and enrichment` pkg/ruleengine/v1/r0003_unexpected_system_call.go Updated to use `apitypes.RuntimeAlert` for rule failure. Added enrichment of rule failure with additional metadata.	+35/-15
csv_exporter.go `Update CsvExporter to use RuntimeAlert` pkg/exporters/csv_exporter.go Updated `CsvExporter` to use `apitypes.RuntimeAlert`. Modified `SendRuleAlert` and `SendMalwareAlert` methods.	+37/-32
r0001_unexpected_process_launched.go `Update unexpected process launched rule to use RuntimeAlert and` `enrichment` pkg/ruleengine/v1/r0001_unexpected_process_launched.go Updated to use `apitypes.RuntimeAlert` for rule failure. Added enrichment of rule failure with additional metadata.	+29/-9
r0004_unexpected_capability_used.go `Update unexpected capability used rule to use RuntimeAlert and` `enrichment` pkg/ruleengine/v1/r0004_unexpected_capability_used.go Updated to use `apitypes.RuntimeAlert` for rule failure. Added enrichment of rule failure with additional metadata.	+24/-9
r1001_exec_binary_not_in_base_image.go `Update exec binary not in base image rule to use RuntimeAlert and` `enrichment` pkg/ruleengine/v1/r1001_exec_binary_not_in_base_image.go Updated to use `apitypes.RuntimeAlert` for rule failure. Added enrichment of rule failure with additional metadata.	+28/-9
r0002_unexpected_file_access.go `Update unexpected file access rule to use RuntimeAlert and enrichment` pkg/ruleengine/v1/r0002_unexpected_file_access.go Updated to use `apitypes.RuntimeAlert` for rule failure. Added enrichment of rule failure with additional metadata.	+24/-9
r1003_malicious_ssh_connection.go `Update malicious SSH connection rule to use RuntimeAlert and` `enrichment` pkg/ruleengine/v1/r1003_malicious_ssh_connection.go Updated to use `apitypes.RuntimeAlert` for rule failure. Added enrichment of rule failure with additional metadata.	+25/-9
malware_manager_interface.go `Refactor MalwareResult interface to use new RuntimeAlert types` pkg/malwaremanager/malware_manager_interface.go Refactored `MalwareResult` interface to use `apitypes.BaseRuntimeAlert` and related types.	+12/-28
stdout_exporter.go `Implement StdoutExporter for logging alerts to stdout` pkg/exporters/stdout_exporter.go Added `StdoutExporter` for logging alerts to stdout. Implemented `SendRuleAlert` and `SendMalwareAlert` methods.	+61/-0

Tests

4 files

http_exporter_test.go `Update HTTPExporter tests for new RuntimeAlert structure` pkg/exporters/http_exporter_test.go Updated tests to reflect changes in `HTTPExporter`. Replaced usage of `GenericRuleFailure` with `ruleenginev1.GenericRuleFailure` and `apitypes.BaseRuntimeAlert`.	+84/-79
syslog_exporter_test.go `Add tests for SyslogExporter with RuntimeAlert structure` pkg/exporters/syslog_exporter_test.go Added tests for `SyslogExporter` reflecting the new `RuntimeAlert` structure.	+144/-0
alert_manager_test.go `Add tests for AlertManagerExporter with RuntimeAlert structure` pkg/exporters/alert_manager_test.go Added tests for `AlertManagerExporter` reflecting the new `RuntimeAlert` structure.	+56/-23
csv_exporter_test.go `Update CsvExporter tests for new RuntimeAlert structure` pkg/exporters/csv_exporter_test.go Updated tests for `CsvExporter` to reflect changes in `RuntimeAlert` structure.	+59/-21

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

PR Code Suggestions

Category	Suggestions
Enhancement	Improve error handling in `InitHTTPExporter` function. ___ Consider handling the error returned by `config.Validate()` more gracefully by logging the error and returning a nil or initializing a default configuration instead of just returning the error. This can improve the robustness of your exporter initialization. [pkg/exporters/http_exporter.go [82-83]](https://github.com/kubescape/node-agent/pull/231/files#diff-6e04fd42d767812ea2855370a21a524371930b320544c0ee0954e1500242fbbdR82-R83) ```diff if err := config.Validate(); err != nil { + logger.L().Error("Failed to validate HTTPExporterConfig", helpers.Error(err)) + // Consider initializing a default config or handling the error differently here return nil, err } ```
	Add detailed logging for error scenarios in `getWorkloadIdentifiers`. ___ For the `getWorkloadIdentifiers` function, consider adding more detailed logging for each error scenario to aid in debugging and operational monitoring. This can include logging the `podNamespace` and `podName` when a workload retrieval fails. [pkg/exporters/http_exporter.go [241-243]](https://github.com/kubescape/node-agent/pull/231/files#diff-6e04fd42d767812ea2855370a21a524371930b320544c0ee0954e1500242fbbdR241-R243) ```diff wl, err := exporter.k8sClient.GetWorkload(podNamespace, "Pod", podName) if err != nil { + logger.L().Error("Failed to get workload", helpers.String("podNamespace", podNamespace), helpers.String("podName", podName), helpers.Error(err)) return "", "", "", fmt.Errorf("failed to get workload: %w", err) } ```
	Implement batch alert sending in `sendInAlertList`. ___ In the `sendInAlertList` function, consider implementing the TODO to accumulate alerts and send them in a batch. This can significantly reduce the number of HTTP requests made and improve the efficiency of the alerting mechanism. [pkg/exporters/http_exporter.go [146-148]](https://github.com/kubescape/node-agent/pull/231/files#diff-6e04fd42d767812ea2855370a21a524371930b320544c0ee0954e1500242fbbdR146-R148) ```diff -// TODO: accumulate alerts and send them in a batch -httpAlertsListSpec := HTTPAlertsListSpec{ - Alerts: []apitypes.RuntimeAlert{httpAlert}, +// Implementation to accumulate alerts and send them in a batch +exporter.accumulateAlerts(httpAlert) +if exporter.readyToSendBatch() { + httpAlertsListSpec := exporter.createBatchAlertsListSpec() + // Send batch alerts logic here } ```
	Include PID in error messages for better debugging context. ___ For error messages, consider including the PID in the message when logging errors related to getting command line or parent process details. This will make debugging easier by providing more context. [pkg/malwaremanager/v1/clamav/clamav.go [198-206]](https://github.com/kubescape/node-agent/pull/231/files#diff-946a7b5957a07b2ec4516a5a7e935c1b17de7cb6a27511e48463ed6f3d8cd159R198-R206) ```diff -logger.L().Error("Error getting command line of pid", helpers.Error(err)) -logger.L().Error("Error getting ppid of pid", helpers.Error(err)) +logger.L().Error(fmt.Sprintf("Error getting command line of pid %d", openEvent.Pid), helpers.Error(err)) +logger.L().Error(fmt.Sprintf("Error getting ppid of pid %d", openEvent.Pid), helpers.Error(err)) ```
	Use meaningful default values instead of `nil` for optional parameters. ___ Instead of using `nil` for the optional parameters in `InitHTTPExporter`, consider defining and passing meaningful default values or configurations. [pkg/exporters/http_exporter_test.go [35]](https://github.com/kubescape/node-agent/pull/231/files#diff-89585231dfa94efdc4cac61ef6426f59bf494acd49c810bb29d56e16574fd350R35-R35) ```diff -}, "", "", nil) +}, defaultAuthKey, defaultAuthValue, defaultHeaders) ```
	Improve error handling after writing the syslog message. ___ Use a more descriptive error handling strategy after attempting to write the syslog message. Logging the error or retrying the write operation could be beneficial. [pkg/exporters/syslog_exporter.go [108]](https://github.com/kubescape/node-agent/pull/231/files#diff-d84d67a80745d135f3f4af7c387c52361f506fc84f626e517ea8b37b150f8bb7R108-R108) ```diff -_, err := message.WriteTo(se.writer) +if _, err := message.WriteTo(se.writer); err != nil { + log.Printf("Failed to write syslog message: %v", err) + // Consider retrying the write operation or handling the error accordingly. +} ```
	Use a more relevant process ID for malware alerts. ___ For the `ProcessID` field in the malware alert, consider using a more relevant process identifier related to the malware event, if available, instead of the exporter's process ID. [pkg/exporters/syslog_exporter.go [121]](https://github.com/kubescape/node-agent/pull/231/files#diff-d84d67a80745d135f3f4af7c387c52361f506fc84f626e517ea8b37b150f8bb7R121-R121) ```diff -ProcessID: fmt.Sprintf("%d", os.Getpid()), // TODO: is this correct? +ProcessID: fmt.Sprintf("%d", relevantProcessID), // Replace `relevantProcessID` with the actual process ID related to the malware event. ```
Maintainability	Refactor `sendAlertLimitReached` for better readability. ___ To enhance the readability and maintainability of the `sendAlertLimitReached` function, consider breaking down the construction of `httpAlert` into smaller functions or blocks, especially the initialization of nested struct fields. [pkg/exporters/http_exporter.go [95-107]](https://github.com/kubescape/node-agent/pull/231/files#diff-6e04fd42d767812ea2855370a21a524371930b320544c0ee0954e1500242fbbdR95-R107) ```diff +baseAlert := createBaseRuntimeAlert("AlertLimitReached", ruleenginev1.RulePrioritySystemIssue, "Check logs for more information") +k8sDetails := createRuntimeAlertK8sDetails(exporter.NodeName, &exporter.ClusterName) httpAlert := apitypes.RuntimeAlert{ - Message: "Alert limit reached", - HostName: exporter.Host, - AlertType: apitypes.AlertTypeRule, // TODO: change this to a new alert type. @bez - BaseRuntimeAlert: apitypes.BaseRuntimeAlert{ - AlertName: "AlertLimitReached", - Severity: ruleenginev1.RulePrioritySystemIssue, - FixSuggestions: "Check logs for more information", - }, - RuntimeAlertK8sDetails: apitypes.RuntimeAlertK8sDetails{ - NodeName: exporter.NodeName, - ClusterName: &exporter.ClusterName, - }, + Message: "Alert limit reached", + HostName: exporter.Host, + AlertType: apitypes.AlertTypeRule, // TODO: change this to a new alert type. @bez + BaseRuntimeAlert: baseAlert, + RuntimeAlertK8sDetails: k8sDetails, } ```
	Move struct definitions into separate files for better organization. ___ For better code organization and readability, consider moving the struct definitions (`HTTPExporterConfig`, `HTTPExporter`, `HTTPAlertsListSpec`, etc.) into separate files. This can make the codebase easier to navigate and maintain. [pkg/exporters/http_exporter.go [23-57]](https://github.com/kubescape/node-agent/pull/231/files#diff-6e04fd42d767812ea2855370a21a524371930b320544c0ee0954e1500242fbbdR23-R57) ```diff -type HTTPExporterConfig struct { -... -} -type HTTPExporter struct { -... -} -type HTTPAlertsListSpec struct { -... -} +// Suggestion to move struct definitions into separate files +// Example: http_exporter_config.go, http_exporter.go, http_alerts_list_spec.go ```
	Use constants for severity levels to improve code clarity. ___ To improve maintainability, consider defining the severity levels as constants with descriptive names. This will make the code easier to understand and modify in the future. [pkg/malwaremanager/v1/clamav/clamav.go [125]](https://github.com/kubescape/node-agent/pull/231/files#diff-946a7b5957a07b2ec4516a5a7e935c1b17de7cb6a27511e48463ed6f3d8cd159R125-R125) ```diff -Severity: 10, // TODO: Get severity from api. +Severity: SEVERITY_HIGH, // Assuming SEVERITY_HIGH is a constant defined elsewhere ```
	Use constants for repeated string literals. ___ Use a constant for the repeated string literals like "testcontainerid", "testnamespace", etc., to avoid typos and make future changes easier. [pkg/exporters/http_exporter_test.go [46-50]](https://github.com/kubescape/node-agent/pull/231/files#diff-89585231dfa94efdc4cac61ef6426f59bf494acd49c810bb29d56e16574fd350R46-R50) ```diff -ContainerID: "testcontainerid", -Namespace: "testnamespace", +const ( + testContainerID = "testcontainerid" + testNamespace = "testnamespace" +) +ContainerID: testContainerID, +Namespace: testNamespace, ```
	Extract URL and summary construction into separate functions. ___ Extract the construction of `sourceUrl` and `summary` into separate functions to improve readability and maintainability of `SendRuleAlert` and `SendMalwareAlert` methods. [pkg/exporters/alert_manager.go [47-57]](https://github.com/kubescape/node-agent/pull/231/files#diff-40568094a80f334c1148d539cedecc426f810a6e32e5eeab274195f4c80693d5R47-R57) ```diff -sourceUrl := fmt.Sprintf("https://armosec.github.io/kubecop/alertviewer/?AlertMessage=%s&AlertRuleName=%s&AlertRuleID=%s&AlertFix=%s&AlertNamespace=%s&AlertPod=%s&AlertContainer=%s&AlertProcess=%s", -summary := fmt.Sprintf("Rule '%s' in '%s' namespace '%s' failed", failedRule.GetBaseRuntimeAlert().AlertName, failedRule.GetRuntimeAlertK8sDetails().PodName, failedRule.GetRuntimeAlertK8sDetails().Namespace) +sourceUrl := constructSourceUrl(failedRule) +summary := constructSummary(failedRule) ```
	Abstract the construction of syslog messages into a separate method. ___ Consider abstracting the construction of the `rfc5424.Message` into a separate method or function to improve code readability and maintainability. [pkg/exporters/syslog_exporter.go [48-106]](https://github.com/kubescape/node-agent/pull/231/files#diff-d84d67a80745d135f3f4af7c387c52361f506fc84f626e517ea8b37b150f8bb7R48-R106) ```diff -message := rfc5424.Message{ - Priority: rfc5424.Error, - Timestamp: failedRule.GetBaseRuntimeAlert().Timestamp, - ... -} +message := constructSyslogMessage(failedRule) +// Implement `constructSyslogMessage` to construct the `rfc5424.Message` from a `failedRule`. ```
Best practice	Handle potential errors from time conversion for robustness. ___ Consider handling the error from `time.Unix` to ensure robustness. While `time.Unix` is unlikely to fail with valid inputs, explicitly checking for errors can prevent potential issues in edge cases or future changes. [pkg/malwaremanager/v1/clamav/clamav.go [127]](https://github.com/kubescape/node-agent/pull/231/files#diff-946a7b5957a07b2ec4516a5a7e935c1b17de7cb6a27511e48463ed6f3d8cd159R127-R127) ```diff -Timestamp: time.Unix(int64(execEvent.Timestamp), 0), +timestamp, err := time.Unix(int64(execEvent.Timestamp), 0) +if err != nil { + logger.L().Error("Error converting timestamp", helpers.Error(err)) + return nil +} +Timestamp: timestamp, ```
	Use explicit boolean values instead of nil for clarity. ___ Instead of using `nil` directly for `IsPartOfImage` in the open event, consider a more explicit approach to indicate the absence of this information, such as using a boolean variable set to `false`. [pkg/malwaremanager/v1/clamav/clamav.go [220]](https://github.com/kubescape/node-agent/pull/231/files#diff-946a7b5957a07b2ec4516a5a7e935c1b17de7cb6a27511e48463ed6f3d8cd159R220-R220) ```diff -IsPartOfImage: nil, // We don't have that enrichement in the open event. +isPartOfImage := false +IsPartOfImage: &isPartOfImage, ```
	Check for nil before dereferencing to improve code safety. ___ To avoid potential nil pointer dereference and improve code safety, check if `execEvent.Runtime` and `execEvent.K8s` are not nil before accessing their fields. [pkg/malwaremanager/v1/clamav/clamav.go [242-247]](https://github.com/kubescape/node-agent/pull/231/files#diff-946a7b5957a07b2ec4516a5a7e935c1b17de7cb6a27511e48463ed6f3d8cd159R242-R247) ```diff -ContainerID: execEvent.Runtime.ContainerID, -HostNetwork: &execEvent.K8s.HostNetwork, +ContainerID: safeString(execEvent.Runtime.ContainerID), +HostNetwork: safeBoolPointer(execEvent.K8s.HostNetwork), ```
	Handle errors returned by `InitHTTPExporter` in tests. ___ Consider handling the error returned by `InitHTTPExporter` in tests by asserting no error occurred. This ensures that the exporter initialization is successful before proceeding with the test logic. [pkg/exporters/http_exporter_test.go [35]](https://github.com/kubescape/node-agent/pull/231/files#diff-89585231dfa94efdc4cac61ef6426f59bf494acd49c810bb29d56e16574fd350R35-R35) ```diff exporter, err := InitHTTPExporter(HTTPExporterConfig{ }, "", "", nil) +assert.NoError(t, err) ```
	Add error handling for system calls in `InitAlertManagerExporter`. ___ Implement error handling for `os.Hostname()` and `os.Getenv("NODE_NAME")` to prevent panic and allow graceful error handling in `InitAlertManagerExporter`. [pkg/exporters/alert_manager.go [34-37]](https://github.com/kubescape/node-agent/pull/231/files#diff-40568094a80f334c1148d539cedecc426f810a6e32e5eeab274195f4c80693d5R34-R37) ```diff hostName, err := os.Hostname() if err != nil { - panic(fmt.Sprintf("failed to get hostname: %v", err)) + log.Errorf("failed to get hostname: %v", err) + return nil, err } ```
Possible issue	Add nil pointer checks before dereferencing to avoid runtime panics. ___ Consider checking for nil pointers before dereferencing `failedRule.GetRuntimeProcessDetails()`, `failedRule.GetRuntimeAlertK8sDetails()`, and `failedRule.GetBaseRuntimeAlert()` to avoid potential runtime panics. [pkg/exporters/syslog_exporter.go [50-53]](https://github.com/kubescape/node-agent/pull/231/files#diff-d84d67a80745d135f3f4af7c387c52361f506fc84f626e517ea8b37b150f8bb7R50-R53) ```diff -Timestamp: failedRule.GetBaseRuntimeAlert().Timestamp, -Hostname: failedRule.GetRuntimeAlertK8sDetails().PodName, -AppName: failedRule.GetRuntimeAlertK8sDetails().ContainerName, -ProcessID: fmt.Sprintf("%d", failedRule.GetRuntimeProcessDetails().PID), +if baseRuntimeAlert := failedRule.GetBaseRuntimeAlert(); baseRuntimeAlert != nil { + Timestamp: baseRuntimeAlert.Timestamp, +} +if k8sDetails := failedRule.GetRuntimeAlertK8sDetails(); k8sDetails != nil { + Hostname: k8sDetails.PodName, + AppName: k8sDetails.ContainerName, +} +if processDetails := failedRule.GetRuntimeProcessDetails(); processDetails != nil { + ProcessID: fmt.Sprintf("%d", processDetails.PID), +} ```
Possible issue	Check for nil before dereferencing the `Size` pointer. ___ For the `Size` field in the malware alert structured data, ensure that the pointer is checked for nil before dereferencing to avoid a potential panic. [pkg/exporters/syslog_exporter.go [152]](https://github.com/kubescape/node-agent/pull/231/files#diff-d84d67a80745d135f3f4af7c387c52361f506fc84f626e517ea8b37b150f8bb7R152-R152) ```diff -Value: malwareResult.GetBasicRuntimeAlert().Size, +sizeValue := "unknown" +if sizePtr := malwareResult.GetBasicRuntimeAlert().Size; sizePtr != nil { + sizeValue = fmt.Sprintf("%d", sizePtr) +} +Value: sizeValue, ```

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

⏱️ Estimated effort to review [1-5]	5, because the PR introduces significant changes across multiple files, including the addition of new features (e.g., `AlertManagerExporter`, `StdoutExporter`), refactoring of existing code to use new data structures (e.g., changes from specific getters to a more generic approach in `MalwareResult` interface), and modifications to the rule processing logic to incorporate these changes. The PR touches on various aspects of the system, including rule processing, malware detection, and alert exporting, requiring a thorough review to ensure compatibility and correctness.
🧪 Relevant tests	No
🔍 Possible issues	Possible Bug: The refactoring introduces changes in how data is passed and processed across different components (e.g., from specific getters to a more generic approach in `MalwareResult` interface). This could potentially introduce bugs if not all instances where the old methods were used have been correctly updated to the new approach.
🔍 Possible issues	Performance Concern: The addition of new exporters and the changes in the rule processing logic could potentially impact the performance of the system. It's important to evaluate the performance implications of these changes, especially in environments with a high volume of events.
🔒 Security concerns	No

Category	Suggestions
Enhancement	Improve error handling in `InitHTTPExporter` function. ___ Consider handling the error returned by `config.Validate()` more gracefully by logging the error and returning a nil or initializing a default configuration instead of just returning the error. This can improve the robustness of your exporter initialization. [pkg/exporters/http_exporter.go [82-83]](https://github.com/kubescape/node-agent/pull/231/files#diff-6e04fd42d767812ea2855370a21a524371930b320544c0ee0954e1500242fbbdR82-R83) ```diff if err := config.Validate(); err != nil { + logger.L().Error("Failed to validate HTTPExporterConfig", helpers.Error(err)) + // Consider initializing a default config or handling the error differently here return nil, err } ```
	Add detailed logging for error scenarios in `getWorkloadIdentifiers`. ___ For the `getWorkloadIdentifiers` function, consider adding more detailed logging for each error scenario to aid in debugging and operational monitoring. This can include logging the `podNamespace` and `podName` when a workload retrieval fails. [pkg/exporters/http_exporter.go [241-243]](https://github.com/kubescape/node-agent/pull/231/files#diff-6e04fd42d767812ea2855370a21a524371930b320544c0ee0954e1500242fbbdR241-R243) ```diff wl, err := exporter.k8sClient.GetWorkload(podNamespace, "Pod", podName) if err != nil { + logger.L().Error("Failed to get workload", helpers.String("podNamespace", podNamespace), helpers.String("podName", podName), helpers.Error(err)) return "", "", "", fmt.Errorf("failed to get workload: %w", err) } ```
	Implement batch alert sending in `sendInAlertList`. ___ In the `sendInAlertList` function, consider implementing the TODO to accumulate alerts and send them in a batch. This can significantly reduce the number of HTTP requests made and improve the efficiency of the alerting mechanism. [pkg/exporters/http_exporter.go [146-148]](https://github.com/kubescape/node-agent/pull/231/files#diff-6e04fd42d767812ea2855370a21a524371930b320544c0ee0954e1500242fbbdR146-R148) ```diff -// TODO: accumulate alerts and send them in a batch -httpAlertsListSpec := HTTPAlertsListSpec{ - Alerts: []apitypes.RuntimeAlert{httpAlert}, +// Implementation to accumulate alerts and send them in a batch +exporter.accumulateAlerts(httpAlert) +if exporter.readyToSendBatch() { + httpAlertsListSpec := exporter.createBatchAlertsListSpec() + // Send batch alerts logic here } ```
	Include PID in error messages for better debugging context. ___ For error messages, consider including the PID in the message when logging errors related to getting command line or parent process details. This will make debugging easier by providing more context. [pkg/malwaremanager/v1/clamav/clamav.go [198-206]](https://github.com/kubescape/node-agent/pull/231/files#diff-946a7b5957a07b2ec4516a5a7e935c1b17de7cb6a27511e48463ed6f3d8cd159R198-R206) ```diff -logger.L().Error("Error getting command line of pid", helpers.Error(err)) -logger.L().Error("Error getting ppid of pid", helpers.Error(err)) +logger.L().Error(fmt.Sprintf("Error getting command line of pid %d", openEvent.Pid), helpers.Error(err)) +logger.L().Error(fmt.Sprintf("Error getting ppid of pid %d", openEvent.Pid), helpers.Error(err)) ```
	Use meaningful default values instead of `nil` for optional parameters. ___ Instead of using `nil` for the optional parameters in `InitHTTPExporter`, consider defining and passing meaningful default values or configurations. [pkg/exporters/http_exporter_test.go [35]](https://github.com/kubescape/node-agent/pull/231/files#diff-89585231dfa94efdc4cac61ef6426f59bf494acd49c810bb29d56e16574fd350R35-R35) ```diff -}, "", "", nil) +}, defaultAuthKey, defaultAuthValue, defaultHeaders) ```
	Improve error handling after writing the syslog message. ___ Use a more descriptive error handling strategy after attempting to write the syslog message. Logging the error or retrying the write operation could be beneficial. [pkg/exporters/syslog_exporter.go [108]](https://github.com/kubescape/node-agent/pull/231/files#diff-d84d67a80745d135f3f4af7c387c52361f506fc84f626e517ea8b37b150f8bb7R108-R108) ```diff -_, err := message.WriteTo(se.writer) +if _, err := message.WriteTo(se.writer); err != nil { + log.Printf("Failed to write syslog message: %v", err) + // Consider retrying the write operation or handling the error accordingly. +} ```
	Use a more relevant process ID for malware alerts. ___ For the `ProcessID` field in the malware alert, consider using a more relevant process identifier related to the malware event, if available, instead of the exporter's process ID. [pkg/exporters/syslog_exporter.go [121]](https://github.com/kubescape/node-agent/pull/231/files#diff-d84d67a80745d135f3f4af7c387c52361f506fc84f626e517ea8b37b150f8bb7R121-R121) ```diff -ProcessID: fmt.Sprintf("%d", os.Getpid()), // TODO: is this correct? +ProcessID: fmt.Sprintf("%d", relevantProcessID), // Replace `relevantProcessID` with the actual process ID related to the malware event. ```
Maintainability	Refactor `sendAlertLimitReached` for better readability. ___ To enhance the readability and maintainability of the `sendAlertLimitReached` function, consider breaking down the construction of `httpAlert` into smaller functions or blocks, especially the initialization of nested struct fields. [pkg/exporters/http_exporter.go [95-107]](https://github.com/kubescape/node-agent/pull/231/files#diff-6e04fd42d767812ea2855370a21a524371930b320544c0ee0954e1500242fbbdR95-R107) ```diff +baseAlert := createBaseRuntimeAlert("AlertLimitReached", ruleenginev1.RulePrioritySystemIssue, "Check logs for more information") +k8sDetails := createRuntimeAlertK8sDetails(exporter.NodeName, &exporter.ClusterName) httpAlert := apitypes.RuntimeAlert{ - Message: "Alert limit reached", - HostName: exporter.Host, - AlertType: apitypes.AlertTypeRule, // TODO: change this to a new alert type. @bez - BaseRuntimeAlert: apitypes.BaseRuntimeAlert{ - AlertName: "AlertLimitReached", - Severity: ruleenginev1.RulePrioritySystemIssue, - FixSuggestions: "Check logs for more information", - }, - RuntimeAlertK8sDetails: apitypes.RuntimeAlertK8sDetails{ - NodeName: exporter.NodeName, - ClusterName: &exporter.ClusterName, - }, + Message: "Alert limit reached", + HostName: exporter.Host, + AlertType: apitypes.AlertTypeRule, // TODO: change this to a new alert type. @bez + BaseRuntimeAlert: baseAlert, + RuntimeAlertK8sDetails: k8sDetails, } ```
	Move struct definitions into separate files for better organization. ___ For better code organization and readability, consider moving the struct definitions (`HTTPExporterConfig`, `HTTPExporter`, `HTTPAlertsListSpec`, etc.) into separate files. This can make the codebase easier to navigate and maintain. [pkg/exporters/http_exporter.go [23-57]](https://github.com/kubescape/node-agent/pull/231/files#diff-6e04fd42d767812ea2855370a21a524371930b320544c0ee0954e1500242fbbdR23-R57) ```diff -type HTTPExporterConfig struct { -... -} -type HTTPExporter struct { -... -} -type HTTPAlertsListSpec struct { -... -} +// Suggestion to move struct definitions into separate files +// Example: http_exporter_config.go, http_exporter.go, http_alerts_list_spec.go ```
	Use constants for severity levels to improve code clarity. ___ To improve maintainability, consider defining the severity levels as constants with descriptive names. This will make the code easier to understand and modify in the future. [pkg/malwaremanager/v1/clamav/clamav.go [125]](https://github.com/kubescape/node-agent/pull/231/files#diff-946a7b5957a07b2ec4516a5a7e935c1b17de7cb6a27511e48463ed6f3d8cd159R125-R125) ```diff -Severity: 10, // TODO: Get severity from api. +Severity: SEVERITY_HIGH, // Assuming SEVERITY_HIGH is a constant defined elsewhere ```
	Use constants for repeated string literals. ___ Use a constant for the repeated string literals like "testcontainerid", "testnamespace", etc., to avoid typos and make future changes easier. [pkg/exporters/http_exporter_test.go [46-50]](https://github.com/kubescape/node-agent/pull/231/files#diff-89585231dfa94efdc4cac61ef6426f59bf494acd49c810bb29d56e16574fd350R46-R50) ```diff -ContainerID: "testcontainerid", -Namespace: "testnamespace", +const ( + testContainerID = "testcontainerid" + testNamespace = "testnamespace" +) +ContainerID: testContainerID, +Namespace: testNamespace, ```
	Extract URL and summary construction into separate functions. ___ Extract the construction of `sourceUrl` and `summary` into separate functions to improve readability and maintainability of `SendRuleAlert` and `SendMalwareAlert` methods. [pkg/exporters/alert_manager.go [47-57]](https://github.com/kubescape/node-agent/pull/231/files#diff-40568094a80f334c1148d539cedecc426f810a6e32e5eeab274195f4c80693d5R47-R57) ```diff -sourceUrl := fmt.Sprintf("https://armosec.github.io/kubecop/alertviewer/?AlertMessage=%s&AlertRuleName=%s&AlertRuleID=%s&AlertFix=%s&AlertNamespace=%s&AlertPod=%s&AlertContainer=%s&AlertProcess=%s", -summary := fmt.Sprintf("Rule '%s' in '%s' namespace '%s' failed", failedRule.GetBaseRuntimeAlert().AlertName, failedRule.GetRuntimeAlertK8sDetails().PodName, failedRule.GetRuntimeAlertK8sDetails().Namespace) +sourceUrl := constructSourceUrl(failedRule) +summary := constructSummary(failedRule) ```
	Abstract the construction of syslog messages into a separate method. ___ Consider abstracting the construction of the `rfc5424.Message` into a separate method or function to improve code readability and maintainability. [pkg/exporters/syslog_exporter.go [48-106]](https://github.com/kubescape/node-agent/pull/231/files#diff-d84d67a80745d135f3f4af7c387c52361f506fc84f626e517ea8b37b150f8bb7R48-R106) ```diff -message := rfc5424.Message{ - Priority: rfc5424.Error, - Timestamp: failedRule.GetBaseRuntimeAlert().Timestamp, - ... -} +message := constructSyslogMessage(failedRule) +// Implement `constructSyslogMessage` to construct the `rfc5424.Message` from a `failedRule`. ```
Best practice	Handle potential errors from time conversion for robustness. ___ Consider handling the error from `time.Unix` to ensure robustness. While `time.Unix` is unlikely to fail with valid inputs, explicitly checking for errors can prevent potential issues in edge cases or future changes. [pkg/malwaremanager/v1/clamav/clamav.go [127]](https://github.com/kubescape/node-agent/pull/231/files#diff-946a7b5957a07b2ec4516a5a7e935c1b17de7cb6a27511e48463ed6f3d8cd159R127-R127) ```diff -Timestamp: time.Unix(int64(execEvent.Timestamp), 0), +timestamp, err := time.Unix(int64(execEvent.Timestamp), 0) +if err != nil { + logger.L().Error("Error converting timestamp", helpers.Error(err)) + return nil +} +Timestamp: timestamp, ```
	Use explicit boolean values instead of nil for clarity. ___ Instead of using `nil` directly for `IsPartOfImage` in the open event, consider a more explicit approach to indicate the absence of this information, such as using a boolean variable set to `false`. [pkg/malwaremanager/v1/clamav/clamav.go [220]](https://github.com/kubescape/node-agent/pull/231/files#diff-946a7b5957a07b2ec4516a5a7e935c1b17de7cb6a27511e48463ed6f3d8cd159R220-R220) ```diff -IsPartOfImage: nil, // We don't have that enrichement in the open event. +isPartOfImage := false +IsPartOfImage: &isPartOfImage, ```
	Check for nil before dereferencing to improve code safety. ___ To avoid potential nil pointer dereference and improve code safety, check if `execEvent.Runtime` and `execEvent.K8s` are not nil before accessing their fields. [pkg/malwaremanager/v1/clamav/clamav.go [242-247]](https://github.com/kubescape/node-agent/pull/231/files#diff-946a7b5957a07b2ec4516a5a7e935c1b17de7cb6a27511e48463ed6f3d8cd159R242-R247) ```diff -ContainerID: execEvent.Runtime.ContainerID, -HostNetwork: &execEvent.K8s.HostNetwork, +ContainerID: safeString(execEvent.Runtime.ContainerID), +HostNetwork: safeBoolPointer(execEvent.K8s.HostNetwork), ```
	Handle errors returned by `InitHTTPExporter` in tests. ___ Consider handling the error returned by `InitHTTPExporter` in tests by asserting no error occurred. This ensures that the exporter initialization is successful before proceeding with the test logic. [pkg/exporters/http_exporter_test.go [35]](https://github.com/kubescape/node-agent/pull/231/files#diff-89585231dfa94efdc4cac61ef6426f59bf494acd49c810bb29d56e16574fd350R35-R35) ```diff exporter, err := InitHTTPExporter(HTTPExporterConfig{ }, "", "", nil) +assert.NoError(t, err) ```
	Add error handling for system calls in `InitAlertManagerExporter`. ___ Implement error handling for `os.Hostname()` and `os.Getenv("NODE_NAME")` to prevent panic and allow graceful error handling in `InitAlertManagerExporter`. [pkg/exporters/alert_manager.go [34-37]](https://github.com/kubescape/node-agent/pull/231/files#diff-40568094a80f334c1148d539cedecc426f810a6e32e5eeab274195f4c80693d5R34-R37) ```diff hostName, err := os.Hostname() if err != nil { - panic(fmt.Sprintf("failed to get hostname: %v", err)) + log.Errorf("failed to get hostname: %v", err) + return nil, err } ```
Possible issue	Add nil pointer checks before dereferencing to avoid runtime panics. ___ Consider checking for nil pointers before dereferencing `failedRule.GetRuntimeProcessDetails()`, `failedRule.GetRuntimeAlertK8sDetails()`, and `failedRule.GetBaseRuntimeAlert()` to avoid potential runtime panics. [pkg/exporters/syslog_exporter.go [50-53]](https://github.com/kubescape/node-agent/pull/231/files#diff-d84d67a80745d135f3f4af7c387c52361f506fc84f626e517ea8b37b150f8bb7R50-R53) ```diff -Timestamp: failedRule.GetBaseRuntimeAlert().Timestamp, -Hostname: failedRule.GetRuntimeAlertK8sDetails().PodName, -AppName: failedRule.GetRuntimeAlertK8sDetails().ContainerName, -ProcessID: fmt.Sprintf("%d", failedRule.GetRuntimeProcessDetails().PID), +if baseRuntimeAlert := failedRule.GetBaseRuntimeAlert(); baseRuntimeAlert != nil { + Timestamp: baseRuntimeAlert.Timestamp, +} +if k8sDetails := failedRule.GetRuntimeAlertK8sDetails(); k8sDetails != nil { + Hostname: k8sDetails.PodName, + AppName: k8sDetails.ContainerName, +} +if processDetails := failedRule.GetRuntimeProcessDetails(); processDetails != nil { + ProcessID: fmt.Sprintf("%d", processDetails.PID), +} ```
Possible issue	Check for nil before dereferencing the `Size` pointer. ___ For the `Size` field in the malware alert structured data, ensure that the pointer is checked for nil before dereferencing to avoid a potential panic. [pkg/exporters/syslog_exporter.go [152]](https://github.com/kubescape/node-agent/pull/231/files#diff-d84d67a80745d135f3f4af7c387c52361f506fc84f626e517ea8b37b150f8bb7R152-R152) ```diff -Value: malwareResult.GetBasicRuntimeAlert().Size, +sizeValue := "unknown" +if sizePtr := malwareResult.GetBasicRuntimeAlert().Size; sizePtr != nil { + sizeValue = fmt.Sprintf("%d", sizePtr) +} +Value: sizeValue, ```

kubescape / node-agent

Feature/new exporters #231