Feat: Add Workload DnsController #1438

Flying-Tom · 2025-07-10T08:48:18Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Supprort Dns resolve for workload generated by ServiceEntry in Dual-Engine mode.

Which issue(s) this PR fixes:
Fixes #1407

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

gemini-code-assist

Summary of Changes

Hello @Flying-Tom, I'm Gemini Code Assist¹! 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 introduces a new DNS resolution capability for workloads, primarily targeting ServiceEntry configurations in a Dual-Engine environment. The core change involves adding a dedicated DnsController that asynchronously resolves hostnames to IP addresses, ensuring that workloads can be correctly processed even if their addresses are not immediately available upon initial discovery. This enhances the system's adaptability to dynamic service discovery scenarios.

Highlights

New DNS Controller for Workloads: I've introduced a new DnsController within the pkg/controller/workload package. This controller is specifically designed to handle DNS resolution for workloads, particularly those originating from ServiceEntry configurations in Dual-Engine mode.
Integration with Workload Processing Pipeline: The new DnsController is now integrated into the WorkloadController and Processor. Workloads received from ServiceEntry that initially lack resolved IP addresses are now channeled to this DnsController for asynchronous DNS lookup. The Processor includes a retry mechanism to ensure these workloads are fully processed once their addresses are resolved.
Dynamic Address Resolution for ServiceEntry Workloads: The WorkloadProcessor has been enhanced to identify ServiceEntry workloads that arrive without pre-resolved IP addresses. It now queues these for DNS resolution via the new DnsResolverChan and includes a robust retry loop to wait for and then apply the resolved addresses, ensuring proper handling of dynamically discovered services.

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.

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

gemini-code-assist

Code Review

This pull request introduces a DnsController to support DNS resolution for ServiceEntry in Dual-Engine mode. There are several critical issues that could lead to panics or incorrect behavior. I've provided detailed comments and suggestions to address these points.

pkg/controller/workload/workload_processor.go

pkg/controller/workload/dns.go

pkg/controller/workload/workload_controller.go

pkg/controller/workload/dns.go

hzxuzhonghu · 2025-07-17T08:46:09Z

/assign @Kuromesi

kmesh-bot · 2025-07-17T08:46:12Z

@hzxuzhonghu: GitHub didn't allow me to assign the following users: Kuromesi.

Note that only kmesh-net members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @Kuromesi

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/controller/workload/workload_processor.go

Copilot

Pull Request Overview

This PR adds DNS resolution capability for workloads generated by ServiceEntry in Dual-Engine mode, enabling proper address resolution for service entries that initially have nil addresses.

Implements a new DNS controller for workload address resolution
Adds retry logic for ServiceEntry workloads with unresolved addresses
Integrates DNS resolution workflow into the existing workload processing pipeline

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File	Description
pkg/controller/workload/workload_processor.go	Adds DNS resolver channel, handles ServiceEntry workloads with retry logic, and includes various bug fixes
pkg/controller/workload/workload_controller.go	Integrates DNS controller into workload controller lifecycle
pkg/controller/workload/dns.go	New DNS controller implementation for workload address resolution
pkg/controller/controller.go	Starts DNS controller in main controller flow

pkg/controller/workload/workload_processor.go

pkg/controller/workload/dns.go

pkg/controller/workload/workload_controller.go

pkg/controller/controller.go

pkg/controller/workload/workload_controller.go

pkg/controller/workload/workload_processor.go

Flying-Tom · 2025-08-07T11:52:45Z

The improvements mentioned in the review will be fixed.

hzxuzhonghu · 2025-08-12T10:11:24Z

@Flying-Tom any update

hzxuzhonghu · 2025-08-12T11:13:32Z

pkg/controller/workload/dns.go

+		if _, ok := r.pendingHostnames[workloadName]; !ok {
+			r.pendingHostnames[workloadName] = []string{}
+		}
+		r.pendingHostnames[workloadName] = append(r.pendingHostnames[workloadName], hostname)


not sure i understand, does a workload has multi hostnames?

hzxuzhonghu · 2025-08-12T11:18:23Z

pkg/controller/workload/dns.go

+		if _, ok := r.pendingHostnames[workloadName]; !ok {
+			r.pendingHostnames[workloadName] = []string{}
+		}
+		r.pendingHostnames[workloadName] = append(r.pendingHostnames[workloadName], hostname)


another problem if this function is called multiple times with same workloads, this will store duplicate hostname

hzxuzhonghu · 2025-08-12T11:18:59Z

pkg/controller/workload/dns.go

+			}
+		}
+		r.workloadCache[hostname].Workloads = append(
+			r.workloadCache[hostname].Workloads, workload,


same here, if processDomains called multitimes with same workloads, duplicate workload will be here

hzxuzhonghu · 2025-08-12T11:23:30Z

Would like to see the ut

Copilot

Pull Request Overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

pkg/controller/workload/workload_processor.go

pkg/controller/workload/dns.go

Copilot

Pull Request Overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-15T07:40:27Z

pkg/controller/workload/dns.go

+		r.Lock()
+		ch, ok := r.ResolvedDomainChanMap[uid]
+		r.Unlock()
+
+		if ok {
+			r.cache.AddOrUpdateWorkload(newWorkload)
+			select {
+			case ch <- newWorkload:
+				log.Infof("workload %s/%s addresses resolved: %v", newWorkload.Namespace, newWorkload.Name, newWorkload.Addresses)
+			case <-time.After(WorkloadChannelSendTimeout):
+				log.Warnf("timeout sending resolved workload %s/%s", newWorkload.Namespace, newWorkload.Name)
+			}
+
+			r.Lock()
+			if _, stillExists := r.ResolvedDomainChanMap[uid]; stillExists {
+				close(r.ResolvedDomainChanMap[uid])
+				delete(r.ResolvedDomainChanMap, uid)
+			}
+			r.Unlock()


There's a potential race condition where the channel could be closed between the first unlock (line 180) and the second lock (line 191). Consider holding the lock across the entire operation or using a different synchronization approach.

Copilot

Pull Request Overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-15T15:20:18Z

pkg/controller/workload/workload_processor.go

 	if service.Waypoint != nil && service.GetWaypoint().GetAddress() != nil && len(service.Addresses) != 0 {
 		// Currently istiod only set the waypoint address to the first address of the service
-		// When waypoints of different granularities are deployed together, the only waypoint service to be determined
+		// when waypoints of different granularities are deployed together, the only waypoint service to be determined


The comment has inconsistent capitalization. It should start with a capital letter: 'When waypoints of different granularities are deployed together, the only waypoint service to be determined'.

Suggested change

// when waypoints of different granularities are deployed together, the only waypoint service to be determined

// When waypoints of different granularities are deployed together, the only waypoint service to be determined

Copilot · 2025-10-15T15:20:19Z

pkg/controller/workload/workload_processor.go

 func (p *Processor) handleAuthorizationTypeResponse(rsp *service_discovery_v3.DeltaDiscoveryResponse, rbac *auth.Rbac) error {
 	if rbac == nil {
-		return fmt.Errorf("Rbac module uninitialized")
+		return fmt.Errorf("rbac module uninitialized")


Error message capitalization is inconsistent with Go conventions. Error messages should start with lowercase unless they begin with a proper noun.

Signed-off-by: Tom <yusencao@outlook.com>

LiZhenCheng9527 · 2025-10-16T11:51:50Z

/lgtm

LiZhenCheng9527 · 2025-10-21T06:09:21Z

/approve

kmesh-bot · 2025-10-21T06:09:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LiZhenCheng9527

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [LiZhenCheng9527]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot AI review requested due to automatic review settings July 10, 2025 08:48

gemini-code-assist bot reviewed Jul 10, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

gemini-code-assist bot reviewed Jul 10, 2025

View reviewed changes

Flying-Tom force-pushed the dual-engine-dns branch from 884c030 to e03a9a5 Compare July 10, 2025 09:38

kmesh-bot added the size/L label Jul 10, 2025

Flying-Tom force-pushed the dual-engine-dns branch 3 times, most recently from c7ce028 to 94ec7fa Compare July 14, 2025 02:38

LiZhenCheng9527 reviewed Jul 15, 2025

View reviewed changes

pkg/controller/workload/dns.go Outdated Show resolved Hide resolved

Kuromesi reviewed Jul 17, 2025

View reviewed changes

pkg/controller/workload/workload_processor.go Outdated Show resolved Hide resolved

Kuromesi reviewed Jul 25, 2025

View reviewed changes

pkg/controller/workload/workload_processor.go Outdated Show resolved Hide resolved

Kuromesi reviewed Jul 25, 2025

View reviewed changes

pkg/controller/workload/workload_processor.go Outdated Show resolved Hide resolved

hzxuzhonghu requested a review from Copilot August 7, 2025 07:41

Copilot AI reviewed Aug 7, 2025

View reviewed changes

hzxuzhonghu reviewed Aug 7, 2025

View reviewed changes

kmesh-bot added the do-not-merge/contains-merge-commits label Aug 12, 2025

hzxuzhonghu reviewed Aug 12, 2025

View reviewed changes

Flying-Tom force-pushed the dual-engine-dns branch from 7966e69 to 8463f14 Compare August 12, 2025 11:41

kmesh-bot removed the do-not-merge/contains-merge-commits label Aug 12, 2025

Flying-Tom force-pushed the dual-engine-dns branch 2 times, most recently from 26ca612 to 415ea33 Compare August 12, 2025 11:49

hzxuzhonghu requested a review from LiZhenCheng9527 August 14, 2025 06:26

Copilot AI reviewed Oct 15, 2025

View reviewed changes

pkg/controller/workload/workload_processor.go Show resolved Hide resolved

pkg/controller/workload/dns.go Show resolved Hide resolved

pkg/controller/workload/dns.go Show resolved Hide resolved

Copilot AI review requested due to automatic review settings October 15, 2025 07:39

Copilot AI reviewed Oct 15, 2025

View reviewed changes

Flying-Tom force-pushed the dual-engine-dns branch from 5a81691 to b9ae304 Compare October 15, 2025 14:44

kmesh-bot added the do-not-merge/contains-merge-commits label Oct 15, 2025

Copilot AI review requested due to automatic review settings October 15, 2025 15:19

Flying-Tom force-pushed the dual-engine-dns branch from 2419f2a to b9ae304 Compare October 15, 2025 15:19

kmesh-bot removed the do-not-merge/contains-merge-commits label Oct 15, 2025

Copilot AI reviewed Oct 15, 2025

View reviewed changes

Flying-Tom added 14 commits October 15, 2025 23:47

Add Workload DnsController

ed365be

Signed-off-by: Tom <yusencao@outlook.com>

update DNS refresh rate

2ae761b

Signed-off-by: Tom <yusencao@outlook.com>

fix golangci-lint

2108268

Signed-off-by: Tom <yusencao@outlook.com>

update workload dnscontroller

9d82997

Signed-off-by: Tom <yusencao@outlook.com>

fix up with reviews

e399e22

Signed-off-by: Tom <yusencao@outlook.com>

fix panic & use chan to callback

f18663a

Signed-off-by: Tom <yusencao@outlook.com>

fix merge conflict

82a4509

Signed-off-by: Tom <yusencao@outlook.com>

fix e2e test

aa706b5

Signed-off-by: Tom <yusencao@outlook.com>

add UT for DNS workload processing and resolution

44af063

Signed-off-by: Tom <yusencao@outlook.com>

add e2e test

a3bcb6a

Signed-off-by: Tom <yusencao@outlook.com>

fix with reviews

1324712

Signed-off-by: Tom <yusencao@outlook.com>

refactor: enhance DNS workload management and cleanup on deletion

9fce1d6

Signed-off-by: Tom <yusencao@outlook.com>

refactor: improve DNS resolution handling and timeout management

a504a7c

Signed-off-by: Tom <yusencao@outlook.com>

refactor: update client and controller constructors to return errors

b74f106

Signed-off-by: Tom <yusencao@outlook.com>

Flying-Tom force-pushed the dual-engine-dns branch from b9ae304 to b74f106 Compare October 15, 2025 15:47

kmesh-bot assigned LiZhenCheng9527 Oct 16, 2025

kmesh-bot added the lgtm label Oct 16, 2025

kmesh-bot added the approved label Oct 21, 2025

	// when waypoints of different granularities are deployed together, the only waypoint service to be determined
	// When waypoints of different granularities are deployed together, the only waypoint service to be determined

Feat: Add Workload DnsController #1438

Are you sure you want to change the base?

Feat: Add Workload DnsController #1438

Conversation

Flying-Tom commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

This comment was marked as outdated.

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hzxuzhonghu commented Jul 17, 2025

Uh oh!

kmesh-bot commented Jul 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Flying-Tom commented Aug 7, 2025

Uh oh!

hzxuzhonghu commented Aug 12, 2025

Uh oh!

hzxuzhonghu Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu commented Aug 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 15, 2025

Flying-Tom commented Jul 10, 2025 •

edited

Loading