From 391045e0f3e163b777d62d16ba7e958b2c9f4325 Mon Sep 17 00:00:00 2001 From: Lalit Chauhan Date: Wed, 10 Dec 2025 23:52:24 +0000 Subject: [PATCH] refactor: Ensure metricIdentifier uses scheme for kind resolution The metricIdentifier function in pkg/registry/rest/validate.go has been updated to consistently use for determining the resource kind. This change ensures that the identifier is derived from the scheme, which is the authoritative source for API type information. Corresponding unit tests in pkg/registry/rest/validate_test.go have been updated to align with this new behavior, explicitly passing the scheme in test cases where kind resolution is expected and verifying the correct unknown_resource fallback when the scheme or object is not sufficient to determine the kind. --- .../apiserver/pkg/registry/rest/validate.go | 26 +++++++------- .../pkg/registry/rest/validate_test.go | 34 +++++++++++++++++-- 2 files changed, 46 insertions(+), 14 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go index 05606c7e4b5..65ffdd4f16d 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go +++ b/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") diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate_test.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/validate_test.go index 81a1786abf3..b1218e01918 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate_test.go +++ b/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) }