polish and refine the content

Signed-off-by: Tom <[email protected]>

Author: Flying-Tom

docs/proposal/dual_engine_dns.md

@@ -25,18 +25,18 @@ documentation such as release notes or a development roadmap.
 A good summary is probably at least a paragraph in length.
 -->
 
+This proposal adds DNS resolution support for Dual-Engine mode workloads, enabling seamless Istio migration by supporting ServiceEntry resources with DNS-based endpoints. Workloads dynamically resolve hostnames to IP addresses without static configuration.
+
 ### Motivation
 
 <!--
 This section is for explicitly listing the motivation, goals, and non-goals of
 this KEP.  Describe why the change is important and the benefits to users.
 -->
 
-In istio, [External Name service](https://kubernetes.io/docs/concepts/services-networking/service/#externalname) and DNS resolution typed [ServiceEntry](https://istio.io/latest/docs/reference/config/networking/service-entry/#ServiceEntry-Resolution) are widely used. For both kind of configs, istiod will generate associated DNS typed clusters.
-
-So many people have depend on this kind services, Kmesh have to support it to make people migrate to it seamlessly.
+In Istio, [ExternalName services](https://kubernetes.io/docs/concepts/services-networking/service/#externalname) and DNS-typed [ServiceEntry](https://istio.io/latest/docs/reference/config/networking/service-entry/#ServiceEntry-Resolution) rely on client-side DNS resolution. When istiod processes these configurations, it generates workloads without pre-resolved IP addresses.
 
-Suppose we create a ServiceEntry like below:
+Example ServiceEntry:
 
 ```yaml
 apiVersion: networking.istio.io/v1
@@ -49,26 +49,26 @@ spec:
   - news.google.com
   ports:
   - number: 80
-    number: http
+    name: http
     protocol: HTTP
   resolution: DNS
 ```
 
-In the previous DNS processing, the CDS refresh process was included, resulting in a high degree of coupling between the DNS module and the kernel-native mode, making it impossible to reuse in dual-engine mode. In Release 1.1, Kmesh refactored the DNS module. Now, the data circulating in the DNS refresh queue is the domain name, rather than a structure containing CDS. Therefore, the DNS module no longer concerns itself with the mode of Kmesh and only provides the hostnames that need to be resolved. Specifically, the DNS logic of the Ads controller was extracted into a separate component called the Ads Dns Controller.
+In Release 1.1, Kmesh refactored the DNS module, extracting DNS logic from the kernel-native Ads controller into a standalone `AdsDnsController`. This decoupling enables reuse in Dual-Engine mode.
 
 ![](./pics/dns-evolution.png)
 
-We want to leverage this capability to build the DNS logic under Dual-Engine mode.
-
 #### Goals
 
 <!--
 List the specific goals of the KEP. What is it trying to achieve? How will we
 know that this has succeeded?
 -->
 
-- Support dns resolution of domain from workload generated by `ServiceEntry` in Dual-engine mode.
-- Improve the existing DNS logic and further abstract a more general paradigm.
+- Support DNS resolution for workloads generated from ServiceEntry resources in Dual-Engine mode
+- Implement asynchronous DNS resolution to avoid blocking workload processing
+- Provide automatic cleanup when workloads with DNS hostnames are removed
+- Support both IPv4 and IPv6 address resolution
 
 #### Non-Goals
 
@@ -90,7 +90,9 @@ The "Design Details" section below is for the real
 nitty-gritty.
 -->
 
-Thanks to the `DNSResolver` in `pkg/dns` which extracted independent DNS resolution logic, we can reuse the DNS capabilities implemented in Dual-engine mode. To be more specific, `dnsController` was implemented (`pkg/controller/ads/dns.go`) and registered as a component of Ads Controller, which receives domains need to be resolved from the Processor (another component of Ads Controller) by a channel `DnsResolverChan`, so the `dnsController` can handle the DNS resolution asynchronously and save the resolved results (address) in a shared cache `AdsCache` to send them back to the Processor, along with flushing them to the BPF map.
+Thanks to the `DNSResolver` in `pkg/dns` which extracted independent DNS resolution logic, we can reuse the DNS capabilities in Dual-engine mode. Inspired by the `AdsDnsController` in kernel-native mode, we implement a similar `WorkloadDnsController` to handle DNS resolution for workloads generated by `ServiceEntry` without address information.
+
+The controller receives workloads needing DNS resolution from the Processor via a channel, handles the DNS resolution asynchronously, and sends the resolved results back to the Processor through per-workload result channels. This design ensures that DNS resolution does not block the workload processing pipeline.
 
 ### Design Details
 
@@ -101,103 +103,102 @@ required) or even code snippets. If there's any ambiguity about HOW your
 proposal will be implemented, this is the place to discuss them.
 -->
 
-Inspired by the AdsDnsController in the Kernel-Native paradigm, we can implement a similar WorkloadDnsController in the Dual-Engine paradigm to handle the DNS resolution for the workloads generated by `ServiceEntry` and without address information. Some key structures and methods are shown below:
-
-```go
-type dnsController struct {
-  workloadsChan chan []*workloadapi.Workload
-  cache         cache.WorkloadCache
-  dnsResolver   *dns.DNSResolver
-  // store the copy of pendingResolveWorkload.
-  // key is the domain name, value is the pendingResolveDomain which contains workloads and refresh rate
-  workloadCache map[string]*pendingResolveDomain
-  // store all pending hostnames in the workloads
-  // key is the workload name, value is the list of related hostnames
-  pendingHostnames map[string][]string
-  sync.RWMutex
-}
-
-// pending resolve domain info of Dual-Engine Mode,
-// workload is used for create the apiworkload
-type pendingResolveDomain struct {
-  Workloads   []*workloadapi.Workload
-  RefreshRate time.Duration
-}
-
-func (r *dnsController) Run(stopCh <-chan struct{}) {
-  go r.dnsResolver.StartDnsResolver(stopCh)
-  go r.refreshWorker(stopCh)
-  go r.processWorkloads()
-  go func() {
-    <-stopCh
-    close(r.workloadsChan)
-  }()
-}
-
-func (r *dnsController) refreshWorker(stop <-chan struct{}) {
-  for {
-    select {
-    case <-stop:
-      return
-    case domain := <-r.dnsResolver.DnsChan:
-      // receive domain need to be resolved and handle it
-      pendingDomain := r.getWorkloadsByDomain(domain)
-      addrs := r.dnsResolver.GetDNSAddresses(domain)
-      // update the cache with resolved addresses
-      r.updateWorkloads(pendingDomain, domain, addrs)
-    }
-  }
-}
-
-func (r *dnsController) processWorkloads() {
-  for workloads := range r.workloadsChan {
-    r.processDomains(workloads)
-  }
-}
-
-// handle the workloads received from the Processor that need 
-// DNS resolution, send them to the dnsResolver.
-func (r *dnsController) processDomains(workload []*workloadapi.Workload) {
-  ...
-}
-```
+#### Architecture Overview
 
-And the Processor need to be updated handle the workloads without address information
-
-```go
-func (p *Processor) handleServicesAndWorkloads(services []*workloadapi.Service, workloads []*workloadapi.Workload) {
-  ...
-
-  for _, workload := range workloads {
-    if workload.GetAddresses() == nil {
-      uid := workload.GetUid()
-      if !strings.Contains(uid, "ServiceEntry") {
-        log.Warnf("workload: %s/%s addresses is nil", workload.Namespace, workload.Name)
-        continue
-      } else {
-        // workload from service entry need address resolving
-        if p.DnsResolverChan != nil {
-          p.DnsResolverChan <- workloads
-        }
-        // send the workload to dnsController for DNS resolution
-        ...
-        // get resolved addresses from dnsController
-        }()
-        // wait for the service entry to be resolved
-      }
-    }
-    if err := p.handleWorkload(workload); err != nil {
-      log.Errorf("handle workload %s failed, err: %v", workload.ResourceName(), err)
-    }
-  }
-}
-```
+The DNS resolution flow in Dual-Engine mode follows this pattern:
 
-Overall, the DNS logic under the Dual-engine mode is roughly illustrated in the block diagram below.
+```txt
+Processor → WorkloadDnsController → DNSResolver → Upstream DNS
+    ↑                                      ↓
+    └────── Resolved Workload ────────────┘
+```
 
 ![](./pics/dual-engine-dns.png)
 
-As mentioned earlier, this implementation largely mirrors the DNS logic in AdsController, resulting in significant structural redundancy. Therefore, after the Step-1 we need to further abstract the current DNS logic. As shown in the diagram, both WorkerController and DnsResolver are already cohesive and self-contained; however, the interface layer highlighted in orange differs across modes. Subsequent refactoring will attempt to treat Cluster or Workload as first-class entities—load objects that Controller and DnsController (Resolver) interact with directly—thereby achieving looser coupling.
+#### Key Components
+
+##### WorkloadDnsController
+
+The WorkloadDnsController manages asynchronous DNS resolution for ServiceEntry-generated workloads. Key data structures:
+
+- **Resolution Queue**: Buffered channel receiving workloads from Processor for non-blocking submission
+- **Result Channel Registry**: Per-workload result channels (indexed by UID) for independent resolution tracking
+- **Domain Resolution Cache**: Bidirectional index between hostnames and pending workloads for batch resolution
+
+The controller operates through three concurrent workers:
+
+1. **Domain Processor**: Consumes workloads, groups by domain, delegates to DNS Resolver
+2. **Refresh Worker**: Receives resolved addresses, constructs workload objects, delivers via result channels
+3. **DNS Resolver**: Executes DNS queries (A/AAAA), maintains TTL-based cache (reused from `pkg/dns`)
+
+##### Resolution Flow
+
+The DNS resolution mechanism follows a producer-consumer pattern with timeout protection:
+
+**Workload Submission**: When the Processor encounters a workload without addresses, it registers a result channel and enqueues the workload for resolution. The Processor blocks on the result channel with a 3-second timeout to prevent pipeline blocking.
+
+**Domain Aggregation**: The Domain Processor maintains a hostname-indexed cache to aggregate workloads requiring the same domain, reducing redundant DNS queries. It checks for cached resolutions before initiating new queries.
+
+**Address Resolution**: The DNS Resolver performs parallel IPv4 (A) and IPv6 (AAAA) queries, respecting DNS TTL values. Resolved addresses are stored in a protocol-agnostic format.
+
+**Result Distribution**: The Refresh Worker reconstructs workload objects with resolved addresses and delivers them via result channels. Channel operations include a 100ms send timeout to prevent deadlocks. After delivery, the controller removes the result channel registration to prevent memory leaks.
+
+##### Cleanup Mechanism
+
+When a workload is deleted, the controller:
+
+- Removes the workload from pending hostname tracking
+- Removes the workload from the hostname's pending domain cache
+- If no more workloads depend on the hostname, unwatches the domain from DNS resolver
+
+This ensures no memory leaks and prevents unnecessary DNS queries.
+
+##### Design Rationale
+
+The WorkloadDnsController design diverges from AdsDnsController in several key aspects to better accommodate workload-level resolution requirements:
+
+| Design Decision | Approach | Rationale |
+|----------------|----------|-----------|
+| **Result Delivery** | Dedicated per-workload channels | Eliminates result filtering overhead and spurious wake-ups; enables direct workload-specific blocking without cross-workload interference |
+| **Timeout Strategy** | Two-tier mechanism (3s processor + 100ms channel) | Processor-level timeout prevents indefinite pipeline blocking; channel-level timeout prevents deadlocks from abandoned consumers |
+| **Address Format** | `netip.ParseAddr().AsSlice()` byte representation | Provides protocol-agnostic representation supporting both IPv4 and IPv6 without conditional logic |
+| **Refresh Interval** | Fixed 200ms rate | Simplifies implementation while maintaining adequate freshness for typical ServiceEntry use cases; trades configurability for consistency |
+
+#### Integration Points
+
+The WorkloadDnsController integrates into the Kmesh control plane through two primary integration points:
+
+| Component | Integration Method | Lifecycle |
+|-----------|-------------------|-----------|
+| **WorkloadController** | Instantiated during `NewController()` | Started via `Run()` context; shutdown via context cancellation |
+| **Processor** | Reference-based invocation | Synchronous blocking on resolution for address-less workloads; ensures data consistency before processing |
+
+**Initialization Sequence**:
+
+1. WorkloadController creates WorkloadDnsController instance
+2. DnsController reference stored in Processor
+3. DnsController goroutines started when WorkloadController.Run() is invoked
+4. Lifecycle bound to WorkloadController's context
+
+**Runtime Interaction**:
+
+1. Processor detects workload without addresses (ServiceEntry-originated)
+2. Processor registers result channel in DnsController
+3. Processor submits workload to resolution queue
+4. Processor blocks on result channel with timeout
+5. DnsController delivers resolved workload or timeout expires
+6. Processor continues with workload processing
+
+#### Comparison with AdsDnsController
+
+| Aspect | AdsDnsController | WorkloadDnsController |
+|--------|------------------|----------------------|
+| **Input** | Clusters with DNS endpoints | Workloads without addresses |
+| **Processing Unit** | Cluster | Workload |
+| **Result Delivery** | Shared channel | Per-workload channels |
+| **Timeout** | No processor timeout | 3s processor + 100ms channel |
+| **Refresh Rate** | From cluster config | Fixed 200ms |
+| **Cleanup** | On cluster removal | On workload removal |
 
 #### Test Plan
 
@@ -211,6 +212,16 @@ that would count as tricky in the implementation, and anything particularly
 challenging to test, should be called out.
 -->
 
+**Unit Tests**
+
+DNS controller unit tests cover IPv4, IPv6, and dual-stack resolution scenarios, workload address overwriting logic, cleanup on workload deletion, and concurrent resolution of multiple workloads.
+
+**E2E Tests**
+
+E2E test validates the end-to-end ServiceEntry DNS resolution flow by creating a ServiceEntry with DNS resolution pointing to a fake hostname, creating a VirtualService routing the fake hostname to a real service, and verifying traffic flows successfully.
+
+Note: DNS proxy is disabled in IPv6-only environments, so tests skip in that configuration.
+
 ### Alternatives
 
 <!--
@@ -219,6 +230,12 @@ not need to be as detailed as the proposal, but should include enough
 information to express the idea and why it was not acceptable.
 -->
 
+**Synchronous Resolution in Processor**: Embedding DNS resolution directly within the Processor's main workload handling loop would introduce blocking behavior, eliminating the possibility of batching concurrent resolutions for identical domains and complicating timeout implementation. This approach violates the separation of concerns principle by coupling network I/O with workload state management.
+
+**Shared Result Channel**: Reusing AdsDnsController's single shared channel pattern would necessitate complex result filtering logic to match responses with their corresponding workload requests. The additional synchronization overhead and potential for spurious wake-ups make this approach less suitable for workload-level granularity.
+
+**Kernel-Space DNS Resolution**: Implementing DNS resolution within eBPF programs would require reimplementing the DNS protocol stack in a constrained execution environment with strict complexity limits. This approach would duplicate existing userspace functionality, significantly increase maintenance burden, and provide minimal performance benefits given the infrequent nature of DNS lookups.
+
 <!--
 Note: This is a simplified version of kubernetes enhancement proposal template.
 https://github.com/kubernetes/enhancements/tree/3317d4cb548c396a430d1c1ac6625226018adf6a/keps/NNNN-kep-template