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.

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

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

restored api-conversion implementation

b76ced7

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

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

olamilekan000 commented Apr 27, 2026 •

edited

Loading

olamilekan000 commented Apr 27, 2026 •

edited

Loading