don't rely on ebpf event for dns resolution #153

matthyx · 2023-12-01T13:57:11Z

type:

bug_fix, refactoring

description:

This PR primarily focuses on refactoring the DNS resolution process and enhancing configuration handling. The main changes include:

DNS resolution is no longer reliant on ebpf events. Instead, it uses the standard net.LookupIP function to resolve DNS names.
The CreateDNSManager function no longer requires context, config, k8sClient, storageClient, and clusterName parameters. It's now a parameterless function.
The CreateRelevancyManagerMock function now returns a relevancyManager object.
The EnableNetworkTracing configuration option has been added to the TestLoadConfig function in the config_test.go file.
The container collection options have been updated to use containercollection.WithContainerFanotifyEbpf instead of containercollection.WithRuncFanotify.
The dnsEventCallback function in container_watcher_private.go now also accepts events of type DEBUG.
The ApiVersionMetadataKey label has been removed from the generateNetworkNeighborsLabels function in network_neighbors.go and the corresponding tests.
The config.json file has been updated with fullPathTracingEnabled and networkServiceEnabled options.

main_files_walkthrough:

files:

main.go: The CreateRelevancyManagerMock function now returns a relevancyManager object. The CreateDNSManager function has been updated to not require any parameters.
pkg/config/config_test.go: The EnableNetworkTracing configuration option has been added to the TestLoadConfig function.
pkg/containerwatcher/v1/container_watcher_private.go: The container collection options have been updated to use containercollection.WithContainerFanotifyEbpf instead of containercollection.WithRuncFanotify.
pkg/dnsmanager/dns_manager.go: The CreateDNSManager function no longer requires context, config, k8sClient, storageClient, and clusterName parameters. It's now a parameterless function. The DNS resolution process has been updated to use the standard net.LookupIP function.
pkg/networkmanager/network_neighbors.go: The ApiVersionMetadataKey label has been removed from the generateNetworkNeighborsLabels function.
configuration/config.json: The config.json file has been updated with fullPathTracingEnabled and networkServiceEnabled options.

codiumai-pr-agent-free · 2023-12-01T13:58:33Z

PR Analysis

🎯 Main theme: Refactoring DNS resolution and configuration handling
📝 PR summary: This PR refactors the DNS resolution process to no longer rely on ebpf events and instead use the standard net.LookupIP function. It also enhances configuration handling, including the addition of new configuration options and the simplification of function parameters. Additionally, it updates container collection options and modifies some test cases.
📌 Type of PR: Bug fix
Refactoring
🧪 Relevant tests added: Yes
⏱️ Estimated effort to review [1-5]: 4, because the PR involves changes in multiple files and includes both refactoring and bug fixes. It also modifies the DNS resolution process, which is a critical part of the system.
🔒 Security concerns: No security concerns found

PR Feedback

💡 General suggestions: The PR seems to be well-structured and addresses important aspects of the system. The changes in the DNS resolution process could potentially improve the system's performance and reliability. However, it would be beneficial to ensure that the changes do not introduce any unexpected side effects, especially since DNS resolution is a critical part of the system. It would also be helpful to include more detailed comments in the code to explain the reasoning behind certain changes, especially for those that might not be immediately obvious to other developers.
🤖 Code feedback:
- relevant file: main.go
  suggestion: It would be beneficial to add error handling for the CreateDNSManager function. Currently, if the function fails, the error will not be caught and the program might behave unexpectedly. [important]
  relevant line: dnsManager := dnsmanager.CreateDNSManager()
- relevant file: pkg/dnsmanager/dns_manager.go
  suggestion: Consider handling the error returned by the net.LookupIP function. Ignoring this error could lead to unexpected behavior if the DNS name cannot be resolved. [important]
  relevant line: addresses, err := net.LookupIP(dnsEvent.DNSName)
- relevant file: pkg/dnsmanager/dns_manager_test.go
  suggestion: It would be beneficial to add assertions to check the error returned by the ResolveIPAddress function in your tests. This will help ensure that the function behaves as expected when it encounters an error. [medium]
  relevant line: ipAddr: "67.225.146.248",
- relevant file: pkg/networkmanager/network_neighbors.go
  suggestion: Consider adding a comment to explain why the ApiVersionMetadataKey label was removed. This will help other developers understand the reasoning behind this change. [medium]
  relevant line: -instanceidhandlerV1.ApiVersionMetadataKey: workload.GetApiVersion(),

How to use

Instructions

To invoke the PR-Agent, add a comment using one of the following commands:
/review: Request a review of your Pull Request.
/describe: Update the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
/ask <QUESTION>: Ask a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.
/add_docs: Generate docstring for new components introduced in the PR.
/generate_labels: Generate labels for the PR based on the PR's contents.
see the tools guide for more details.

To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

github-actions · 2023-12-01T14:05:36Z

Summary:

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

Daniel-GrunbergerCA · 2023-12-03T06:34:15Z

pkg/dnsmanager/dns_manager.go

-		dm.addressToDomainMap.Set(address, dnsEvent.DNSName)
+	if dnsEvent.NumAnswers > 0 {
+		// FIXME use dnsEvent.Nameserver to resolve the DNS name
+		addresses, err := net.LookupIP(dnsEvent.DNSName)


we want to specifically use the ig dns resolution...

I know but in my cluster we also don't get DNS addresses with the gadget, so Maurizio said it's also the compressed vs uncompressed issues for which Amit opened an issue. I don't think we can rely on IG for that

why did we update the ig version?

For a fix on arm processors... But are you sure it works with the older IG?

the uncompressed issue doesn't bother us since it's only inside the cluster

yes I am sure. I will try to figure out what happened on the last version

We should still have a workaround when there's no address

when there is no address the dns field will be empty

Daniel-GrunbergerCA · 2023-12-03T06:35:01Z

pkg/networkmanager/network_neighbors.go

@@ -54,7 +54,7 @@ func generateNetworkNeighborsNameFromWlid(parentWlid string) string {

 func generateNetworkNeighborsLabels(workload k8sinterface.IWorkload) map[string]string {
 	return map[string]string{
-		instanceidhandlerV1.ApiVersionMetadataKey:      workload.GetApiVersion(),
+		//instanceidhandlerV1.ApiVersionMetadataKey:      workload.GetApiVersion(), FIXME invalid label, move to annotations


is this a fix for all our crds?

This should be a discussion between David and Eran

github-actions · 2023-12-04T17:07:52Z

Summary:

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

Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

matthyx · 2023-12-05T09:53:10Z

@Daniel-GrunbergerCA I think this is ready to review, you can try the image quay.io/matthiasb_1/node-agent:fanotify

github-actions · 2023-12-05T09:56:36Z

Summary:

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

matthyx requested a review from Daniel-GrunbergerCA December 1, 2023 13:57

codiumai-pr-agent-free bot added Refactoring bug_fix labels Dec 1, 2023

Daniel-GrunbergerCA suggested changes Dec 3, 2023

View reviewed changes

matthyx force-pushed the fixdns branch from f653e02 to 80b843b Compare December 4, 2023 16:59

fix dns resolution

f3a2a37

Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

matthyx force-pushed the fixdns branch from 80b843b to f3a2a37 Compare December 5, 2023 09:48

Daniel-GrunbergerCA approved these changes Dec 5, 2023

View reviewed changes

matthyx added the release Create release label Dec 5, 2023

matthyx merged commit f645867 into main Dec 5, 2023
6 checks passed

matthyx deleted the fixdns branch December 5, 2023 10:47

alban mentioned this pull request Nov 26, 2024

Remove runcfanotify package inspektor-gadget/inspektor-gadget#3699

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't rely on ebpf event for dns resolution #153

don't rely on ebpf event for dns resolution #153

matthyx commented Dec 1, 2023 •

edited by codiumai-pr-agent-free bot

Loading

codiumai-pr-agent-free bot commented Dec 1, 2023

github-actions bot commented Dec 1, 2023

Daniel-GrunbergerCA Dec 3, 2023

matthyx Dec 3, 2023

Daniel-GrunbergerCA Dec 3, 2023

matthyx Dec 3, 2023

Daniel-GrunbergerCA Dec 3, 2023 •

edited

Loading

Daniel-GrunbergerCA Dec 3, 2023

matthyx Dec 3, 2023

Daniel-GrunbergerCA Dec 3, 2023

Daniel-GrunbergerCA Dec 3, 2023

matthyx Dec 3, 2023

github-actions bot commented Dec 4, 2023

matthyx commented Dec 5, 2023

github-actions bot commented Dec 5, 2023

don't rely on ebpf event for dns resolution #153

don't rely on ebpf event for dns resolution #153

Conversation

matthyx commented Dec 1, 2023 • edited by codiumai-pr-agent-free bot Loading

type:

description:

main_files_walkthrough:

codiumai-pr-agent-free bot commented Dec 1, 2023

PR Analysis

PR Feedback

How to use

github-actions bot commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel-GrunbergerCA Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 4, 2023

matthyx commented Dec 5, 2023

github-actions bot commented Dec 5, 2023

matthyx commented Dec 1, 2023 •

edited by codiumai-pr-agent-free bot

Loading

Daniel-GrunbergerCA Dec 3, 2023 •

edited

Loading