feat(charts): centralise DNS-1123 subdomain validation in talm helper#158
Conversation
Replace per-site inline regex validation of values.yaml clusterName
with a shared talm.validate.dns1123subdomain helper, apply it to
clusterDomain (cozystack), and add Go-side validation of
`talm init --name` against the same upstream Kubernetes contract.
Three input channels accept DNS-name values from the operator:
1. `talm init --name <X>` — Go code path. Now validated in
PreRunE via k8s.io/apimachinery/pkg/util/validation
.IsDNS1123Subdomain so the operator sees the precise upstream
error (length, character class) before any file is written.
Previously this flag accepted anything and the failure
surfaced downstream as an opaque bundle-generator error.
2. values.yaml clusterName — Helm template path in cozystack and
generic charts. Previously a per-site inline regex with a
misleading 'DNS-1123 label' message and no length cap. Now
piped through talm.validate.dns1123subdomain which:
- matches the same RFC 1123 subdomain regex as the upstream
k8s validator (so chart and CLI agree byte-for-byte)
- enforces total length <= 253 chars (was missing)
- emits a precise message naming the field and quoting the
offending value
3. values.yaml clusterDomain (cozystack only) — previously
unvalidated. Same helper, same contract.
Test coverage:
- pkg/commands/contract_init_validation_test.go pins the Go-side
contract: every shipped chart name passes; uppercase, underscore,
leading/trailing dash, space, double dot, 254-char length all
rejected with messages quoting the offending value and naming
'DNS-1123 subdomain'. Validator skipped under --encrypt /
--decrypt / --update where --name is not required.
- pkg/engine/contract_dns1123_test.go pins the Helm template
contract: helper accepts canonical happy paths (single label,
multi-label, leading digit, single character); rejects every
invalid shape with a message naming the field. Same negative
set tested for clusterDomain. Default cozy.local still passes
on cozystack. Generic chart shares one negative case to catch
template-only regressions.
Per-label length (63 chars) intentionally NOT enforced — upstream
IsDNS1123Subdomain does not enforce it, and there is no
Talos-side cap that aligns with a 63-char floor. Stay symmetric
with Go-side validation.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces DNS-1123 subdomain validation for cluster names and domains across Helm templates and CLI commands. It adds a reusable Helm validation helper, updates chart templates to enforce validation, adds CLI-side validation for the ChangesDNS-1123 Subdomain Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces centralized DNS-1123 subdomain validation for cluster names and domains across Helm charts and the CLI. It adds a new Helm helper, talm.validate.dns1123subdomain, and integrates Go-side validation using the Kubernetes apimachinery package. Comprehensive contract tests are included for both CLI and chart rendering. Feedback suggests quoting the dnsDomain output for YAML safety and improving the descriptiveness of validation error messages for consistency.
| cni: | ||
| name: none | ||
| dnsDomain: {{ .Values.clusterDomain }} | ||
| dnsDomain: {{ include "talm.validate.dns1123subdomain" (dict "value" .Values.clusterDomain "field" "clusterDomain") }} |
There was a problem hiding this comment.
The dnsDomain value should be quoted to ensure it is always interpreted as a string by YAML parsers. This is particularly important for values that might otherwise be parsed as numbers (e.g., a subdomain consisting only of digits like 123.456). This change also brings consistency with the clusterName field on line 106. Note: If this change is applied, the test case TestContract_DNS1123_ClusterDomain_DefaultPasses in pkg/engine/contract_dns1123_test.go will need to be updated to expect the quoted value.
dnsDomain: {{ include "talm.validate.dns1123subdomain" (dict "value" .Values.clusterDomain "field" "clusterDomain") | quote }}
There was a problem hiding this comment.
Right — leaving dnsDomain as a raw scalar leaves it open to numeric reparse on a digits-only subdomain. Piped through | quote for consistency with clusterName, and updated TestContract_DNS1123_ClusterDomain_DefaultPasses plus TestContract_Cluster_ClusterDomain_Cozystack to expect the quoted form. See ac9d64b.
| {{- $value := .value -}} | ||
| {{- $field := .field -}} | ||
| {{- if not $value -}} | ||
| {{- fail (printf "values.yaml: %s must be a non-empty DNS-1123 subdomain (lowercase alphanumeric, '-', and '.'; total max 253 chars)" $field) -}} |
There was a problem hiding this comment.
This error message is less descriptive than the one used for regex validation failures on line 516. For consistency and better user feedback, it should explicitly mention the requirements (lowercase, allowed characters, and start/end constraints) enforced by the DNS-1123 contract.
{{- fail (printf "values.yaml: %s must be a non-empty DNS-1123 subdomain (must be lowercase, only [a-z0-9-.], start and end with [a-z0-9], max 253 chars)" $field) -}}
There was a problem hiding this comment.
Agreed — the empty branch was less descriptive than the regex branch a few lines down. Aligned both messages on the same constraint list (lowercase, [a-z0-9-.], start/end with [a-z0-9], max 253 chars) so operators see the same rule regardless of which guard fires. See 122194e.
Address review feedback from gemini-code-assist on
charts/cozystack/templates/_helpers.tpl:101: dnsDomain was being
emitted as an unquoted scalar, leaving it open to YAML re-parse
into a number when the value happens to look numeric (e.g. a
subdomain made of digits). Pipe through | quote to lock it in as
a string and align with the clusterName convention.
Tests in pkg/engine/contract_{dns1123,cluster}_test.go updated to
expect the quoted form.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Address review feedback from gemini-code-assist on charts/talm/templates/_helpers.tpl:510: the empty-value branch emitted a shorter, less specific error than the regex branch a few lines down. Both should describe the same constraint set so operators do not have to consult two messages to learn the rule. Match the wording used by the regex branch — must be lowercase, only [a-z0-9-.], start and end with [a-z0-9], max 253 chars. Signed-off-by: Aleksei Sviridkin <f@lex.la>
![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)
pkg/engine/contract_dns1123_test.go (1)
90-95: ⚡ Quick winPin quoted offending-value behavior in render-error assertions.
The contract comments require quoted offending values, but current rejection tests only assert field name + generic substring. Add explicit quoted-value checks so message precision is locked.
✅ Suggested test hardening
@@ if !strings.Contains(err.Error(), "clusterName") { t.Errorf("error must name 'clusterName' field, got: %v", err) } + if !strings.Contains(err.Error(), `"`+tc.clusterName+`"`) { + t.Errorf("error must quote offending clusterName, got: %v", err) + } if !strings.Contains(err.Error(), tc.wantInError) { t.Errorf("error must contain %q, got: %v", tc.wantInError, err) } @@ if !strings.Contains(err.Error(), "clusterDomain") { t.Errorf("error must name 'clusterDomain' field, got: %v", err) } + if !strings.Contains(err.Error(), `"`+tc.clusterDomain+`"`) { + t.Errorf("error must quote offending clusterDomain, got: %v", err) + } if !strings.Contains(err.Error(), tc.wantInError) { t.Errorf("error must contain %q, got: %v", tc.wantInError, err) }Also applies to: 126-131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/engine/contract_dns1123_test.go` around lines 90 - 95, The test assertions in contract_dns1123_test.go currently only check for the field name and a generic substring (tc.wantInError); update the test to also assert that the rendered error message includes the offending value wrapped in quotes (e.g., the quoted form of the invalid value) so the contract’s requirement for quoted offending-values is enforced; locate the checks around the block that contains strings.Contains(err.Error(), "clusterName") and the tc.wantInError assertion (also duplicate at the later block around lines 126-131) and add an assertion that err.Error() contains the quoted offending value (construct the quoted form from the test case’s offending value variable) to harden the expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/talm/templates/_helpers.tpl`:
- Around line 507-513: The template currently uses .value directly (symbols:
$value, $field, len, fail) which breaks for non-string YAML scalars; coerce
.value to a string first (e.g., via printf "%v") and then perform the emptiness
and length checks so len operates on a string and the falsy check doesn't
mis-handle numeric 0—update references to use the canonical string variable when
calling if not, len, and in the fail messages to ensure safe validation of
DNS-1123 subdomain values.
---
Nitpick comments:
In `@pkg/engine/contract_dns1123_test.go`:
- Around line 90-95: The test assertions in contract_dns1123_test.go currently
only check for the field name and a generic substring (tc.wantInError); update
the test to also assert that the rendered error message includes the offending
value wrapped in quotes (e.g., the quoted form of the invalid value) so the
contract’s requirement for quoted offending-values is enforced; locate the
checks around the block that contains strings.Contains(err.Error(),
"clusterName") and the tc.wantInError assertion (also duplicate at the later
block around lines 126-131) and add an assertion that err.Error() contains the
quoted offending value (construct the quoted form from the test case’s offending
value variable) to harden the expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2581cb5-dc65-4925-8c73-ec0a65dd1eed
📒 Files selected for processing (7)
charts/cozystack/templates/_helpers.tplcharts/generic/templates/_helpers.tplcharts/talm/templates/_helpers.tplpkg/commands/contract_init_validation_test.gopkg/commands/init.gopkg/engine/contract_cluster_test.gopkg/engine/contract_dns1123_test.go
Address review feedback from coderabbitai on charts/talm/templates/_helpers.tpl:513: an unquoted numeric YAML scalar (e.g. `clusterName: 123` in values.yaml) is parsed by Helm as an int, and Go templates' `len` panics with 'len of type int' when it hits the first length check — bypassing the fail() message entirely. Reproduced locally before the fix: template: cozystack/charts/talm/templates/_helpers.tpl:512:11: executing "talm.validate.dns1123subdomain" at <len $value>: error calling len: len of type int Apply the suggested coercion: render .value through `printf "%v"` into a guaranteed-string form before any length / regex / equality check. Replace the `if not $value` falsy check (which would treat numeric 0 as empty) with an explicit `eq $value ""` after coercion. Tests in pkg/engine/contract_dns1123_test.go: - ClusterName_NumericYAMLScalarCoerced: `clusterName: 123` no longer crashes the template; the value coerces to "123" which happens to be a valid DNS-1123 subdomain so render succeeds. The pin is specifically against the `len of type int` panic. - ClusterName_InvalidNumericFailsCleanly: `clusterName: -1` coerces to "-1" which is not a valid DNS-1123 subdomain; validator emits the regular field-name + value error instead of a template panic. Signed-off-by: Aleksei Sviridkin <f@lex.la>
2 participants
Summary
Replace per-site inline regex validation of
values.yaml: clusterNamewith a sharedtalm.validate.dns1123subdomainhelper, apply it toclusterDomain(cozystack), and add Go-side validation oftalm init --nameagainst the same upstream Kubernetes contract.Three input channels accept DNS-name values from the operator:
talm init --name <X>— Go path. Now validated in PreRunE viak8s.io/apimachinery/pkg/util/validation.IsDNS1123Subdomain. Previously accepted anything; failure surfaced downstream as an opaque bundle-generator error.values.yaml: clusterName— Helm template path in cozystack and generic charts. Previously a per-site inline regex with a misleading "DNS-1123 label" message and no length cap. Now piped throughtalm.validate.dns1123subdomainwhich matches the same RFC 1123 subdomain regex as the upstream k8s validator, enforces total length ≤ 253 chars, and emits a precise message naming the field and quoting the offending value.values.yaml: clusterDomain(cozystack only) — previously unvalidated. Same helper, same contract.Test coverage
pkg/commands/contract_init_validation_test.go— every shipped chart name passes; uppercase, underscore, leading/trailing dash, space, double dot, 254-char length all rejected with messages quoting the offending value and naming "DNS-1123 subdomain". Validator skipped under--encrypt/--decrypt/--updatewhere--nameis not required.pkg/engine/contract_dns1123_test.go— helper accepts canonical happy paths (single label, multi-label, leading digit, single character); rejects every invalid shape with a message naming the field. Same negative set tested forclusterDomain. Defaultcozy.localstill passes on cozystack. Generic chart shares one negative case to catch template-only regressions.Notes
Per-label length (63 chars per RFC 1035) intentionally NOT enforced — upstream
IsDNS1123Subdomaindoes not enforce it, and there is no Talos-side cap that aligns with a 63-char floor. Stay symmetric with Go-side validation.Closes the follow-ups raised on #148.
Summary by CodeRabbit
New Features
Improvements
init --namecommand now validates input upfront with detailed error messaging for invalid values.Tests