Add kmeshctl dnsproxy command to start/stop DNS proxy at runtime#1582
Add kmeshctl dnsproxy command to start/stop DNS proxy at runtime#1582karankoder wants to merge 2 commits intokmesh-net:mainfrom
Conversation
Signed-off-by: karankoder <mkaran4249@gmail.com>
kmesh-bot
requested review from
Okabe-Rintarou-0,
bitcoffeeiux and
hzxuzhonghu
|
[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 |
Summary of ChangesHello @karankoder, 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 operational flexibility of Kmesh by introducing a new command-line interface, 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, once static, now bends to command, With `kmeshctl`, its fate in your hand. Enable, disable, a swift, silent plea, For DNS traffic, now flowing free. Footnotes
|
|
Please review : @LiZhenCheng9527 @bitcoffeeiux @hzxuzhonghu @Okabe-Rintarou-0 |
![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 kmeshctl dnsproxy command to dynamically manage the DNS proxy, which is a valuable feature. The implementation is mostly sound, but there are a few significant issues that need to be addressed. The new CLI command code relies on os.Exit(1) for error handling, which should be replaced with proper error propagation. Additionally, there's a critical issue where the daemon could panic due to unsafe parsing of an environment variable. I've also noted an opportunity to reduce code duplication for better long-term maintainability. Addressing these points will improve the robustness and quality of the new functionality.
| log.Infof("enabling DNS proxy, ip: %s", podIp) | ||
|
|
||
| bv := &bpf.BackendValue{} | ||
| nets.CopyIpByteFromSlice(&bv.Ip, netip.MustParseAddr(podIp).AsSlice()) |
There was a problem hiding this comment.
netip.MustParseAddr will panic if the IP address string from the INSTANCE_IP environment variable is invalid. This can crash the entire kmesh-daemon. You should use netip.ParseAddr instead and handle the potential error gracefully.
| nets.CopyIpByteFromSlice(&bv.Ip, netip.MustParseAddr(podIp).AsSlice()) | |
| addr, err := netip.ParseAddr(podIp) | |
| if err != nil { | |
| return fmt.Errorf("invalid INSTANCE_IP %q: %w", podIp, err) | |
| } | |
| nets.CopyIpByteFromSlice(&bv.Ip, addr.AsSlice()) |
ctl/dnsproxy/dnsproxy.go
Outdated
| Run: func(cmd *cobra.Command, args []string) { | ||
| cli, err := utils.CreateKubeClient() | ||
| if err != nil { | ||
| log.Errorf("failed to create cli client: %v", err) | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| // Determine which pods to query. | ||
| var podNames []string | ||
| if len(args) == 0 { | ||
| podList, err := cli.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 { | ||
| podNames = append(podNames, pod.GetName()) | ||
| } | ||
| } else { | ||
| podNames = args | ||
| } | ||
|
|
||
| // Prepare a slice of podStatuses. | ||
| type podStatus struct { | ||
| Pod string | ||
| Status string | ||
| } | ||
| statuses := make([]podStatus, 0, len(podNames)) | ||
|
|
||
| // Collect the status for each pod. | ||
| for _, podName := range podNames { | ||
| status, err := fetchDNSProxyStatus(cli, podName) | ||
| if err != nil { | ||
| log.Errorf("failed to get DNS proxy status for pod %s: %v", podName, err) | ||
| continue | ||
| } | ||
| statuses = append(statuses, podStatus{Pod: podName, Status: status}) | ||
| } | ||
|
|
||
| // Output the results in a table format. | ||
| var buf bytes.Buffer | ||
| tw := tabwriter.NewWriter(&buf, 0, 0, 2, ' ', 0) | ||
| fmt.Fprintln(tw, "POD\tDNS PROXY STATUS") | ||
| for _, s := range statuses { | ||
| fmt.Fprintf(tw, "%s\t%s\n", s.Pod, s.Status) | ||
| } | ||
| tw.Flush() | ||
| fmt.Print(buf.String()) | ||
| }, |
There was a problem hiding this comment.
The Run function for this command calls os.Exit(1) upon failure. This is a problematic practice in CLI applications as it prevents proper error handling, testing, and reusability. It's recommended to use RunE which allows returning an error. Cobra will then handle printing the error and exiting with a non-zero status code. This principle should be applied to NewEnableCmd and NewDisableCmd as well, and helper functions like SetDNSProxyForPods should be refactored to return errors.
RunE: func(cmd *cobra.Command, args []string) error {
cli, err := utils.CreateKubeClient()
if err != nil {
return fmt.Errorf("failed to create cli client: %v", err)
}
// Determine which pods to query.
var podNames []string
if len(args) == 0 {
podList, err := cli.PodsForSelector(context.TODO(), utils.KmeshNamespace, utils.KmeshLabel)
if err != nil {
return fmt.Errorf("failed to get kmesh podList: %v", err)
}
for _, pod := range podList.Items {
podNames = append(podNames, pod.GetName())
}
} else {
podNames = args
}
// Prepare a slice of podStatuses.
type podStatus struct {
Pod string
Status string
}
statuses := make([]podStatus, 0, len(podNames))
// Collect the status for each pod.
for _, podName := range podNames {
status, err := fetchDNSProxyStatus(cli, podName)
if err != nil {
log.Errorf("failed to get DNS proxy status for pod %s: %v", podName, err)
continue
}
statuses = append(statuses, podStatus{Pod: podName, Status: status})
}
// Output the results in a table format.
var buf bytes.Buffer
tw := tabwriter.NewWriter(&buf, 0, 0, 2, ' ', 0)
fmt.Fprintln(tw, "POD\tDNS PROXY STATUS")
for _, s := range statuses {
fmt.Fprintf(tw, "%s\t%s\n", s.Pod, s.Status)
}
tw.Flush()
fmt.Print(buf.String())
return nil
},| // SetDNSProxyEnabled dynamically enables or disables the DNS proxy. | ||
| // When enabled, registers the kmesh daemon IP in the BPF backend map. | ||
| // When disabled, removes the registration from the BPF map. | ||
| func (p *Processor) SetDNSProxyEnabled(enabled bool) error { | ||
| bk := &bpf.BackendKey{ | ||
| BackendUid: 0, | ||
| } | ||
|
|
||
| if !enabled { | ||
| log.Info("disabling DNS proxy") | ||
| return p.bpf.BackendDelete(bk) | ||
| } | ||
|
|
||
| podIp := os.Getenv("INSTANCE_IP") | ||
| if podIp == "" { | ||
| return fmt.Errorf("ip of kmesh daemon is not set") | ||
| } | ||
|
|
||
| log.Infof("enabling DNS proxy, ip: %s", podIp) | ||
|
|
||
| bv := &bpf.BackendValue{} | ||
| nets.CopyIpByteFromSlice(&bv.Ip, netip.MustParseAddr(podIp).AsSlice()) | ||
| if err := p.bpf.BackendUpdate(bk, bv); err != nil { | ||
| log.Errorf("failed to register kmesh daemon ip, err: %+v", err) | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The function SetDNSProxyEnabled has almost the same implementation as PrepareDNSProxy. This code duplication can lead to maintenance issues where one function is updated but the other is forgotten. To improve maintainability, PrepareDNSProxy should be refactored to call SetDNSProxyEnabled.
There was a problem hiding this comment.
Pull request overview
This PR adds dynamic DNS proxy control to Kmesh via new kmeshctl dnsproxy commands, allowing users to enable, disable, and check the status of the DNS proxy at runtime instead of relying solely on environment variables at startup.
Changes:
- Added
SetDNSProxyEnabledandGetDNSProxyEnabledmethods to the workload controller with atomic state management - Implemented HTTP handler (
dnsProxyHandler) supporting GET (status) and POST (enable/disable) operations - Created new
kmeshctl dnsproxycommand withenable,disable, andstatussubcommands
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/controller/workload/workload_processor.go | Added SetDNSProxyEnabled method to dynamically update BPF backend map for DNS proxy control |
| pkg/controller/workload/workload_controller.go | Added atomic state tracking for DNS proxy status with getter/setter methods |
| pkg/status/status_server.go | Added dnsProxyHandler to handle HTTP GET/POST requests for DNS proxy status and control |
| ctl/dnsproxy/dnsproxy.go | Implemented CLI commands for DNS proxy management (enable, disable, status) |
| ctl/common/common.go | Registered new dnsproxy command with root command |
| docs/ctl/kmeshctl_dnsproxy*.md | Added documentation for new dnsproxy commands |
| docs/ctl/kmeshctl.md | Updated main documentation to include dnsproxy command reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (p *Processor) SetDNSProxyEnabled(enabled bool) error { | ||
| bk := &bpf.BackendKey{ | ||
| BackendUid: 0, | ||
| } | ||
|
|
||
| if !enabled { | ||
| log.Info("disabling DNS proxy") | ||
| return p.bpf.BackendDelete(bk) | ||
| } | ||
|
|
||
| podIp := os.Getenv("INSTANCE_IP") | ||
| if podIp == "" { | ||
| return fmt.Errorf("ip of kmesh daemon is not set") | ||
| } | ||
|
|
||
| log.Infof("enabling DNS proxy, ip: %s", podIp) | ||
|
|
||
| bv := &bpf.BackendValue{} | ||
| nets.CopyIpByteFromSlice(&bv.Ip, netip.MustParseAddr(podIp).AsSlice()) | ||
| if err := p.bpf.BackendUpdate(bk, bv); err != nil { | ||
| log.Errorf("failed to register kmesh daemon ip, err: %+v", err) | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
This function duplicates significant logic from PrepareDNSProxy (lines 108-133). Consider refactoring to extract the common logic for enabling DNS proxy (getting INSTANCE_IP, creating BackendValue, and calling BackendUpdate) into a shared helper function that both PrepareDNSProxy and SetDNSProxyEnabled can call. This would make the code more maintainable and reduce the risk of the two implementations diverging.
| // Initialize DNS proxy state based on the environment variable setting | ||
| c.dnsProxyEnabled.Store(EnableDNSProxy) |
There was a problem hiding this comment.
The dnsProxyEnabled atomic state is initialized based on the EnableDNSProxy environment variable before PrepareDNSProxy is called. If PrepareDNSProxy fails during Controller.Run, the dnsProxyEnabled state will incorrectly indicate that DNS proxy is enabled when it's actually not functional. Consider either: (1) initializing dnsProxyEnabled to false and only setting it to true after PrepareDNSProxy succeeds, or (2) updating dnsProxyEnabled based on the result of PrepareDNSProxy in the Run method.
| // Initialize DNS proxy state based on the environment variable setting | |
| c.dnsProxyEnabled.Store(EnableDNSProxy) | |
| // Initialize DNS proxy state as disabled; it will be enabled only after | |
| // DNS proxy preparation (e.g., PrepareDNSProxy) has successfully completed. | |
| c.dnsProxyEnabled.Store(false) |
Signed-off-by: karankoder <mkaran4249@gmail.com>
LiZhenCheng9527
left a comment
LiZhenCheng9527
left a comment
There was a problem hiding this comment.
Add a flag in daemon/options/bpf.go to set the default value for enableDnsProxy
| Short: "Manage DNS proxy for Kmesh", | ||
| } | ||
|
|
||
| dnsProxyCmd.AddCommand(NewEnableCmd()) |
There was a problem hiding this comment.
Can you give more example about kmeshctl dnsproxy?
|
You're doing exactly the same thing as #1581 |
3 participants
What type of PR is this?
/kind feature
Fixes #1574
What this PR does :
This PR adds the ability to dynamically enable and disable the DNS proxy via
kmeshctlinstead of relying on environment variables. This allows users to control the DNS proxy at any time during the operation of Kmesh.New Commands:
kmeshctl dnsproxy enable [podNames...]- Enable DNS proxy for Kmeshkmeshctl dnsproxy disable [podNames...]- Disable DNS proxy for Kmeshkmeshctl dnsproxy status [podNames...]- Display current DNS proxy statusExample Usage: