fix(api): tighten tenant name regex and add regression guards
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>
This commit is contained in:
parent
fef0f10bfd
commit
be40ac55c6
3 changed files with 79 additions and 16 deletions
|
|
@ -200,8 +200,28 @@ EOF
|
|||
kubectl wait hr/keycloak hr/keycloak-configure hr/keycloak-operator -n cozy-keycloak --timeout=10m --for=condition=ready
|
||||
}
|
||||
|
||||
@test "Aggregated API rejects Tenant name with dashes" {
|
||||
# Regression guard: the tenant Helm chart's tenant.name helper splits the
|
||||
# Release.Name on "-" and fails unless the result is exactly
|
||||
# ["tenant", "<name>"]. The aggregated API must catch tenant names
|
||||
# containing dashes up-front with a tenant-specific error, instead of
|
||||
# silently accepting the Application and letting Flux fail later.
|
||||
local output
|
||||
output=$(kubectl apply -f - <<EOF 2>&1 || true
|
||||
apiVersion: apps.cozystack.io/v1alpha1
|
||||
kind: Tenant
|
||||
metadata:
|
||||
name: foo-bar
|
||||
namespace: tenant-root
|
||||
spec: {}
|
||||
EOF
|
||||
)
|
||||
echo "$output" | grep -q "tenant names must"
|
||||
! echo "$output" | grep -qi "created"
|
||||
}
|
||||
|
||||
@test "Create tenant with isolated mode enabled" {
|
||||
kubectl -n tenant-root get tenants.apps.cozystack.io test ||
|
||||
kubectl -n tenant-root get tenants.apps.cozystack.io test ||
|
||||
kubectl apply -f - <<EOF
|
||||
apiVersion: apps.cozystack.io/v1alpha1
|
||||
kind: Tenant
|
||||
|
|
|
|||
|
|
@ -23,20 +23,23 @@ import (
|
|||
"k8s.io/apimachinery/pkg/util/validation/field"
|
||||
)
|
||||
|
||||
// tenantNameRegex enforces alphanumeric-only tenant names. This is stricter
|
||||
// than DNS-1035 because the tenant Helm chart's tenant.name helper
|
||||
// (packages/apps/tenant/templates/_helpers.tpl) splits Release.Name on "-"
|
||||
// and fails unless the result is exactly ["tenant", "<name>"]. Any dash
|
||||
// inside <name> would break that invariant at Helm template time, so the
|
||||
// aggregated API must reject such names up-front with a specific error.
|
||||
var tenantNameRegex = regexp.MustCompile(`^[a-z0-9]+$`)
|
||||
// tenantNameRegex enforces alphanumeric-only tenant names that begin with a
|
||||
// lowercase letter. This is stricter than DNS-1035 because the tenant Helm
|
||||
// chart's tenant.name helper (packages/apps/tenant/templates/_helpers.tpl)
|
||||
// splits Release.Name on "-" and fails unless the result is exactly
|
||||
// ["tenant", "<name>"]. Any dash inside <name> would break that invariant at
|
||||
// Helm template time, so the aggregated API must reject such names up-front
|
||||
// with a specific error. Requiring a leading letter (rather than letting
|
||||
// leading-digit names fall through to DNS-1035) keeps the error message
|
||||
// tenant-specific for all invalid inputs.
|
||||
var tenantNameRegex = regexp.MustCompile(`^[a-z][a-z0-9]*$`)
|
||||
|
||||
// ValidateApplicationName validates that an Application name is acceptable for
|
||||
// the given kind. All applications must conform to DNS-1035 because their
|
||||
// names are used to create Kubernetes resources (Services, Namespaces, etc.)
|
||||
// that require DNS-1035 compliance. Tenant applications additionally must be
|
||||
// alphanumeric (no dashes) because of the Helm chart constraint described on
|
||||
// tenantNameRegex.
|
||||
// alphanumeric and begin with a lowercase letter because of the Helm chart
|
||||
// constraint described on tenantNameRegex.
|
||||
// Note: length validation is handled separately by validateNameLength in the
|
||||
// REST handler, which computes dynamic limits based on Helm release prefix.
|
||||
func ValidateApplicationName(name, kindName string, fldPath *field.Path) field.ErrorList {
|
||||
|
|
@ -47,12 +50,13 @@ func ValidateApplicationName(name, kindName string, fldPath *field.Path) field.E
|
|||
return allErrs
|
||||
}
|
||||
|
||||
// Tenant names must be alphanumeric — see tenantNameRegex comment for the
|
||||
// reason. Check before DNS-1035 so the error message is specific to the
|
||||
// tenant contract, not the generic DNS label rules.
|
||||
// Tenant names must be alphanumeric starting with a letter — see
|
||||
// tenantNameRegex comment for the reason. Check before DNS-1035 so the
|
||||
// error message is specific to the tenant contract, not the generic DNS
|
||||
// label rules.
|
||||
if kindName == "Tenant" && !tenantNameRegex.MatchString(name) {
|
||||
allErrs = append(allErrs, field.Invalid(fldPath, name,
|
||||
"tenant names must be alphanumeric (lowercase letters and digits only); dashes are not allowed"))
|
||||
"tenant names must start with a lowercase letter and contain only lowercase letters and digits; dashes are not allowed"))
|
||||
return allErrs
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -86,8 +86,9 @@ func TestValidateApplicationName(t *testing.T) {
|
|||
{"tenant uppercase", "Foo", "Tenant", true},
|
||||
{"tenant underscore", "foo_bar", "Tenant", true},
|
||||
{"tenant empty", "", "Tenant", true},
|
||||
// Leading digit is alphanumeric (tenant regex passes) but falls through
|
||||
// to DNS-1035, which requires a leading alphabetic character.
|
||||
// Leading digit must be caught by the tenant-specific regex (not by
|
||||
// falling through to DNS-1035) so the error message reflects the
|
||||
// tenant contract — see TestValidateApplicationName_TenantErrorMessage.
|
||||
{"tenant leading digit", "123foo", "Tenant", true},
|
||||
}
|
||||
|
||||
|
|
@ -101,3 +102,41 @@ func TestValidateApplicationName(t *testing.T) {
|
|||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestValidateApplicationName_TenantErrorMessage pins the contract that when
|
||||
// a tenant name is invalid, the returned error message is specific to the
|
||||
// tenant naming rule — not the generic DNS-1035 message. Otherwise users get
|
||||
// back "must start with an alphabetic character" or similar and have no way
|
||||
// to know the constraint is tied to the tenant Helm chart.
|
||||
func TestValidateApplicationName_TenantErrorMessage(t *testing.T) {
|
||||
// Every tenant-invalid name below must surface a tenant-specific error
|
||||
// message. In particular, "123foo" starts with a digit — the original
|
||||
// implementation let that fall through to DNS-1035 with a generic error;
|
||||
// the regex is tightened specifically so this case fails up-front.
|
||||
invalidTenantNames := []string{
|
||||
"foo-bar", // dash
|
||||
"-foo", // leading dash
|
||||
"foo-", // trailing dash
|
||||
"foo--bar", // double dash
|
||||
"Foo", // uppercase
|
||||
"foo_bar", // underscore
|
||||
"foo.bar", // dot
|
||||
"foo bar", // space
|
||||
"123foo", // leading digit — must not fall through to DNS-1035
|
||||
}
|
||||
|
||||
const wantSubstring = "tenant names must"
|
||||
|
||||
for _, name := range invalidTenantNames {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
errs := ValidateApplicationName(name, "Tenant", field.NewPath("metadata").Child("name"))
|
||||
if len(errs) == 0 {
|
||||
t.Fatalf("expected error for tenant name %q, got none", name)
|
||||
}
|
||||
if !strings.Contains(errs[0].Detail, wantSubstring) {
|
||||
t.Errorf("tenant name %q: error detail = %q, want substring %q (generic DNS-1035 message is not tenant-specific)",
|
||||
name, errs[0].Detail, wantSubstring)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue