Add a startup parameter for the DNS proxy#1581
Add a startup parameter for the DNS proxy#1581FAUST-BENCHOU wants to merge 2 commits intokmesh-net:mainfrom
Conversation
Signed-off-by: zhoujinyu <2319109590@qq.com>
Signed-off-by: zhoujinyu <2319109590@qq.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @FAUST-BENCHOU! It looks like this is your first PR to kmesh-net/kmesh 🎉 |
Summary of ChangesHello @FAUST-BENCHOU, 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 significantly enhances the control and flexibility of Kmesh's DNS proxy feature. It provides both a new command-line interface for dynamic runtime management and a dedicated startup parameter for initial configuration. These changes streamline the process of enabling or disabling the DNS proxy, making it more robust and easier to integrate into existing deployment workflows. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. A proxy for DNS, Resolves names with swift finesse, No more lost requests. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a startup parameter --enable-dns-proxy and a kmeshctl dnsproxy command to control the DNS proxy feature. The changes are well-structured, covering the daemon, CLI tool, deployment configurations, and tests. My review focuses on improving the robustness and error handling of the new functionality. I've identified a critical issue in the kmeshctl command that could cause it to fail when targeting multiple pods, and some high-severity issues related to ignored errors in cleanup paths that could lead to a broken state. The suggested changes will make the new command more reliable and the error handling in the controller more explicit.
| if podName != "" { | ||
| SetDnsproxyPerKmeshDaemon(client, podName, enableStr) | ||
| return | ||
| } | ||
|
|
||
| // Apply to all kmesh daemons | ||
| podList, err := client.PodsForSelector(context.TODO(), utils.KmeshNamespace, utils.KmeshLabel) | ||
| if err != nil { | ||
| log.Errorf("failed to get kmesh podList: %v", err) | ||
| os.Exit(1) | ||
| } | ||
| for _, pod := range podList.Items { | ||
| SetDnsproxyPerKmeshDaemon(client, pod.GetName(), enableStr) | ||
| } |
There was a problem hiding this comment.
After modifying SetDnsproxyPerKmeshDaemon to return an error, ControlDnsproxy should be updated to handle these errors. When a single pod is targeted, an error should cause the command to exit with a non-zero status. When all pods are targeted, an error for one pod should be logged, but the command should continue to process the remaining pods.
if podName != "" {
if err := SetDnsproxyPerKmeshDaemon(client, podName, enableStr); err != nil {
log.Errorf("failed to set dnsproxy for pod %s: %v", podName, err)
os.Exit(1)
}
return
}
// Apply to all kmesh daemons
podList, err := client.PodsForSelector(context.TODO(), utils.KmeshNamespace, utils.KmeshLabel)
if err != nil {
log.Errorf("failed to get kmesh podList: %v", err)
os.Exit(1)
}
for _, pod := range podList.Items {
if err := SetDnsproxyPerKmeshDaemon(client, pod.GetName(), enableStr); err != nil {
log.Errorf("failed to set dnsproxy for pod %s: %v", pod.GetName(), err)
}
}| func SetDnsproxyPerKmeshDaemon(cli kube.CLIClient, podName, info string) { | ||
| var status string | ||
| if info == "enable" { | ||
| status = "true" | ||
| } else { | ||
| status = "false" | ||
| } | ||
|
|
||
| fw, err := utils.CreateKmeshPortForwarder(cli, podName) | ||
| if err != nil { | ||
| log.Errorf("failed to create port forwarder for Kmesh daemon pod %s: %v", podName, err) | ||
| os.Exit(1) | ||
| } | ||
| if err := fw.Start(); err != nil { | ||
| log.Errorf("failed to start port forwarder for Kmesh daemon pod %s: %v", podName, err) | ||
| os.Exit(1) | ||
| } | ||
| defer fw.Close() | ||
|
|
||
| url := fmt.Sprintf("http://%s%s?enable=%s", fw.Address(), patternDnsproxy, status) | ||
|
|
||
| req, err := http.NewRequest(http.MethodPost, url, nil) | ||
| if err != nil { | ||
| log.Errorf("Error creating request: %v", err) | ||
| return | ||
| } | ||
|
|
||
| req.Header.Set("Content-Type", "application/json") | ||
| httpClient := &http.Client{} | ||
| resp, err := httpClient.Do(req) | ||
| if err != nil { | ||
| log.Errorf("failed to make HTTP request: %v", err) | ||
| return | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| log.Errorf("Error: received status code %d", resp.StatusCode) | ||
| bodyBytes, readErr := io.ReadAll(resp.Body) | ||
| if readErr != nil { | ||
| log.Errorf("Error reading response body: %v", readErr) | ||
| return | ||
| } | ||
| log.Errorf("response: %s", string(bodyBytes)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The function SetDnsproxyPerKmeshDaemon calls os.Exit(1) on failure. When kmeshctl dnsproxy enable is run without a pod name, it iterates over all kmesh daemons. If an error occurs for one daemon (e.g., it's not reachable), the entire command will exit, preventing it from configuring the other daemons.
The function should be refactored to return an error instead of exiting. This allows the caller (ControlDnsproxy) to handle the error gracefully, such as logging it and continuing to the next pod in the loop.
func SetDnsproxyPerKmeshDaemon(cli kube.CLIClient, podName, info string) error {
var status string
if info == "enable" {
status = "true"
} else {
status = "false"
}
fw, err := utils.CreateKmeshPortForwarder(cli, podName)
if err != nil {
return fmt.Errorf("failed to create port forwarder for Kmesh daemon pod %s: %v", podName, err)
}
if err := fw.Start(); err != nil {
return fmt.Errorf("failed to start port forwarder for Kmesh daemon pod %s: %v", podName, err)
}
defer fw.Close()
url := fmt.Sprintf("http://%s%s?enable=%s", fw.Address(), patternDnsproxy, status)
req, err := http.NewRequest(http.MethodPost, url, nil)
if err != nil {
return fmt.Errorf("Error creating request: %v", err)
}
req.Header.Set("Content-Type", "application/json")
httpClient := &http.Client{}
resp, err := httpClient.Do(req)
if err != nil {
return fmt.Errorf("failed to make HTTP request: %v", err)
}
defer resp.Body.Close()
if resp.StatusCode != http.StatusOK {
bodyBytes, readErr := io.ReadAll(resp.Body)
if readErr != nil {
return fmt.Errorf("error reading response body for status %d: %v", resp.StatusCode, readErr)
}
return fmt.Errorf("received status code %d, response: %s", resp.StatusCode, string(bodyBytes))
}
return nil
}| if err := c.setupDNSProxy(); err != nil { | ||
| c.client.WorkloadController.SetDnsProxyTrigger(false) | ||
| _ = c.client.WorkloadController.Processor.PrepareDNSProxy(false) | ||
| return err | ||
| } |
There was a problem hiding this comment.
In this error handling path, the error from PrepareDNSProxy(false) is ignored. If cleaning up the BPF map fails, it might leave a stale entry, which could lead to unexpected behavior (e.g., DNS requests still being redirected). The error should be logged to aid in debugging.
if err := c.setupDNSProxy(); err != nil {
c.client.WorkloadController.SetDnsProxyTrigger(false)
if cleanupErr := c.client.WorkloadController.Processor.PrepareDNSProxy(false); cleanupErr != nil {
log.Warnf("Failed to cleanup DNS proxy on setup error: %v", cleanupErr)
}
return err
}| func (c *Controller) StopDnsProxy() error { | ||
| if c.client == nil || c.client.WorkloadController == nil { | ||
| return fmt.Errorf("dns proxy not supported in this mode") | ||
| } | ||
| c.dnsProxyMu.Lock() | ||
| defer c.dnsProxyMu.Unlock() | ||
| if c.dnsServer == nil { | ||
| c.client.WorkloadController.SetDnsProxyTrigger(false) | ||
| _ = c.client.WorkloadController.Processor.PrepareDNSProxy(false) | ||
| return nil | ||
| } | ||
| c.client.WorkloadController.SetDnsProxyTrigger(false) | ||
| _ = c.client.WorkloadController.Processor.PrepareDNSProxy(false) | ||
| c.dnsServer.Close() | ||
| c.dnsServer = nil | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The error from c.client.WorkloadController.Processor.PrepareDNSProxy(false) is ignored. If cleaning up the BPF map fails, it might leave a stale entry, which could lead to unexpected behavior (e.g., DNS requests still being redirected). The error should be logged. This happens in two places in this function.
func (c *Controller) StopDnsProxy() error {
if c.client == nil || c.client.WorkloadController == nil {
return fmt.Errorf("dns proxy not supported in this mode")
}
c.dnsProxyMu.Lock()
defer c.dnsProxyMu.Unlock()
if c.dnsServer == nil {
c.client.WorkloadController.SetDnsProxyTrigger(false)
if err := c.client.WorkloadController.Processor.PrepareDNSProxy(false); err != nil {
log.Warnf("Failed to cleanup DNS proxy state: %v", err)
}
return nil
}
c.client.WorkloadController.SetDnsProxyTrigger(false)
if err := c.client.WorkloadController.Processor.PrepareDNSProxy(false); err != nil {
log.Warnf("Failed to cleanup DNS proxy state: %v", err)
}
c.dnsServer.Close()
c.dnsServer = nil
return nil
}
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
hzxuzhonghu
left a comment
hzxuzhonghu
left a comment
There was a problem hiding this comment.
This is a great feature, but i am concerned with the on-flying change could break traffic
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1574
Tests
TestDnsproxyAPI: Enables and disables the DNS proxy via the status server's HTTP interface (POST /dnsproxy?enable=true|false), verifying consistency with the underlying behavior of
kmeshctl dnsproxy.TestDnsproxyKmeshctl: Enables and disables the DNS proxy via the
kmeshctlcommand line (kmeshctl dnsproxy <pod> enable/disable), verifying that the CLI can correctly control a single daemon.TestDnsproxyStartupParameter: Configures the DNS proxy switch via startup parameters/environment variables (such as
KMESH_ENABLE_DNS_PROXY), and after patching the DaemonSet, verifies that the kmesh pod remains ready, confirming the startup configuration is effective.