Skip to content

chore: fix for external domain resolution (#148)#150

Closed
mayankpande88 wants to merge 3 commits into
testfrom
main
Closed

chore: fix for external domain resolution (#148)#150
mayankpande88 wants to merge 3 commits into
testfrom
main

Conversation

@mayankpande88
Copy link
Copy Markdown
Contributor

  • fix: fix for external domain resolution

  • chore: fix for test

  • chore: added logger for domain resolution

  • chore: removed arm build

  • chore: fix for domain discovery

  • chore: fix for test

  • fix: fix for test

* fix: fix for external domain resolution

* chore: fix for test

* chore: added logger for domain resolution

* chore: removed arm build

* chore: fix for domain discovery

* chore: fix for test

* fix: fix for test
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @mayankpande88, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the handling of external domain resolution and L7 request processing. The primary goal is to simplify the Domain object's SpecifyIP logic and to centralize destination workload information within the DestinationKey itself. This change streamlines L7 processing by ensuring that all necessary workload details are available directly from the key, reducing the need for external lookups or redundant parameter passing.

Highlights

  • Domain Object Simplification: The NewDomain constructor in common/net.go has been simplified by removing the complex logic that determined the SpecifyIP field based on the presence of private IPs. Now, SpecifyIP is always initialized to true when a new domain is created.
  • Enriched External Destination Keys: The NewDestinationKey function in common/net.go now populates actualDestination, destinationWorkload, and actualDestinationWorkload fields for external domains where SpecifyIP is false. This provides more comprehensive context directly within the DestinationKey for external traffic.
  • L7 Request Processing Refactor: The onL7Request function in containers/container.go no longer accepts an ip2fqdn map and has removed the logic that attempted to derive external hostnames from HTTP requests. Instead, it now relies on the DestinationKey to already contain the necessary workload information.
  • L7 Metrics Simplification: The L7Stats.get function in containers/l7.go has been refactored to remove explicit dstWorkload and actualDstWorkload parameters. It now directly retrieves these workload details from the common.DestinationKey object, streamlining the metric label generation.
  • Test Updates: Corresponding tests in common/net_test.go have been updated to reflect the new behavior of NewDomain and NewDestinationKey, ensuring correctness after the changes.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the logic for handling external domain resolution. The key change is to always use the resolved IP address for a domain, simplifying the logic by removing the distinction for domains with multiple public IPs. The changes also include refactoring how L7 metrics are created by simplifying function signatures and removing redundant parameters.

Comment thread common/net.go
Comment on lines 219 to 221
func NewDomain(fqdn string, ips []netaddr.IP) *Domain {
d := &Domain{FQDN: fqdn, SpecifyIP: true}
if len(ips) > 1 {
containsPrivateIPs := false
for _, ip := range ips {
if !IsIpExternal(ip) {
containsPrivateIPs = true
break
}
}
if !containsPrivateIPs {
d.SpecifyIP = false
}
}
return d
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The ips parameter is no longer used within the NewDomain function. To improve code clarity and prevent potential misuse, it should be removed from the function signature.

func NewDomain(fqdn string) *Domain {
	return &Domain{FQDN: fqdn, SpecifyIP: true}
}

Comment thread common/net_test.go
Comment on lines +54 to 57
assert.Equal(t, "Domain(fqdn,true)", NewDomain("fqdn", []netaddr.IP{
netaddr.MustParseIP("1.1.1.1"),
netaddr.MustParseIP("1.1.1.2"),
}).String())
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Following the change to NewDomain to remove the ips parameter, this test case should be updated to call the function with its new signature.

	assert.Equal(t, "Domain(fqdn,true)", NewDomain("fqdn").String())

Comment thread common/net.go
Comment on lines +240 to 242
} else if IsIpExternal(actualDst.IP()) && domain != nil && domain.SpecifyIP {
klog.Infof("ip %q is external, but domain %q specifies IP, using IP as destination", actualDst.IP(), domain.FQDN)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This else if block only contains a logging statement and doesn't alter the control flow, which can be confusing. Consider removing the else if and placing the logging statement before the final return statement.

if IsIpExternal(actualDst.IP()) && domain != nil && !domain.SpecifyIP {
		return DestinationKey{
			destination:       HostPortWithEmptyIP(domain.FQDN, dst.Port()),
			actualDestination: HostPortFromIPPort(actualDst),
			destinationWorkload: Workload{
				Kind:      "external",
				Name:      domain.FQDN,
				Namespace: "external",
				},
			actualDestinationWorkload: Workload{
				Kind:      "external",
				Name:      actualDst.IP().String(),
				Namespace: "external",
				},
			}
		}
		if IsIpExternal(actualDst.IP()) && domain != nil && domain.SpecifyIP {
			klog.Infof("ip %q is external, but domain %q specifies IP, using IP as destination", actualDst.IP(), domain.FQDN)
		}
		return DestinationKey{
			destination:               HostPortFromIPPort(dst),
			actualDestination:         HostPortFromIPPort(actualDst),
			destinationWorkload:       dstWorkload,
			actualDestinationWorkload: actualDestWorkload,
		}

Comment thread containers/l7.go
Comment on lines +56 to +58
"destination_workload_kind": key.GetDestinationWorkload().Kind,
"destination_workload_name": key.GetDestinationWorkload().Name,
"destination_workload_namespace": key.GetDestinationWorkload().Namespace,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The destination workload name is being retrieved using key.GetDestinationWorkload().Name which is not consistent with the other fields. It should be retrieved directly from the key.

Suggested change
"destination_workload_kind": key.GetDestinationWorkload().Kind,
"destination_workload_name": key.GetDestinationWorkload().Name,
"destination_workload_namespace": key.GetDestinationWorkload().Namespace,
"destination_workload_name": key.GetDestinationWorkload().Name,

Comment thread containers/l7.go
Comment on lines +62 to +64
"actual_destination_workload_kind": key.GetActualDestinationWorkload().Kind,
"actual_destination_workload_name": key.GetActualDestinationWorkload().Name,
"actual_destination_workload_namespace": key.GetActualDestinationWorkload().Namespace,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The actual destination workload name is being retrieved using key.GetActualDestinationWorkload().Name which is not consistent with the other fields. It should be retrieved directly from the key.

Suggested change
"actual_destination_workload_kind": key.GetActualDestinationWorkload().Kind,
"actual_destination_workload_name": key.GetActualDestinationWorkload().Name,
"actual_destination_workload_namespace": key.GetActualDestinationWorkload().Namespace,
"actual_destination_workload_name": key.GetActualDestinationWorkload().Name,

dependabot Bot added 2 commits September 4, 2025 14:25
Bumps [github.com/ulikunitz/xz](https://github.com/ulikunitz/xz) from 0.5.12 to 0.5.14.
- [Commits](ulikunitz/xz@v0.5.12...v0.5.14)

---
updated-dependencies:
- dependency-name: github.com/ulikunitz/xz
  dependency-version: 0.5.14
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github.com/docker/docker](https://github.com/docker/docker) from 27.4.0+incompatible to 28.0.0+incompatible.
- [Release notes](https://github.com/docker/docker/releases)
- [Commits](moby/moby@v27.4.0...v28.0.0)

---
updated-dependencies:
- dependency-name: github.com/docker/docker
  dependency-version: 28.0.0+incompatible
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant