Merge pull request #135714 from lalitc375/fix-validation-identifier

Ensure metricIdentifier uses scheme for kind resolution

staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go

@@ -18,6 +18,7 @@ package rest
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"slices"
 	"strings"
@@ -296,23 +297,26 @@ func panicSafeValidateFunc(
 	}
 }
 
-func metricIdentifier(ctx context.Context, obj runtime.Object, opType operation.Type) (string, error) {
+func metricIdentifier(ctx context.Context, scheme *runtime.Scheme, obj runtime.Object, opType operation.Type) (string, error) {
+	var errs error
 	var identifier string
-	var err error
 
 	identifier = "unknown_resource"
 	// Use kind for identifier.
-	if obj != nil {
-		gvk := obj.GetObjectKind().GroupVersionKind()
-		if gvk.Kind != "" {
-			identifier = strings.ToLower(gvk.Kind)
+	if obj != nil && scheme != nil {
+		gvks, _, err := scheme.ObjectKinds(obj)
+		if err != nil {
+			errs = errors.Join(errs, err)
+		}
+		if len(gvks) > 0 {
+			identifier = strings.ToLower(gvks[0].Kind)
 		}
 	}
 
 	// Use requestInfo for subresource.
 	requestInfo, found := genericapirequest.RequestInfoFrom(ctx)
 	if !found {
-		err = fmt.Errorf("could not find requestInfo in context")
+		errs = errors.Join(errs, fmt.Errorf("could not find requestInfo in context"))
 	} else if len(requestInfo.Subresource) > 0 {
 		// subresource can be a path, so replace '/' with '_'
 		identifier += "_" + strings.ReplaceAll(requestInfo.Subresource, "/", "_")
@@ -324,12 +328,10 @@ func metricIdentifier(ctx context.Context, obj runtime.Object, opType operation.
 	case operation.Update:
 		identifier += "_update"
 	default:
-		if err == nil {
-			err = fmt.Errorf("unknown operation type: %v", opType)
-		}
+		errs = errors.Join(errs, fmt.Errorf("unknown operation type: %v", opType))
 		identifier += "_unknown_op"
 	}
-	return identifier, err
+	return identifier, errs
 }
 
 // ValidateDeclarativelyWithMigrationChecks is a helper function that encapsulates the logic for running declarative validation.
@@ -342,7 +344,7 @@ func ValidateDeclarativelyWithMigrationChecks(ctx context.Context, scheme *runti
 
 	takeover := utilfeature.DefaultFeatureGate.Enabled(features.DeclarativeValidationTakeover)
 
-	validationIdentifier, err := metricIdentifier(ctx, obj, opType)
+	validationIdentifier, err := metricIdentifier(ctx, scheme, obj, opType)
 	if err != nil {
 		// Log the error, but continue with the best-effort identifier.
 		klog.FromContext(ctx).Error(err, "failed to generate complete validation identifier for declarative validation")

staging/src/k8s.io/apiserver/pkg/registry/rest/validate_test.go

@@ -125,7 +125,7 @@ func TestValidateDeclaratively(t *testing.T) {
 	scheme.AddKnownTypes(internalGV, &Pod{})
 	scheme.AddKnownTypes(v1GV, &v1.Pod{})
 
-	scheme.AddValidationFunc(&v1.Pod{}, func(ctx context.Context, op operation.Operation, object, oldObject interface{}) field.ErrorList {
+	scheme.AddValidationFunc(&v1.Pod{}, func(ctx context.Context, op operation.Operation, object, oldObject any) field.ErrorList {
 		results := field.ErrorList{}
 		if op.HasOption("option1") {
 			results = append(results, invalidIfOptionErr)
@@ -735,10 +735,14 @@ func equalErrorLists(a, b field.ErrorList) bool {
 }
 
 func TestMetricIdentifier(t *testing.T) {
+	scheme := runtime.NewScheme()
+	scheme.AddKnownTypes(schema.GroupVersion{Version: "v1"}, &v1.Pod{})
+
 	testCases := []struct {
 		name        string
 		opType      operation.Type
 		obj         runtime.Object
+		scheme      *runtime.Scheme
 		subresource string
 		expected    string
 		expectErr   bool
@@ -747,6 +751,7 @@ func TestMetricIdentifier(t *testing.T) {
 			name:        "with subresource",
 			opType:      operation.Create,
 			obj:         &v1.Pod{TypeMeta: metav1.TypeMeta{Kind: "Pod"}},
+			scheme:      scheme,
 			subresource: "status",
 			expected:    "pod_status_create",
 			expectErr:   false,
@@ -755,6 +760,7 @@ func TestMetricIdentifier(t *testing.T) {
 			name:      "without subresource",
 			opType:    operation.Update,
 			obj:       &v1.Pod{TypeMeta: metav1.TypeMeta{Kind: "Pod"}},
+			scheme:    scheme,
 			expected:  "pod_update",
 			expectErr: false,
 		},
@@ -762,6 +768,7 @@ func TestMetricIdentifier(t *testing.T) {
 			name:      "unknown operation",
 			opType:    3, // not a valid operation.Type
 			obj:       &v1.Pod{TypeMeta: metav1.TypeMeta{Kind: "Pod"}},
+			scheme:    scheme,
 			expected:  "pod_unknown_op",
 			expectErr: true,
 		},
@@ -772,6 +779,29 @@ func TestMetricIdentifier(t *testing.T) {
 			expected:  "unknown_resource_create",
 			expectErr: true,
 		},
+		{
+			name:      "known type without kind",
+			opType:    operation.Update,
+			obj:       &v1.Pod{},
+			scheme:    scheme,
+			expected:  "pod_update",
+			expectErr: false,
+		},
+		{
+			name:      "unknown type with scheme",
+			opType:    operation.Create,
+			obj:       &runtime.Unknown{}, // Not registered in the scheme
+			scheme:    scheme,
+			expected:  "unknown_resource_create",
+			expectErr: true,
+		},
+		{
+			name:      "unknown type without scheme",
+			opType:    operation.Type(4),
+			obj:       &runtime.Unknown{}, // Not registered in the scheme
+			expected:  "unknown_resource_unknown_op",
+			expectErr: true,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -783,7 +813,7 @@ func TestMetricIdentifier(t *testing.T) {
 				})
 			}
 
-			result, err := metricIdentifier(ctx, tc.obj, tc.opType)
+			result, err := metricIdentifier(ctx, tc.scheme, tc.obj, tc.opType)
 			if (err != nil) != tc.expectErr {
 				t.Errorf("expected error: %v, got: %v", tc.expectErr, err)
 			}