refactor based on feedbacks

Author: cychiang

pkg/validate/validate.go

@@ -18,7 +18,6 @@ package validate
 
 import (
 	"context"
-	"fmt"
 
 	ext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 	extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -60,41 +59,29 @@ func SchemaValidate(ctx context.Context, resources []*unstructured.Unstructured,
 		return nil, errors.Wrap(err, "cannot create schema validators")
 	}
 
-	// Index old resources by GVK+name+namespace so each resource can be paired
-	// with its previous state. On duplicate keys the last entry wins.
-	oldByKey := make(map[string]*unstructured.Unstructured, len(oldResources))
-	for _, o := range oldResources {
-		oldByKey[resourceKey(o)] = o
-	}
-
 	result := &ValidationResult{
 		Resources: make([]ResourceValidationResult, 0, len(resources)),
 	}
 	for _, r := range resources {
-		result.Resources = append(result.Resources, validateResource(ctx, r, objectOf(oldByKey[resourceKey(r)]), schemaValidators, structurals, crds))
+		// Find this resource's previous state, if supplied, so CEL transition
+		// rules (those referencing oldSelf) can be evaluated. A resource is
+		// matched to its old state by GroupVersionKind, namespace, and name;
+		// with no match oldObject stays nil and transition rules are skipped,
+		// exactly as on a Kubernetes create.
+		gvk, namespace, name := r.GroupVersionKind(), r.GetNamespace(), getResourceName(r)
+		var oldObject map[string]any
+		for _, o := range oldResources {
+			if o.GroupVersionKind() == gvk && o.GetNamespace() == namespace && getResourceName(o) == name {
+				oldObject = o.Object
+				break
+			}
+		}
+		result.Resources = append(result.Resources, validateResource(ctx, r, oldObject, schemaValidators, structurals, crds))
 	}
 	result.Summary = computeSummary(result.Resources)
 	return result, nil
 }
 
-// resourceKey identifies a resource by GroupVersionKind, name, and namespace.
-// It is used to match a resource under validation to its previous state so CEL
-// transition rules see the right old object.
-func resourceKey(r *unstructured.Unstructured) string {
-	gvk := r.GetObjectKind().GroupVersionKind()
-	return fmt.Sprintf("%s-%s-%s", gvk.String(), getResourceName(r), r.GetNamespace())
-}
-
-// objectOf returns the unstructured content of u, or nil when u is nil. It
-// keeps the nil check for an unmatched old resource in one place so callers can
-// pass the result straight to the CEL validator's oldObject argument.
-func objectOf(u *unstructured.Unstructured) map[string]any {
-	if u == nil {
-		return nil
-	}
-	return u.Object
-}
-
 // validateResource runs every check (schema, CEL, unknown fields, defaulting)
 // against a single resource and returns its ResourceValidationResult. It is
 // the per-resource decomposition of SchemaValidate; pulling it out keeps the

pkg/validate/validate_test.go

@@ -180,6 +180,56 @@ var testCRDNoMatchingVersion = &extv1.CustomResourceDefinition{
 	},
 }
 
+// TestSchemaValidateDistinguishesAmbiguousKeys is a regression test for
+// old-resource matching. A naive "<gvk>-<name>-<namespace>" string key collides
+// whenever a name or namespace contains the "-" separator: (name=app,
+// namespace=test-1) and (name=app-test, namespace=1) both render as
+// "<gvk>-app-test-1". Under such a collision a resource gets matched to the
+// WRONG previous state and its CEL transition rule is evaluated against the
+// wrong old object. SchemaValidate matches on GroupVersionKind, namespace, and
+// name as separate values, which keeps these two resources distinct.
+func TestSchemaValidateDistinguishesAmbiguousKeys(t *testing.T) {
+	mk := func(name, namespace, param string) *unstructured.Unstructured {
+		return &unstructured.Unstructured{Object: map[string]any{
+			"apiVersion": "test.org/v1alpha1",
+			"kind":       "TestTransition",
+			"metadata":   map[string]any{"name": name, "namespace": namespace},
+			"spec":       map[string]any{"param": param},
+		}}
+	}
+
+	// newA and newB collide to "<gvk>-app-test-1" under a naive string key, as
+	// do their matching old resources.
+	newA := mk("app", "test-1", "keep")    // unchanged vs oldA -> should be Valid
+	newB := mk("app-test", "1", "changed") // changed vs oldB   -> should be Invalid
+	oldA := mk("app", "test-1", "keep")
+	oldB := mk("app-test", "1", "original")
+
+	result, err := SchemaValidate(
+		t.Context(),
+		[]*unstructured.Unstructured{newA, newB},
+		[]*unstructured.Unstructured{oldA, oldB},
+		[]*extv1.CustomResourceDefinition{testCRDWithTransition},
+	)
+	if err != nil {
+		t.Fatalf("SchemaValidate() unexpected error: %v", err)
+	}
+
+	// Matching on separate fields, each resource sees its own old state. A naive
+	// string key would map both to the last old resource (oldB), wrongly
+	// flipping newA to Invalid and yielding Valid=0, Invalid=2.
+	want := ValidationSummary{Total: 2, Valid: 1, Invalid: 1}
+	if diff := cmp.Diff(want, result.Summary); diff != "" {
+		t.Errorf("Summary mismatch (-want +got):\n%s", diff)
+	}
+	if result.Resources[0].Status != ValidationStatusValid {
+		t.Errorf("newA (app/test-1) Status = %q; want Valid — matched to the wrong old resource?", result.Resources[0].Status)
+	}
+	if result.Resources[1].Status != ValidationStatusInvalid {
+		t.Errorf("newB (app-test/1) Status = %q; want Invalid", result.Resources[1].Status)
+	}
+}
+
 func TestSchemaValidate(t *testing.T) {
 	validResource := &unstructured.Unstructured{Object: map[string]any{
 		"apiVersion": "test.org/v1alpha1",