restored api-conversion implementation by olamilekan000 · Pull Request #4075 · kcp-dev/kcp

olamilekan000 · 2026-04-27T16:55:22Z

Summary

changes restores the  APIConversion  implementation

What Type of PR Is This?

/kind feature

Related Issue(s)

Fixes 3427

Release Notes

added APIConversion implementation

kcp-ci-bot · 2026-04-27T16:55:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign embik for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS
pkg/admission/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

olamilekan000 · 2026-04-27T18:50:46Z

While testing the API conversions, the conversion from one api to the other was failing for standard local CRDs because the admission plugin strictly required an APIResourceSchema to exist. Since pure local CRDs don't generate those schemas (it has to be generated through apibinding and apiexport), the apiconversion webhook was rejecting them with a NotFound error. To fix this, I added a provision in the admission validator using the dynamic client to check if a matching CustomResourceDefinition exists locally instead. If it does, we gracefully bypass the strict schema mapping and let the CEL interpreter handle the validation at runtime.

mjudeikis

How much of this is original implementation and how much is your change? I feel like original implementation had few corners cut :/ Tried to leave few comments but I suspect there will be more

mjudeikis · 2026-04-29T08:08:34Z

+	defer f.lock.Unlock()
+
+	if f.delegate != nil {
+		return f.delegate, nil


If we update APIResources post create (but dont update CRD) - Think this will return stale converter?

The problem in plain terms: once the first conversion happens, f.delegate is populated with a *Converter whose CEL programs were compiled against whatever APIConversion existed at that moment. There's no read of the lister on subsequent calls, no resourceVersion check, no event handler — so a kubectl edit apiconversion ... does nothing for already-running converters in this process.

We need to track APIConversion RV to see if converted is still valid.

mjudeikis · 2026-04-29T08:16:31Z

+		// to use pure local CRDs with the CEL engine. This fallback natively inspects the local Virtual Workspace
+		// for a raw CustomResourceDefinition matching the conversion rule.
+		if crdErr == nil {
+			klog.FromContext(ctx).


Maybe this would allow not to skip?

if crdErr == nil { crdU, _ := o.dynamicClient.Cluster(...).Resource(gvr).Get(...) // already retrieved above so we have the object already crd := &apiextensionsv1.CustomResourceDefinition{} if err := runtime.DefaultUnstructuredConverter.FromUnstructured(crdU.Object, crd); err != nil { return admission.NewForbidden(a, fmt.Errorf("decoding CRD: %w", err)) } structuralSchemas, err := buildStructuralSchemasFromCRD(crd.Spec.Versions) if err != nil { return admission.NewForbidden(a, err) } apiConversion := &apisv1alpha1.APIConversion{} if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, apiConversion); err != nil { return fmt.Errorf("failed to convert unstructured to APIConversion: %w", err) } if _, err := conversion.Compile(apiConversion, structuralSchemas); err != nil { return fmt.Errorf("error compiling conversion rules: %w", err) } return nil }

so we don't need for the validation?

mjudeikis · 2026-04-29T08:18:13Z

-	if err != nil {
-		return nil, err
-	}
+	_ = authInfoResolver


Why do we do this? It's not used?

Shouldn't this also be caught by the linter?

I was trying to retain some of the changes from the old PR, hence why I left it that way. I think it can be removed

mjudeikis · 2026-04-29T08:18:32Z

-			ConversionFactory: &nopCRConversionFactory{},
+			ConversionFactory: kcpconversion.NewCRConverterFactory(
+				c.KcpSharedInformerFactory.Apis().V1alpha1().APIConversions(),
+				5*time.Second,


opts.Extra.ConversionCELTransformationTimeout ?

mjudeikis · 2026-04-29T08:21:59Z

+func compileRule(rulePath *field.Path, rule apisv1alpha1.APIConversionRule, schema *structuralschema.Structural) (*CompiledRule, error) {
+	// if rule.Field starts with a ".", such as .spec.fieldName, strip the leading "."
+	fromPath := rule.Field
+	if fromPath[0] == '.' {


This function is called from Compile, and from NewConverter, hence toPath and fromPath never checked for len

mjudeikis · 2026-04-29T08:22:55Z

+	if errors.IsNotFound(err) {
+		switch f.crd.Spec.Conversion.Strategy {
+		case apiextensionsv1.NoneConverter:
+			return conversion.NewNOPConverter(), nil


We should be setting f.delegate = conversion.NewNOPConverter() else this code path will be hot on every request

mjudeikis · 2026-04-29T08:23:20Z

@@ -0,0 +1,83 @@
+metadata:


Is this even used?

It's used in the unit tests.

olamilekan000 · 2026-04-29T08:31:47Z

How much of this is original implementation and how much is your change? I feel like original implementation had few corners cut :/ Tried to leave few comments but I suspect there will be more

Almost all is from the original PR. I only refactored the CEL flow to use EnvSet and StoredExpressionsEnv, and also updated the admission plugin with that dynamic client fallback so it also handles local CRDs instead of just looking for APIResourceSchemas.

ntnn

I think this skips the .ConvertToVersion that is implemented for kcp's native types, or?
The old conversion webhook in pkg/server (that did this conversion) should be refactored into the new conversion factory.

ntnn · 2026-04-30T08:28:33Z

-	if err != nil {
-		return nil, err
-	}
+	_ = authInfoResolver


Shouldn't this also be caught by the linter?

ntnn · 2026-04-30T08:33:19Z

@@ -0,0 +1,83 @@
+metadata:


It's used in the unit tests.

ntnn · 2026-05-28T07:10:42Z

-			ConversionFactory: &nopCRConversionFactory{},
+			ConversionFactory: kcpconversion.NewCRConverterFactory(
+				c.KcpSharedInformerFactory.Apis().V1alpha1().APIConversions(),
+				5*time.Second,


I'd prefer this be a constant with a comment explaining why 5s.

ntnn · 2026-05-28T07:17:54Z

+	}
+
+	ast, issues := env.Compile(rule.Transformation)
+	if issues != nil {


Suggested change

if issues != nil {

if issues.Err() != nil {

issues is never nil iirc

ntnn · 2026-05-28T07:35:33Z

 		{Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Alpha},
 	},
+	APIConversion: {
+		{Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Alpha},


Suggested change

{Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Alpha},

{Version: version.MustParse("1.33"), Default: false, PreRelease: featuregate.Alpha},

Since the rebase landed inbetween

ntnn · 2026-05-28T07:36:12Z


+	// Ensure the cache server natively listens for APIConversions
+	_ = c.KcpSharedInformerFactory.Apis().V1alpha1().APIConversions().Informer()


Suggested change

// Ensure the cache server natively listens for APIConversions

_ = c.KcpSharedInformerFactory.Apis().V1alpha1().APIConversions().Informer()

The same is a few lines below

ntnn · 2026-05-28T07:37:00Z

+		APIEnablement:                      genericoptions.NewAPIEnablementOptions(),
+		EmbeddedEtcd:                       *etcdoptions.NewOptions(rootDir),
+		Logs:                               logs.NewOptions(),
+		ConversionCELTransformationTimeout: time.Second,


Is the 1s timeout arbitrary or do we have an educated guess on this default? I think a comment explaining why that is the default would be good.

ntnn · 2026-05-28T07:40:44Z

+		_, crdErr := o.dynamicClient.Cluster(cluster.Name.Path()).Resource(gvr).Get(ctx, u.GetName(), metav1.GetOptions{})
+
+		// In kcp, local CustomResourceDefinitions do not generate APIResourceSchemas.
+		// Therefore, restricting this validation entirely to the APIResourceSchema lister breaks the ability
+		// to use pure local CRDs with the CEL engine. This fallback natively inspects the local Virtual Workspace
+		// for a raw CustomResourceDefinition matching the conversion rule.
+		if crdErr == nil {
+			crdU, err := o.dynamicClient.Cluster(cluster.Name.Path()).Resource(gvr).Get(ctx, u.GetName(), metav1.GetOptions{})


These two GETs are identical. Should the second one use a different client?

ntnn · 2026-05-28T07:41:25Z

+func (o *apiConversionAdmission) ValidateInitialization() error {
+	if o.getAPIResourceSchema == nil {
+		return fmt.Errorf("getAPIResourceSchema is unset")
+	}
+
+	return nil
+}


Missing check the dynamicClient is not nil

ntnn · 2026-05-28T07:43:10Z

+		if crd.Spec.Conversion != nil && crd.Spec.Conversion.Strategy != apiextensionsv1.NoneConverter {
+			return nil, fmt.Errorf("conversion strategy %q is not supported for CRD %s", crd.Spec.Conversion.Strategy, crd.Name)
+		}


This errors for webhook converters, or? Wouldn't that also prevent e.g. the webhook converter on CRDs that have it configured?

Yes it will prevent them. so, if strategy is anything aside "none" and conversion is disabled, we throw an error. Or to put it another way, if strategy is Webhook and the gate is off, we throw an error because kcp doesn’t support conversion webhooks

olamilekan000 · 2026-05-28T08:32:23Z

/retest

olamilekan000 · 2026-06-01T08:06:45Z

@ntnn I haven't been chanced to properly look at your feedback, and I do not want to respond hastily to it. Once im a bit free, I'll respond do not also hesitate to let me know if it's blocking anything. Thanks

olamilekan000 · 2026-07-17T00:33:36Z

/retest

Signed-off-by: olalekan odukoya <odukoyaonline@gmail.com>

olamilekan000 · 2026-07-17T02:14:46Z

/retest

olamilekan000 · 2026-07-17T02:36:05Z

/retest

olamilekan000 · 2026-07-20T08:29:59Z

/retest

kcp-ci-bot · 2026-07-20T08:46:02Z

@olamilekan000: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kcp-test-e2e-sharded-embedded-vw	`e1f55d0`	link	false	`/test pull-kcp-test-e2e-sharded-embedded-vw`

Full PR test history

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

mjudeikis

We are regressing conversion api.

To confirm this we need to add e2e, because tests use the storage version (v1alpha2) and rarely request v1alpha1, so conversion never fires. We need to add an e2e that GETs a stored v1alpha2 APIBinding as v1alpha1 (and vice-versa) and asserts permission-claim fields round-trip.

mjudeikis · 2026-07-20T12:07:14Z

-	if err != nil {
-		return nil, err
-	}
+	conversionFactory := kcpconversion.NewCRConverterFactory(


This breaks existing our conversion machinery. We need e2e test to close the gap, and we need better handling for this.

my ai agent reivew bellow:

Both [pkg/server/config.go:719](vscode-webview://0pikil8f7g1qhrf2uek89gkocorlns8ks47u870endnhaeslejc6/pkg/server/config.go#L719) and [pkg/cache/server/config.go:262](vscode-webview://0pikil8f7g1qhrf2uek89gkocorlns8ks47u870endnhaeslejc6/pkg/cache/server/config.go#L262) replace the conversion factory with kcpconversion.NewCRConverterFactory. The old factory ([pkg/server/conversion_webhook.go:60-69](vscode-webview://0pikil8f7g1qhrf2uek89gkocorlns8ks47u870endnhaeslejc6/pkg/server/conversion_webhook.go#L60)) special-cased the apis.kcp.io group and ran native scheme-based conversion for it: if crd.Spec.Group == apis.GroupName { return &schemaBasedConverter{...}, nil // scheme.ConvertToVersion } return f.delegate.NewConverter(crd) That handler was added deliberately (commit dad2aea9e "add conversion handler for core APIs") to convert APIBinding/APIExport between v1alpha1 and v1alpha2 — a non-trivial hand-written conversion (permission-claim reshaping + JSON round-trip annotations, see staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/types_apibinding_conversion.go). Storage version is v1alpha2; both versions are served. The new factory has no group special-casing. For an apibindings.apis.kcp.io CRD (whose spec.conversion.strategy: None), the deferred converter resolves to NewNOPConverter(): getAPIConversion(...) → NotFound (no user APIConversion for core types) crd.Spec.Conversion.Strategy == None → [deferred_converter.go:112](vscode-webview://0pikil8f7g1qhrf2uek89gkocorlns8ks47u870endnhaeslejc6/pkg/conversion/deferred_converter.go#L112) returns NOP. And crucially this happens even with the feature gate off — [conversion_factory.go:62-67](vscode-webview://0pikil8f7g1qhrf2uek89gkocorlns8ks47u870endnhaeslejc6/pkg/conversion/conversion_factory.go#L62) also returns NOP for None-strategy CRDs. So a client requesting an APIBinding/APIExport in a non-storage version gets the object back with only apiVersion relabeled and fields not converted → corrupt/lossy response and a real v1alpha1 API-compatibility break. This likely escapes existing e2e because tests use the storage version (v1alpha2) and rarely request v1alpha1, so conversion never fires. Recommend adding an e2e that GETs a stored v1alpha2 APIBinding as v1alpha1 (and vice-versa) and asserts permission-claim fields round-trip. Fix: the new CRConverterFactory.NewConverter must keep the apis.GroupName → scheme-based-converter branch (delegate to the existing schemaBasedConverter) before falling through to the deferred CEL/NOP path.

mjudeikis · 2026-07-20T12:08:13Z

+			switch f.crd.Spec.Conversion.Strategy {
+			case apiextensionsv1.NoneConverter:
+				// handled below
+			case apiextensionsv1.WebhookConverter:


We are dropping this. But do we support this?

mjudeikis · 2026-07-20T12:09:53Z

+		return nil, fmt.Errorf("error executing transformation: %w", err)
+	}
+
+	return evalResult.Value(), nil


This will panic for some reason; this is not JSON payload. (Internally, it uses runtime.DeepCopyJSONValue )

kcp-ci-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 27, 2026

olamilekan000 force-pushed the re-implemt-api-conversions branch 2 times, most recently from 262acd0 to 6d14148 Compare April 27, 2026 17:22

olamilekan000 requested a review from mjudeikis April 27, 2026 17:24

olamilekan000 force-pushed the re-implemt-api-conversions branch 3 times, most recently from cfcdaf9 to 6165992 Compare April 27, 2026 19:10

mjudeikis reviewed Apr 29, 2026

View reviewed changes

ntnn reviewed Apr 30, 2026

View reviewed changes

ntnn added this to tbd May 4, 2026

ntnn moved this to Reviewing in tbd May 4, 2026

olamilekan000 force-pushed the re-implemt-api-conversions branch from 6165992 to a89c072 Compare May 6, 2026 00:12

olamilekan000 requested review from mjudeikis and ntnn May 6, 2026 00:13

olamilekan000 force-pushed the re-implemt-api-conversions branch from a89c072 to b76ced7 Compare May 6, 2026 00:34

kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2026

ntnn force-pushed the re-implemt-api-conversions branch from b76ced7 to 46a7741 Compare May 28, 2026 05:33

kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2026

ntnn force-pushed the re-implemt-api-conversions branch from 46a7741 to f38d258 Compare May 28, 2026 07:23

ntnn reviewed May 28, 2026

View reviewed changes

kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2026

olamilekan000 force-pushed the re-implemt-api-conversions branch from f38d258 to ec1dca0 Compare July 16, 2026 21:14

kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2026

olamilekan000 force-pushed the re-implemt-api-conversions branch from ec1dca0 to 3ba2a08 Compare July 16, 2026 21:26

restored api-conversion implementation

e1f55d0

Signed-off-by: olalekan odukoya <odukoyaonline@gmail.com>

olamilekan000 force-pushed the re-implemt-api-conversions branch from 3ba2a08 to e1f55d0 Compare July 17, 2026 01:01

mjudeikis reviewed Jul 20, 2026

View reviewed changes

	{Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Alpha},
	{Version: version.MustParse("1.33"), Default: false, PreRelease: featuregate.Alpha},


		// Ensure the cache server natively listens for APIConversions
		_ = c.KcpSharedInformerFactory.Apis().V1alpha1().APIConversions().Informer()

Uh oh!

Conversation

olamilekan000 commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What Type of PR Is This?

Related Issue(s)

Release Notes

Uh oh!

kcp-ci-bot commented Apr 27, 2026

Uh oh!

olamilekan000 commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjudeikis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olamilekan000 commented Apr 29, 2026

Uh oh!

ntnn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olamilekan000 Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olamilekan000 commented May 28, 2026

Uh oh!

olamilekan000 commented Jun 1, 2026

Uh oh!

olamilekan000 commented Jul 17, 2026

Uh oh!

olamilekan000 commented Jul 17, 2026

olamilekan000 commented Apr 27, 2026 •

edited

Loading

olamilekan000 commented Apr 27, 2026 •

edited

Loading

olamilekan000 Jul 17, 2026 •

edited

Loading