Repository navigation
[system] Fix monitoring-agents FQDN resolution for tenant workload clusters - #2075
Conversation
…logs endpoints Monitoring agents in tenant workload clusters failed to deliver metrics and logs because service addresses used short DNS names without the cluster domain suffix. Tenant CoreDNS could not resolve these names across cluster boundaries. Append the configured cluster domain from _cluster.cluster-domain to all vmagent remoteWrite URLs and fluent-bit output hosts, falling back to cluster.local when not set. Signed-off-by: IvanHunters <[email protected]>
Summary of ChangesHello IvanHunters, 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 resolves critical issues with monitoring agents in tenant workload clusters failing to deliver metrics and logs. By modifying the configuration to include the full cluster domain in service addresses, it ensures proper DNS resolution across cluster boundaries, thereby restoring the functionality of 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
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 (@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. Footnotes
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughConfiguration updated to dynamically append cluster-domain suffixes to monitoring service endpoints and hostnames using Helm template expressions with "cluster.local" as the default fallback value. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 correctly addresses the FQDN resolution issue for monitoring agents in tenant workload clusters by appending the cluster domain to service hostnames. The changes are consistent across vmagent and fluent-bit configurations, resolving potential DNS issues in multi-cluster environments. I have one suggestion to improve maintainability by reducing code duplication.
| - http://vminsert-shortterm.{{ .Values.global.target }}.svc.{{ (index .Values._cluster "cluster-domain") | default "cluster.local" }}:8480/insert/0/prometheus | ||
| - http://vminsert-longterm.{{ .Values.global.target }}.svc.{{ (index .Values._cluster "cluster-domain") | default "cluster.local" }}:8480/insert/0/prometheus |
There was a problem hiding this comment.
The logic to determine the cluster domain, {{ (index .Values._cluster "cluster-domain") | default "cluster.local" }}, is repeated in four places in this file (here and for fluent-bit outputs). This violates the DRY (Don't Repeat Yourself) principle and makes future changes more difficult and error-prone.
To improve maintainability, I recommend defining a common helper template in your Helm chart, for example in a _helpers.tpl file:
{{/*
Cluster domain suffix
*/}}
{{- define "monitoring-agents.clusterDomain" -}}
{{- (index .Values._cluster "cluster-domain") | default "cluster.local" -}}
{{- end -}}
Then you could use {{ include "monitoring-agents.clusterDomain" . }} in all places where the cluster domain is needed. This would centralize the logic.
I am not providing a direct code suggestion as it would require creating or modifying a helper file which is outside the scope of the current changes.
dosubot
Bot
added
the
size/S
PR #2075 introduced _cluster.cluster-domain references in monitoring-agents values.yaml for FQDN resolution. This broke fluent-bit because it's a subchart and _cluster values are not accessible from the subchart context (only global values are shared with subcharts). Replace _cluster references with global.clusterDomain variable: - Empty by default (management cluster uses short DNS names) - Set to the management cluster domain for tenant clusters, enabling FQDN resolution for cross-cluster service discovery Closes #2084 Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
…n tenant HelmRelease PR #2075 added _cluster.cluster-domain references to monitoring-agents values.yaml for FQDN resolution. This broke fluent-bit because it is a subchart where _cluster values are not accessible (only global values are shared with subcharts), causing "index of untyped nil". Revert values.yaml to short DNS names (.svc) for the management cluster where they resolve locally, and add FQDN with cluster-domain suffix in the tenant kubernetes HelmRelease where _cluster values are available and cross-cluster DNS resolution is needed. Closes #2084 Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
…2086) ## What this PR does PR #2075 introduced `_cluster.cluster-domain` references in monitoring-agents `values.yaml` for FQDN resolution in tenant clusters. This broke the fluent-bit subchart because `_cluster` values are not accessible from the Helm subchart context — only `global` values are shared with subcharts. This PR replaces `_cluster` references with a new `global.clusterDomain` variable: - Empty by default (management cluster uses short DNS names like `service.namespace.svc`) - Set to the management cluster domain (e.g. `cozy.local`) for tenant clusters, enabling FQDN resolution for cross-cluster service discovery Fixes #2084 ### Release note ```release-note [system] Fix monitoring-agents installation failure caused by inaccessible _cluster values in fluent-bit subchart context. Introduce global.clusterDomain for proper FQDN resolution in tenant workload clusters. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Monitoring agent configuration updated to support configurable cluster domain names for greater flexibility. * Remote write and log-forwarding endpoints adjusted to align with cluster domain handling, improving compatibility when deploying across different cluster DNS setups. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Monitoring agents (vmagent, fluent-bit) in tenant workload clusters failed to deliver metrics and logs because service addresses used short DNS names (e.g.
vminsert-longterm.tenant-root.svc) without the cluster domain suffix. Tenant CoreDNS could not resolve these names across cluster boundaries.This PR appends the configured cluster domain from
_cluster.cluster-domainto all vmagent remoteWrite URLs and fluent-bit output hosts, with a fallback tocluster.localwhen not set.Release note
Summary by CodeRabbit