feat(extra): add external-dns as standalone extra package#1988
Conversation
mattia-eleuteri
requested review from
androndo,
kvaps,
lexfrei and
lllamnyp
as code owners
dosubot
bot
added
size:L
|
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:
📝 WalkthroughWalkthroughAdds a new External DNS package: Helm charts, values/schema, templates, package source, system release descriptors, and an ApplicationDefinition for CozyStack, enabling multi-provider ExternalDNS deployments and packaging metadata. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 @mattia-eleuteri, 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 significantly enhances the multi-tenant capabilities of the platform by enabling individual tenants to manage their own External-DNS instances. Previously, a single cluster-wide External-DNS limited flexibility for tenants requiring different DNS providers or domain management strategies. The changes introduce a robust, isolated, and configurable solution, allowing tenants to integrate their specific DNS infrastructure seamlessly while maintaining security and preventing conflicts. 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-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
This pull request introduces per-tenant external-dns support, which is a great feature for multi-tenant clusters. The implementation is solid, using a dedicated HelmRelease per tenant and ensuring DNS record isolation with unique txtOwnerId.
I have a few suggestions to improve the robustness and security of this feature:
- Add validation to ensure the DNS provider is specified when external-dns is enabled.
- Strengthen the
values.schema.jsonfor theenvarray to provide better validation. - Most importantly, add a strong warning against storing plaintext credentials in the values file, as this is a critical security risk. The current examples could mislead users into insecure configurations.
| "items": { | ||
| "type": "object" | ||
| }, |
There was a problem hiding this comment.
The schema for env items is currently a generic object. To provide better validation and guide users towards secure practices (like using valueFrom), it's better to define the expected properties for environment variables. This aligns with the Kubernetes EnvVar structure and allows for static validation of tenant values.
"items": {
"type": "object",
"properties": {
"name": { "type": "string" },
"value": { "type": "string" },
"valueFrom": { "type": "object" }
},
"required": ["name"]
},![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
🤖 Fix all issues with AI agents
In `@packages/apps/tenant/templates/external-dns.yaml`:
- Around line 43-44: When external DNS is enabled but no provider is supplied
the chart will render an empty provider name; update the Helm template in
external-dns.yaml to validate .Values.externalDns.provider whenever
.Values.externalDns.enabled is true by adding a conditional check that fails
with a clear message (e.g., use an if .Values.externalDns.enabled -> if not
.Values.externalDns.provider -> fail "externalDns.provider is required when
externalDns.enabled is true") so templates referencing
.Values.externalDns.provider (the provider: {{ .Values.externalDns.provider |
quote }} line) never render an empty value.
🧹 Nitpick comments (2)
packages/apps/tenant/templates/external-dns.yaml (1)
22-28: Infinite retries may mask deployment failures.Setting
retries: -1for both install and upgrade means Flux will retry indefinitely. While this provides resilience for transient issues, it can also mask persistent configuration problems (e.g., invalid credentials, wrong provider settings). Consider whether a finite retry count with alerting might be more appropriate for tenant-scoped resources.packages/apps/tenant/values.schema.json (1)
91-97: Consider strengthening theenvitems schema.The
envarray items are typed as genericobject, which accepts any structure. For Kubernetes environment variables, you could specify the expected schema to provide better validation and documentation.♻️ Optional: More specific env items schema
"env": { "description": "Environment variables for provider credentials.", "type": "array", "items": { - "type": "object" + "type": "object", + "properties": { + "name": { + "type": "string", + "description": "Environment variable name" + }, + "value": { + "type": "string", + "description": "Environment variable value" + }, + "valueFrom": { + "type": "object", + "description": "Source for the environment variable's value" + } + }, + "required": ["name"] }, "default": [] }
packages/apps/tenant/values.yaml
Outdated
| externalDns: | ||
| enabled: false | ||
| provider: "" | ||
| domainFilters: [] | ||
| policy: upsert-only | ||
| credentialsSecretName: "" | ||
| extraArgs: [] | ||
| env: [] |
There was a problem hiding this comment.
Hey @mattia-eleuteri,
Are these parameters expected to be configurable by the tenant themselves, or by the parent tenants?
The problem is that tenants only have visibility into their own namespace, not the parent namespace where the tenant application is installed. Because of that, they can’t modify their tenant application parameters directly.
If this is intended to be tenant-managed, please consider creating a separate additional applicationin packages/extra that would install the HelmRelease from the packages/system.
Also, please check the latest update to the developers guide — it explains this pattern in a bit more detail:
cozystack/website#413
There was a problem hiding this comment.
Hi @kvaps, thanks for the feedback!
You're right — this should be tenant self-managed. I've refactored the implementation following the pattern you described:
- Removed the
externalDnsblock frompackages/apps/tenant/(template, values, schema) - Created
packages/extra/external-dns/as a standalone application using the HelmRelease-based pattern (same approach asingressandseaweedfs) - Added
packages/core/platform/sources/external-dns-application.yaml(PackageSource referencingsystem/external-dnsandextra/external-dns)
The extra package now supports per-provider credential configuration (cloudflare, aws, azure, google, digitalocean, linode, ovh, exoscale, godaddy) with full JSON schema validation. Tenants can deploy and configure it directly from the dashboard.
I also reviewed PR #413 on the website repo — the new structure should align with the developers guide.
mattia-eleuteri
force-pushed
the
feature/external-dns-per-tenant
branch
from
a1b3d43 to
12aff0e
Compare
dosubot
bot
added
size:XL
mattia-eleuteri
force-pushed
the
feature/external-dns-per-tenant
branch
from
12aff0e to
55afcf1
Compare
mattia-eleuteri
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/extra/external-dns/Chart.yaml`:
- Around line 1-6: Create a new README.md for the external-dns chart referenced
by Chart.yaml (name: external-dns) because the Makefile's cozyvalues-gen step
passes -r README.md and the file is missing; add a minimal markdown README.md
(title, short description, usage placeholder) next to the chart so the generate
step can read it, matching the pattern used by other packages/extra charts.
In `@packages/extra/external-dns/templates/external-dns.yaml`:
- Around line 43-53: The template is emitting an empty "env:" key when provider
is "cloudflare" (and similarly for AWS) if no credential fields are set; change
the template so the "env:" key is rendered only when at least one credential
variable exists by moving the env: line inside the credential checks (use the
same conditional that checks .Values.cloudflare.apiToken or the
.Values.cloudflare.apiKey and .Values.cloudflare.apiEmail) and likewise wrap the
AWS env: under a conditional that checks
.Values.aws.accessKey/.Values.aws.secretKey (or whichever AWS credential fields
you use) so no empty env list is produced when credentials are absent.
- Around line 129-137: The template emits a second extraArgs: when provider ==
"godaddy", which overwrites any user-provided .Values.extraArgs; fix by merging
GoDaddy flags into the same extraArgs block instead of emitting a new key:
remove the separate godaddy extraArgs block and inside the existing extraArgs
rendering (where .Values.extraArgs is iterated) append conditional lines for
--godaddy-api-key={{ .Values.godaddy.apiKey }} and --godaddy-api-secret={{
.Values.godaddy.apiSecret }} only when .Values.provider == "godaddy" and the
values exist, so the final rendered extraArgs contains both user-supplied
entries and GoDaddy flags without duplicate keys.
- Around line 22-31: The HelmRelease currently places provider credentials
inline under values.external-dns (the block containing external-dns, txtOwnerId,
provider.name, sources), so update the HelmRelease to use valuesFrom referencing
the existing secret pattern (e.g., the cozystack-values Secret) instead of
embedding sensitive keys; remove all credential keys currently in the inline
values (CF_API_TOKEN, AWS_SECRET_ACCESS_KEY, AWS_ACCESS_KEY_ID,
DIGITALOCEAN_TOKEN, GOOGLE_APPLICATION_CREDENTIALS, etc.) and add a valuesFrom
entry that pulls those keys from the Secret (matching the repository pattern
used by seaweedfs/nginx-ingress/tenant apps) so the template no longer exposes
plaintext provider credentials.
| apiVersion: v2 | ||
| name: external-dns | ||
| description: External DNS for automatic DNS record management | ||
| icon: /logos/external-dns.svg | ||
| type: application | ||
| version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other packages/extra charts have a README.md
echo "=== README.md files in packages/extra ==="
fd README.md packages/extra --type f 2>/dev/null || echo "No README.md files found"
echo ""
echo "=== Checking external-dns directory contents ==="
ls -la packages/extra/external-dns/ 2>/dev/null || echo "Directory not found"
echo ""
echo "=== Checking cozyvalues-gen usage in packages/extra Makefiles ==="
rg -n 'cozyvalues-gen' packages/extra/*/Makefile 2>/dev/null | head -30
echo ""
echo "=== Checking cozyvalues-gen usage in packages/apps Makefiles ==="
rg -n 'cozyvalues-gen' packages/apps/*/Makefile 2>/dev/null | head -30Repository: cozystack/cozystack
Length of output: 3762
Create missing README.md file.
The make generate step fails because the Makefile references README.md via the -r flag in cozyvalues-gen, but the file doesn't exist. All other packages/extra/* charts have this file and use the same pattern. Create an initial README.md file (even if minimal) to resolve the pipeline failure.
🧰 Tools
🪛 GitHub Actions: Pre-Commit Checks
[error] 1-1: make generate failed. README: open README.md: no such file or directory
🤖 Prompt for AI Agents
In `@packages/extra/external-dns/Chart.yaml` around lines 1 - 6, Create a new
README.md for the external-dns chart referenced by Chart.yaml (name:
external-dns) because the Makefile's cozyvalues-gen step passes -r README.md and
the file is missing; add a minimal markdown README.md (title, short description,
usage placeholder) next to the chart so the generate step can read it, matching
the pattern used by other packages/extra charts.
93488d9 to
076d890
Compare
lexfrei
left a comment
lexfrei
left a comment
There was a problem hiding this comment.
Consider adding optional Gateway API support as a source.
Cozystack has Gateway API CRDs available in tenant clusters (packages/system/gateway-api-crds/), enabled via addons.gatewayAPI.enabled. When a tenant uses Gateway API (HTTPRoute, etc.), external-dns should be able to pick up hostnames from those resources too.
Suggestion: add a boolean value (e.g. gatewayAPI: false) and conditionally include gateway-httproute in sources:
sources:
- ingress
- service
{{- if .Values.gatewayAPI }}
- gateway-httproute
{{- end }}
mattia-eleuteri
force-pushed
the
feature/external-dns-per-tenant
branch
2 times, most recently
from
7414a3c to
a574359
Compare
|
Thanks @lexfrei for the suggestion! I've added optional Gateway API support. A new sources:
- ingress
- service
{{- if .Values.gatewayAPI }}
- gateway-httproute
{{- end }} |
mattia-eleuteri
force-pushed
the
feature/external-dns-per-tenant
branch
2 times, most recently
from
b18fe55 to
699750f
Compare
|
And so two Claudes had a conversation through their human puppets 🤖🤝🤖 |
|
The upstream external-dns chart v1.20.0 added support for Could you expose |
mattia-eleuteri
force-pushed
the
feature/external-dns-per-tenant
branch
from
699750f to
2da6688
Compare
dosubot
bot
removed
the
size:XL
|
Thanks for the detailed review! Missing |
|
1.
Suggested fix: either remove the default and make 2. Currently: ## @param {string} policy="upsert-only" - How DNS records are synchronized (sync or upsert-only).
policy: "upsert-only"The upstream chart supports three values: ## @enum {string} Policy - How DNS records are synchronized.
## @value create-only
## @value sync
## @value upsert-only
## @param {Policy} policy="upsert-only" - How DNS records are synchronized.
policy: "upsert-only"3. Azure, DigitalOcean, Linode, OVH render credentials unconditionally Cloudflare has a guard ( Suggested fix: add similar guards, or better — use JSON Schema conditional validation ( 4. No e2e test There is no ## @enum {string} Provider - DNS provider.
## @value inmemory
## @value cloudflare
...The The test should verify that two independent instances can coexist — one with the default annotation prefix, another with a custom |
mattia-eleuteri
force-pushed
the
feature/external-dns-per-tenant
branch
3 times, most recently
from
d2ee231 to
3ee2f9a
Compare
|
Thanks for the review @lexfrei — all four points have been addressed in the latest force-push: 1.
|
mattia-eleuteri
force-pushed
the
feature/external-dns-per-tenant
branch
7 times, most recently
from
ef27242 to
6c6c2af
Compare
lexfrei
force-pushed
the
feature/external-dns-per-tenant
branch
from
abb5dd1 to
27fc025
Compare
|
I rebased your branch onto the current The branch was significantly behind — notably missing the E2E install refactor (#2060, The E2E |
mattia-eleuteri
force-pushed
the
feature/external-dns-per-tenant
branch
from
27fc025 to
edd6c9e
Compare
|
E2E test The CI failure is in Attempt 1:
Attempts 2-3:
The root cause is the nested HelmRelease pattern. The There are three issues in the test: 1. The 60s timeout is too short for nested HelmRelease reconciliation The outer HR becomes ready as soon as its chart is installed (it just creates a manifest with the inner HR). The inner HR then needs to be picked up by Flux, fetch the chart via ExternalArtifact, and deploy. The first test passes because Flux picks it up quickly, but the second test fails because Flux is still processing the cleanup from the first test. Consider waiting for the inner HR as well: kubectl -n tenant-test wait hr ${name}-system --timeout=120s --for=condition=ready2. The label selector is too broad kubectl -n tenant-test get deploy -l app.kubernetes.io/name=external-dns -o jsonpath='{.items[0].status.readyReplicas}'
3. No cleanup on test failure The teardown() {
kubectl -n tenant-test delete externaldns.apps.cozystack.io --all --ignore-not-found 2>/dev/null || true
} |
mattia-eleuteri
force-pushed
the
feature/external-dns-per-tenant
branch
2 times, most recently
from
4c80baf to
253e449
Compare
|
Thanks for the rebase and the detailed E2E failure analysis! All three points have been addressed and CI is now fully green. 1. Wait for inner HelmReleaseAdded 2. More specific label selectorChanged from 3. Cleanup on test failureAdded a Additional fixThe |
|
Let's plan this feature for v1.1, we need to cut v1.0 finally 
|
Signed-off-by: mattia-eleuteri <mattia@hidora.io>
kvaps
force-pushed
the
feature/external-dns-per-tenant
branch
from
253e449 to
f201fe4
Compare
4 participants
Summary
Add external-dns as a standalone self-managed application in
packages/extra/external-dns/, allowing tenants to deploy and configure their own DNS management directly from the dashboard.Motivation
Tenants need the ability to manage their own DNS domains with their own provider. Following the developers guide, this is implemented as an extra package (like
ingressandseaweedfs) using the HelmRelease-based pattern, rather than embedding it in the tenant chart.This enables multi-tenant scenarios where:
domain-a.comdomain-b.comChanges
packages/extra/external-dns/— standalone HelmRelease-based applicationpackages/core/platform/sources/external-dns-application.yaml— referencessystem/external-dnsandextra/external-dnscomponentsexternalDnsblock frompackages/apps/tenant/Features
domainFilterssyncorupsert-only)namespaced: true) for tenant isolationtxtOwnerIdper namespace to prevent DNS record conflictsUsage Example
Deploy from the dashboard, or via values:
Test plan
helm template external-dns packages/extra/external-dns/ --set provider=cloudflare --set cloudflare.apiToken=testrenders correctlyhelm template external-dns packages/extra/external-dns/fails (provider required)helm template wrong-name packages/extra/external-dns/ --set provider=cloudflarefails (release name check)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation