You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(tenant): ship sub-tenant logs and metrics to the ancestor monitoring - #5257
A tenant (and the guest clusters inside it) whose monitoring resolves to an ancestor runs monitoring agents (Fluent Bit + VMAgent) that write to the owner tenant's vlinsert (logs) and vminsert (metrics). The generated tenant Cilium egress dropped part of this traffic:
The egress opened vminsert but never vlinsert, so log shipping was dropped at the source (fluent-bit no upstream connections available to vlinsert-generic.<owner>.svc:9481).
The egress derived the target by walking the namespace prefix, which never reaches tenant-root from below the first level. In the common layout where only tenant-root has monitoring and descendants inherit it, a guest two levels deep had its ingest traffic dropped on egress.
This change replaces the prefix walk with a single egress rule that targets the monitoring owner namespace directly. The owner is the value already computed in namespace.yaml and inherited through _namespace.monitoring, so it resolves to the real owner at any depth and covers both vminsert and vlinsert in one rule. The rule is skipped when the tenant owns its monitoring (writes stay in-namespace) or when no ancestor owns it.
No ingress policy is added on the owner side: the owner tenant already accepts the traffic through its allow-external-communication rule (fromEntities: [world, cluster]), which matches every pod in the cluster.
Helm unittests cover the owner-targeted egress one level down, the deep case targeting tenant-root, the self-owned case (no rule rendered), and the absence of a separate ingress policy.
No downstream repository is affected by this change
Release note
fix(tenant): sub-tenant monitoring agents now ship logs and metrics to the monitoring owner tenant's VictoriaLogs/VictoriaMetrics at any nesting depth; previously the generated Cilium egress dropped logs and dropped all ingest below the first level.
Summary by CodeRabbit
Updates
Tenant monitoring write traffic now targets the configured monitoring owner’s namespace for vminsert and vlinsert, including when that owner is not the tenant’s parent.
Tenants that own their monitoring no longer receive a separate monitoring-owner egress rule.
When no monitoring owner is configured, the cross-namespace monitoring rule is not added; other tenant egress rules remain unchanged.
A sub-tenant that resolves its monitoring to an ancestor (the usual case
when the sub-tenant runs no monitoring of its own) runs monitoring agents
that write to the ancestor's vlinsert and vminsert. Two tenant-policy gaps
dropped that traffic: the sub-tenant egress allowed vminsert but not
vlinsert, and the ancestor had no ingress accepting descendants on the
ingest endpoints. Logs and metrics never arrived, while the agents'
HelmRelease still reported Ready, which hid the failure.
Add vlinsert to the ancestor egress, and add <tenant>-ingress-monitoring,
which accepts the descendants that point their
namespace.cozystack.io/monitoring label at this tenant on vlinsert and
vminsert.
Assisted-by: LLM
Signed-off-by: IvanHunters <[email protected]>
No actionable comments were generated in the recent review.
Walkthrough
The tenant egress policy replaces namespace-prefix traversal with monitoring-owner selection. It adds rules for vminsert and vlinsert in the owner namespace when the owner differs from the current tenant. Helm tests cover inherited and tenant-owned monitoring configurations.
Changes: Tenant monitoring egress
Layer / File(s)
Summary
Owner-based monitoring egress and validation packages/apps/tenant/templates/networkpolicy.yaml, packages/apps/tenant/tests/networkpolicy_monitoring_logs_test.yaml
The policy selects vminsert and vlinsert in the monitoring owner namespace when an owner exists and differs from the tenant. Tests cover inherited owners, including a non-parent owner, and tenant-owned monitoring without a cross-namespace monitoring rule.
The change lets child tenants send logs and metrics directly to the ancestor's monitoring ingest services. No actionable merge-blocking risk remains.
Pre-merge checks: 4
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title names the main change: sub-tenant logs and metrics are now sent to the ancestor's monitoring owner. The raw summary and objectives confirm the change: tenant egress policy targets the monito…
Linked Issues check
Issue #5256 has one directly linked issue. Its coding requirements are: (1) child-tenant egress to the ancestor's vlinsert (logs) and vminsert (metrics), and (2) owner-side ingress that lets descendan…
Out of Scope Changes check
The whole-PR diff touches packages/apps/tenant/templates/networkpolicy.yaml (monitoring egress block only) and adds the Helm test file. The removed prefix-walk vminsert rule and the new owner-targeted…
Full details: Title check
Explanation
The title names the main change: sub-tenant logs and metrics are now sent to the ancestor's monitoring owner. The raw summary and objectives confirm the change: tenant egress policy targets the monitoring owner via _namespace.monitoring, replacing the namespace-prefix traversal that generated vminsert destinations, and the tests cover the child-to-ancestor case. The 'fix' type fits the issue #5256 delivery failure. The title is 72 characters, slightly above the usual length, but it stays within a single, specific sentence.
Full details: Linked Issues check
Explanation
Issue #5256 has one directly linked issue. Its coding requirements are: (1) child-tenant egress to the ancestor's vlinsert (logs) and vminsert (metrics), and (2) owner-side ingress that lets descendant tenants reach those ingest endpoints. Requirement (1) is met. At the reviewed head, packages/apps/tenant/templates/networkpolicy.yaml adds one egress rule in the <tenant>-egress CiliumClusterwideNetworkPolicy. The rule targets the owner namespace from _namespace.monitoring (or the current tenant when .Values.monitoring is set). It selects app.kubernetes.io/name In [vminsert, vlinsert]. It renders only when the owner exists and differs from the current tenant. The prefix walk that never reached tenant-root is removed. Requirement (2) is met by existing behavior. The owner's CiliumNetworkPolicy allow-external-communication (endpointSelector {}, ingress fromEntities [world, cluster]) accepts traffic from descendant pods. This rule is broader than the issue's suggested scoping to ingest pods. The issue's suggested scoping is not implemented as written, but the functional access requirement is met. The issue's separate suggestion to surface monitoringAgents delivery health is not a coding acceptance criterion and is not addressed here. Helm unit tests in packages/apps/tenant/tests/networkpolicy_monitoring_logs_test.yaml are reported to cover the cases. I did not execute the tests. The whole-PR diff against the merge base was used. The incremental diff from the previous reviewed head was unavailable.
Full details: Out of Scope Changes check
Explanation
The whole-PR diff touches packages/apps/tenant/templates/networkpolicy.yaml (monitoring egress block only) and adds the Helm test file. The removed prefix-walk vminsert rule and the new owner-targeted vminsert/vlinsert rule both belong to the monitoring-egress path described in #5256. The etcd, ingress, and other existing rules are unchanged at head. No unrelated change is evident. The test file supports the change. The author's commit that removes an ingress-monitoring absence test is test-only and does not change generated policy.
Finishing Touches
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Only half of root cause from #5256 is real, and the fix doesn't cover the most common layout.
Missing vlinsert in sub-tenant egress is real, fluent-bit timeouts from the issue are explained by it alone. But same change already sits in your #3439, which is approved code-wise and waits only for description edits, so please keep only one of them.
Ancestor ingress doesn't block descendants. allow-external-communication in the same template (networkpolicy.yaml:17-30) gives every pod of every tenant ingress fromEntities: [world, cluster], and cluster entity in cilium matches all pods of the cluster, allow rules are additive. Metrics in #5256 were not arriving because of wrong remote write URL (#5258, fixed by #5024), and workaround in the issue was tested with both policies at once. So -ingress-monitoring changes nothing and its comment ("Without it the egress allow has no counterpart and delivery is dropped") describes mechanism that doesn't exist. Please drop it, or if you have hubble output with ingress drop on ancestor vlinsert while only egress fix is applied, add it to PR.
Prefix walk never reaches tenant-root below first level (#4726). Common setup is monitoring only in tenant-root (tenant default is monitoring: false) and others inherit it, and with this PR rendered egress is:
So guest cluster in tenant-a-b sending to tenant-root is still dropped on egress, while release note claims general fix. Walk also opens vlinsert of every ancestor instead of the one that owns monitoring for this tenant. Effective owner is already computed in namespace.yaml:64-67 (_namespace.monitoring, or tenant itself with monitoring: true), so egress can target it with one rule: owner namespace + app.kubernetes.io/name in vlinsert, vminsert. It works for any depth.
If ingress policy stays for some reason, absence test doesn't catch anything: I replaced the guard with {{- if true }}, policy rendered with monitoring: false and all 3 tests still passed.
Sub-tenant monitoring agents that resolve to an ancestor shipped metrics
and logs through a namespace-prefix walk that never reached tenant-root
from below the first level. Target the inherited monitoring owner
namespace directly with one egress rule covering vminsert and vlinsert,
so delivery works at any depth.
Drop the -ingress-monitoring policy: the ancestor already accepts the
ingress through its allow-external-communication rule (fromEntities:
cluster), so the separate policy added nothing.
Assisted-by: LLM
Signed-off-by: IvanHunters <[email protected]>
github-actionsBot
added
area/tenant
Issues or PRs related to the tenant chart and multi-tenancy
kind/bug
Categorizes issue or PR as related to a bug
size/L
This PR changes 100-499 lines, ignoring generated files
labels
Nov 12, 2026
The reason will be displayed to describe this comment to others. Learn more.
Previous points are fixed, depth case renders right now. Only tests are left.
"renders no monitoring egress rule when the tenant owns its monitoring" doesn't set _namespace.monitoring, but in real cluster it is never empty (cozystack-basics always sets it for tenant-root and every child inherits it). So removing the if .Values.monitoring override keeps all 82 tests green, while this override is the only thing that makes self-owned tenant differ from inheriting one. Please add _namespace.monitoring: tenant-root to its set, with it test fails on removed override.
"does not render the removed ingress-monitoring policy" can't fail: I put old policy back on its place, it renders with monitoring: true and test still passes. Main never had this policy, so just drop the test.
Set _namespace.monitoring on the self-owned case so the monitoring: true
override is what the assertion depends on; without it the owner is empty
either way and the test passes even if the override is removed. Drop the
ingress-monitoring absence test: that policy never existed on main, so
there is nothing to guard against.
Assisted-by: LLM
Signed-off-by: IvanHunters <[email protected]>
Added _namespace.monitoring: tenant-root to the self-owned test. Verified by mutation: with the if .Values.monitoring override removed, that test now fails (owner falls back to the inherited tenant-root and the egress rule renders).
Dropped the ingress-monitoring absence test, since that policy never existed on main.
The reason will be displayed to describe this comment to others. Learn more.
Both points fixed, tests now fail on removal of any part of owner rule (vlinsert, owner namespace, self guard, monitoring: true override).
PR description still says unittests cover absence of separate ingress policy, please drop that part before merge. Also release note says egress dropped all ingest below first level, but it was only when owner is tenant-root, with intermediate owner metrics passed before too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
area/tenantIssues or PRs related to the tenant chart and multi-tenancykind/bugCategorizes issue or PR as related to a bugsize/LThis PR changes 100-499 lines, ignoring generated files
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
myasnikovdaniil
left a comment
github-actions
Bot
added
area/tenant
What this PR does
A tenant (and the guest clusters inside it) whose monitoring resolves to an ancestor runs monitoring agents (Fluent Bit + VMAgent) that write to the owner tenant's
vlinsert(logs) andvminsert(metrics). The generated tenant Cilium egress dropped part of this traffic:vminsertbut nevervlinsert, so log shipping was dropped at the source (fluent-bitno upstream connections available to vlinsert-generic.<owner>.svc:9481).tenant-rootfrom below the first level. In the common layout where onlytenant-roothas monitoring and descendants inherit it, a guest two levels deep had its ingest traffic dropped on egress.This change replaces the prefix walk with a single egress rule that targets the monitoring owner namespace directly. The owner is the value already computed in
namespace.yamland inherited through_namespace.monitoring, so it resolves to the real owner at any depth and covers bothvminsertandvlinsertin one rule. The rule is skipped when the tenant owns its monitoring (writes stay in-namespace) or when no ancestor owns it.No ingress policy is added on the owner side: the owner tenant already accepts the traffic through its
allow-external-communicationrule (fromEntities: [world, cluster]), which matches every pod in the cluster.Helm unittests cover the owner-targeted egress one level down, the deep case targeting
tenant-root, the self-owned case (no rule rendered), and the absence of a separate ingress policy.Fixes #5256
Downstream repositories
Release note
Summary by CodeRabbit
vminsertandvlinsert, including when that owner is not the tenant’s parent.