feat(extra): add external-dns as standalone extra package#1988
feat(extra): add external-dns as standalone extra package#1988mattia-eleuteri wants to merge 1 commit intocozystack:mainfrom
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
|
Done! Bumped the upstream chart from v1.15.0 to v1.20.0 (appVersion 0.20.0) and exposed When set, it's passed through to the HelmRelease; when empty (default), it's omitted so the upstream default ( Changes:
|
dosubot
bot
added
the
size:XXL
|
The
type ApplicationDefinitionApplication struct {
Kind string `json:"kind"`
OpenAPISchema string `json:"openAPISchema"`
Plural string `json:"plural"`
Singular string `json:"singular"`
}All four fields have no Suggested fix: spec:
application:
kind: ExternalDNS
plural: externaldns
singular: externaldns
openAPISchema: ...
type ApplicationDefinitionDashboard struct {
Singular string `json:"singular"`
Plural string `json:"plural"`
Category string `json:"category"`
// ...
}Same pattern — no Suggested fix: dashboard:
category: Networking
singular: External DNS
plural: External DNS
description: External DNS for automatic DNS record management
icon: ... |
mattia-eleuteri
force-pushed
the
feature/external-dns-per-tenant
branch
from
2da6688 to
5c65355
Compare
|
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.
|
Add external-dns as a per-tenant extra package with support for multiple DNS providers (Cloudflare, AWS, Azure, Google, DigitalOcean, Linode, OVH, Exoscale, GoDaddy). Features: - Provider is required (enum-validated, no empty default) - Policy field constrained to create-only, sync, upsert-only - Conditional credential rendering to avoid empty string failures - Custom annotationPrefix support for multi-instance isolation - Gateway API HTTPRoute source support - ApplicationDefinition with full openAPISchema - E2e test with cloudflare provider and fake credentials Signed-off-by: mattia-eleuteri <mattia@hidora.io> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mattia-eleuteri
force-pushed
the
feature/external-dns-per-tenant
branch
from
3ee2f9a to
1261ff7
Compare
3 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