Network neighborhoods, fix DNS & network tracers enrichment, container watcher bug

[amirmalka]

User description

Overview

---

Type

enhancement, bug_fix

---

Description

• Transitioned from using NetworkNeighbors to NetworkNeighborhood across various modules.
• Implemented and integrated NetworkNeighborhoodCache with full functionality including unit tests.
• Updated rule engine and object cache implementations to utilize the new NetworkNeighborhood structure.
• Removed the old NetworkNeighborsCache implementation.

---

Changes walkthrough

	Relevant files
Enhancement	8 files main.go Update Network Management and Caching Implementations        main.go Imported networkmanagerv2 and networkneighborhoodcache replacing networkneighborscache. Updated cache initialization to use NewNetworkNeighborhoodCache instead of NewNetworkNeighborsCache. Changed network manager client initialization to use networkmanagerv2. +4/-5      networkneighborhoodcache.go Implement NetworkNeighborhoodCache with Full Functionality pkg/objectcache/networkneighborhoodcache/networkneighborhoodcache.go Implemented the NetworkNeighborhoodCache with methods for managing network neighborhood data. Added watcher and handler methods for network neighborhood resources. +317/-0  networkneighborhoodcache_interface.go Define NetworkNeighborhoodCache Interface                                pkg/objectcache/networkneighborhoodcache_interface.go Defined NetworkNeighborhoodCache interface with a method for retrieving network neighborhood data. +16/-0    networkneighborscache.go Remove Old NetworkNeighborsCache Implementation                    pkg/objectcache/networkneighborscache/networkneighborscache.go Removed the old NetworkNeighborsCache implementation. +0/-253  objectcache.go Update ObjectCache to Use NetworkNeighborhoodCache              pkg/objectcache/v1/objectcache.go Updated ObjectCacheImpl to use NetworkNeighborhoodCache instead of NetworkNeighborsCache. +3/-3      helpers.go Add Helper for NetworkNeighborhood Container Data Retrieval pkg/ruleengine/v1/helpers.go Added helper function to retrieve container data from a NetworkNeighborhood object. +19/-0    r0005_unexpected_domain_request.go Update Domain Request Rule to Use NetworkNeighborhood        pkg/ruleengine/v1/r0005_unexpected_domain_request.go Updated rule processing to use NetworkNeighborhood instead of NetworkNeighbors. +9/-4      r0007_kubernetes_client_executed.go Adjust Network Event Handling for Kubernetes Client Execution Rule pkg/ruleengine/v1/r0007_kubernetes_client_executed.go Adjusted network event handling to support NetworkNeighborhood. +7/-3
Tests	2 files readfiles.go Update Mocks for NetworkNeighborhood Transition                    mocks/readfiles.go Updated test constants and mock data paths to reflect the transition from NetworkNeighbors to NetworkNeighborhood. +6/-6      networkneighborhoodcache_test.go Unit Tests for NetworkNeighborhoodCache                                    pkg/objectcache/networkneighborhoodcache/networkneighborhoodcache_test.go Comprehensive unit tests for NetworkNeighborhoodCache covering various scenarios. +787/-0

---

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

PR Description updated to latest commit (b7eb8a4)

[codiumai-pr-agent-free]

PR Review

⏱️ Estimated effort to review [1-5]	4, due to the extensive changes across multiple files, including the introduction of a new cache system and modifications to existing rule engines to adapt to the new network neighborhood structure. The PR also includes updates to tests, which need thorough review to ensure they align with the new logic.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: The PR introduces a new NetworkNeighborhoodCache and deprecates the old NetworkNeighborsCache. It is crucial to ensure that all references to the old cache are properly updated to prevent runtime errors.
	Performance Concern: The new cache system's impact on performance should be evaluated, especially in large-scale environments where network neighborhood data might be extensive.
🔒 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 by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

• When commenting, to edit configurations 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, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

[codiumai-pr-agent-free]

PR Code Suggestions

Category	Suggestions
Bug	Correct the logger component name to match the context in which it is used. The method addPod logs an error using the incorrect component name "ApplicationProfileCacheImpl" instead of "NetworkNeighborhoodCacheImpl". This can lead to confusion when debugging. Update the logger component name to reflect the correct context. pkg/objectcache/networkneighborhoodcache/networkneighborhoodcache.go [137] -logger.L().Error("ApplicationProfileCacheImpl: failed to get slug", helpers.String("namespace", podU.GetNamespace()), helpers.String("pod", podU.GetName()), helpers.Error(err)) +logger.L().Error("NetworkNeighborhoodCacheImpl: failed to get slug", helpers.String("namespace", podU.GetNamespace()), helpers.String("pod", podU.GetName()), helpers.Error(err))
Enhancement	Make error messages more specific by including the operation (add or delete) in the log. The addPod and deletePod methods use the same error message for different operations. It is recommended to specify the operation (add or delete) in the error message to provide clearer debugging information. pkg/objectcache/networkneighborhoodcache/networkneighborhoodcache.go [145] -logger.L().Error("ApplicationProfileCacheImpl: failed to unmarshal pod", helpers.String("namespace", podU.GetNamespace()), helpers.String("pod", podU.GetName()), helpers.Error(err)) +logger.L().Error("NetworkNeighborhoodCacheImpl: failed to unmarshal pod for addition", helpers.String("namespace", podU.GetNamespace()), helpers.String("pod", podU.GetName()), helpers.Error(err))
	Include the method name in the error log for better traceability. The addNetworkNeighborhood method logs an error without specifying the operation context. Include the operation context in the error message for better traceability. pkg/objectcache/networkneighborhoodcache/networkneighborhoodcache.go [228] -logger.L().Error("failed to unmarshal network neighborhood", helpers.String("name", nnName), helpers.Error(err)) +logger.L().Error("addNetworkNeighborhood: failed to unmarshal network neighborhood", helpers.String("name", nnName), helpers.Error(err))
	Add error handling for potentially empty values in network neighborhood data. Consider handling errors when generating the patch command. Currently, if nn.GetName() or nn.GetNamespace() returns an empty string, the command might be malformed. It's good practice to check for such cases and handle them appropriately. pkg/ruleengine/v1/r0005_unexpected_domain_request.go [56] -return fmt.Sprintf(baseTemplate, nn.GetName(), nn.GetNamespace(), - event.GetContainer(), event.DNSName) +nnName := nn.GetName() +nnNamespace := nn.GetNamespace() +if nnName == "" || nnNamespace == "" { + return "", fmt.Errorf("invalid network neighborhood data") +} +return fmt.Sprintf(baseTemplate, nnName, nnNamespace, event.GetContainer(), event.DNSName)
	Improve error handling by logging errors when fetching containers from network neighborhoods. Add error handling for the case where getContainerFromNetworkNeighborhood returns an error. Currently, the error is ignored, which could lead to runtime issues if the container is not found. pkg/ruleengine/v1/r0005_unexpected_domain_request.go [80-82] nnContainer, err := getContainerFromNetworkNeighborhood(nn, domainEvent.GetContainer()) if err != nil { + log.Errorf("Failed to get container from network neighborhood: %v", err) return nil }
	Improve the descriptiveness of error messages in container lookup failures. The error message in getContainerFromNetworkNeighborhood could be more descriptive by including the type of container (regular, init, or ephemeral) that was being searched. This would help in debugging issues related to container lookups. pkg/ruleengine/v1/helpers.go [65] -return v1beta1.NetworkNeighborhoodContainer{}, fmt.Errorf("container %s not found in network neighborhood profile", containerName) +return v1beta1.NetworkNeighborhoodContainer{}, fmt.Errorf("container %s not found in any of regular, init, or ephemeral containers in network neighborhood profile", containerName)
Maintainability	Refactor the condition to reduce code nesting and improve readability. The addPod method checks if the network neighborhood state is not complete and continues without caching the pod. This logic could be made clearer by inverting the condition and reducing the nesting of the code. pkg/objectcache/networkneighborhoodcache/networkneighborhoodcache.go [163-165] -if s := nn.slugToState.Get(uniqueSlug); s.mode != helpersv1.Complete { - // if NN is not complete, do not cache the pod - continue +if s := nn.slugToState.Get(uniqueSlug); s.mode == helpersv1.Complete { + // add the container to the cache + if nn.containerToSlug.Has(container) { + continue + } + nn.containerToSlug.Set(container, uniqueSlug) }
	Combine loops for different container types into a single loop to improve code maintainability. Refactor the getContainerFromNetworkNeighborhood function to reduce redundancy and improve maintainability. The loops for containers, init containers, and ephemeral containers can be combined into a single loop over a concatenated slice. pkg/ruleengine/v1/helpers.go [49-64] -for i := range nn.Spec.Containers { - if nn.Spec.Containers[i].Name == containerName { - return nn.Spec.Containers[i], nil - } -} -for i := range nn.Spec.InitContainers { - if nn.Spec.InitContainers[i].Name == containerName { - return nn.Spec.InitContainers[i], nil - } -} -for i := range nn.Spec.EphemeralContainers { - if nn.Spec.EphemeralContainers[i].Name == containerName { - return nn.Spec.EphemeralContainers[i], nil +allContainers := append(nn.Spec.Containers, nn.Spec.InitContainers...) +allContainers = append(allContainers, nn.Spec.EphemeralContainers...) +for _, container := range allContainers { + if container.Name == containerName { + return container, nil } }
Security	Enhance security by sanitizing inputs used in command construction. The generatePatchCommand function constructs a command string using direct string interpolation, which can be prone to injection attacks or errors if the inputs are not properly sanitized. Consider using a more robust method to construct the command or ensure input validation. pkg/ruleengine/v1/r0005_unexpected_domain_request.go [54-56] -baseTemplate := "kubectl patch networkneighborhood %s --namespace %s --type merge -p '{\"spec\": {\"containers\": [{\"name\": \"%s\", \"dns\": [{\"dnsName\": \"%s\"}]}]}}'" +baseTemplate := "kubectl patch networkneighborhood %s --namespace %s --type merge -p '%s'" +patchPayload := fmt.Sprintf("{\"spec\": {\"containers\": [{\"name\": \"%s\", \"dns\": [{\"dnsName\": \"%s\"}]}]}}", event.GetContainer(), event.DNSName) +safePatchPayload := escapeForShell(patchPayload) +return fmt.Sprintf(baseTemplate, nn.GetName(), nn.GetNamespace(), safePatchPayload)

---

✨ 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 every time a new PR is opened, or can be invoked manually by commenting on a PR.

• When commenting, to edit configurations 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, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

Summary:

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

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

Summary:

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

[dwertent]

I'm leaving all the logs you added for now, I believe we will clean them later :)

[amirmalka]

@dwertent yes, not ready for review.. I added lots of logs for the debugging purposes

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

✨ Artifacts are available here.

[github-actions]

Summary:

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

[github-actions]

Summary:

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

[matthyx]

much more understandable with the unique list of pods