[platform] Introduce expose-services, expose-ingress and expose-external-ips options #929
Conversation
WalkthroughThis change migrates ingress and exposure configuration from Helm values and resource annotations to centralized management via the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MigrationScript
participant HelmRelease (ingress)
participant ConfigMap (cozystack)
participant IngressTemplates
User->>MigrationScript: Run migration script
MigrationScript->>HelmRelease (ingress): Extract dashboard, cdi-uploadproxy, vm-exportproxy, externalIPs
MigrationScript->>ConfigMap (cozystack): Patch with expose-services, expose-external-ips
MigrationScript->>HelmRelease (ingress): Remove migrated keys, update chart version
MigrationScript->>ConfigMap (cozystack-version): Record new version
User->>IngressTemplates: Deploy or upgrade
IngressTemplates->>ConfigMap (cozystack): Read expose-services, expose-ingress, root-host, external-ips
IngressTemplates-->>IngressTemplates: Conditionally render ingress resources based on ConfigMap
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/system/kubevirt/templates/vm-exportproxy-ingress.yaml (1)
1-4: 🛠️ Refactor suggestionReapply missing ConfigMap fallback logic: As above,
lookupcan yieldnil. Implement adefaultfallback to avoid runtime errors:- {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} + {{- $cozyConfig := default (dict "data" (dict)) (lookup "v1" "ConfigMap" "cozy-system" "cozystack") }}
🧹 Nitpick comments (1)
packages/system/dashboard/templates/dashboard-ingress.yaml (1)
13-16: Simplify the emptycloudflarebranch
Theif eq $issuerType "cloudflare"branch is empty, which reduces readability. Invert the condition to remove the empty block:- {{- if eq $issuerType "cloudflare" }} - {{- else }} - acme.cert-manager.io/http01-ingress-class: {{ $exposeIngress }} - {{- end }} + {{- if ne $issuerType "cloudflare" }} + acme.cert-manager.io/http01-ingress-class: {{ $exposeIngress }} + {{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/extra/ingress/Chart.yaml(1 hunks)packages/extra/ingress/README.md(1 hunks)packages/extra/ingress/templates/cdi-uploadproxy.yaml(0 hunks)packages/extra/ingress/values.schema.json(0 hunks)packages/extra/ingress/values.yaml(0 hunks)packages/extra/ingress/vm-exportproxy.yaml(0 hunks)packages/extra/versions_map(1 hunks)packages/system/cozystack-api/templates/api-ingress.yaml(1 hunks)packages/system/dashboard/templates/dashboard-ingress.yaml(2 hunks)packages/system/keycloak-configure/templates/configure-kk.yaml(0 hunks)packages/system/keycloak/templates/ingress.yaml(2 hunks)packages/system/keycloak/templates/sts.yaml(0 hunks)packages/system/kubevirt-cdi/templates/cdi-uploadproxy-ingress.yaml(1 hunks)packages/system/kubevirt/templates/vm-exportproxy-ingress.yaml(1 hunks)
💤 Files with no reviewable changes (6)
- packages/system/keycloak-configure/templates/configure-kk.yaml
- packages/extra/ingress/values.yaml
- packages/extra/ingress/vm-exportproxy.yaml
- packages/extra/ingress/values.schema.json
- packages/system/keycloak/templates/sts.yaml
- packages/extra/ingress/templates/cdi-uploadproxy.yaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (16)
packages/extra/versions_map (1)
22-22: Ingress version bump to 1.6.0: Aligns with the Chart.yaml update and the structural changes removing service-specific toggles.packages/extra/ingress/Chart.yaml (1)
6-6: Upgrade ingress Chart version to 1.6.0: Matches the version mapping inpackages/extra/versions_map.packages/system/kubevirt-cdi/templates/cdi-uploadproxy-ingress.yaml (4)
7-9: Conditional creation based onexpose-services: Theifguard correctly ensures this ingress is rendered only when"cdi-uploadproxy"is in theexpose-serviceslist.
10-16: Verify metadata and annotations: Thename,namespace, and NGINX annotations (backend-protocol: HTTPS,ssl-passthrough) are correctly set. Ensure that the target namespacecozy-kubevirt-cdiexists before applying.
17-29: Ingress spec configuration looks correct:ingressClassName,rules.host, backendserviceName/port, andpathType: Prefixalign with expected routing behavior.
1-4: 🛠️ Refactor suggestionHandle missing
cozystackConfigMap:lookupmay returnnilif the ConfigMap doesn't exist, causing a panic when indexing. Consider usingdefaultfor a safe fallback:- {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} + {{- $cozyConfig := default (dict "data" (dict)) (lookup "v1" "ConfigMap" "cozy-system" "cozystack") }}⛔ Skipped due to learnings
Learnt from: lllamnyp PR: cozystack/cozystack#818 File: packages/core/platform/templates/configuration-hash.yaml:1-14 Timestamp: 2025-04-18T12:33:50.950Z Learning: In Helm templates for CozyStack, when using the pattern `index (lookup "v1" "ConfigMap" namespace name) "data"`, if the ConfigMap doesn't exist, the variable is set to null and the templating continues without errors, making explicit error handling unnecessary in this case.packages/system/kubevirt/templates/vm-exportproxy-ingress.yaml (3)
6-6: Conditional creation based onexpose-services: Good use ofhasto control rendering for"vm-exportproxy".
7-15: Validate metadata and annotations: Resourcename,namespace, and annotations are correctly specified. Confirmcozy-kubevirtis present.
16-27: ConfirmpathTypeselection: This ingress usesImplementationSpecificwhilecdi-uploadproxyusesPrefix. Please verify this is intentional and supported by your Ingress controller.packages/system/cozystack-api/templates/api-ingress.yaml (2)
13-14: Clarify Ingress resource naming and service target
The Ingress is namedkubernetesin thedefaultnamespace but only rendered when"api"is in the expose-services list. Confirm that thekubernetesservice indefaultis indeed the intended backend forapi.{{ $host }}, and that this name won’t collide with other Ingresses or Services.
11-12: Verify NGINX SSL passthrough configuration
You’ve added:nginx.ingress.kubernetes.io/backend-protocol: HTTPS nginx.ingress.kubernetes.io/ssl-passthrough: "true"but no
spec.tlsblock. Ensure your NGINX Ingress controller supports TLS passthrough without atls[]entry. If not, you may need:spec: tls: - hosts: - api.{{ $host }} secretName: <tls-secret>Also applies to: 16-27
packages/system/keycloak/templates/ingress.yaml (2)
15-16: Review Cloudflare issuer logic vs. ingressClassName
You skip the HTTP01 annotation whenclusterissuer=cloudflarebut still setspec.ingressClassNameunconditionally. Confirm this matches your DNS-01 flow with Cloudflare and that the Ingress class you inject will work for both issuers.Also applies to: 21-22
1-5: 🛠️ Refactor suggestionGuard against missing ConfigMap and default values
As with the API ingress, a missingcozystackConfigMap will cause a failure. Wrap your lookups indefaultor check that$cozyConfigis non-nil:- {{- $host := index $cozyConfig.data "root-host" }} + {{- $host := default "cozystack.local" (index $cozyConfig.data "root-host") }}⛔ Skipped due to learnings
Learnt from: lllamnyp PR: cozystack/cozystack#818 File: packages/core/platform/templates/configuration-hash.yaml:1-14 Timestamp: 2025-04-18T12:33:50.950Z Learning: In Helm templates for CozyStack, when using the pattern `index (lookup "v1" "ConfigMap" namespace name) "data"`, if the ConfigMap doesn't exist, the variable is set to null and the templating continues without errors, making explicit error handling unnecessary in this case.packages/system/dashboard/templates/dashboard-ingress.yaml (3)
21-23: Validate resource naming and namespace
The Ingress is nameddashboardin thecozy-dashboardnamespace. Ensure this follows your naming conventions and won’t collide with other Ingresses or Services in that namespace.
24-25: Approve dynamic ingressClassName injection
Using$exposeIngressforspec.ingressClassNamecorrectly unifies ingress class management via the ConfigMap.
36-39: Verify TLS section and secret existence
You introduced:tls: - hosts: - dashboard.{{ $host }} secretName: dashboard-tlsConfirm that the
dashboard-tlsSecret exists incozy-dashboard, and that the indentation aligns with the expected Kubernetes YAML structure.
| | Name | Description | Value | | ||
| | ---------------- | ----------------------- 8000 ------------------------------------------ | ------- | | ||
| | `replicas` | Number of ingress-nginx replicas | `2` | | ||
| | `externalIPs` | List of externalIPs for service. | `[]` | | ||
| | `whitelist` | List of client networks | `[]` | | ||
| | `clouflareProxy` | Restoring original visitor IPs when Cloudflare proxied is enabled | `false` | |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Typo in parameter name: The table lists clouflareProxy, but the description references "Cloudflare". For consistency and to avoid confusion, consider renaming this to cloudflareProxy in both documentation and code.
Please verify occurrences with:
🏁 Script executed:
#!/bin/bash
rg -n 'clouflareProxy' -A2 packages/extra/ingress/values.yaml
rg -n 'clouflareProxy' -A2 packages/extra/ingress/values.schema.jsonLength of output: 413
Fix typo in clouflareProxy parameter name
Please rename the misspelled parameter clouflareProxy to cloudflareProxy throughout the chart and its documentation to avoid confusion:
• In docs (packages/extra/ingress/README.md)
• In default values (packages/extra/ingress/values.yaml)
• In the JSON schema (packages/extra/ingress/values.schema.json)
Since this key is part of your chart’s public API, consider one of the following to avoid breaking existing configurations:
- Add
clouflareProxyas a deprecated alias that maps tocloudflareProxy, or - Bump the chart’s major version to signal the breaking change.
| {{- with .Values.ingress.annotations }} | ||
| annotations: | ||
| {{- if ne $issuerType "cloudflare" }} | ||
| acme.cert-manager.io/http01-ingress-class: {{ $ingress }} | ||
| acme.cert-manager.io/http01-ingress-class: {{ $exposeIngress }} | ||
| {{- end }} | ||
| cert-manager.io/cluster-issuer: letsencrypt-prod | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Always apply cert-manager annotations
Currently your cert-manager annotations are inside with .Values.ingress.annotations, so if users don’t set that block, cert-manager.io/cluster-issuer and the HTTP01 annotation are omitted. Extract them outside the with:
metadata:
name: keycloak-ingress
- {{- with .Values.ingress.annotations }}
- annotations:
- {{- if ne $issuerType "cloudflare" }}
- acme.cert-manager.io/http01-ingress-class: {{ $exposeIngress }}
- {{- end }}
- cert-manager.io/cluster-issuer: letsencrypt-prod
- {{- toYaml . | nindent 4 }}
- {{- end }}
+ annotations:
+ {{- if ne $issuerType "cloudflare" }}
+ acme.cert-manager.io/http01-ingress-class: {{ $exposeIngress }}
+ {{- end }}
+ cert-manager.io/cluster-issuer: letsencrypt-prod
+ {{- with .Values.ingress.annotations }}
+ {{ toYaml . | nindent 4 }}
+ {{- end }}There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/extra/ingress/templates/nginx-ingress.yaml(2 hunks)packages/extra/ingress/values.yaml(0 hunks)scripts/migrations/10(1 hunks)
💤 Files with no reviewable changes (1)
- packages/extra/ingress/values.yaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (4)
packages/extra/ingress/templates/nginx-ingress.yaml (2)
1-2: Centralized configuration approach using ConfigMapThese new variables shift the configuration source from Helm values to the centralized
cozystackConfigMap, making service exposure more consistent across the platform.However, I recommend adding comments to explain the purpose of these variables and their relationship to the broader configuration strategy.
36-38: Updated external IPs sourceThe conditional now correctly uses the centralized
$exposeExternalIPsvariable instead of the previously used.Values.externalIPs.scripts/migrations/10 (2)
4-17: Migration logic for centralizing configurationThe script effectively extracts service exposure flags and external IPs from the ingress HelmRelease and prepares them for the ConfigMap. This supports the centralized configuration approach.
28-32: Cleanup and version updateThe script properly removes the old configuration values from the HelmRelease and updates the chart version to match the new ingress chart.
The
|| trueafter each remove operation is a good practice to ensure the script doesn't fail if a field doesn't exist.
8979ca9 to
c2750ef
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (3)
scripts/migrations/12 (3)
28-31: Ensure HelmRelease exists before patching.The script attempts to patch the ingress HelmRelease without verifying its existence, which could lead to failures if the HelmRelease doesn't exist.
+ # Check if ingress HelmRelease exists + if kubectl get hr -n tenant-root ingress > /dev/null 2>&1; then kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/dashboard"}]' || true kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/cdiUploadProxy"}]' || true kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/virtExportProxy"}]' || true kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/externalIPs"}]' || true kubectl patch hr -n tenant-root ingress --type merge -p='{"spec":{"chart":{"spec":{"version":"1.6.0"}}}}' + else + echo "Warning: ingress HelmRelease not found in namespace tenant-root, skipping cleanup" + fi
36-36: Add error handling for version update.The final command to update the version ConfigMap lacks error handling.
-kubectl create configmap -n cozy-system cozystack-version --from-literal=version=13 --dry-run=client -o yaml | kubectl apply -f- +if ! kubectl create configmap -n cozy-system cozystack-version --from-literal=version=13 --dry-run=client -o yaml | kubectl apply -f- ; then + echo "Error: Failed to update cozystack-version ConfigMap" + exit 1 +fi +echo "Migration completed successfully"
1-37: Add comments explaining the migration purpose.Consider adding descriptive comments to explain what this migration does and why.
#!/bin/sh # Migration 12 --> 13 +# This migration script transfers ingress-related configuration from the HelmRelease +# resource named "ingress" in the "tenant-root" namespace to the centralized "cozystack" +# ConfigMap in the "cozy-system" namespace. It extracts service exposure flags and +# external IPs from the ingress HelmRelease and consolidates them into the ConfigMap. +# After migrating these values, it removes the now-obsolete keys from the ingress +# HelmRelease and updates the ingress Helm chart version.
🛑 Comments failed to post (3)
scripts/migrations/12 (3)
24-26:
⚠️ Potential issueInconsistent key name for exposed services.
There's a mismatch between variables and keys. The variable is named
expose_servicesbut you're storing it in "external-services" in line 25. This inconsistency could lead to configuration issues.- kubectl patch cm -n cozy-system cozystack --type merge -p="{\"data\":{\"external-services\":\"$expose_services\"}}" + kubectl patch cm -n cozy-system cozystack --type merge -p="{\"data\":{\"expose-services\":\"$expose_services\"}}"Also ensure that line 18 and 24 are using the same key name for consistency.
18-18:
⚠️ Potential issueWrong key used for retrieving existing exposed services.
This line attempts to retrieve existing exposed services but incorrectly uses "external-ips" instead of a key t 8000 hat would store the exposed services.
- existing_expose_services=$(kubectl get cm -n cozy-system cozystack -o go-template='{{ index .data "external-ips" }}') + existing_expose_services=$(kubectl get cm -n cozy-system cozystack -o go-template='{{ index .data "expose-services" }}')
5-33: 🛠️ Refactor suggestion
Add error handling and ConfigMap existence check.
The script tries to access the
cozystackConfigMap without first verifying its existence. Also, most kubectl commands lack error handling.Consider adding a check for the ConfigMap existence before attempting to access or modify it:
if kubectl get hr -n tenant-root tenant-root > /dev/null; then + # Check if cozystack ConfigMap exists + if ! kubectl get cm -n cozy-system cozystack > /dev/null 2>&1; then + echo "Error: cozystack ConfigMap not found in namespace cozy-system" + exit 1 + fi expose_services=$( kubectl get hr -n tenant-root ingress -o go-template='{{ with .spec }}{{ with .values }}{{ if .dashboard }}dashboard,{{ end }}{{ if .cdiUploadProxy }}cdi-uploadproxy,{{ end }}{{ if .virtExportProxy }}vm-exportproxy,{{ end }}{{ end }}{{ end }}' kubectl get cm -n cozy-system cozystack -o go-template='{{ if eq (index .data "oidc-enabled") "true" }}keycloak,{{ end }}' ) expose_services=$(echo "$expose_services" | awk '{sub(/,$/,""); print}') expose_external_ips=$( kubectl get hr -n tenant-root ingress -o go-template='{{ with .spec }}{{ with .values }}{{ if .externalIPs }}{{ range .externalIPs }}{{ . }},{{ end }}{{ end }}{{ end }}{{ end }}' ) expose_external_ips=$(echo "$expose_external_ips" | awk '{sub(/,$/,""); print}')Also add error handling for critical commands:
- kubectl patch cm -n cozy-system cozystack --type merge -p="{\"data\":{\"external-ips\":\"$expose_external_ips\"}}" + if ! kubectl patch cm -n cozy-system cozystack --type merge -p="{\"data\":{\"external-ips\":\"$expose_external_ips\"}}" ; then + echo "Error: Failed to update external-ips in cozystack ConfigMap" + exit 1 + fi📝 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.if kubectl get hr -n tenant-root tenant-root > /dev/null; then # Check if cozystack ConfigMap exists if ! kubectl get cm -n cozy-system cozystack > /dev/null 2>&1; then echo "Error: cozystack ConfigMap not found in namespace cozy-system" exit 1 fi expose_services=$( kubectl get hr -n tenant-root ingress -o go-template='{{ with .spec }}{{ with .values }}{{ if .dashboard }}dashboard,{{ end }}{{ if .cdiUploadProxy }}cdi-uploadproxy,{{ end }}{{ if .virtExportProxy }}vm-exportproxy,{{ end }}{{ end }}{{ end }}' kubectl get cm -n cozy-system cozystack -o go-template='{{ if eq (index .data "oidc-enabled") "true" }}keycloak,{{ end }}' ) expose_services=$(echo "$expose_services" | awk '{sub(/,$/,""); print}') expose_external_ips=$( kubectl get hr -n tenant-root ingress -o go-template='{{ with .spec }}{{ with .values }}{{ if .externalIPs }}{{ range .externalIPs }}{{ . }},{{ end }}{{ end }}{{ end }}{{ end }}' ) expose_external_ips=$(echo "$expose_external_ips" | awk '{sub(/,$/,""); print}') existing_expose_external_ips=$(kubectl get cm -n cozy-system cozystack -o go-template='{{ index .data "external-ips" }}') existing_expose_services=$(kubectl get cm -n cozy-system cozystack -o go-template='{{ index .data "external-ips" }}') if [ "$existing_expose_external_ips" == "<no value>" ]; then if ! kubectl patch cm -n cozy-system cozystack --type merge -p="{\"data\":{\"external-ips\":\"$expose_external_ips\"}}" ; then echo "Error: Failed to update external-ips in cozystack ConfigMap" exit 1 fi fi if [ "$existing_expose_services" == "<no value>" ]; then kubectl patch cm -n cozy-system cozystack --type merge -p="{\"data\":{\"external-services\":\"$expose_services\"}}" fi kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/dashboard"}]' || true kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/cdiUploadProxy"}]' || true kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/virtExportProxy"}]' || true kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/externalIPs"}]' || true kubectl patch hr -n tenant-root ingress --type merge -p='{"spec":{"chart":{"spec":{"version":"1.6.0"}}}}' fi
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/system/kubevirt-cdi/templates/cdi-uploadproxy-ingress.yaml (1)
1-4: Guard against missing ConfigMap and default valuesIf the
cozystackConfigMap isn't present at deployment time, trying to index$cozyConfig.datawill cause nil dereference errors during template rendering. Add default values and error checks:{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} -{{- $host := index $cozyConfig.data "root-host" }} -{{- $exposeServices := splitList "," ((index $cozyConfig.data "expose-services") | default "") }} -{{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }} +{{- $host := default "cozystack.local" (dig "data" "root-host" "" $cozyConfig) }} +{{- $exposeServices := splitList "," (default "" (dig "data" "expose-services" "" $cozyConfig)) }} +{{- $exposeIngress := default "tenant-root" (dig "data" "expose-ingress" "" $cozyConfig) }}🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/kubevirt/templates/vm-exportproxy-ingress.yaml (1)
1-4: Guard against missing ConfigMap and default valuesIf the
cozystackConfigMap isn't present at deployment time, trying to index$cozyConfig.datawill cause nil dereference errors during template rendering. Add default values and error checks:{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} -{{- $host := index $cozyConfig.data "root-host" }} -{{- $exposeServices := splitList "," ((index $cozyConfig.data "expose-services") | default "") }} -{{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }} +{{- $host := default "cozystack.local" (dig "data" "root-host" "" $cozyConfig) }} +{{- $exposeServices := splitList "," (default "" (dig "data" "expose-services" "" $cozyConfig)) }} +{{- $exposeIngress := default "tenant-root" (dig "data" "expose-ingress" "" $cozyConfig) }}🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/cozystack-api/templates/api-ingress.yaml (1)
1-4: Guard against missing ConfigMap and default valuesIf the
cozystackConfigMap isn't present at deployment time, trying to index$cozyConfig.datawill cause nil dereference errors during template rendering. Add default values and error checks:{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} -{{- $host := index $cozyConfig.data "root-host" }} -{{- $exposeServices := splitList "," ((index $cozyConfig.data "expose-services") | default "") }} -{{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }} +{{- $host := default "cozystack.local" (dig "data" "root-host" "" $cozyConfig) }} +{{- $exposeServices := splitList "," (default "" (dig "data" "expose-services" "" $cozyConfig)) }} +{{- $exposeIngress := default "tenant-root" (dig "data" "expose-ingress" "" $cozyConfig) }}🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🧹 Nitpick comments (2)
packages/system/kubevirt/templates/vm-exportproxy-ingress.yaml (1)
27-27: Inconsistent pathType between ingress resourcesThis template uses
pathType: ImplementationSpecificwhile other similar templates (like cdi-uploadproxy-ingress.yaml) usepathType: Prefix. Consider standardizing for consistency unless there's a specific reason for the difference.scripts/migrations/12 (1)
27-32: Consider error checking or verbose outputThe script uses
|| trueto ignore errors when removing fields, but it would be helpful to provide some verbose output about what's happening and if any issues were encountered.+ echo "Removing deprecated fields from ingress HelmRelease..." kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/dashboard"}]' || true kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/cdiUploadProxy"}]' || true kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/virtExportProxy"}]' || true kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/externalIPs"}]' || true + echo "Updating ingress chart version to 1.6.0..." kubectl patch hr -n tenant-root ingress --type merge -p='{"spec":{"chart":{"spec":{"version":"1.6.0"}}}}'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (17)
hack/e2e.sh(1 hunks)packages/extra/ingress/Chart.yaml(1 hunks)packages/extra/ingress/README.md(1 hunks)packages/extra/ingress/templates/cdi-uploadproxy.yaml(0 hunks)packages/extra/ingress/templates/nginx-ingress.yaml(2 hunks)packages/extra/ingress/values.schema.json(0 hunks)packages/extra/ingress/values.yaml(0 hunks)packages/extra/ingress/vm-exportproxy.yaml(0 hunks)packages/extra/versions_map(1 hunks)packages/system/cozystack-api/templates/api-ingress.yaml(1 hunks)packages/system/dashboard/templates/dashboard-ingress.yaml(2 hunks)packages/system/keycloak-configure/templates/configure-kk.yaml(0 hunks)packages/system/keycloak/templates/ingress.yaml(2 hunks)packages/system/keycloak/templates/sts.yaml(0 hunks)packages/system/kubevirt-cdi/templates/cdi-uploadproxy-ingress.yaml(1 hunks)packages/system/kubevirt/templates/vm-exportproxy-ingress.yaml(1 hunks)scripts/migrations/12(1 hunks)
💤 Files with no reviewable changes (6)
- packages/system/keycloak-configure/templates/configure-kk.yaml
- packages/extra/ingress/values.yaml
- packages/system/keycloak/templates/sts.yaml
- packages/extra/ingress/templates/cdi-uploadproxy.yaml
- packages/extra/ingress/values.schema.json
- packages/extra/ingress/vm-exportproxy.yaml
✅ Files skipped from review due to trivial changes (2)
- packages/extra/versions_map
- packages/extra/ingress/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/extra/ingress/Chart.yaml
- hack/e2e.sh
- packages/system/keycloak/templates/ingress.yaml
- packages/system/dashboard/templates/dashboard-ingress.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/extra/ingress/templates/nginx-ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/cozystack-api/templates/api-ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/kubevirt-cdi/templates/cdi-uploadproxy-ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/kubevirt/templates/vm-exportproxy-ingress.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 and Test
🔇 Additional comments (6)
packages/system/kubevirt-cdi/templates/cdi-uploadproxy-ingress.yaml (1)
7-29: LGTM - Conditional ingress creation based on ConfigMapThe conditional creation of the Ingress resource based on the exposed services list is a good approach for centralized configuration management.
packages/system/cozystack-api/templates/api-ingress.yaml (1)
13-14: Is using "kubernetes" in default namespace intentional?This ingress is named "kubernetes" in the "default" namespace, which seems unusual compared to the other ingresses that have more specific names in dedicated namespaces. Verify that this is intentional as it could potentially conflict with other resources in the default namespace.
packages/extra/ingress/templates/nginx-ingress.yaml (1)
36-38: LGTM - Conditional externalIPs based on ConfigMapThe conditional setting of
externalIPsbased on the centralized configuration in thecozystackConfigMap is a good approach.scripts/migrations/12 (3)
1-15: LGTM - Comprehensive migration scriptThe script effectively extracts service exposure flags and external IPs from the ingress HelmRelease and transfers them to the centralized
cozystackConfigMap. The string manipulation to ensure proper comma-separated format is also well handled.
16-25: Correctly preserves existing valuesThe script checks if values already exist in the ConfigMap before updating, which prevents overwriting user-defined configurations during migration.
34-35: LGTM - Version updateProperly updates the version ConfigMap to reflect the migration.
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/system/kubevirt/templates/vm-exportproxy-ingress.yaml (1)
1-4: 🛠️ Refactor suggestionGuard against missing ConfigMap or invalid values
Similar to the cdi-uploadproxy ingress, this template doesn't handle cases where the ConfigMap might be missing or contain invalid data. Add proper error handling to prevent rendering failures.
Apply this diff to add proper error handling:
{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} -{{- $host := index $cozyConfig.data "root-host" }} -{{- $exposeServices := splitList "," ((index $cozyConfig.data "expose-services") | default "") }} -{{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }} +{{- if not $cozyConfig }} +{{- $host := "cozystack.local" }} +{{- $exposeServices := list }} +{{- $exposeIngress := "tenant-root" }} +{{- else }} +{{- $host := default "cozystack.local" (index $cozyConfig.data "root-host" | default "") }} +{{- $exposeServices := splitList "," ((index $cozyConfig.data "expose-services") | default "") }} +{{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }} +{{- end }}🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/extra/ingress/templates/nginx-ingress.yaml (1)
1-2:⚠️ Potential issueMissing $cozyConfig initialization
The template attempts to use
$cozyConfigwithout initializing it first. This will cause the template to fail during rendering. You must add the lookup statement to initialize the variable.Apply this diff to initialize the missing variable:
+{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} {{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }} {{- $exposeExternalIPs := splitList "," ((index $cozyConfig.data "expose-external-ips") | default "") }}🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/cozystack-api/templates/api-ingress.yaml (1)
1-4: 🛠️ Refactor suggestionGuard against missing ConfigMap or invalid values
Similar to other ingress templates, this file doesn't handle cases where the ConfigMap might be missing or contain invalid data. Add proper safeguards to prevent rendering failures.
Apply this diff to add proper error handling:
{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} -{{- $host := index $cozyConfig.data "root-host" }} -{{- $exposeServices := splitList "," ((index $cozyConfig.data "expose-services") | default "") }} -{{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }} +{{- if not $cozyConfig }} +{{- $host := "cozystack.local" }} +{{- $exposeServices := list }} +{{- $exposeIngress := "tenant-root" }} +{{- else }} +{{- $host := default "cozystack.local" (index $cozyConfig.data "root-host" | default "") }} +{{- $exposeServices := splitList "," ((index $cozyConfig.data "expose-services") | default "") }} +{{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }} +{{- end }}🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🧹 Nitpick comments (3)
packages/system/kubevirt-cdi/templates/cdi-uploadproxy-ingress.yaml (1)
16-28: Consider adding TLS configurationThe ingress is configured for SSL passthrough, but doesn't include explicit TLS configuration, which is typically used to define certificates for secure connections. Consider adding a TLS spec section if certificate management is required.
If you're using cert-manager or have certificates available, consider adding TLS configuration:
ingressClassName: {{ $exposeIngress }} rules: - host: cdi-uploadproxy.{{ $host }} http: paths: - backend: service: name: cdi-uploadproxy port: number: 443 path: / p A3DB athType: Prefix + tls: + - hosts: + - cdi-uploadproxy.{{ $host }} + secretName: cdi-uploadproxy-tlspackages/system/cozystack-api/templates/api-ingress.yaml (1)
13-14: Clarify ingress namingThe ingress is named "kubernetes" which might be confusing since it's specifically for the API service. Consider renaming it to "api" or "kubernetes-api" to better reflect its purpose, ensuring consistency with the service name if needed.
- name: kubernetes + name: kubernetes-api namespace: defaultscripts/migrations/12 (1)
35-35: Add migration status outputThe script would benefit from status output to help operators understand what was done during the migration.
# Write version to cozystack-version config +echo "Migration to version 13 complete" kubectl create configmap -n cozy-system cozystack-version --from-literal=version=13 --dry-run=client -o yaml | kubectl apply -f-
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (17)
hack/e2e.sh(1 hunks)packages/extra/ingress/Chart.yaml(1 hunks)packages/extra/ingress/README.md(1 hunks)packages/extra/ingress/templates/cdi-uploadproxy.yaml(0 hunks)packages/extra/ingress/templates/nginx-ingress.yaml(2 hunks)packages/extra/ingress/values.schema.json(0 hunks)packages/extra/ingress/values.yaml(0 hunks)packages/extra/ingress/vm-exportproxy.yaml(0 hunks)packages/extra/versions_map(1 hunks)packages/system/cozystack-api/templates/api-ingress.yaml(1 hunks)packages/system/dashboard/templates/dashboard-ingress.yaml(2 hunks)packages/system/keycloak-configure/templates/configure-kk.yaml(0 hunks)packages/system/keycloak/templates/ingress.yaml(2 hunks)packages/system/keycloak/templates/sts.yaml(0 hunks)packages/system/kubevirt-cdi/templates/cdi-uploadproxy-ingress.yaml(1 hunks)packages/system/kubevirt/templates/vm-exportproxy-ingress.yaml(1 hunks)scripts/migrations/12(1 hunks)
💤 Files with no reviewable changes (6)
- packages/system/keycloak/templates/sts.yaml
- packages/extra/ingress/values.yaml
- packages/system/keycloak-configure/templates/configure-kk.yaml
- packages/extra/ingress/vm-exportproxy.yaml
- packages/extra/ingress/values.schema.json
- packages/extra/ingress/templates/cdi-uploadproxy.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/extra/versions_map
- packages/extra/ingress/Chart.yaml
- packages/extra/ingress/README.md
- packages/system/keycloak/templates/ingress.yaml
- hack/e2e.sh
- packages/system/dashboard/templates/dashboard-ingress.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/extra/ingress/templates/nginx-ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/cozystack-api/templates/api-ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/kubevirt-cdi/templates/cdi-uploadproxy-ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/kubevirt/templates/vm-exportproxy-ingress.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 and Test
🔇 Additional comments (1)
packages/system/kubevirt/templates/vm-exportproxy-ingress.yaml (1)
27-27:❓ Verification inconclusive
Inconsistent pathType between ingress templates
This ingress uses
pathType: ImplementationSpecificwhile the cdi-uploadproxy ingress usespathType: Prefix. Ensure this difference is intentional and required by the specific service behavior.Verify whether the different pathType values are deliberate or if they should be standardized:
🏁 Script executed:
#!/bin/bash # Check if other ingress resources use consistent pathType values echo "Checking pathType values in other ingress templates:" fd --type f --extension yaml --extension yml ingress.yaml --exec grep -l pathType {} \; | \ xargs grep -A 1 "pathType:" | grep -v "^\-\-"Length of output: 3689
Double-check intentional pathType choice for vm-exportproxy ingress
A scan of other ingress templates shows a mix of values:
- ImplementationSpecific (seaweedfs s3/filer/master, apps/kubernetes, matchbox, and vm-exportproxy here)
- Prefix (keycloak, cdi-uploadproxy, cozystack-api, dashboard, fluent-bit, bucket, cilium/hubble-ui, opencost, etc.)
- Dynamic (
{{ .Values.ingress.pathType }}in nats, goldpinger, kubeapps)Ensure that
ImplementationSpecificis required for vm-exportproxy’s path matching under your Ingress controller. If standard prefix-based matching suffices (or is preferred for consistency), switch this topathType: Prefix.• File: packages/system/kubevirt/templates/vm-exportproxy-ingress.yaml (line 27)
• Compare with Prefix usage in packages/system/kubevirt-cdi/templates/cdi-uploadproxy-ingress.yaml and others above.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | ||
| {{- $host := index $cozyConfig.data "root-host" }} | ||
| {{- $exposeServices := splitList "," ((index $cozyConfig.data "expose-services") | default "") }} | ||
| {{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing ConfigMap or invalid values
The template assumes the cozystack ConfigMap exists and contains the expected keys. If the ConfigMap doesn't exist or is missing keys, this will cause template rendering errors. Add proper safeguards to handle these cases gracefully.
Apply this diff to add proper error handling:
{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
-{{- $host := index $cozyConfig.data "root-host" }}
-{{- $exposeServices := splitList "," ((index $cozyConfig.data "expose-services") | default "") }}
-{{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }}
+{{- if not $cozyConfig }}
+{{- $host := "cozystack.local" }}
+{{- $exposeServices := list }}
+{{- $exposeIngress := "tenant-root" }}
+{{- else }}
+{{- $host := default "cozystack.local" (index $cozyConfig.data "root-host" | default "") }}
+{{- $exposeServices := splitList "," ((index $cozyConfig.data "expose-services") | default "") }}
+{{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }}
+{{- end }}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
In packages/system/kubevirt-cdi/templates/cdi-uploadproxy-ingress.yaml lines 1
to 4, the template assumes the 'cozystack' ConfigMap exists and contains the
keys 'root-host', 'expose-services', and 'expose-ingress'. To fix this, add
checks to verify that the ConfigMap was successfully retrieved before accessing
its data, and provide default values or fallback logic if the ConfigMap or any
keys are missing. Use conditional statements to guard against nil or empty
values to prevent template rendering errors.
| if kubectl get hr -n tenant-root tenant-root > /dev/null; then | ||
| expose_services=$( | ||
| kubectl get hr -n tenant-root ingress -o go-template='{{ with .spec }}{{ with .values }}{{ if .dashboard }}dashboard,{{ end }}{{ if .cdiUploadProxy }}cdi-uploadproxy,{{ end }}{{ if .virtExportProxy }}vm-exportproxy,{{ end }}{{ end }}{{ end }}' | ||
| ) | ||
| expose_services=$(echo "$expose_services" | awk '{sub(/,$/,""); print}') | ||
|
|
||
| expose_external_ips=$( | ||
| kubectl get hr -n tenant-root ingress -o go-template='{{ with .spec }}{{ with .values }}{{ if .externalIPs }}{{ range .externalIPs }}{{ . }},{{ end }}{{ end }}{{ end }}{{ end }}' | ||
| ) | ||
| expose_external_ips=$(echo "$expose_external_ips" | awk '{sub(/,$/,""); print}') | ||
|
|
||
| existing_expose_external_ips=$(kubectl get cm -n cozy-system cozystack -o go-template='{{ index .data "expose-external-ips" }}') | ||
| existing_expose_services=$(kubectl get cm -n cozy-system cozystack -o go-template='{{ index .data "expose-services" }}') | ||
|
|
||
| if [ "$existing_expose_external_ips" == "<no value>" ]; then | ||
| kubectl patch cm -n cozy-system cozystack --type merge -p="{\"data\":{\ F438 "expose-external-ips\":\"$expose_external_ips\"}}" | ||
| fi | ||
|
|
||
| if [ "$existing_expose_services" == "<no value>" ]; then | ||
| kubectl patch cm -n cozy-system cozystack --type merge -p="{\"data\":{\"expose-services\":\"$expose_services\"}}" | ||
| fi | ||
|
|
||
| kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/dashboard"}]' || true | ||
| kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/cdiUploadProxy"}]' || true | ||
| kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/virtExportProxy"}]' || true | ||
| kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/externalIPs"}]' || true | ||
| kubectl patch hr -n tenant-root ingress --type merge -p='{"spec":{"chart":{"spec":{"version":"1.6.0"}}}}' | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add script validation and handling for empty service lists
The current implementation doesn't handle the case where all services might be disabled (no boolean flags set to true). It also doesn't validate the migration results.
Consider adding validation checks and handling for empty service lists:
#!/bin/sh
# Migration 12 --> 13
+ set -e # Exit on error
# Copy configuration from ingress to cozystack configmap
if kubectl get hr -n tenant-root tenant-root > /dev/null; then
expose_services=$(
kubectl get hr -n tenant-root ingress -o go-template='{{ with .spec }}{{ with .values }}{{ if .dashboard }}dashboard,{{ end }}{{ if .cdiUploadProxy }}cdi-uploadproxy,{{ end }}{{ if .virtExportProxy }}vm-exportproxy,{{ end }}{{ end }}{{ end }}'
)
expose_services=$(echo "$expose_services" | awk '{sub(/,$/,""); print}')
expose_external_ips=$(
kubectl get hr -n tenant-root ingress -o go-template='{{ with .spec }}{{ with .values }}{{ if .externalIPs }}{{ range .externalIPs }}{{ . }},{{ end }}{{ end }}{{ end }}{{ end }}'
)
expose_external_ips=$(echo "$expose_external_ips" | awk '{sub(/,$/,""); print}')
existing_expose_external_ips=$(kubectl get cm -n cozy-system cozystack -o go-template='{{ index .data "expose-external-ips" }}')
existing_expose_services=$(kubectl get cm -n cozy-system cozystack -o go-template='{{ index .data "expose-services" }}')
if [ "$existing_expose_external_ips" == "<no value>" ]; then
kubectl patch cm -n cozy-system cozystack --type merge -p="{\"data\":{\"expose-external-ips\":\"$expose_external_ips\"}}"
fi
if [ "$existing_expose_services" == "<no value>" ]; then
kubectl patch cm -n cozy-system cozystack --type merge -p="{\"data\":{\"expose-services\":\"$expose_services\"}}"
fi
+ # Validate the migration was successful
+ echo "Verifying migration..."
+ migrated_services=$(kubectl get cm -n cozy-system cozystack -o go-template='{{ index .data "expose-services" }}')
+ if [ "$existing_expose_services" == "<no value>" ] && [ -z "$migrated_services" ] && [ ! -z "$expose_services" ]; then
+ echo "Warning: Failed to migrate services list to ConfigMap"
+ exit 1
+ fi
kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/dashboard"}]' || true
kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/cdiUploadProxy"}]' || true
kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/virtExportProxy"}]' || true
kubectl patch hr -n tenant-root ingress --type json -p='[{"op": "remove", "path": "/spec/values/externalIPs"}]' || true
kubectl patch hr -n tenant-root ingress --type merge -p='{"spec":{"chart":{"spec":{"version":"1.6.0"}}}}'
fi🤖 Prompt for AI Agents
In scripts/migrations/12 around lines 5 to 32, the script lacks validation for
empty service lists when all service flags are false and does not verify the
success of migration commands. Add checks after retrieving expose_services and
expose_external_ips to handle empty values gracefully, such as skipping patches
or setting defaults. Also, include validation to confirm that kubectl patch
commands succeed, and handle failures appropriately to ensure migration
reliability.
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
packages/extra/ingress/templates/nginx-ingress.yaml(2 hunks)packages/extra/ingress/values.yaml(0 hunks)scripts/migrations/12(1 hunks)
💤 Files with no reviewable changes (1)
- packages/extra/ingress/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/migrations/12
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/extra/ingress/templates/nginx-ingress.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 and Test
| {{- if and (eq $exposeIngress .Release.Namespace) $exposeExternalIPs }} | ||
| externalIPs: | ||
| {{- toYaml .Values.externalIPs | nindent 12 }} | ||
| {{- toYaml $exposeExternalIPs | nindent 12 }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a guard for $cozyConfig in the externalIPs conditional.
The current check may still fail or misbehave if $cozyConfig is undefined or returns no data. Strengthen the conditional to verify both the ConfigMap existence and that there is at least one valid IP:
- {{- if and (eq $exposeIngress .Release.Namespace) $exposeExternalIPs }}
+ {{- if and $cozyConfig (eq $exposeIngress .Release.Namespace) (gt (len $exposeExternalIPs) 0) }}This makes the template safe against missing ConfigMaps and empty IP lists.
📝 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.
| {{- if and (eq $exposeIngress .Release.Namespace) $exposeExternalIPs }} | |
| externalIPs: | |
| {{- toYaml .Values.externalIPs | nindent 12 }} | |
| {{- toYaml $exposeExternalIPs | nindent 12 }} | |
| {{- if and $cozyConfig (eq $exposeIngress .Release.Namespace) (gt (len $exposeExternalIPs) 0) }} | |
| externalIPs: | |
| {{- toYaml $exposeExternalIPs | nindent 12 }} |
🤖 Prompt for AI Agents
In packages/extra/ingress/templates/nginx-ingress.yaml around lines 37 to 39,
the conditional rendering externalIPs lacks a guard for $cozyConfig, which can
cause errors if $cozyConfig is undefined or empty. Update the if condition to
also check that $cozyConfig exists and contains at least one valid IP before
rendering externalIPs, ensuring the template handles missing ConfigMaps or empty
IP lists safely.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | ||
| {{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }} | ||
| {{- $exposeExternalIPs := splitList "," ((index $cozyConfig.data "expose-external-ips") | default "") }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure robust handling for missing ConfigMap and empty external IPs.
While $cozyConfig is now initialized, if the ConfigMap doesn’t exist, .data will be nil and index will error at runtime. Also, splitting an empty string yields [""], which is truthy and generates an invalid externalIPs: [""] entry. Consider defaulting the lookup to an empty data map and filtering out empty items:
- {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
+ {{- $cozyConfig := (lookup "v1" "ConfigMap" "cozy-system" "cozystack") | default (dict "data" (dict)) }}
- {{- $exposeExternalIPs := splitList "," ((index $cozyConfig.data "expose-external-ips") | default "") }}
+ {{- $exposeExternalIPs := splitList "," ((index $cozyConfig.data "expose-external-ips") | default "") | compact }}This guarantees cozyConfig.data is always a map and removes empty-string entries from the IP list.
📝 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" }} | |
| {{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }} | |
| {{- $exposeExternalIPs := splitList "," ((index $cozyConfig.data "expose-external-ips") | default "") }} | |
| {{- $cozyConfig := (lookup "v1" "ConfigMap" "cozy-system" "cozystack") | default (dict "data" (dict)) }} | |
| {{- $exposeIngress := index $cozyConfig.data "expose-ingress" | default "tenant-root" }} | |
| {{- $exposeExternalIPs := splitList "," ((index $cozyConfig.data "expose-external-ips") | default "") | compact }} |
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
In packages/extra/ingress/templates/nginx-ingress.yaml lines 1 to 3, the code
does not safely handle the case when the ConfigMap "cozystack" in "cozy-system"
namespace is missing, causing .data to be nil and index calls to fail. Also,
splitting an empty string results in a list with an empty string element, which
leads to invalid externalIPs entries. To fix this, modify the lookup to default
to an empty map if the ConfigMap is missing, ensure cozyConfig.data is always a
map, and filter out empty strings from the exposeExternalIPs list after
splitting to avoid invalid entries.
Fix regression introduced by #929 (comment) becasue of `splitList "," "" == [""]` Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…nal-ips options (#929) docs update: cozystack/website#197 Signed-off-by: Andrei Kvapil <kvapss@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Added automated migration script to transition configuration from HelmRelease to ConfigMap for service exposure and external IPs. - Introduced new ingress templates for API, CDI upload proxy, and VM export proxy services, enabling dynamic exposure based on centralized configuration. - **Bug Fixes** - Updated NGINX Ingress Controller Helm chart version to 1.6.0. - **Refactor** - Centralized ingress configuration using a ConfigMap, simplifying and unifying service exposure and ingress class management. - Removed legacy parameters and templates for dashboard, CDI upload proxy, and VM export proxy from values and schema files. - Simplified ingress templates for dashboard and Keycloak to rely on centralized ConfigMap data and exposure lists. - Adjusted ingress controller service to conditionally use external IPs based on centralized configuration. - **Documentation** - Updated documentation to reflect the removal of deprecated parameters and clarify current configuration options. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit f8210cf) S B588 igned-off-by: Timofei Larkin <lllamnyp@gmail.com>
Fix regression introduced by #929 (comment) becasue of `splitList "," "" == [""]` Signed-off-by: Andrei Kvapil <kvapss@gmail.com> (cherry picked from commit 0369852) Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
docs update: cozystack/website#197
Signed-off-by: Andrei Kvapil kvapss@gmail.com
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation