[api] Reject tenant names with dashes at Create time by lexfrei · Pull Request #2380 · cozystack/cozystack

lexfrei · 2026-04-12T10:56:14Z

What this PR does

The aggregated API previously accepted tenant names containing hyphens (e.g. foo-bar) because ValidateApplicationName delegated entirely to IsDNS1035Label, which permits them. The tenant Helm chart's tenant.name helper then rejected the release at template time — so users saw a successful kubectl apply followed by a confusing Flux/HelmRelease reconciliation error.

This PR extends ValidateApplicationName with a kindName parameter and enforces an alphanumeric-only rule (^[a-z][a-z0-9]*$) for the Tenant kind. A TenantKind constant centralizes the kind string, and REST.validateNameFormat wraps the check symmetrically with the existing validateNameLength. The tenant chart README is updated to match the already-correct website documentation.

Test coverage includes:

Unit tests for the validation package (format + error message contract)
REST wrapper and Update→Create fall-through path tests with a fake client
E2E BATS regression test with explicit exit-code checks and cleanup

Fixes #2375

Release note

[api] Tenant names containing dashes are now rejected at API level during creation, with a specific error message, instead of being silently accepted and failing later during Helm reconciliation.

Summary by CodeRabbit

New Features
- Tenant names now require lowercase letters and digits only, must start with a letter; dashes are rejected with a tenant-specific error.
Documentation
- Tenant naming guidelines updated to reflect the stricter alphanumeric-only requirement.
Tests
- New unit and end-to-end tests added to verify tenant naming enforcement and tenant-specific error messaging.

coderabbitai · 2026-04-12T10:56:22Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5e3dc14a-e710-4c6c-9bde-41196423bbf5

📥 Commits

Reviewing files that changed from the base of the PR and between d188aaf and 637dd73.

📒 Files selected for processing (1)

pkg/registry/apps/application/rest_validation_test.go

📝 Walkthrough

Walkthrough

Kind-gated validation was added so Tenant names containing dashes are rejected at the aggregated API. Validation signature now accepts kind info, a Tenant-specific regex and constant were added, REST delegates to the updated validator, tests and README were updated, and an e2e Bats test was added.

Changes

Cohort / File(s)	Summary
Validation Core Logic `pkg/apis/apps/validation/validation.go`, `pkg/apis/apps/validation/validation_test.go`	Added `TenantKind` constant and tenant regex `^[a-z][a-z0-9]*$`; changed `ValidateApplicationName` signature to include `kindName`; enforce tenant-specific name rule with tenant-specific error messaging; expanded tests for tenant vs non-tenant behavior, error wording, and length fallthrough.
REST Layer Integration `pkg/registry/apps/application/rest.go`, `pkg/registry/apps/application/rest_validation_test.go`	Replaced inline name check in `REST.Create` with `REST.validateNameFormat` that calls `validation.ValidateApplicationName(name, r.kindName, ...)`; switched tenant comparisons to `validation.TenantKind`; added tests for validateNameFormat, update-with-force tenant rejection, and convertHelmRelease tenant-kind gating.
Documentation & E2E Testing `packages/apps/tenant/README.md`, `hack/e2e-install-cozystack.bats`	Updated tenant README to require lowercase alphanumeric-only names (no dashes) and must start with a lowercase letter. Added Bats e2e test asserting `kubectl apply` of `foo-bar` fails with tenant-specific error and cleans up; minor whitespace tweak in adjacent test line.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I sniffed the dash and gave a hop,
Foo-bar tripped and couldn't stop.
Alphanums line up, neat and bright,
No more hyphen hustle at night.
thump-thump ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and specifically describes the main change: rejecting tenant names with dashes at API Create time.
Linked Issues check	✅ Passed	The PR fully implements the required objectives from issue `#2375`: kind-gated tenant name validation rejecting dashes before generic checks, tenant-specific error messaging, and regression tests.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to tenant name validation: validation logic, validation tests, REST wrapper, E2E tests, and documentation updates. No unrelated changes detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/tenant-name-dash-validation-2375

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

gemini-code-assist

Code Review

This pull request introduces stricter naming validation for Tenant applications, requiring them to be alphanumeric and start with a lowercase letter to avoid ambiguity in Helm-generated resource names. The changes include updates to the validation logic in the aggregated API, documentation updates, and the addition of comprehensive E2E and unit tests to verify the new constraints and error messages. I have no feedback to provide as there were no review comments to assess.

coderabbitai

🧹 Nitpick comments (1)

pkg/registry/apps/application/rest_validation_test.go (1)

53-60: Prefer validation.TenantKind over "Tenant" literals in test cases.

Using the shared constant consistently will avoid silent drift if the kind string is ever renamed.

Proposed refactor

- {"tenant accepts alphanumeric", "Tenant", "foo", false},
+ {"tenant accepts alphanumeric", validation.TenantKind, "foo", false},
...
- Application: config.ApplicationConfig{Kind: "Tenant"},
+ Application: config.ApplicationConfig{Kind: validation.TenantKind},
...
- kindName: "Tenant",
+ kindName: validation.TenantKind,

Also applies to: 98-99, 118-120

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/registry/apps/application/rest_validation_test.go` around lines 53 - 60,
Replace the hard-coded "Tenant" string literals in the test table entries with
the shared constant validation.TenantKind (e.g., change occurrences in
rest_validation_test.go where the test case kind is "Tenant" to use
validation.TenantKind) so the tests track the canonical kind name; update all
other similar occurrences in the same file noted in the comment (the additional
test cases referenced) to use validation.TenantKind as well. Ensure imports
include the validation package if not already present and run tests to verify no
compile errors.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pkg/registry/apps/application/rest_validation_test.go`:
- Around line 53-60: Replace the hard-coded "Tenant" string literals in the test
table entries with the shared constant validation.TenantKind (e.g., change
occurrences in rest_validation_test.go where the test case kind is "Tenant" to
use validation.TenantKind) so the tests track the canonical kind name; update
all other similar occurrences in the same file noted in the comment (the
additional test cases referenced) to use validation.TenantKind as well. Ensure
imports include the validation package if not already present and run tests to
verify no compile errors.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9935b725-0c17-4be2-8575-ec65458b80ee

📥 Commits

Reviewing files that changed from the base of the PR and between 1436fee and 2e7b7dd.

📒 Files selected for processing (6)

hack/e2e-install-cozystack.bats
packages/apps/tenant/README.md
pkg/apis/apps/validation/validation.go
pkg/apis/apps/validation/validation_test.go
pkg/registry/apps/application/rest.go
pkg/registry/apps/application/rest_validation_test.go

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>

The README stated that dashes are 'allowed but discouraged' in tenant names, but the tenant.name Helm helper explicitly fails on release names that contain more than one dash, and the platform guide on the website already says tenant names must be alphanumeric. Bring the chart README in line with both the website documentation and the enforcement now present on the aggregated API. 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>

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>

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>

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>

The convertHelmReleaseToApplication fall-through that populates Status.Namespace for tenant resources still compared r.kindName against the bare string "Tenant". The validation package already exposes TenantKind for exactly this reason: if the kind string ever diverges from the ApplicationDefinition source of truth, the constant and the call site drift together rather than silently desynchronizing. Add TestConvertHelmReleaseToApplication_TenantNamespaceKindGate to pin both sides of the gate: tenant kind populates Status.Namespace, non-tenant kind leaves it empty. Also rewrite the TestUpdate comment to describe the upsert invariant directly instead of referencing an issue number. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>

coderabbitai

🧹 Nitpick comments (1)

pkg/registry/apps/application/rest.go (1)
157-176: Use validation.TenantKind consistently in Create tenant gate.

Create still checks r.kindName == "Tenant" for namespace-length validation. Since this PR centralizes tenant-kind identity via validation.TenantKind, this should use the same constant to avoid drift.
♻️ Proposed consistency fix
-	if r.kindName == "Tenant" {
+	if r.kindName == validation.TenantKind {
 		if nsErrs := r.validateTenantNamespaceLength(app.Namespace, app.Name); len(nsErrs) > 0 {
 			return nil, apierrors.NewInvalid(r.gvk.GroupKind(), app.Name, nsErrs)
 		}
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/registry/apps/application/rest.go` around lines 157 - 176, Replace the
hard-coded kind string check in the Create flow with the centralized constant:
change the conditional that uses r.kindName == "Tenant" to compare against
validation.TenantKind (i.e., if r.kindName == validation.TenantKind) so the
namespace-length gate that calls r.validateTenantNamespaceLength(app.Namespace,
app.Name) uses the shared tenant-kind identifier consistently.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pkg/registry/apps/application/rest.go`:
- Around line 157-176: Replace the hard-coded kind string check in the Create
flow with the centralized constant: change the conditional that uses r.kindName
== "Tenant" to compare against validation.TenantKind (i.e., if r.kindName ==
validation.TenantKind) so the namespace-length gate that calls
r.validateTenantNamespaceLength(app.Namespace, app.Name) uses the shared
tenant-kind identifier consistently.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8544bdea-3629-489e-b345-1e2c638763b1

📥 Commits

Reviewing files that changed from the base of the PR and between 2e7b7dd and d188aaf.

📒 Files selected for processing (6)

hack/e2e-install-cozystack.bats
packages/apps/tenant/README.md
pkg/apis/apps/validation/validation.go
pkg/apis/apps/validation/validation_test.go
pkg/registry/apps/application/rest.go
pkg/registry/apps/application/rest_validation_test.go

✅ Files skipped from review due to trivial changes (1)

packages/apps/tenant/README.md

🚧 Files skipped from review as they are similar to previous changes (1)

hack/e2e-install-cozystack.bats

Replace bare "Tenant" string literals with the validation.TenantKind constant in all test table entries, struct fields, and TypeMeta. Prevents silent drift if the canonical kind string is ever renamed. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>

sircthulhu

lgtm

gemini-code-assist Bot reviewed Apr 12, 2026

View reviewed changes

lexfrei marked this pull request as ready for review April 12, 2026 10:57

lexfrei requested review from IvanHunters, androndo, kvaps, lllamnyp and sircthulhu as code owners April 12, 2026 10:57

dosubot Bot added the size/L This PR changes 100-499 lines, ignoring generated files label Apr 12, 2026

coderabbitai Bot reviewed Apr 12, 2026

View reviewed changes

lexfrei added 7 commits April 12, 2026 14:16

lexfrei force-pushed the fix/tenant-name-dash-validation-2375 branch from 2e7b7dd to d188aaf Compare April 12, 2026 11:18

coderabbitai Bot reviewed Apr 12, 2026

View reviewed changes

lexfrei self-assigned this Apr 12, 2026

sircthulhu approved these changes Apr 13, 2026

View reviewed changes

dosubot Bot added the lgtm This PR has been approved by a maintainer label Apr 13, 2026

lllamnyp approved these changes Apr 13, 2026

View reviewed changes

lexfrei merged commit c77d0b8 into main Apr 13, 2026
9 checks passed

lexfrei deleted the fix/tenant-name-dash-validation-2375 branch April 13, 2026 14:25

lexfrei mentioned this pull request Apr 15, 2026

Add deletion protection webhook for critical platform resources #2402

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api] Reject tenant names with dashes at Create time#2380

[api] Reject tenant names with dashes at Create time#2380
lexfrei merged 8 commits intomainfrom
fix/tenant-name-dash-validation-2375

lexfrei commented Apr 12, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 12, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

sircthulhu left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lexfrei commented Apr 12, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Release note

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sircthulhu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lexfrei commented Apr 12, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 12, 2026 •

edited

Loading