From ac1132e16a1e349bf2c8c62b5b70cf08a12f5da7 Mon Sep 17 00:00:00 2001
From: Aleksei Sviridkin <f@lex.la>
Date: Sun, 12 Apr 2026 03:47:02 +0300
Subject: [PATCH] test(api): address review round 4 findings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 hack/e2e-install-cozystack.bats               | 17 ++--
 pkg/apis/apps/validation/validation.go        |  9 +-
 pkg/apis/apps/validation/validation_test.go   |  6 ++
 .../apps/application/rest_validation_test.go  | 90 +++++++++++++++++++
 4 files changed, 116 insertions(+), 6 deletions(-)

diff --git a/hack/e2e-install-cozystack.bats b/hack/e2e-install-cozystack.bats
index d57002f0..cf66b73f 100644
--- a/hack/e2e-install-cozystack.bats
+++ b/hack/e2e-install-cozystack.bats
@@ -222,8 +222,12 @@ EOF
   # server-side name check runs and the error we grep for is the tenant
   # contract error, not a kubectl schema rejection. (--validate=false is the
   # deprecated alias.)
-  local output
-  output=$(kubectl apply --validate=ignore -f - <<EOF 2>&1 || true
+  local output rc
+  # Run the apply in its own subshell so we can capture BOTH stdout+stderr
+  # AND the exit code explicitly, without `|| true` swallowing a real failure
+  # mode (e.g. network error, auth failure) that should also fail the test.
+  output=$(
+    kubectl apply --validate=ignore -f - 2>&1 <<EOF
 apiVersion: apps.cozystack.io/v1alpha1
 kind: Tenant
 metadata:
@@ -231,12 +235,15 @@ metadata:
   namespace: tenant-root
 spec: {}
 EOF
-)
+  ) && rc=0 || rc=$?
+  echo "kubectl apply exit=$rc, output=$output"
+  # kubectl MUST have failed: success would mean validation regressed.
+  [ "$rc" -ne 0 ]
   # Assert the tenant-specific message is present (distinguishes from
   # generic DNS-1035 errors and from network/auth failures).
   echo "$output" | grep -q "tenant names must"
-  # And assert kubectl did NOT report creation — if validation regressed,
-  # the server would accept the object and kubectl would print "... created".
+  # And assert kubectl did NOT report creation — if validation regressed
+  # into a "warn" variant, the server could still accept the object.
   ! echo "$output" | grep -qi "created"
 
   # Post-condition cleanup: even though we expect validation to reject the
diff --git a/pkg/apis/apps/validation/validation.go b/pkg/apis/apps/validation/validation.go
index 8f0f6681..d62f2f22 100644
--- a/pkg/apis/apps/validation/validation.go
+++ b/pkg/apis/apps/validation/validation.go
@@ -23,6 +23,13 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
+// TenantKind is the Application.Kind string that gates the tenant-specific
+// name rules below. It must stay in sync with the `kind` field of the tenant
+// ApplicationDefinition (packages/system/tenant-rd/cozyrds/tenant.yaml) which
+// is the upstream source the aggregated API reads at startup via
+// config.Application.Kind.
+const TenantKind = "Tenant"
+
 // 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)
@@ -54,7 +61,7 @@ func ValidateApplicationName(name, kindName string, fldPath *field.Path) field.E
 	// 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) {
+	if kindName == TenantKind && !tenantNameRegex.MatchString(name) {
 		allErrs = append(allErrs, field.Invalid(fldPath, name,
 			"tenant names must start with a lowercase letter and contain only lowercase letters and digits; dashes are not allowed"))
 		return allErrs
diff --git a/pkg/apis/apps/validation/validation_test.go b/pkg/apis/apps/validation/validation_test.go
index 51f36fd6..7930f8ae 100644
--- a/pkg/apis/apps/validation/validation_test.go
+++ b/pkg/apis/apps/validation/validation_test.go
@@ -151,6 +151,12 @@ func TestValidateApplicationName_TenantErrorMessage(t *testing.T) {
 // actually reach 64 characters end-to-end — this test only pins the package-
 // level fallthrough so a future refactor does not accidentally promote the
 // length error into tenant-specific wording.
+//
+// NOTE: this is an architectural decision, not a user-facing requirement.
+// If tenant length is ever promoted into a tenant-specific rule (e.g. to
+// include the Helm release prefix budget in this package's error message),
+// this test should be updated or deleted — it is not a backwards-compat
+// guarantee, just a checkpoint on the current layering.
 func TestValidateApplicationName_TenantLengthFallthrough(t *testing.T) {
 	name := strings.Repeat("a", 64) // valid tenant pattern, too long for DNS-1035
 
diff --git a/pkg/registry/apps/application/rest_validation_test.go b/pkg/registry/apps/application/rest_validation_test.go
index c0e5f9f5..a1f69cf1 100644
--- a/pkg/registry/apps/application/rest_validation_test.go
+++ b/pkg/registry/apps/application/rest_validation_test.go
@@ -17,10 +17,21 @@ limitations under the License.
 package application
 
 import (
+	"context"
 	"fmt"
 	"strings"
 	"testing"
 
+	helmv2 "github.com/fluxcd/helm-controller/api/v2"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apiserver/pkg/endpoints/request"
+	"k8s.io/apiserver/pkg/registry/rest"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	appsv1alpha1 "github.com/cozystack/cozystack/pkg/apis/apps/v1alpha1"
 	"github.com/cozystack/cozystack/pkg/config"
 )
 
@@ -64,6 +75,85 @@ func TestValidateNameFormat(t *testing.T) {
 	}
 }
 
+// TestUpdate_ForceAllowCreate_RejectsTenantDashName pins the wiring from the
+// Update → Create fall-through path. When a user runs `kubectl apply` and
+// the object does not yet exist, Kubernetes routes the request through
+// REST.Update with forceAllowCreate=true, which delegates to REST.Create
+// (rest.go:452). This test ensures tenant name validation fires on that
+// upsert path — otherwise a future refactor could silently regress the
+// fix for #2375 while unit tests of r.validateNameFormat alone keep passing.
+func TestUpdate_ForceAllowCreate_RejectsTenantDashName(t *testing.T) {
+	scheme := runtime.NewScheme()
+	if err := helmv2.AddToScheme(scheme); err != nil {
+		t.Fatalf("register helmv2 scheme: %v", err)
+	}
+	// Register the dynamic Tenant kind so the Application type round-trips
+	// through the scheme the same way the real aggregated API server wires
+	// it at startup.
+	resourceCfg := &config.ResourceConfig{
+		Resources: []config.Resource{
+			{
+				Application: config.ApplicationConfig{Kind: "Tenant"},
+			},
+		},
+	}
+	if err := appsv1alpha1.RegisterDynamicTypes(scheme, resourceCfg); err != nil {
+		t.Fatalf("register dynamic Tenant type: %v", err)
+	}
+
+	fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
+
+	r := &REST{
+		c: fakeClient,
+		gvr: schema.GroupVersionResource{
+			Group:    appsv1alpha1.GroupName,
+			Version:  "v1alpha1",
+			Resource: "tenants",
+		},
+		gvk: schema.GroupVersionKind{
+			Group:   appsv1alpha1.GroupName,
+			Version: "v1alpha1",
+			Kind:    "Tenant",
+		},
+		kindName: "Tenant",
+		releaseConfig: config.ReleaseConfig{
+			Prefix: "tenant-",
+		},
+	}
+
+	newApp := &appsv1alpha1.Application{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "apps.cozystack.io/v1alpha1",
+			Kind:       "Tenant",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "foo-bar",
+			Namespace: "tenant-root",
+		},
+	}
+
+	ctx := request.WithNamespace(context.Background(), "tenant-root")
+
+	_, _, err := r.Update(
+		ctx,
+		"foo-bar",
+		rest.DefaultUpdatedObjectInfo(newApp),
+		nil,                      // createValidation
+		nil,                      // updateValidation
+		true,                     // forceAllowCreate → routes through Create on NotFound
+		&metav1.UpdateOptions{},
+	)
+	if err == nil {
+		t.Fatalf("expected Update to reject tenant name with dashes, got no error")
+	}
+	if !apierrors.IsInvalid(err) {
+		t.Errorf("expected Invalid status error, got %T: %v", err, err)
+	}
+	if !strings.Contains(err.Error(), "tenant names must") {
+		t.Errorf("expected tenant-specific error in %q", err.Error())
+	}
+}
+
 func TestValidateNameLength(t *testing.T) {
 	tests := []struct {
 		name      string