-
Notifications
You must be signed in to change notification settings - Fork 136
[tenant] Allow egress to parent ingress pods #1765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Nested Kubernetes clusters with exposeMethod: Proxied cannot reach their own external domains because the clusterwide egress policy blocks traffic to ingress pods in parent namespaces. This breaks cert-manager HTTP-01 self-check and any scenario where pods need to access services exposed through parent ingress. Add egress rule allowing traffic to ingress pods (cozystack.io/service: ingress) in parent namespaces, following the same pattern as existing vminsert and etcd rules. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughA conditional egress rule is added to the network policy template that, when the tenant is not "tenant-root" and the Release.Namespace follows the "tenant-*" pattern, inserts additional matchLabels entries targeting ingress services with progressively extended namespace labels derived from namespace segments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Summary of ChangesHello @lexfrei, 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 implements a crucial network policy adjustment for nested Kubernetes environments utilizing 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. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly adds an egress policy to allow traffic from tenant pods to ingress pods in parent namespaces, which is a necessary fix for features like cert-manager's HTTP-01 self-check in nested clusters. The logic for identifying parent namespaces is sound. However, the implementation introduces significant code duplication, as it mimics two existing blocks. My review includes a suggestion to make the new rule more precise by avoiding a redundant case, and I also strongly recommend a refactoring to address the code duplication for better long-term maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/apps/tenant/templates/networkpolicy.yaml (1)
70-82: Implementation correctly enables egress to parent ingress pods.The new egress rule successfully allows traffic to ingress pods in parent namespaces, which resolves the cert-manager HTTP-01 challenge issue and aligns with the PR objectives. The logic correctly follows the established pattern used for vminsert and etcd.
However, as noted in the previous review, this block duplicates the same pattern three times in this file and includes a redundant rule for the tenant's own namespace (the last loop iteration generates a rule for the current namespace, which is already covered by the
allow-to-ingresspolicy at lines 212-223).While these concerns don't affect correctness, consider the suggestions from the previous review to improve maintainability when time permits.
Based on past review comments, the following improvements could be made in a follow-up:
- Extract the repeated pattern into a named template in
_helpers.tpl- Exclude the redundant last iteration by adding
(lt (add $i 1) (len $parts))to the conditional on line 75
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/apps/tenant/templates/networkpolicy.yaml
🧰 Additional context used
📓 Path-based instructions (2)
packages/apps/**
📄 CodeRabbit inference engine (AGENTS.md)
Use Helm Chart umbrella pattern with vendored upstream charts in
charts/directory
Files:
packages/apps/tenant/templates/networkpolicy.yaml
**/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Do NOT directly edit upstream Helm charts in vendored charts directory; use proper chart vendoring mechanisms
Files:
packages/apps/tenant/templates/networkpolicy.yaml
🧠 Learnings (1)
📚 Learning: 2025-07-11T06:28:13.696Z
Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1160
File: packages/system/hetzner-robotlb/charts/robotlb/templates/role.yaml:6-8
Timestamp: 2025-07-11T06:28:13.696Z
Learning: In Helm templates, the `{{-` directive chomps all leading whitespace including newlines back to the previous content, so `{{- toYaml .Values.something | nindent 2 }}` will render correctly even with apparent indentation issues. However, for better style, it's cleaner to put the template directive on the same line as the parent key (e.g., `rules: {{- toYaml .Values.serviceAccount.permissions | nindent 2 }}`).
Applied to files:
packages/apps/tenant/templates/networkpolicy.yaml
|
Successfully created backport PR for |
## What this PR does Fixes egress policy for nested Kubernetes clusters using `exposeMethod: Proxied`. The clusterwide egress policy blocks traffic from tenant pods to ingress pods in parent namespaces. This breaks: - cert-manager HTTP-01 self-check - Any scenario where pods need to access services exposed through parent ingress Adds egress rule allowing traffic to ingress pods (`cozystack.io/service: ingress`) in parent namespaces, following the same pattern as existing vminsert and etcd rules. ### Release note ```release-note [tenant] Fixed tenant egress policy to allow traffic to parent ingress pods, enabling cert-manager HTTP-01 challenges and external domain access for nested clusters with exposeMethod: Proxied ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Enhanced network policies for multi-tenant environments with improved traffic routing based on namespace hierarchies, enabling more granular egress control. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## What this PR does Fixes egress policy for nested Kubernetes clusters using `exposeMethod: Proxied`. The clusterwide egress policy blocks traffic from tenant pods to ingress pods in parent namespaces. This breaks: - cert-manager HTTP-01 self-check - Any scenario where pods need to access services exposed through parent ingress Adds egress rule allowing traffic to ingress pods (`cozystack.io/service: ingress`) in parent namespaces, following the same pattern as existing vminsert and etcd rules. ### Release note ```release-note [tenant] Fixed tenant egress policy to allow traffic to parent ingress pods, enabling cert-manager HTTP-01 challenges and external domain access for nested clusters with exposeMethod: Proxied ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Enhanced network policies for multi-tenant environments with improved traffic routing based on namespace hierarchies, enabling more granular egress control. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## What this PR does Fixes egress policy for nested Kubernetes clusters using `exposeMethod: Proxied`. The clusterwide egress policy blocks traffic from tenant pods to ingress pods in parent namespaces. This breaks: - cert-manager HTTP-01 self-check - Any scenario where pods need to access services exposed through parent ingress Adds egress rule allowing traffic to ingress pods (`cozystack.io/service: ingress`) in parent namespaces, following the same pattern as existing vminsert and etcd rules. ### Release note ```release-note [tenant] Fixed tenant egress policy to allow traffic to parent ingress pods, enabling cert-manager HTTP-01 challenges and external domain access for nested clusters with exposeMethod: Proxied ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Enhanced network policies for multi-tenant environments with improved traffic routing based on namespace hierarchies, enabling more granular egress control. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Fixes egress policy for nested Kubernetes clusters using
exposeMethod: Proxied.The clusterwide egress policy blocks traffic from tenant pods to ingress pods in parent namespaces. This breaks:
Adds egress rule allowing traffic to ingress pods (
cozystack.io/service: ingress) in parent namespaces, following the same pattern as existing vminsert and etcd rules.Release note
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.