[platform] Introduce cluster-domain option and unhardcode cozy.local - #1039
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces dynamic cluster domain configuration across multiple components by extracting the domain from the "cozystack" ConfigMap. The domain is injected into ClickHouse, NATS, Kubernetes, Keycloak, and several operator releases, replacing hardcoded values. Chart versions and the versions map are also updated to reflect these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HelmTemplate
participant K8sAPI
participant ConfigMap
participant Component
User->>HelmTemplate: Deploy chart
HelmTemplate->>K8sAPI: Get "cozystack" ConfigMap (cluster-domain)
K8sAPI->>ConfigMap: Retrieve cluster-domain value
ConfigMap-->>K8sAPI: Return value or default ("cozy.local")
K8sAPI-->>HelmTemplate: Provide cluster-domain
HelmTemplate->>Component: Inject cluster-domain into configuration
Component-->>User: Runs with correct domain settings
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml (1)
1-2: Duplicate: Handle missing ConfigMap to prevent runtime errors
Same as in other templates, wraplookupwithdefaultto ensure a non-nil.dataand avoid panics.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🧹 Nitpick comments (19)
packages/system/keycloak/templates/sts.yaml (1)
117-117: Refactor Java opts via printf: For clearer templating and consistent quoting, useprintfinstead of inline string concatenation.Example diff:
- value: "-Djgroups.dns.query=keycloak-headless.cozy-keycloak.svc.{{ $clusterDomain }}" + value: {{ printf "\"-Djgroups.dns.query=keycloak-headless.cozy-keycloak.svc.%s\"" $clusterDomain }}packages/apps/nats/templates/nats.yaml (1)
56-58: Quote dynamic cluster domain for YAML safety
When injecting{{ $clusterDomain }}, wrap the value in quotes to avoid parsing issues:- k8sClusterDomain: {{ $clusterDomain }} + k8sClusterDomain: "{{ $clusterDomain }}"packages/core/platform/bundles/distro-full.yaml (3)
17-21: Promote DRY: Extract cluster domain lookup
Repeated cluster-domain retrieval across bundles suggests centralizing this Helm logic into a shared helper (e.g.,{{ include "cozystack.clusterDomain" . }}) for maintainability.
130-133: Quote cluster domain value
Wrap{{ $clusterDomain }}in quotes to ensure valid YAML even if the domain contains special characters:- clusterName: {{ $clusterDomain }} + clusterName: "{{ $clusterDomain }}"
147-150: Quote cluster domain for YAML
For consistency and to avoid parsing issues, wrap the domain in quotes:- kubernetesServiceDnsDomain: {{ $clusterDomain }} + kubernetesServiceDnsDomain: "{{ $clusterDomain }}"packages/core/platform/bundles/distro-hosted.yaml (3)
17-21: Promote DRY: Extract cluster domain lookup
Centralize cluster-domain lookup into a shared Helm template helper (cozystack.clusterDomain) to avoid duplication across bundles.
92-95: Quote cluster domain in values
WrapclusterNamevalue in quotes for robust YAML:- clusterName: {{ $clusterDomain }} + clusterName: "{{ $clusterDomain }}"
109-112: Quote dynamic domain for YAML safety
Wrap the domain in quotes to avoid parsing issues:- kubernetesServiceDnsDomain: {{ $clusterDomain }} + kubernetesServiceDnsDomain: "{{ $clusterDomain }}"packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml (1)
18-18: Quote injected domain in Prometheus address
Wrap{{ $clusterDomain }}in quotes within the URL to maintain valid YAML:- prometheus-address: http://vmselect-shortterm.{{ $targetTenant }}.svc.{{ $clusterDomain }}:8481/select/0/prometheus/ + prometheus-address: "http://vmselect-shortterm.{{ $targetTenant }}.svc.{{ $clusterDomain }}:8481/select/0/prometheus/"packages/core/platform/bundles/paas-full.yaml (5)
2-2: Wrap$clusterDomaindefault in quotes
Unquoted template values containing dots can be misinterpreted by YAML parsers.
26-30: Quote injected cluster domain
Ensure valid YAML by wrapping the template in quotes:- domain: {{ $clusterDomain }} + domain: "{{ $clusterDomain }}"
202-205: QuoteclusterNamefor mariadb-operator
Wrap the injected domain to avoid parsing issues:- clusterName: {{ $clusterDomain }} + clusterName: "{{ $clusterDomain }}"
217-219: QuotekubernetesServiceDnsDomain
Add quotes around the template to maintain YAML validity:- kubernetesServiceDnsDomain: {{ $clusterDomain }} + kubernetesServiceDnsDomain: "{{ $clusterDomain }}"
385-389: Quote Prometheus address templating
Wrap the full URL (including the templated domain) in quotes:- prometheus-address: http://vmselect-shortterm.tenant-root.svc.{{ $clusterDomain }}:8481/select/0/prometheus/ + prometheus-address: "http://vmselect-shortterm.tenant-root.svc.{{ $clusterDomain }}:8481/select/0/prometheus/"packages/core/platform/bundles/paas-hosted.yaml (5)
2-2: Wrap$clusterDomaindefault in quotes
As above, quote template values containing dots to ensure proper YAML parsing.
26-30: Quote injected cluster domain- domain: {{ $clusterDomain }} + domain: "{{ $clusterDomain }}"
101-104: QuoteclusterNamefor mariadb-operator- clusterName: {{ $clusterDomain }} + clusterName: "{{ $clusterDomain }}"
116-119: QuotekubernetesServiceDnsDomain- kubernetesServiceDnsDomain: {{ $clusterDomain }} + kubernetesServiceDnsDomain: "{{ $clusterDomain }}"
221-225: Quote Prometheus address in vpa config- prometheus-address: http://vmselect-shortterm.tenant-root.svc.{{ $clusterDomain }}:8481/select/0/prometheus/ + prometheus-address: "http://vmselect-shortterm.tenant-root.svc.{{ $clusterDomain }}:8481/select/0/prometheus/"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/templates/clickhouse.yaml(2 hunks)packages/apps/kubernetes/Chart.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml(2 hunks)packages/apps/nats/Chart.yaml(1 hunks)packages/apps/nats/templates/nats.yaml(2 hunks)packages/apps/versions_map(3 hunks)packages/core/platform/bundles/distro-full.yaml(4 hunks)packages/core/platform/bundles/distro-hosted.yaml(4 hunks)packages/core/platform/bundles/paas-full.yaml(5 hunks)packages/core/platform/bundles/paas-hosted.yaml(5 hunks)packages/system/keycloak/templates/sts.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
packages/apps/nats/templates/nats.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/apps/clickhouse/templates/clickhouse.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (7)
packages/apps/kubernetes/Chart.yaml (1)
19-19: Bump chart version: Version increment to0.23.1aligns with the coordinated rollout of the dynamiccluster-domainfeature across platform charts.packages/apps/clickhouse/Chart.yaml (1)
19-19: Updated chart version: The bump to0.9.2corresponds to the template changes for dynamiccluster-domainresolution.packages/apps/nats/Chart.yaml (1)
19-19: Incremented chart version: Version updated to0.6.1in lockstep with the domain‐parameterization changes applied across other charts.packages/apps/clickhouse/templates/clickhouse.yaml (1)
37-37: Dynamic clusterDomain used: ThenamespaceDomainPatternnow correctly references the templatedcluster-domainvalue.packages/apps/versions_map (3)
73-73:Details
✅ Verification successful
Verify Chart.yaml bump for Kubernetes
Ensure that the Kubernetes chart version inpackages/apps/kubernetes/Chart.yamlhas been updated to0.23.1to match this mapping.
🏁 Script executed:
#!/bin/bash # Validate Kubernetes Chart.yaml version matches versions_map rg "^version:\\s*0\\.23\\.1" packages/apps/kubernetes/Chart.yamlLength of output: 79
Kubernetes Chart.yaml version matches versions_map
The Kubernetes chart version inpackages/apps/kubernetes/Chart.yamlis correctly set to0.23.1, matching the mapping inversions_map.
13-13:Details
✅ Verification successful
Verify Chart.yaml bump for ClickHouse
Ensure that the ClickHouse chart version inpackages/apps/clickhouse/Chart.yamlhas been updated to0.9.2to match this mapping.
🏁 Script executed:
#!/bin/bash # Validate ClickHouse Chart.yaml version matches versions_map rg "^version:\\s*0\\.9\\.2" packages/apps/clickhouse/Chart.yamlLength of output: 77
ClickHouse Chart.yaml version matches versions_map
Theversioninpackages/apps/clickhouse/Chart.yamlis correctly set to0.9.2, aligning withpackages/apps/versions_map. No further updates needed.
92-93:Details
✅ Verification successful
Verify NATS Chart.yaml version bump
Confirm that the NATS chart version inpackages/apps/nats/Chart.yamlhas been upgraded to0.6.1to align with this mapping.
🏁 Script executed:
#!/bin/bash # Validate NATS Chart.yaml version matches versions_map rg "^version:\\s*0\\.6\\.1" packages/apps/nats/Chart.yamlLength of output: 71
NATS Chart.yaml Version Verified
Theversionfield inpackages/apps/nats/Chart.yamlis set to 0.6.1, matching the entry inpackages/apps/versions_map.
- packages/apps/nats/Chart.yaml:
version: 0.6.1
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | ||
| {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing ConfigMap: Directly indexing .data on a nil lookup result will panic if the ConfigMap doesn’t exist. Wrap the lookup in a default (or a with block) to ensure .data is always a map.
Proposed diff:
- {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
- {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }}
+ {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" | default (dict "data" (dict)) }}
+ {{- $clusterDomain := index $cozyConfig.data "cluster-domain" | default "cozy.local" }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | |
| {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }} | |
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" | default (dict "data" (dict)) }} | |
| {{- $clusterDomain := index $cozyConfig.data "cluster-domain" | default "cozy.local" }} |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
In packages/apps/clickhouse/templates/clickhouse.yaml at lines 1 to 2, the code
directly accesses .data on the result of lookup, which can be nil if the
ConfigMap does not exist, causing a panic. To fix this, wrap the lookup call in
a default or with block to ensure that .data is always a valid map before
indexing it. This prevents runtime errors when the ConfigMap is missing.
| @@ -1,5 +1,6 @@ | |||
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | |||
| {{- $host := index $cozyConfig.data "root-host" }} | |||
| {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle missing ConfigMap: As with ClickHouse, a nil lookup causes a crash when indexing .data. Wrap $cozyConfig in a default to guarantee a .data map.
Example diff:
- {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }}
+ {{- $clusterDomain := index ($cozyConfig | default (dict "data" (dict))).data "cluster-domain" | default "cozy.local" }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }} | |
| {{- $clusterDomain := index ($cozyConfig | default (dict "data" (dict))).data "cluster-domain" | default "cozy.local" }} |
🤖 Prompt for AI Agents
In packages/system/keycloak/templates/sts.yaml at line 3, the code assigns
$clusterDomain by indexing $cozyConfig.data without ensuring $cozyConfig is not
nil, which can cause a crash. To fix this, wrap $cozyConfig in a default
function that provides an empty map if $cozyConfig is nil, ensuring that
indexing .data is safe and does not cause a runtime error.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | ||
| {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle missing ConfigMap to prevent runtime errors
lookup can return nil if the ConfigMap is absent, causing index to panic. Wrap the lookup call with a default to guarantee a non-nil .data map:
- {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
+ {{- $cozyConfig := default (dict "data" (dict)) (lookup "v1" "ConfigMap" "cozy-system" "cozystack") }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | |
| {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }} | |
| {{- $cozyConfig := default (dict "data" (dict)) (lookup "v1" "ConfigMap" "cozy-system" "cozystack") }} | |
| {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }} |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
In packages/apps/nats/templates/nats.yaml at lines 1 to 2, the lookup call for
the ConfigMap may return nil if the ConfigMap is missing, causing the subsequent
index call on .data to panic. To fix this, wrap the lookup call with a default
that provides an empty ConfigMap object or at least a non-nil .data map,
ensuring that the index function always operates on a valid map and preventing
runtime errors.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | ||
| {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle ConfigMap lookup nil case
Guard against a missing ConfigMap by defaulting to an empty data map:
- {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
- {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }}
+ {{- $cozyConfig := default (dict "data" (dict)) (lookup "v1" "ConfigMap" "cozy-system" "cozystack") }}
+ {{- $clusterDomain := default "cozy.local" (index $cozyConfig.data "cluster-domain") }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | |
| {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }} | |
| {{- $cozyConfig := default (dict "data" (dict)) (lookup "v1" "ConfigMap" "cozy-system" "cozystack") }} | |
| {{- $clusterDomain := default "cozy.local" (index $cozyConfig.data "cluster-domain") }} |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
In packages/core/platform/bundles/distro-full.yaml at lines 1 to 2, the code
does not handle the case where the ConfigMap lookup returns nil, which can cause
errors when accessing its data. Add a guard to check if the lookup result is nil
and default to an empty map for the data field before accessing
"cluster-domain". This ensures safe access and prevents runtime errors.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | ||
| {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle ConfigMap lookup nil case
Guard against a missing ConfigMap by defaulting to an empty data map:
- {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
- {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }}
+ {{- $cozyConfig := default (dict "data" (dict)) (lookup "v1" "ConfigMap" "cozy-system" "cozystack") }}
+ {{- $clusterDomain := default "cozy.local" (index $cozyConfig.data "cluster-domain") }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | |
| {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }} | |
| {{- $cozyConfig := default (dict "data" (dict)) (lookup "v1" "ConfigMap" "cozy-system" "cozystack") }} | |
| {{- $clusterDomain := default "cozy.local" (index $cozyConfig.data "cluster-domain") }} |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
In packages/core/platform/bundles/distro-hosted.yaml at lines 1 to 2, the lookup
for the ConfigMap "cozystack" in namespace "cozy-system" may return nil if the
ConfigMap is missing, which can cause errors when accessing its data. Add a
guard to check if the lookup result is nil and if so, default $cozyConfig to an
object with an empty data map to safely handle missing ConfigMap cases.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | ||
| {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for missing ConfigMap
You should guard the result of lookup before indexing .data, otherwise a missing cozystack ConfigMap will panic.
Consider adding right after line 1:
{{- if not $cozyConfig }}
{{- fail "ERROR: cozystack ConfigMap not found in namespace cozy-system" }}
{{- end }}🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
In packages/core/platform/bundles/paas-full.yaml at lines 1 to 2, the code uses
lookup to get the cozystack ConfigMap but does not check if the result is nil
before accessing .data, which can cause a panic if the ConfigMap is missing. To
fix this, add a conditional check right after line 1 to verify if $cozyConfig is
nil, and if so, call fail with an error message indicating the ConfigMap was not
found in the cozy-system namespace. This will prevent runtime errors by stopping
execution with a clear error when the ConfigMap is missing.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | ||
| {{- $clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local" }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for missing ConfigMap
Mirror the guard from paas-full.yaml to prevent panics when the ConfigMap is absent.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
In packages/core/platform/bundles/paas-hosted.yaml at lines 1 to 2, the code
assigns $cozyConfig without checking if the ConfigMap exists, which can cause
panics if it's missing. Add a validation guard similar to the one in
paas-full.yaml by checking if $cozyConfig is nil before accessing its data. If
$cozyConfig is nil, handle the case gracefully to prevent runtime errors.
Signed-off-by: Andrei Kvapil <[email protected]>
df7772b to
cee820e
Compare
1 participant
Signed-off-by: Andrei Kvapil [email protected]
Summary by CodeRabbit
New Features
Chores