[feature] Add templating for kubespan and allowSchedulingOnControlPla… #69
Conversation
…nes in values.yaml
WalkthroughThe template logic in the Helm chart was updated to conditionally include or configure Changes
Poem
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: 1
🧹 Nitpick comments (1)
charts/cozystack/templates/_helpers.tpl (1)
92-92: Good boolean handling, but consider default value protection.The explicit boolean rendering is correct for Helm templates. However, consider adding a default value or checking for existence to handle undefined values gracefully.
Consider this pattern for safer default handling:
-allowSchedulingOnControlPlanes: {{ if eq .Values.allowSchedulingOnControlPlanes true }}true{{ else }}false{{ end }} +allowSchedulingOnControlPlanes: {{ if .Values.allowSchedulingOnControlPlanes | default false }}true{{ else }}false{{ end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
charts/cozystack/templates/_helpers.tpl(3 hunks)
🔇 Additional comments (2)
charts/cozystack/templates/_helpers.tpl (2)
59-62: LGTM! Well-structured conditional kubespan configuration.The conditional logic correctly includes the kubespan section only when explicitly enabled. The explicit boolean comparison and proper YAML indentation are good practices.
115-115: Logical coupling between kubespan and discovery is appropriate.The conditional logic correctly links cluster discovery enablement to kubespan configuration, which makes architectural sense since these features work together.
| {{- if eq .Values.kubespan true }} | ||
| kubespan: | ||
| enabled: true | ||
| {{- end }} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify values.yaml contains the new configuration options.
The template now references .Values.kubespan and .Values.allowSchedulingOnControlPlanes which must be defined in values.yaml to prevent template rendering errors.
Also applies to: 92-92, 115-115
🏁 Script executed:
#!/bin/bash
# Verify that the new Helm values are defined in values.yaml
echo "Checking for kubespan configuration in values.yaml..."
if fd -t f -e yaml values.yaml | xargs grep -l "kubespan" > /dev/null 2>&1; then
echo "✓ kubespan found in values.yaml"
fd -t f -e yaml values.yaml | xargs grep -A 2 -B 2 "kubespan"
else
echo "⚠ kubespan not found in values.yaml"
fi
echo -e "\nChecking for allowSchedulingOnControlPlanes configuration in values.yaml..."
if fd -t f -e yaml values.yaml | xargs grep -l "allowSchedulingOnControlPlanes" > /dev/null 2>&1; then
echo "✓ allowSchedulingOnControlPlanes found in values.yaml"
fd -t f -e yaml values.yaml | xargs grep -A 2 -B 2 "allowSchedulingOnControlPlanes"
else
echo "⚠ allowSchedulingOnControlPlanes not found in values.yaml"
fi
echo -e "\nChecking for any default values that might be missing..."
rg -A 5 -B 5 "kubespan|allowSchedulingOnControlPlanes" --type yamlLength of output: 1997
Add missing default values for kubespan and allowSchedulingOnControlPlanes
The Helm template now references .Values.kubespan and .Values.allowSchedulingOnControlPlanes, but neither key is defined in your values.yaml. This will cause rendering errors when installing or upgrading the chart.
Please update charts/cozystack/values.yaml to include defaults, for example:
# charts/cozystack/values.yaml
# Enable Kubespan integration
kubespan: false
# Allow pods to be scheduled on control-plane nodes
allowSchedulingOnControlPlanes: false• Location of references in the template:
- lines 59–62 (
.Values.kubespan) - line 92 (
.Values.allowSchedulingOnControlPlanes) - line 115 (
.Values.allowSchedulingOnControlPlanes)
🤖 Prompt for AI Agents
In charts/cozystack/values.yaml, add default values for the keys `kubespan` and
`allowSchedulingOnControlPlanes` to prevent rendering errors. Set `kubespan` to
false and `allowSchedulingOnControlPlanes` to false as default booleans. This
ensures the Helm chart templates referencing these values at lines 59-62, 92,
and 115 in charts/cozystack/templates/_helpers.tpl will have defined defaults
during installation or upgrade.
…owSchedulingOnControlPlanes Signed-off-by: lb0o <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
charts/cozystack/values.yaml (1)
13-14: Add inline documentation for new valuesAll other top-level keys in this file are preceded by a comment that explains their purpose (e.g.,
endpoint,clusterDomain, etc.). Adding a short comment for the newly introducedkubespanandallowSchedulingOnControlPlanesflags will help users discover and correctly configure these features directly fromvalues.yaml, without having to open templates or PR discussion.-# Enable Talos' encrypted mesh networking between nodes +kubespan: false # Enable Talos' encrypted mesh networking between nodes -# Allow user workloads to be scheduled on control-plane nodes +allowSchedulingOnControlPlanes: true # Allow user workloads to be scheduled on control-plane nodes(Slight wording tweak is optional—main goal is to add any explanatory comment lines.)
…an and allowschedulingoncontrolplanes Signed-off-by: lb0o <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
templates/_helpers.tpl (1)
1-121: Add input validation for required values.Consider adding validation for required values to provide better error messages during template rendering.
Add validation at the beginning of the template:
{{- define "talos.config" }} +{{- if not .Values.advertisedSubnets -}} + {{- fail "advertisedSubnets is required in values" -}} +{{- end -}} +{{- if not .Values.endpoint -}} + {{- fail "endpoint is required in values" -}} +{{- end -}} machine:
🧹 Nitpick comments (1)
templates/_helpers.tpl (1)
20-30: Consider making kernel modules configurable.The hardcoded list of kernel modules might not be necessary for all deployments. Consider making this list configurable to reduce attack surface and boot time.
kernel: + {{- with .Values.kernelModules }} modules: + {{- toYaml . | nindent 4 }} + {{- else }} - name: openvswitch - name: drbd parameters: - usermode_helper=disabled - name: zfs - name: spl - name: vfio_pci - name: vfio_iommu_type1 + {{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
templates/_helpers.tpl(1 hunks)
🔇 Additional comments (4)
templates/_helpers.tpl (4)
1-7: LGTM!The conditional removal of the
exclude-from-external-load-balancerslabel for control plane nodes is appropriate and follows Talos configuration patterns.
8-15: LGTM!The kubelet configuration with static CPU manager policy and increased pod limit is appropriate for production workloads.
41-48: LGTM!The containerd configuration properly sets device ownership from security context for both plugin paths, ensuring compatibility across different containerd versions.
112-113: Verify proxy replacement strategy.The kube-proxy is disabled, which requires an alternative solution for service networking. Ensure that the CNI or another component will handle service proxy functionality.
Since CNI is set to "none" (line 82), please verify that the cluster will have a working service proxy solution installed post-deployment.
| controlPlane: | ||
| endpoint: "{{ .Values.endpoint }}" | ||
| {{- if eq .MachineType "controlplane" }} | ||
| allowSchedulingOnControlPlanes: {{ if eq .Values.allowSchedulingOnControlPlanes true }}true{{ else }}false{{ end }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify boolean value assignment.
The conditional boolean conversion is redundant.
- allowSchedulingOnControlPlanes: {{ if eq .Values.allowSchedulingOnControlPlanes true }}true{{ else }}false{{ end }}
+ allowSchedulingOnControlPlanes: {{ .Values.allowSchedulingOnControlPlanes }}📝 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.
| allowSchedulingOnControlPlanes: {{ if eq .Values.allowSchedulingOnControlPlanes true }}true{{ else }}false{{ end }} | |
| allowSchedulingOnControlPlanes: {{ .Values.allowSchedulingOnControlPlanes }} |
🤖 Prompt for AI Agents
In templates/_helpers.tpl at line 92, the boolean assignment uses a redundant
conditional expression. Simplify it by directly assigning the boolean value from
.Values.allowSchedulingOnControlPlanes without using an if-else statement.
| {{- if eq .Values.kubespan true }} | ||
| kubespan: | ||
| enabled: true | ||
| {{- end }} |
There was a problem hiding this comment.
Fix boolean comparison for kubespan.
The boolean comparison should be direct without using the eq function.
- {{- if eq .Values.kubespan true }}
+ {{- if .Values.kubespan }}
kubespan:
enabled: true
{{- end }}📝 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 eq .Values.kubespan true }} | |
| kubespan: | |
| enabled: true | |
| {{- end }} | |
| {{- if .Values.kubespan }} | |
| kubespan: | |
| enabled: true | |
| {{- end }} |
🤖 Prompt for AI Agents
In templates/_helpers.tpl around lines 59 to 62, the boolean comparison for
.Values.kubespan is done using the eq function, which is unnecessary. Replace
the condition to directly check the boolean value by using {{- if
.Values.kubespan }} instead of {{- if eq .Values.kubespan true }} to simplify
and correct the boolean check.
| proxy: | ||
| disabled: true | ||
| discovery: | ||
| enabled: {{ if eq .Values.kubespan true }}true{{ else }}false{{ end }} |
There was a problem hiding this comment.
Fix boolean comparison and simplify assignment.
The boolean comparison and conversion are incorrect and redundant.
- enabled: {{ if eq .Values.kubespan true }}true{{ else }}false{{ end }}
+ enabled: {{ .Values.kubespan }}📝 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.
| enabled: {{ if eq .Values.kubespan true }}true{{ else }}false{{ end }} | |
| enabled: {{ .Values.kubespan }} |
🤖 Prompt for AI Agents
In templates/_helpers.tpl at line 115, the boolean comparison using eq
.Values.kubespan true is redundant and incorrect. Replace the entire expression
with a direct boolean evaluation of .Values.kubespan to assign enabled, removing
the if-else and eq comparison for a simpler and correct boolean assignment.
| registries: | ||
| mirrors: | ||
| docker.io: | ||
| endpoints: | ||
| - https://mirror.gcr.io | ||
| files: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make registry mirrors configurable.
The hardcoded mirror endpoint https://mirror.gcr.io might not be accessible in all environments. Consider making this configurable through values.
registries:
mirrors:
+ {{- with .Values.registryMirrors }}
+ {{- toYaml . | nindent 6 }}
+ {{- else }}
docker.io:
endpoints:
- https://mirror.gcr.io
+ {{- end }}📝 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.
| registries: | |
| mirrors: | |
| docker.io: | |
| endpoints: | |
| - https://mirror.gcr.io | |
| files: | |
| registries: | |
| mirrors: | |
| {{- with .Values.registryMirrors }} | |
| {{- toYaml . | nindent 6 }} | |
| {{- else }} | |
| docker.io: | |
| endpoints: | |
| - https://mirror.gcr.io | |
| {{- end }} | |
| files: |
🤖 Prompt for AI Agents
In templates/_helpers.tpl around lines 35 to 40, the mirror endpoint for
docker.io is hardcoded as https://mirror.gcr.io, which limits flexibility.
Modify the template to accept the mirror endpoint as a configurable value passed
through the values file, and use that value instead of the hardcoded URL. Ensure
to provide a default value or handle the case when no mirror is specified.
1 participant
[feature] Add templating for kubespan and allowSchedulingOnControlPlanes in values.yaml
Summary by CodeRabbit
New Features
Refactor