Add --enable-dns-proxy startup parameter for DNS proxy#1575
Add --enable-dns-proxy startup parameter for DNS proxy#1575vinayakjeet wants to merge 3 commits intokmesh-net:mainfrom
Conversation
|
[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 @vinayakjeet! It looks like this is your first PR to kmesh-net/kmesh 🎉 |
Summary of ChangesHello @vinayakjeet, 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 DNS proxy enablement mechanism within the Kmesh daemon. It transitions from using environment variables or global flags to a dedicated 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. 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 new flag takes its stand, No more env vars, a clear command. Consistency's the key, For features, wild and free. 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 --enable-dns-proxy startup parameter to control the DNS proxy feature, replacing the previous environment variable-based configuration. This change improves consistency with other features. The implementation correctly adds the new flag and plumbs the configuration value down to the component that uses it. The changes are well-executed. I've added a couple of suggestions to improve maintainability by refactoring functions with long parameter lists into using options structs. This will make the code more robust and prevent potential bugs from incorrect parameter ordering.
| } | ||
|
|
||
| func NewXdsClient(mode string, bpfAds *bpfads.BpfAds, bpfWorkload *bpfwl.BpfWorkload, enableMonitoring, enableProfiling bool) (*XdsClient, error) { | ||
| func NewXdsClient(mode string, bpfAds *bpfads.BpfAds, bpfWorkload *bpfwl.BpfWorkload, enableMonitoring, enableProfiling, enableDNSProxy bool) (*XdsClient, error) { |
There was a problem hiding this comment.
The function NewXdsClient has a growing number of parameters, including several booleans in a row. This can make the function calls hard to read and error-prone, as it's easy to mix up the boolean flags. To improve maintainability and reduce the risk of future bugs, consider refactoring this to use an options struct for the configuration parameters.
For example:
type XdsClientOptions struct {
Mode string
BpfAds *bpfads.BpfAds
BpfWorkload *bpfwl.BpfWorkload
EnableMonitoring bool
EnableProfiling bool
EnableDNSProxy bool
}
func NewXdsClient(opts *XdsClientOptions) (*XdsClient, error) { /* ... */ }
This makes callsites clearer, e.g., NewXdsClient(&XdsClientOptions{Mode: c.mode, ...}).
| } | ||
|
|
||
| func NewController(bpfWorkload *bpfwl.BpfWorkload, enableMonitoring, enablePerfMonitor bool) (*Controller, error) { | ||
| func NewController(bpfWorkload *bpfwl.BpfWorkload, enableMonitoring, enablePerfMonitor, enableDNSProxy bool) (*Controller, error) { |
There was a problem hiding this comment.
Similar to NewXdsClient, the function NewController also has a growing list of boolean parameters. This pattern is fragile and can lead to bugs if parameters are accidentally swapped. For instance, the caller passes a variable named enableProfiling for the enablePerfMonitor parameter, which can be confusing. Refactoring to use an options struct with clearly named fields would make the code more robust, readable, and easier to maintain.
For example:
type ControllerOptions struct {
BpfWorkload *bpfwl.BpfWorkload
EnableMonitoring bool
EnablePerfMonitor bool
EnableDNSProxy bool
}
func NewController(opts *ControllerOptions) (*Controller, error) { /* ... */ }
There was a problem hiding this comment.
Pull request overview
Adds a new --enable-dns-proxy startup flag and threads this configuration through the daemon/controller stack so DNS proxy behavior is controlled via startup configuration instead of a workload env var.
Changes:
- Introduces
daemon/options/dns.goand wiresDNSConfigintoBootstrapConfigs. - Passes
enableDNSProxyfrom daemon options → controller → XDS client → workload controller/processor. - Updates DNS proxy preparation/setup paths to use the new config value.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/controller/workload/workload_processor.go | Makes DNS proxy preparation depend on an explicit enableDNSProxy parameter. |
| pkg/controller/workload/workload_controller.go | Stores and forwards enableDNSProxy into PrepareDNSProxy. |
| pkg/controller/controller.go | Reads enableDNSProxy from bootstrap config and uses it to decide DNS proxy startup. |
| pkg/controller/client.go | Extends NewXdsClient to accept and forward enableDNSProxy. |
| daemon/options/options.go | Adds DNSConfig to bootstrap configs and attaches its flags. |
| daemon/options/dns.go | Adds the --enable-dns-proxy flag definition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| func NewXdsClient(mode string, bpfAds *bpfads.BpfAds, bpfWorkload *bpfwl.BpfWorkload, enableMonitoring, enableProfiling bool) (*XdsClient, error) { | ||
| func NewXdsClient(mode string, bpfAds *bpfads.BpfAds, bpfWorkload *bpfwl.BpfWorkload, enableMonitoring, enableProfiling, enableDNSProxy bool) (*XdsClient, error) { |
There was a problem hiding this comment.
NewXdsClient now takes an additional enableDNSProxy parameter, but there are still call sites using the old 5-arg signature (e.g., pkg/controller/client_test.go). This will cause compilation/test failures; update those call sites to pass the new argument (likely false for existing tests).
| bpfAdsObj: bpfLoader.GetBpfKmesh(), | ||
| bpfWorkloadObj: bpfLoader.GetBpfWorkload(), | ||
| enableSecretManager: opts.SecretManagerConfig.Enable, | ||
| enableDNSProxy: opts.DNSConfig.EnableDNSProxy, |
There was a problem hiding this comment.
Controller.enableDNSProxy is now sourced only from the new --enable-dns-proxy flag (opts.DNSConfig.EnableDNSProxy). Existing deployment manifests in this repo (e.g., deploy/charts/kmesh-helm/templates/daemonset.yaml and deploy/yaml/kmesh.yaml) currently enable DNS proxy via the KMESH_ENABLE_DNS_PROXY environment variable, which will be ignored with this change—resulting in DNS proxy silently not starting after upgrade. Either update those manifests to pass --enable-dns-proxy when dnsProxy is enabled, or keep a backward-compatible fallback to the env var.
vinayakjeet
force-pushed
the
add-dns-proxy-flag
branch
from
cd8f22d to
ff02bd3
Compare
signed-off-by: vinayakjeet <vinayakjeetog@gmail.com>
vinayakjeet
force-pushed
the
add-dns-proxy-flag
branch
from
ff02bd3 to
0bcebf6
Compare
Signed-off-by: vinayakjeet <vinayakjeetog@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| func NewXdsClient(mode string, bpfAds *bpfads.BpfAds, bpfWorkload *bpfwl.BpfWorkload, enableMonitoring, enableProfiling bool) (*XdsClient, error) { | ||
| func NewXdsClient(mode string, bpfAds *bpfads.BpfAds, bpfWorkload *bpfwl.BpfWorkload, enableMonitoring, enableProfiling, enableDNSProxy bool) (*XdsClient, error) { |
There was a problem hiding this comment.
NewXdsClient now takes three boolean feature flags (enableMonitoring, enableProfiling, enableDNSProxy). This API shape is hard to read at call sites and makes it easy to accidentally swap arguments as more feature flags get added. Consider replacing the boolean parameter list with an options struct (e.g., XdsClientOptions) or passing a config object so future feature flags are self-documenting and backward-compatible.
| } | ||
|
|
||
| func NewController(bpfWorkload *bpfwl.BpfWorkload, enableMonitoring, enablePerfMonitor bool) (*Controller, error) { | ||
| func NewController(bpfWorkload *bpfwl.BpfWorkload, enableMonitoring, enablePerfMonitor, enableDNSProxy bool) (*Controller, error) { |
There was a problem hiding this comment.
The third parameter name enablePerfMonitor is confusing given it is passed from BpfConfig.EnableProfiling (and NewXdsClient names it enableProfiling). Consider renaming this parameter (and/or the related config flag) so the meaning is consistent across layers (e.g., enableProfiling or enableBpfMetrics).
| enableSecretManager: opts.SecretManagerConfig.Enable, | ||
| enableDNSProxy: opts.DNSConfig.EnableDNSProxy, | ||
| bpfConfig: opts.BpfConfig, |
There was a problem hiding this comment.
After switching to opts.DNSConfig.EnableDNSProxy as the source of truth, pkg/controller/workload/features.go's EnableDNSProxy env var registration appears to be unused (no remaining references). To avoid stale documentation and multiple configuration entry points, consider removing it or wiring the flag parsing to reuse the same variable/registration.
Signed-off-by: vinayakjeet <vinayakjeetog@gmail.com>
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
@nlgwcy @YaoZengzeng @lec-bit |
There was a problem hiding this comment.
Thank you for your help.
But your current implementation still relying on setting environment variables. Perhaps the issue description was unclear. We wish to use kmeshctl to start and stop the DNS proxy at any time during the operation of the kmesh.
You can refer to: #935
This adds a startup parameter --enable-dns-proxy to control DNS proxy instead of using environment variables, making it consistent with other features like --enable-bypass and --enable-mda.
Changes:
Create daemon/options/dns.go with DNS config struct
Add DNSConfig to BootstrapConfigs
Update controller to use enableDNSProxy from config
Pass enableDNSProxy through client to workload controller
Update PrepareDNSProxy to accept enableDNSProxy parameter
Fixes #1574
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: