feat: add configurable cluster domain support#1872
feat: add configurable cluster domain support#1872Yuni-sa wants to merge 6 commits intoargoproj-labs:masterfrom
Conversation
Yuni-sa
force-pushed
the
feat/configurable-cluster-domain-1149
branch
from
f661422 to
0bb5720
Compare
olivergondza
left a comment
olivergondza
left a comment
There was a problem hiding this comment.
Looks good to me on a high level. Added a minor comment.
Yuni-sa
force-pushed
the
feat/configurable-cluster-domain-1149
branch
from
c97df98 to
b4a9660
Compare
📝 WalkthroughWalkthroughAdds a configurable Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API as ArgoCD CR
participant Controller
participant K8s as Kubernetes (Services, Secrets)
participant Cert as CertManager
User->>API: create/update ArgoCD CR (spec.clusterDomain)
Controller->>API: read ArgoCD CR (clusterDomain)
Controller->>K8s: construct FQDNs using clusterDomain, reconcile Services/Endpoints
Controller->>Cert: request certificate with SANs using FQDNs
Cert->>K8s: create Secret (TLS cert) for services
Controller->>K8s: update Redis/Dex/Repo connection strings using clusterDomain
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In `@bundle/manifests/argocd-operator.clusterserviceversion.yaml`:
- Around line 1082-1090: The displayName value for the clusterDomain property
contains a stray trailing apostrophe; update the displayName in the manifest
(the entry with displayName: Cluster Domain' and path: clusterDomain) to remove
the trailing apostrophe so it reads simply "Cluster Domain".
In `@deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yaml`:
- Around line 11139-11144: The schema for ArgoCDSpec's clusterDomain field
declares in its description that it defaults to "cluster.local" but lacks a
schema default; update the clusterDomain entry in the CRD (the clusterDomain
property under ArgoCDSpec) to include default: "cluster.local" alongside the
existing description and type so the apiextensions.k8s.io/v1 schema will apply
the default value consistently.
🧹 Nitpick comments (5)
controllers/argocd/util.go (1)
669-681: Normalize ClusterDomain input before use.
Trimming whitespace and trailing dots helps prevent accidental malformed FQDNs (e.g.,svc..).♻️ Suggested normalization
func getClusterDomain(cr *argoproj.ArgoCD) string { - if cr.Spec.ClusterDomain != "" { - return cr.Spec.ClusterDomain - } - return common.ArgoCDDefaultClusterDomain + domain := strings.TrimSpace(cr.Spec.ClusterDomain) + domain = strings.TrimPrefix(domain, ".") + domain = strings.TrimSuffix(domain, ".") + if domain != "" { + return domain + } + return common.ArgoCDDefaultClusterDomain }examples/argocd-custom-cluster-domain.yaml (1)
1-7: Useful example demonstrating the new cluster domain feature.The example clearly shows how to configure a custom cluster domain. Consider adding a brief comment at the top explaining when this configuration is needed (e.g., for clusters using non-default DNS suffixes like Exoscale SKS).
💡 Optional: Add explanatory comment
+# Example ArgoCD configuration for clusters using a custom DNS domain suffix. +# Use this when your cluster does not use the default "cluster.local" domain +# (e.g., Exoscale SKS clusters use "svc.CLUSTER_ID.cluster.local"). apiVersion: argoproj.io/v1beta1 kind: ArgoCD metadata: name: example-argocd namespace: argocd spec: - clusterDomain: "abc123.cluster.local" + clusterDomain: "abc123.cluster.local"config/crd/bases/argoproj.io_argocds.yaml (1)
11892-11897: Add minimal schema validation to avoid invalid cluster domains.Line 11892: since FQDN construction assumes a non-empty suffix, an empty string (or accidental whitespace-only value) would generate invalid service names and DNS failures. Consider adding a small OpenAPI guard (e.g.,
minLength: 1) to prevent the most common misconfiguration without optional strict patterning.🔧 Suggested schema guard
clusterDomain: description: |- ClusterDomain is the cluster domain suffix used for constructing service FQDNs. Defaults to "cluster.local". The full FQDN will be: <service>.<namespace>.svc.<clusterDomain> This is useful for clusters that use a different DNS suffix (e.g., "CLUSTER_ID.cluster.local", "edge.local"). type: string + minLength: 1bundle/manifests/argoproj.io_argocds.yaml (1)
11903-11908: Consider basic validation forclusterDomainto prevent malformed FQDNs.
Empty or malformed values can yieldservice.namespace.svc.or double-dot domains. A minimal schema guard helps catch misconfigurations early.🔧 Optional schema hardening
clusterDomain: description: |- ClusterDomain is the cluster domain suffix used for constructing service FQDNs. Defaults to "cluster.local". The full FQDN will be: <service>.<namespace>.svc.<clusterDomain> This is useful for clusters that use a different DNS suffix (e.g., "CLUSTER_ID.cluster.local", "edge.local"). type: string + minLength: 1deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yaml (1)
11139-11144: Optional: Consider adding validation for the domain format.Adding a pattern or format validation could help catch invalid domain values early, though this is not critical since the operator likely validates domains during reconciliation.
💡 Optional validation pattern
clusterDomain: description: |- ClusterDomain is the cluster domain suffix used for constructing service FQDNs. Defaults to "cluster.local". The full FQDN will be: <service>.<namespace>.svc.<clusterDomain> This is useful for clusters that use a different DNS suffix (e.g., "CLUSTER_ID.cluster.local", "edge.local"). type: string + pattern: '^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$' + maxLength: 253This follows RFC 1123 DNS label constraints and limits the total length to 253 characters (DNS FQDN limit).
| clusterDomain: | ||
| description: |- | ||
| ClusterDomain is the cluster domain suffix used for constructing service FQDNs. Defaults to "cluster.local". | ||
| The full FQDN will be: <service>.<namespace>.svc.<clusterDomain> | ||
| This is useful for clusters that use a different DNS suffix (e.g., "CLUSTER_ID.cluster.local", "edge.local"). | ||
| type: string |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check CRD API version and look for default usage patterns
# Check the API version of this CRD
echo "=== Checking API version in OLM catalog CRD ==="
head -20 deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yaml | grep apiVersion
# Check if the base CRD file exists
echo "=== Checking if base CRD files exist ==="
find . -name "*argoproj.io_argocds*" -type f 2>/dev/null | head -20
# Check for default usage in any CRD files
echo "=== Checking for 'default:' usage in CRD files ==="
rg -l "default:" --type yaml | grep -i crd | head -10
# Check the specific clusterDomain field in OLM catalog
echo "=== Checking clusterDomain field in OLM catalog CRD ==="
sed -n '11135,11150p' deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yamlRepository: argoproj-labs/argocd-operator
Length of output: 3214
🏁 Script executed:
#!/bin/bash
# Check if clusterDomain has default in base CRD files and verify controller code
echo "=== Checking clusterDomain in base CRD ==="
rg -n "clusterDomain" config/crd/bases/argoproj.io_argocds.yaml -A 5 -B 1
echo "=== Checking clusterDomain in bundle manifests ==="
rg -n "clusterDomain" bundle/manifests/argoproj.io_argocds.yaml -A 5 -B 1
echo "=== Searching for ArgoCDDefaultClusterDomain in code ==="
rg -r "ArgoCDDefaultClusterDomain\|cluster\.local" --type go | head -20
echo "=== Checking how defaults.go handles clusterDomain ==="
find . -name "defaults.go" -type f | xargs grep -l "clusterDomain\|Cluster.*Domain" 2>/dev/null
</parameter>Repository: argoproj-labs/argocd-operator
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Simpler queries to check clusterDomain defaults
echo "=== Checking clusterDomain in config/crd/bases ==="
grep -n "clusterDomain" config/crd/bases/argoproj.io_argocds.yaml -A 4 | head -15
echo "=== Checking if there is a default: near clusterDomain ==="
sed -n '9100,9150p' config/crd/bases/argoproj.io_argocds.yaml | grep -A 5 "clusterDomain"Repository: argoproj-labs/argocd-operator
Length of output: 721
🏁 Script executed:
#!/bin/bash
# Check if other optional fields in ArgoCDSpec use default values
echo "=== Looking for other optional string fields with defaults in ArgoCDSpec ==="
sed -n '10700,12000p' config/crd/bases/argoproj.io_argocds.yaml | grep -B 1 "default:" | head -30
echo "=== Count of default usage in ArgoCDSpec ==="
sed -n '10700,12000p' config/crd/bases/argoproj.io_argocds.yaml | grep -c "default:"Repository: argoproj-labs/argocd-operator
Length of output: 1092
🏁 Script executed:
#!/bin/bash
# Check if clusterDomain exists in earlier OLM versions
echo "=== Checking clusterDomain in OLM 0.15.0 ==="
grep -n "clusterDomain" deploy/olm-catalog/argocd-operator/0.15.0/argoproj.io_argocds.yaml -A 3 | head -10
echo "=== Checking clusterDomain in OLM 0.10.0 ==="
grep -n "clusterDomain" deploy/olm-catalog/argocd-operator/0.10.0/argoproj.io_argocds.yaml -A 3 | head -10
echo "=== Checking if clusterDomain field is new or existing ==="
for version in 0.15.0 0.14.0 0.10.0; do
if grep -q "clusterDomain" deploy/olm-catalog/argocd-operator/$version/argoproj.io_argocds.yaml 2>/dev/null; then
echo "Found in $version"
else
echo "Not found in $version"
fi
doneRepository: argoproj-labs/argocd-operator
Length of output: 281
Add default value to the clusterDomain schema.
The description states that clusterDomain defaults to "cluster.local", but the schema lacks a default: key. The CRD uses apiextensions.k8s.io/v1, which fully supports schema defaults. This pattern is already established in the codebase—16 other optional fields in ArgoCDSpec define defaults in the schema. Adding the default directly to the schema ensures the value is applied consistently and makes the specification explicit.
Suggested schema change
clusterDomain:
default: "cluster.local"
description: |-
ClusterDomain is the cluster domain suffix used for constructing service FQDNs. Defaults to "cluster.local".
The full FQDN will be: <service>.<namespace>.svc.<clusterDomain>
This is useful for clusters that use a different DNS suffix (e.g., "CLUSTER_ID.cluster.local", "edge.local").
type: string🤖 Prompt for AI Agents
In `@deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yaml` around
lines 11139 - 11144, The schema for ArgoCDSpec's clusterDomain field declares in
its description that it defaults to "cluster.local" but lacks a schema default;
update the clusterDomain entry in the CRD (the clusterDomain property under
ArgoCDSpec) to include default: "cluster.local" alongside the existing
description and type so the apiextensions.k8s.io/v1 schema will apply the
default value consistently.
|
Hi @olivergondza , thanks for the review, fixed comment and regenerated manifests. |
Fixes argoproj-labs#1149 Signed-off-by: Yonatan Sasson <yonatanxd7@gmail.com>
Signed-off-by: Yonatan Sasson <yonatanxd7@gmail.com>
Signed-off-by: Yonatan Sasson <yonatanxd7@gmail.com>
Signed-off-by: Yonatan Sasson <yonatanxd7@gmail.com>
Signed-off-by: Yonatan Sasson <yonatanxd7@gmail.com>
Yuni-sa
force-pushed
the
feat/configurable-cluster-domain-1149
branch
from
701b937 to
2310a0f
Compare
api/v1beta1/argocd_types.go
Outdated
| // ClusterDomain is the cluster domain suffix used for constructing service FQDNs. Defaults to "cluster.local". | ||
| // The full FQDN will be: <service>.<namespace>.svc.<clusterDomain> | ||
| // This is useful for clusters that use a different DNS suffix (e.g., "CLUSTER_ID.cluster.local", "edge.local"). | ||
| //+operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Cluster Domain'",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:text","urn:alm:descriptor:com.tectonic.ui:advanced"} |
There was a problem hiding this comment.
As @coderabbitai pointed out, mind the extra ' here - it bubbles to the manifest schema.
There was a problem hiding this comment.
@olivergondza, you're absolutely right! There's an extra apostrophe in the displayName annotation. It should be:
//+operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Cluster Domain",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:text","urn:alm:descriptor:com.tectonic.ui:advanced"}The trailing ' after "Cluster Domain" needs to be removed. Since this annotation is used to generate the ClusterServiceVersion manifests, the fix will need to be followed by regenerating the manifests to ensure the corrected display name propagates to all the CSV files.
olivergondza
left a comment
olivergondza
left a comment
There was a problem hiding this comment.
Apart from the typo, this looks good to me. Thanks!
2 participants
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
Removes the hardcoded cluster.local DNS suffix and makes it configurable so the operator works on clusters with custom DNS domains.
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #1149
How to test changes / Special notes to the reviewer:
Change default cluster domain on a kubernetes cluster or change coredns settings to do a rewrite from a custom domain to cluster.local.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.