feat(monitoring): auto-route tenant logs using namespace labels - #2195
Conversation
dosubot
Bot
added
size/S
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChanges update Fluent Bit configuration rendered from Changes
Sequence Diagram(s)sequenceDiagram
participant Pod as App Pod
participant FB as Fluent Bit
participant VInsert as vlinsert-generic (tenant)
Pod->>FB: emit logs (kube.* tags)
FB->>FB: FILTER rewrite_tag (per-namespace) -> retag to tenant.{target}.{namespace}.<stream>
FB->>FB: FILTER modify (Match *) -> add tenant {{ .Values.global.target }}
FB->>VInsert: OUTPUT Match tenant.{target}.* -> HTTP/JSON POST to vlinsert-generic.{target}.svc:9481
VInsert-->>FB: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/system/monitoring-agents/values.yaml (1)
382-394: Consider adding input validation for tenantLogRouting entries.If a user provides a
tenantLogRoutingentry without the requirednamespaceorvlogsfields, the template will generate an invalid fluent-bit config that could cause deployment failures with unhelpful error messages.🛡️ Suggested validation approach
Add validation at the start of the range loop:
{{- range .Values.tenantLogRouting }} + {{- if not .namespace }} + {{- fail "tenantLogRouting entry missing required 'namespace' field" }} + {{- end }} + {{- if not .vlogs }} + {{- fail "tenantLogRouting entry missing required 'vlogs' field" }} + {{- end }} [OUTPUT]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/monitoring-agents/values.yaml` around lines 382 - 394, The template iterates over .Values.tenantLogRouting without checking required fields, which can produce invalid fluent-bit OUTPUT blocks when namespace or vlogs are missing; update the range loop over tenantLogRouting to validate each entry by testing that .namespace and .vlogs are non-empty (e.g., using conditional/if checks) and either skip the entry or emit a clear failure/warning message before rendering the OUTPUT stanza for that item; reference the existing range over .Values.tenantLogRouting and the fields .namespace and .vlogs when implementing the checks so only entries with both fields produce the Name http / Match / Host / header blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/system/monitoring-agents/values.yaml`:
- Around line 444-449: The Rule line in the Helm template that builds the
rewrite_tag filter interpolates .Values.tenantLogRouting[].namespace directly
into a regex (the "Rule $kubernetes_namespace_name ^({{ .namespace }})$
tenant.{{ .namespace }}.$TAG false"), which can break when namespaces contain
regex metacharacters; update that template to escape the namespace using Helm's
regexQuoteMeta (or equivalent) when constructing both the capture group and the
replacement token so the generated regex is literal and safe, ensuring you
reference the template block iterating over .Values.tenantLogRouting and the
rewrite_tag Rule to locate and change the interpolation.
---
Nitpick comments:
In `@packages/system/monitoring-agents/values.yaml`:
- Around line 382-394: The template iterates over .Values.tenantLogRouting
without checking required fields, which can produce invalid fluent-bit OUTPUT
blocks when namespace or vlogs are missing; update the range loop over
tenantLogRouting to validate each entry by testing that .namespace and .vlogs
are non-empty (e.g., using conditional/if checks) and either skip the entry or
emit a clear failure/warning message before rendering the OUTPUT stanza for that
item; reference the existing range over .Values.tenantLogRouting and the fields
.namespace and .vlogs when implementing the checks so only entries with both
fields produce the Name http / Match / Host / header blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ffb4485-2409-4778-bcf4-7be38544caca
📒 Files selected for processing (1)
packages/system/monitoring-agents/values.yaml
Summary of ChangesHello, 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 addresses the current limitation where 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 (@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
|
![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
The pull request introduces a new Fluent Bit configuration for per-tenant log routing, allowing logs from specific namespaces to be routed to dedicated VLogs services. This involves dynamically generating HTTP output and rewrite tag filter blocks based on user-defined tenantLogRouting values. However, the current implementation has several security vulnerabilities: the .vlogs parameter, used as an HTTP output host, is user-controlled and creates a Server-Side Request Forgery (SSRF) risk; the .namespace parameter, embedded in a regular expression for tag rewriting, is user-controlled and leads to a Regular Expression Denial of Service (ReDoS) vulnerability; and the .namespace parameter, embedded in glob patterns for log matching, is user-controlled and creates a log routing injection vulnerability. Remediation requires strict validation and escaping of these user-controlled parameters, along with refactoring duplicated output configurations for better maintainability.
| {{- if not .namespace }}{{ fail "tenantLogRouting: namespace is required" }}{{- end }} | ||
| [OUTPUT] | ||
| Name http | ||
| Match tenant.{{ .namespace }}.* |
There was a problem hiding this comment.
The .vlogs parameter is directly used as the Host for an HTTP output, which is a user-controlled value. This creates a Server-Side Request Forgery (SSRF) vulnerability, allowing an attacker to specify an arbitrary hostname for Fluent Bit to send logs, potentially scanning internal networks or exfiltrating data. Implement strict validation and allow-listing for .vlogs to restrict it to trusted hosts. Additionally, the configuration for this [OUTPUT] block is duplicated, increasing maintenance overhead. Consider refactoring for future maintainability.
There was a problem hiding this comment.
This is a Helm values.yaml template — .vlogs is set by the platform operator during tenant provisioning, not by end users. There is no user-controlled input path here, so SSRF is not applicable.
| {{- range .Values.tenantLogRouting }} | ||
| [FILTER] | ||
| Name rewrite_tag | ||
| Match kube.* |
There was a problem hiding this comment.
The .namespace parameter is directly embedded into a regular expression within the Rule field of a rewrite_tag filter. This user-controlled value creates a Regular Expression Denial of Service (ReDoS) vulnerability, allowing an attacker to inject malicious regex patterns that could consume excessive CPU and lead to a denial of service. Implement strict validation and escape special characters for .namespace. Additionally, the current regex ^({{ .namespace }})$ uses an unnecessary capturing group; simplifying it could improve clarity and avoid minor overhead.
Rule $kubernetes_namespace_name ^{{ .namespace }}$ tenant.{{ .namespace }}.$TAG falseThere was a problem hiding this comment.
Fixed — regexQuoteMeta is now applied to escape the namespace value before it's used in the regex pattern.
| {{- range .Values.tenantLogRouting }} | ||
| {{- if not .namespace }}{{ fail "tenantLogRouting: namespace is required" }}{{- end }} | ||
| [OUTPUT] | ||
| Name http |
There was a problem hiding this comment.
The .namespace parameter is directly embedded into the Match field of an [OUTPUT] block, which uses glob patterns. An attacker controlling this value could inject glob pattern wildcards (*, ?, []) to broaden log matching criteria.
Impact: This could lead to misrouting of logs, potential information leakage between tenants if a malicious pattern matches other tenants' logs, or a denial of service by causing Fluent Bit to process an excessive amount of logs.
Remediation: Implement strict validation for the .namespace parameter to ensure it only contains valid and expected namespace names. Avoid allowing wildcard characters or other special glob pattern characters in user-supplied namespace values.
There was a problem hiding this comment.
The Match field uses fluent-bit tag matching, not arbitrary glob. The namespace value is set by the platform during provisioning, not by tenant users, so glob injection is not a concern here.
6eb1707 to
70274a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/system/monitoring-agents/values.yaml`:
- Around line 382-387: The template validates .namespace but not .vlogs, so
missing vlogs silently defaults Host to localhost; update the tenantLogRouting
loop in values.yaml to check for .vlogs (e.g., add {{- if not .vlogs }}{{ fail
"tenantLogRouting: vlogs is required for namespace <namespace>" }}{{- end }})
before rendering the [OUTPUT] block so rendering fails fast when .vlogs is
omitted; keep the existing Host {{ .vlogs }} usage and include the namespace in
the failure message to aid debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92ca9944-7a8d-42fc-9f26-3d5af186d017
📒 Files selected for processing (1)
packages/system/monitoring-agents/values.yaml
70274a7 to
28a6826
Compare
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
Thanks for tackling this! The problem is real — tenant VLogs instances receiving no logs is a gap we need to close.
However, I think the manual tenantLogRouting approach has some drawbacks:
- No automation — someone has to manually add entries to
values.yamlevery time a tenant with monitoring is created/deleted. This is error-prone and doesn't scale. - No sub-tenant support — nested tenants won't get their logs routed automatically.
- Diverges from the existing pattern — vmagent already solves the equivalent problem for metrics automatically.
How vmagent does it (for reference)
- Each tenant namespace gets a label
namespace.cozystack.io/monitoring: <monitoring-namespace>(namespace.yaml) - The
cozystack-valuesSecret injects_namespace.monitoringinto every tenant, pointing to the correct monitoring stack - The monitoring-agents HelmRelease for each Kubernetes cluster uses
$targetTenant := .Values._namespace.monitoringto route both metrics and logs to the right destination automatically
For child Kubernetes clusters, fluent-bit already sends all logs to vlinsert-generic.{{ $targetTenant }}.svc — so logs already work automatically there. The gap is only on the root cluster, where all namespace logs go to tenant-root without per-tenant splitting.
Suggested approach
Instead of a manual list in monitoring-agents values, consider generating the fluent-bit routing config at the tenant chart level (packages/apps/tenant/templates/), using the same _namespace.monitoring value that vmagent already relies on. This way:
- Tenant creation automatically sets up log routing
- Sub-tenant inheritance works out of the box (child tenants inherit
_namespace.monitoringfrom parent) - The pattern stays consistent with how metrics collection already works
For reference, PR #1684 added per-tenant vmagent with automatic wiring — the same pattern should be followed for log routing.
28a6826 to
29a4a64
Compare
dosubot
Bot
added
size/M
|
Andrei Kvapil (@kvaps) Thanks for the detailed review and the pointer to how vmagent handles this! I've reworked the implementation to follow the exact same automatic pattern. Here's what changed: Approach: Instead of a manual How it works:
Also fixed: Default outputs now correctly use Edge cases handled:
Only |
da4c3ac to
1e8f354
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/system/monitoring-agents/values.yaml (1)
442-459: Consider order ofmodifyandrewrite_tagfilters for tenant field accuracy.The
modifyfilter at line 442-445 addstenant {{ .Values.global.target }}to all logs (Match *) before therewrite_tagfilters process them. This means logs routed to tenant-specific VLogs instances will still havetenant: cozy-monitoring(or whateverglobal.targetis) rather than the actual tenant target.If tenants should see their own tenant name in the
tenantfield, consider either:
- Moving the tenant modify filter after rewrite_tag and adjusting the match patterns, or
- Adding tenant-specific modify filters after rewrite_tag
This may be intentional if the tenant field is meant for platform-level identification, or could be addressed in the follow-up PR
#2196(metadata redaction).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/monitoring-agents/values.yaml` around lines 442 - 459, The current global modify filter (Name modify that adds tenant {{ .Values.global.target }}) is applied before the rewrite_tag rules, causing tenant-specific routing to keep the global tenant value; update the config so tenant is set after tags are rewritten: either move the global "Name modify Add tenant {{ .Values.global.target }}" block below the rewrite_tag blocks and narrow its Match to only platform-level logs, or add per-namespace tenant-specific modify filters immediately after the rewrite_tag Rule (matching the rewritten tag pattern such as tenant.{{ $monTarget }}.*) and set Add tenant to the corresponding {{ $monTarget }}; adjust Match patterns accordingly so the modify that sets tenant runs after the rewrite_tag transformation (refer to the existing [FILTER] blocks, the Name modify filter and the Name rewrite_tag filter with Rule $kubernetes_namespace_name ^({{ $ns.metadata.name }})$ tenant.{{ $monTarget }}.{{ $ns.metadata.name }}.$TAG).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/system/monitoring-agents/values.yaml`:
- Around line 347-348: Add a missing ExternalName Service entry for
vlinsert-generic in the monitoring-external-services.yaml template: create an
ExternalName service resource named vlinsert-generic that mirrors the existing
vlogs-generic entry but points to vlinsert-generic.tenant-root.svc.cluster.local
and exposes port 9481 so Fluent Bit outputs (references to vlinsert-generic:9481
in values.yaml) can resolve; update the template where other services are
defined (use the vlogs-generic Service as the pattern) and ensure the same
entries are added for the other occurrences noted (lines near 358-359, 369-370,
392).
---
Nitpick comments:
In `@packages/system/monitoring-agents/values.yaml`:
- Around line 442-459: The current global modify filter (Name modify that adds
tenant {{ .Values.global.target }}) is applied before the rewrite_tag rules,
causing tenant-specific routing to keep the global tenant value; update the
config so tenant is set after tags are rewritten: either move the global "Name
modify Add tenant {{ .Values.global.target }}" block below the rewrite_tag
blocks and narrow its Match to only platform-level logs, or add per-namespace
tenant-specific modify filters immediately after the rewrite_tag Rule (matching
the rewritten tag pattern such as tenant.{{ $monTarget }}.*) and set Add tenant
to the corresponding {{ $monTarget }}; adjust Match patterns accordingly so the
modify that sets tenant runs after the rewrite_tag transformation (refer to the
existing [FILTER] blocks, the Name modify filter and the Name rewrite_tag filter
with Rule $kubernetes_namespace_name ^({{ $ns.metadata.name }})$ tenant.{{
$monTarget }}.{{ $ns.metadata.name }}.$TAG).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5220f786-1b26-476a-87f9-623825339d61
📒 Files selected for processing (1)
packages/system/monitoring-agents/values.yaml
| Host vlinsert-generic.{{ .Values.global.target }}.svc | ||
| port 9481 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for vlinsert-generic service definitions in the codebase
echo "=== Searching for vlinsert-generic service definitions ==="
rg -n "vlinsert-generic" --type yaml
echo ""
echo "=== Checking monitoring package templates ==="
fd -e yaml . packages/system/monitoring/templates -x grep -l "vlinsert" {} \;Repository: cozystack/cozystack
Length of output: 610
🏁 Script executed:
#!/bin/bash
# Search for all external service definitions
echo "=== Checking for external services definitions ==="
rg -n "ExternalName" --type yaml -A 3 | head -50
echo ""
echo "=== Looking for vlogs references ==="
rg -n "vlogs" --type yaml | grep -i service
echo ""
echo "=== Checking monitoring-external-services.yaml ==="
find . -name "monitoring-external-services.yaml" -exec cat {} \;Repository: cozystack/cozystack
Length of output: 9071
🏁 Script executed:
#!/bin/bash
# Check Victoria Logs configuration and services
echo "=== Searching for victoria logs related configs ==="
fd -name "*.yaml" | xargs grep -l "victoria" | head -20
echo ""
echo "=== Check if vlinsert is mentioned in any documentation ==="
fd -name "*.md" | xargs grep -l "vlinsert" 2>/dev/null || echo "No vlinsert in markdown files"Repository: cozystack/cozystack
Length of output: 684
🏁 Script executed:
#!/bin/bash
# Search for victoria-logs package
echo "=== Finding victoria-logs related files ==="
find . -type d -name "*victoria*logs*" | head -20
echo ""
echo "=== Searching for any vlinsert mention (case-insensitive) ==="
rg -i "vlinsert" --type yaml
echo ""
echo "=== Check monitoring package structure ==="
ls -la packages/system/monitoring/templates/ | head -30
echo ""
echo "=== Check if monitoring package defines any services ==="
fd -e yaml . packages/system/monitoring/templates -exec basename {} \;Repository: cozystack/cozystack
Length of output: 2170
🏁 Script executed:
#!/bin/bash
# Check vlogs configuration in monitoring package
echo "=== Content of vlogs.yaml ==="
cat packages/system/monitoring/templates/vlogs/vlogs.yaml
echo ""
echo "=== Content of vlogs grafana-datasource.yaml ==="
cat packages/system/monitoring/templates/vlogs/grafana-datasource.yamlRepository: cozystack/cozystack
Length of output: 1146
🏁 Script executed:
#!/bin/bash
# Check monitoring-agents deployment/values
echo "=== Check monitoring-agents to understand service discovery ==="
head -100 packages/system/monitoring-agents/values.yaml
echo ""
echo "=== Search for VLogs CRD definitions ==="
rg -i "kind.*vlogs" --type yaml
echo ""
echo "=== Check if VLogs operator creates services automatically ==="
find . -name "*.yaml" -o -name "*.md" | xargs grep -l "VLogs.*service" 2>/dev/null | head -5Repository: cozystack/cozystack
Length of output: 4352
🏁 Script executed:
#!/bin/bash
# Check VLogs CRD to understand service creation
echo "=== Searching for VLogs CRD service-related fields ==="
rg -A 5 -B 5 "vlinsert\|service" packages/system/victoria-metrics-operator/charts/victoria-metrics-operator/crd.yaml | head -100
echo ""
echo "=== Check if there's any documentation about VLogs services ==="
find . -name "*.md" -o -name "README*" | xargs grep -l "VLogs\|vlinsert" 2>/dev/null | head -5Repository: cozystack/cozystack
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Look at the full diff context around the changes
echo "=== Check git diff for this file if available ==="
git log --oneline packages/system/monitoring-agents/values.yaml 2>/dev/null | head -5
echo ""
echo "=== Check recent changes context ==="
git show HEAD:packages/system/cozystack-basics/templates/monitoring-external-services.yaml 2>/dev/null || echo "Not available"Repository: cozystack/cozystack
Length of output: 783
🏁 Script executed:
#!/bin/bash
# Check the actual changes in monitoring-agents/values.yaml
echo "=== Context around lines 340-400 in monitoring-agents/values.yaml ==="
sed -n '340,400p' packages/system/monitoring-agents/values.yaml | cat -n
echo ""
echo "=== Check what the OUTPUT is for ==="
sed -n '330,350p' packages/system/monitoring-agents/values.yaml | cat -nRepository: cozystack/cozystack
Length of output: 4108
🏁 Script executed:
#!/bin/bash
# Final verification: check if VLogs operator would create vlinsert services
echo "=== Search for any indication that vlinsert services are created ==="
rg -i "vlinsert" . --type yaml
echo ""
echo "=== Check if there's a pattern for how services are named in monitoring ==="
rg -i "port.*948|9428" . --type yaml | grep -v node_modules | head -20
echo ""
echo "=== Verify the monitoring namespace and how external services work ==="
grep -r "cozy-monitoring" packages/system/cozystack-basics/templates/monitoring-external-services.yamlRepository: cozystack/cozystack
Length of output: 1308
Add vlinsert-generic ExternalName service to monitoring-external-services.yaml.
Fluent Bit outputs (lines 347-348 and others) send logs to vlinsert-generic:9481 (VictoriaLogs insert endpoint), but this service is not defined in packages/system/cozystack-basics/templates/monitoring-external-services.yaml. Without it, logs will fail to be inserted into VictoriaLogs. Add the missing ExternalName service mapping to vlinsert-generic.tenant-root.svc.cluster.local similar to the existing vlogs-generic service.
Also applies to: 358-359, 369-370, 392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/monitoring-agents/values.yaml` around lines 347 - 348, Add a
missing ExternalName Service entry for vlinsert-generic in the
monitoring-external-services.yaml template: create an ExternalName service
resource named vlinsert-generic that mirrors the existing vlogs-generic entry
but points to vlinsert-generic.tenant-root.svc.cluster.local and exposes port
9481 so Fluent Bit outputs (references to vlinsert-generic:9481 in values.yaml)
can resolve; update the template where other services are defined (use the
vlogs-generic Service as the pattern) and ensure the same entries are added for
the other occurrences noted (lines near 358-359, 369-370, 392).
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the change points all log outputs at a VictoriaLogs service that isn't deployed (breaks log shipping); the per-tenant routing also can't reconcile under Flux and diverges from the linked issue's design.
Business context: route each tenant's container logs to that tenant's VictoriaLogs instance (issue #2194), instead of everything landing in the root tenant.
Blockers
B1: outputs target vlinsert-generic:9481, which is not deployed — breaks all log shipping
File: packages/system/monitoring-agents/values.yaml:347 (also 358, 369, 392)
All three baseline outputs (kube/events/audit) and the new per-tenant output are switched to vlinsert-generic.<target>.svc:9481 — the cluster-mode VictoriaLogs ingest endpoint (VLCluster → vlinsert, port 9481). The platform deploys single-node VictoriaLogs only: packages/system/monitoring/templates/vlogs/vlogs.yaml creates kind: VLogs named generic → service vlogs-generic:9428. There is no VLCluster/vlinsert anywhere in the repo. Corroboration: the tenant kubernetes app ships to vlogs-generic.<tenant>.svc:9428 (packages/apps/kubernetes/templates/helmreleases/monitoring-agents.yaml:76-77), the Grafana datasource reads vlogs-generic:9428, and issue #2194 itself names the target vlogs-generic.<tenant>.svc. After this merges, fluent-bit posts to a non-existent service and root-tenant container/event/audit log shipping stops. Revert host/port to vlogs-generic:9428, or land the VLogs→VLCluster migration in the same PR.
B2: per-tenant routing via lookup can't reconcile under Flux
File: packages/system/monitoring-agents/values.yaml (the two new lookup "v1" "Namespace" blocks)
monitoring-agents is a Flux HelmRelease; helm-controller's upgrade trigger is digest-based on chart + values, and cluster state read via lookup is invisible to that hash. Creating a tenant namespace or toggling the namespace.cozystack.io/monitoring label changes no digest, so the config won't re-render and the new tenant gets no routing until an unrelated change forces a reconcile. It's also why helm template renders both blocks to nothing offline (the PR's three test-plan items are unchecked). Route this through values (the tenantLogRouting list the PR body / issue #2194 describe) instead of lookup.
B3: implementation diverges from the documented contract
The PR body and issue #2194 specify an explicit tenantLogRouting values list of {namespace, vlogs} pairs targeting single-node vlogs-…:9428. The diff instead implements label-driven auto-discovery (namespace.cozystack.io/monitoring) against cluster-mode endpoints. That label is defined nowhere else in the repo (grep finds it only here), so its contract — who sets it, to what value — is undefined and undocumented. Reconcile code and design (the values-list approach also fixes B2).
B4: the re-tagged tenant copy keeps the root's identity labels
File: packages/system/monitoring-agents/values.yaml (the new rewrite_tag filter)
rewrite_tag runs after the Add tenant {{ .Values.global.target }} / Add cluster root-cluster modify filters, and fluent-bit's modify Add does not overwrite an existing key. So the re-emitted tenant.<target>.* record carries tenant=<root target> / cluster=root-cluster, not the destination tenant's identity — which undercuts the per-tenant isolation goal. Verify the routed copy's labels on a live cluster.
Non-blocking
- The events output
_stream_fieldshas a pre-existing typomeatdata_namespace→metadata_namespace(the events nest filter adds themetadata_prefix); this line is rewritten here, so it's in scope. The audit output similarly listsrequestUriwhile_msg_fieldusesrequestURI— confirm the emitted field name and make them consistent. - A new routing label/knob ships with no docs or values-schema description; add docs (and the website docs PR) for a user-facing routing control.
| Match kube.* | ||
| Host vlogs-generic.{{ .Values.global.target }}.svc | ||
| port 9428 | ||
| Host vlinsert-generic.{{ .Values.global.target }}.svc |
There was a problem hiding this comment.
This (and the events/audit outputs, and the new per-tenant output) targets vlinsert-generic:9481 — the cluster-mode VictoriaLogs ingest endpoint. The platform only deploys single-node kind: VLogs (vlogs-generic:9428); there's no VLCluster/vlinsert anywhere in the repo, and issue #2194 names the target as vlogs-generic.<tenant>.svc. As-is this posts to a service that doesn't exist and stops log shipping. Revert to vlogs-generic:9428, or land the VLogs→VLCluster migration in this PR.
There was a problem hiding this comment.
Checked against current main: VictoriaLogs was migrated VLogs→VLCluster in 62d36ec4e (2026-03-10), so vlogs.yaml:4 is now kind: VLCluster and vlinsert-generic:9481 is what main uses everywhere — the baseline outputs here (values.yaml:347/358/369), the tenant app (apps/kubernetes/templates/helmreleases/monitoring-agents.yaml:71), and the ExternalName (cozystack-basics/templates/monitoring-external-services.yaml:5). vlogs-generic:9428 isn't in the repo anymore, so reverting would break shipping; after a rebase onto main these three lines are a no-op. Full context + the B2/B4 and design path in my PR comment below.
|
Thanks Aleksei Sviridkin (@lexfrei), Andrei Kvapil (@kvaps). Digging into this against current B1 —
|
|
Two things: the base edit ( |
Aleksei Sviridkin (lexfrei)
added
the
area/monitoring
1e8f354 to
0ff2cf2
Compare
0ff2cf2 to
b320401
Compare
mattia-eleuteri
changed the title
|
Direction call, as promised — going with per-tenant fluent-bit at the tenant chart level, mirroring the per-tenant vmagent pattern. Rationale, building on the earlier synthesis:
I'll rework this PR accordingly: drop the root-agent routing changes and add a fluent-bit collector to the tenant monitoring stack, scoped to the tenant's namespaces via the existing |
b320401 to
66e4a87
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
mattia-eleuteri NOT LGTM. The branch still routes tenant logs through a lookup in the root fluent-bit config, which is the approach you dropped in favour of a per-tenant collector in your July comment, and two of the earlier problems are still there.
Two of my earlier points were wrong and are closed. vlinsert-generic:9481 is what main uses after the VLCluster migration, and namespace.cozystack.io/monitoring is an established label set by the tenant chart. I also checked whether a tenant can steer its logs into someone else's store through that label. I found no path: the tenant chart sets it from the parent's values, tenant users only get namespaced RoleBindings so they cannot patch the Namespace, and a label value cannot carry a newline into the config.
Blockers
B1: new tenants are not picked up until something else changes
Helm-controller only upgrades the release when the chart or values digest changes, and lookup results are not part of that digest. A tenant that turns on monitoring gets no routing, and a deleted one keeps its filter, until an unrelated change forces an upgrade. Your July comment reached the same conclusion. Main now has a per-tenant collector for traces (#3763), so the tenant-chart layer already has a pattern to follow.
B2: routed records still say tenant=tenant-root
I rendered the configmap with three mocked namespaces. The rewrite_tag filters come after Add tenant tenant-root and Add cluster root-cluster, and modify Add does not overwrite an existing key. So every record that lands in a tenant's VictoriaLogs is labelled as the root tenant.
Match *
Add tenant tenant-root
...
Name rewrite_tag
Match kube.*
Rule $kubernetes_namespace_name ^(tenant-a)$ tenant.tenant-a.tenant-a.$TAG false
B3: no tests
monitoring-agents has a helm-unittest suite, and lookup can be mocked with kubernetesProvider (that is how I got the render above). The new logic needs a test that pins which namespaces get a filter and an output, and which are skipped: an unlabelled namespace, an empty label, and a label equal to global.target.
B4: PR body and provenance
The body describes a tenantLogRouting values list and a usage example that the diff does not have. It also ends with a "Generated with Claude Code" line, and the PR body becomes the merge commit message. Please rewrite the body to match the change, drop that line, and add an Assisted-by: LLM trailer to the commit.
Non-blocking
- With
keep falsethe tenant's logs leave the root store completely. That may be intended, but it is a visible change for the platform operator and should be stated. - Each
rewrite_tagfilter creates its own emitter, so this adds one emitter per tenant namespace. One filter with severalRulelines avoids that.
A tenant that runs its own monitoring stack gets a VictoriaLogs instance, but the node fluent-bit sends every container log to global.target, so that instance stays empty. The tenant chart already names the stack a namespace reports to in namespace.cozystack.io/monitoring, the label the per-tenant vmagent selects on, so the log path keys off the same label. The routed record leaves the default outputs (keep false): a tenant with its own monitoring holds its logs, the root store no longer does. The re-tagged record re-enters the filter chain, where the Match * "Add tenant" cannot overwrite the key, so a per-target Set labels it with the tenant it is sent to. One rewrite_tag carries every rule, keeping a single emitter however many tenants are routed. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <[email protected]>
The node fluent-bit builds its tenant routes from a lookup of namespace labels, and helm-controller upgrades a release only when its chart or values digest changes. A tenant turning monitoring on therefore got no route, and a removed one kept its route, until an unrelated change upgraded monitoring-agents. The reconciler digests the label across all namespaces and requests a forced upgrade of the release when the digest moves. It digests every labelled namespace rather than repeating the chart's selection, so it cannot drift from the chart; the cost is an occasional upgrade that renders the same config. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <[email protected]>
66e4a87 to
e510d49
Compare
github-actions
Bot
added
size/XL
|
Aleksei Sviridkin (@lexfrei) Reworked in cdff5ed and e510d49. First, why the branch does not follow the per-tenant collector direction from my July comment: I checked it against the tree and it does not hold for logs. The #3763 collector receives OTLP over the network, while a log collector has to tail B1. New B2. Each routed target gets a B3. B4. Body rewritten to match the diff, the byline is gone, and both commits carry Non-blocking 1. Stated in the body: once a tenant runs its own monitoring, its container logs leave the root store; events and audit logs are not routed. Non-blocking 2. One |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
mattia-eleuteri LGTM. The blockers from my last review are fixed.
Business context: a tenant that runs its own monitoring gets a VictoriaLogs instance that stays empty, because the node fluent-bit sends every container log to the root store.
I rendered the configmap with mocked namespaces. The re-tagged record comes back into the chain as tenant.tenant-a.*, the Match * Add leaves the key alone, and Set tenant tenant-a overwrites it before the tenant output. The kube.* filters don't run a second time on the new tag.
The reconciler checks out too. createOrUpdateHelmRelease in the operator merges existing annotations, so the digest survives a Package reconcile. The ClusterRole already allows patch on helmreleases, the Namespace informer is already there for tenantquota, and the controller sits behind the manager's leader election like the others. Removing the digest comparison, the sort, the forceAt token, the global.target guard or the empty-label guard each fails a test.
The hostPath argument against a per-tenant collector holds. Tenant namespaces run under a PodSecurity level that rejects hostPath, which is the same wall the opensearch sysctl DaemonSet goes around.
Non-blocking:
- Any change to the routed set changes the configmap, and
checksum/configthen rolls fluent-bit on every node. The container tail input has noDB, so after a restart fluent-bit starts at the end of the files it already knew, and lines written during the restart are lost. This already happens on platform upgrades. With this PR it also happens each time a namespace joins or leaves a routed tenant, for example a child tenant created under a monitored one. ADBon the tail input, or the chart'shotReload, would close that gap in a follow-up. - The body leaves the website question open. I think
logs.mdshould get a sentence saying that a tenant with its own monitoring no longer has its container logs in the root store.
3 participants
What this PR does
A tenant that runs its own monitoring stack gets a VictoriaLogs instance, but the node fluent-bit in
cozy-monitoringsends every container log toglobal.target(tenant-root), so that instance stays empty. This PR routes each namespace's container logs to the VictoriaLogs named by itsnamespace.cozystack.io/monitoringlabel, the label the tenant chart already sets and the per-tenant vmagent already selects on.Chart (
packages/system/monitoring-agents).templates/_tenant-log-routes.tpllooks up the namespaces and keeps those whose label is set and names a tenant other thanglobal.target. For those, fluent-bit gets onerewrite_tagfilter with aRuleper target (a single emitter however many tenants are routed), onehttpoutput per target tovlinsert-generic.<target>.svc:9481, and a per-targetmodify Set tenant <target>, because the re-tagged record re-enters the filter chain where theMatch *Add tenantcannot overwrite the key. Unlabelled namespaces, empty labels and labels equal toglobal.targetstay on the default outputs.Controller (
internal/controller/tenantlogrouting).lookupresults are not part of the digest helm-controller compares, so the chart alone would not pick up a tenant that turns monitoring on, or drop one that is removed, until an unrelated upgrade. The new reconciler in cozystack-controller digests the monitoring label across all namespaces and, when the digest moves, setsreconcile.fluxcd.io/requestedAtandreconcile.fluxcd.io/forceAtoncozy-monitoring/monitoring-agents, which forces an upgrade and re-runs the lookup. It digests every labelled namespace rather than repeating the chart's selection, so it cannot drift from the chart; the cost is an occasional upgrade that renders the same config. The digest it acted on is kept in its own annotation, so a manualflux reconciledoes not make it force another upgrade. The existing ClusterRole already grants the verbs it needs.Behaviour change for the platform operator. Routed records are re-tagged with
keep false: once a tenant runs its own monitoring, its namespaces' container logs go to that tenant's VictoriaLogs and no longer to the root one. Events and audit logs are not routed and stay on the root store.Why not a per-tenant collector. An earlier comment on this PR proposed a fluent-bit per tenant, following the per-tenant vmagent and the OTLP collector from #3763. That collector receives OTLP over the network; a log collector has to tail
/var/logthroughhostPath, which the PodSecurity level of tenant namespaces rejects, and running it in a privileged system namespace instead would mean one DaemonSet per tenant tailing every node's logs. The routing therefore stays in the one node DaemonSet, with the reconciler closing the reconcile gap.Tests:
tests/fluent-bit_tenant_log_routing_test.yamlmocks the namespaces withkubernetesProviderand pins which get a rule and an output, which are skipped (unlabelled, empty label, label equal toglobal.target), and that a routed record carries the target tenant. Three of its cases fail against the previous revision of this branch.tests/fluent-bit_tenant_log_routing_offline_test.yamlpins that an offline render routes nothing.internal/controller/tenantlogrouting/reconciler_test.gocovers first sight, no-op while labels are unchanged (including after a manual reconcile), a tenant turning monitoring on, a routed namespace being deleted, and a missing release.Refs #2194
Screenshots
Downstream repositories
No entry of the trigger map matches the diff: it touches
packages/system/monitoring-agents(values, a template helper, tests), a new reconciler ininternal/controller, its registration incmd/cozystack-controller, and a comment in the cozystack-controller ClusterRole. The one open question iscozystack/website:content/en/docs/next/operations/services/monitoring/logs.mddescribes log collection in general terms and does not say where a tenant's container logs land, so the behaviour change above could deserve a sentence there. I have left every box empty for a maintainer to decide.Release note