Four follow-ups from review round 4:
1. BLOCKER: the Update(forceAllowCreate=true) path delegates to Create() when the object does not yet exist (rest.go:452) — the typical kubectl apply upsert flow. Add TestUpdate_ForceAllowCreate_RejectsTenantDashName using a fake client so a future refactor of that delegation cannot silently bypass the tenant name check that r.validateNameFormat alone cannot catch.
2. BLOCKER: the e2e BATS test used || true inside the command substitution, which swallowed the kubectl exit code. Rework the test to capture exit code and stdout+stderr explicitly, then assert the exit code is non-zero before asserting on the error message. This distinguishes validation-success (kubectl exit 0 — regression) from environmental failures (exit non-zero but wrong message) from the happy path.
3. Extract TenantKind = "Tenant" as a named constant in the validation package with a comment pointing at the upstream ApplicationDefinition source of truth, and switch the kindName check to use it.
4. Add a clarifying comment on TestValidateApplicationName_TenantLengthFallthrough that it pins an architectural layering decision and is not a user-facing requirement, so a future promotion of tenant length into tenant-specific wording is a legitimate change rather than a test regression.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Three follow-ups from review:
1. Add defensive cleanup of the foo-bar tenant before and after the e2e regression test. If a prior run left the object in the cluster (e.g. a transient validation regression), the test no longer inherits that state; and if the test itself trips a regression in the future, the cluster is left clean for subsequent tests.
2. Switch kubectl --validate=false to --validate=ignore. The bool form was deprecated in kubectl 1.25 and only kept as an alias; --validate=ignore is the modern, stable spelling.
3. Drop the brittle Contains("63") assertion in TestValidateApplicationName_TenantLengthFallthrough. It pinned the test to the exact DNS-1035 error text from k8s.io/apimachinery, which could break on unrelated upstream wording changes. The surviving assertions still cover the intent: an error exists and it is not the tenant-specific message.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Follow-up from review round 2:
The BATS regression test now pins a namespace precondition, disables kubectl client-side validation with --validate=false so the request is guaranteed to reach the server-side name check, and documents the intent of each assertion. Previously a client-side schema rejection could have produced a false negative on the tenant-specific error grep, and a missing tenant-root namespace could have produced an unrelated failure that was hard to diagnose.
Also pin the edge case where a tenant name consists of valid characters but exceeds the DNS-1035 63-char label limit. The resulting error is intentionally the generic DNS-1035 message because length is not a tenant-specific constraint — the package-level function is not responsible for the stricter Helm-release-prefix length budget that REST.validateNameLength enforces.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Two follow-ups from review:
1. The original tenantNameRegex ^[a-z0-9]+$ let leading-digit names like 123foo slip past the tenant check and receive a generic DNS-1035 error, which defeated the goal of surfacing a tenant-specific message. Require a leading lowercase letter so every tenant-invalid name returns the tenant-contract error.
2. Add a BATS regression test in hack/e2e-install-cozystack.bats that exercises the aggregated API end-to-end. Unit tests construct REST{kindName: "Tenant"} by hand, so a future change to ApplicationDefinition kind registration could break real behavior without breaking the unit tests.
Also pin the error-message contract with a new TestValidateApplicationName_TenantErrorMessage that asserts every tenant-invalid input returns a message containing 'tenant names must' (not the generic DNS-1035 text).
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
ValidateApplicationName previously delegated to IsDNS1035Label, which permits hyphens. The tenant Helm chart's tenant.name helper rejects them at template time, so users saw a successful kubectl apply followed by a Flux reconciliation error.
Extend ValidateApplicationName with a kindName parameter and enforce alphanumeric-only names for the Tenant kind, matching the documented naming rules. Wire the check into REST.Create via a small validateNameFormat wrapper that mirrors validateNameLength.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Replace custom DNS-1035 regex with k8s.io/apimachinery IsDNS1035Label.
Remove hardcoded maxApplicationNameLength=40 from both validation and
OpenAPI — length validation is now handled entirely by validateNameLength
which computes dynamic limits based on Helm release prefix and root-host.
Fix README to reflect that max length depends on cluster configuration.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add validation to ensure Application names (including Tenants) conform
to DNS-1035 format. This prevents creation of resources with names
starting with digits, which would cause Kubernetes resource creation
failures (e.g., Services, Namespaces).
DNS-1035 requires names to:
- Start with a lowercase letter [a-z]
- Contain only lowercase alphanumeric or hyphens [-a-z0-9]
- End with an alphanumeric character [a-z0-9]
Also fixes broken validation.go that referenced non-existent internal
types (apps.Application, apps.ApplicationSpec).
Fixes: https://github.com/cozystack/cozystack/issues/1538
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
