✨ Add conversions + CEL transformations for APIResourceSchemas #2105

ncdc · 2022-09-29T22:43:10Z

Summary

Add support for CEL-based conversions for APIResourceSchemas.

Currently implemented:

Added a new APIConversion resource
Its name must match the name of an APIResourceSchema exactly
When an APIBinding is processed, if any of the APIResourceSchemas it's binding to have APIConversions, the controller creates a copy of each APIConversion in system:bound-crds
- The name of the APIConversion is set to the name of the corresponding CRD (which matches the UID of the APIResourceSchema)
Here is an example APIConversion. These are the only supported features in this PR (we can iterate in future PRs):

apiVersion: apis.kcp.dev/v1alpha1
kind: APIConversion
metadata:
  name: rev0002.widgets.example.io
spec:
  conversions:
  - from: v1
    to: v2
    rules:
    - field: .spec.firstName
      destination: .spec.name.first
    - field: .spec.lastName
      destination: .spec.name.last
      transformation: self
    - field: .spec.lastName
      destination: .spec.name.lastUpper
      transformation: self.upperAscii()
  - from: v2
    to: v1
    rules:
    - field: .spec.name.first
      destination: .spec.firstName
    - field: .spec.name.last
      destination: .spec.lastName
    preserve:
    - .spec.someNewField

When an APIConversion is created or updated, admission compiles every CEL expression, rejecting upon any errors
- We need to decide if we want to allow updates; I just realized updates are not carried forward to the copy in system:bound-crds

Possible follow-ups in separate PRs:

Figure out how to support indexing in maps and lists in CEL rules
Figure out how to support indexing in maps and lists in "to" fields
See if we can identify invalid evolutions and reject them
What happens if a field's type changes / can we support that?
Fully spec out preserve fields annotation-based flows

Related issue(s)

Fixes #

Requires kcp-dev/kubernetes#104

Part of #1671

ncdc · 2022-09-30T11:35:06Z

Nice to see that I at least didn't break anything in the e2es, even if the conversion code isn't being exercised yet 😄

ncdc · 2022-10-07T16:19:57Z

/test all

pkg/apis/apis/v1alpha1/types_apiresourceschema.go

ncdc · 2022-10-14T21:32:06Z

Pushed an update based on refactoring in kcp-dev/kubernetes#104

pkg/server/api_conversion.go

pkg/admission/apiconversion/apiconversion_admission.go

pkg/apis/apis/v1alpha1/types_apiresourceschema.go

pkg/server/api_conversion.go

stevekuznetsov

How are conversion compilation errors surfaced to the user?

pkg/admission/apiconversion/apiconversion_admission.go

stevekuznetsov · 2022-10-20T17:03:25Z

pkg/admission/apiconversion/apiconversion_admission.go

+		return nil
+	}
+
+	cluster, err := genericapirequest.ValidClusterFrom(ctx)


Totally unrelated question - I have recently hacked cluster name into admission attributes, necessary for webhook plubming. Can we rally around that and not need this "genericapirequest" stuff?

@stevekuznetsov I see that our generic webhook admission impl sets the cluster in the attrs, but I don't see it happening anywhere else. Do we need to plumb this in to the various handlers e.g. createHandler upstream?

what about the inverse, remove the getters from the attributes? Am not a big fan of changing an upstream interface.

If we don't have it up there, we have to refactor the entire call-chain to pass it through for generic webhook admission.

If needed, will do in a follow-up

#2621 to track

pkg/apis/apis/v1alpha1/types_apiresourceschema.go

pkg/conversion/conversion.go

stevekuznetsov · 2022-12-14T13:53:06Z

Part of #2014

ncdc · 2023-01-05T19:46:58Z

@stevekuznetsov

How are conversion compilation errors surfaced to the user?

Directly, as errors, implemented via admission

vincepri

~ Half way through

pkg/admission/apiconversion/apiconversion_admission.go

pkg/apis/apis/v1alpha1/types_apiresourceschema.go

pkg/conversion/conversion_rules.go

pkg/reconciler/apis/apibinding/apibinding_reconcile.go

pkg/conversion/converter.go

pkg/reconciler/apis/apibinding/apibinding_reconcile.go

pkg/conversion/conversion_rules.go

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>

vincepri

/lgtm

ncdc · 2023-01-13T20:04:37Z

/approve

openshift-ci · 2023-01-13T20:04:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc, vincepri

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ncdc]
~~config/crds/OWNERS~~ [ncdc]
~~pkg/admission/OWNERS~~ [ncdc]
~~pkg/apis/OWNERS~~ [ncdc]

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

vincepri · 2023-01-13T20:19:37Z

/lgtm

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>

vincepri · 2023-01-13T20:52:30Z

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 29, 2022

openshift-ci bot requested review from csams and qiujian16 September 29, 2022 22:43

openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Sep 29, 2022

ncdc force-pushed the cel-conversion branch from 2e594cc to 7b3103d Compare October 7, 2022 16:19

sttts reviewed Oct 7, 2022

View reviewed changes

pkg/apis/apis/v1alpha1/types_apiresourceschema.go Outdated Show resolved Hide resolved

ncdc changed the title ~~WIP: ✨ Add CEL-based conversions for APIResourceSchemas~~ ✨ Add CEL-based conversions for APIResourceSchemas Oct 7, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 7, 2022

sttts reviewed Oct 7, 2022

View reviewed changes

pkg/apis/apis/v1alpha1/types_apiresourceschema.go Outdated Show resolved Hide resolved

sttts reviewed Oct 7, 2022

View reviewed changes

pkg/apis/apis/v1alpha1/types_apiresourceschema.go Show resolved Hide resolved

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2022

ncdc force-pushed the cel-conversion branch from f590b72 to e564b4f Compare October 14, 2022 21:29

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2022

ncdc changed the title ~~✨ Add CEL-based conversions for APIResourceSchemas~~ ✨ Add conversions + CEL transformations for APIResourceSchemas Oct 14, 2022

ncdc force-pushed the cel-conversion branch 2 times, most recently from 8c50705 to cbfc29c Compare October 17, 2022 16:39

csams reviewed Oct 19, 2022

View reviewed changes

pkg/server/api_conversion.go Outdated Show resolved Hide resolved

nrb reviewed Oct 19, 2022

View reviewed changes

pkg/admission/apiconversion/apiconversion_admission.go Outdated Show resolved Hide resolved

pkg/apis/apis/v1alpha1/types_apiresourceschema.go Outdated Show resolved Hide resolved

pkg/server/api_conversion.go Outdated Show resolved Hide resolved

stevekuznetsov reviewed Oct 20, 2022

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2022

ncdc force-pushed the cel-conversion branch from cbfc29c to 853b15e Compare January 5, 2023 19:33

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 5, 2023

ncdc force-pushed the cel-conversion branch 2 times, most recently from 0b531da to 43614ec Compare January 5, 2023 20:27

ncdc changed the title ~~✨ Add conversions + CEL transformations for APIResourceSchemas~~ ✨ [WIP] Add conversions + CEL transformations for APIResourceSchemas Jan 5, 2023

ncdc force-pushed the cel-conversion branch from 70d3da3 to 30652b1 Compare January 11, 2023 22:14

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2023

stevekuznetsov mentioned this pull request Jan 12, 2023

Figure Out Correct Way To Separate Codegen Tags and GoDoc #2614

Closed

ncdc force-pushed the cel-conversion branch from 30652b1 to 87173c6 Compare January 12, 2023 19:43

vincepri reviewed Jan 12, 2023

View reviewed changes

pkg/reconciler/apis/apibinding/apibinding_reconcile.go Outdated Show resolved Hide resolved

pkg/conversion/converter.go Outdated Show resolved Hide resolved

ncdc force-pushed the cel-conversion branch from 595e650 to a5d332e Compare January 13, 2023 15:22

vincepri reviewed Jan 13, 2023

View reviewed changes

ncdc added 2 commits January 13, 2023 15:01

bump-k8s.sh: update kube branch

41bcbca

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>

Update k8s for CR conversion support

4fc4997

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>

ncdc force-pushed the cel-conversion branch from beda191 to 75d5961 Compare January 13, 2023 20:01

vincepri approved these changes Jan 13, 2023

View reviewed changes

openshift-ci bot assigned vincepri Jan 13, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2023

ncdc changed the title ~~✨ [WIP] Add conversions + CEL transformations for APIResourceSchemas~~ ✨ Add conversions + CEL transformations for APIResourceSchemas Jan 13, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2023

ncdc mentioned this pull request Jan 13, 2023

Resolve genericapirequest.ValidClusterFrom(ctx) vs. admission attributes .GetCluster() #2621

Closed

ncdc force-pushed the cel-conversion branch from 75d5961 to ad8b1ed Compare January 13, 2023 20:18

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2023

Add APIConversion support

301f064

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>

ncdc force-pushed the cel-conversion branch from ad8b1ed to 301f064 Compare January 13, 2023 20:36

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2023

openshift-merge-robot merged commit 9358474 into kcp-dev:main Jan 13, 2023

ncdc deleted the cel-conversion branch January 13, 2023 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add conversions + CEL transformations for APIResourceSchemas #2105

✨ Add conversions + CEL transformations for APIResourceSchemas #2105

ncdc commented Sep 29, 2022 •

edited

Loading

ncdc commented Sep 30, 2022

ncdc commented Oct 7, 2022

ncdc commented Oct 14, 2022

stevekuznetsov left a comment

stevekuznetsov Oct 20, 2022

ncdc Oct 20, 2022

ncdc Jan 5, 2023

sttts Jan 9, 2023

stevekuznetsov Jan 11, 2023

ncdc Jan 13, 2023

ncdc Jan 13, 2023

stevekuznetsov commented Dec 14, 2022

ncdc commented Jan 5, 2023

vincepri left a comment

vincepri left a comment

ncdc commented Jan 13, 2023

openshift-ci bot commented Jan 13, 2023

vincepri commented Jan 13, 2023

vincepri commented Jan 13, 2023

✨ Add conversions + CEL transformations for APIResourceSchemas #2105

✨ Add conversions + CEL transformations for APIResourceSchemas #2105

Conversation

ncdc commented Sep 29, 2022 • edited Loading

Summary

Related issue(s)

ncdc commented Sep 30, 2022

ncdc commented Oct 7, 2022

ncdc commented Oct 14, 2022

stevekuznetsov left a comment

Choose a reason for hiding this comment

stevekuznetsov Oct 20, 2022

Choose a reason for hiding this comment

ncdc Oct 20, 2022

Choose a reason for hiding this comment

ncdc Jan 5, 2023

Choose a reason for hiding this comment

sttts Jan 9, 2023

Choose a reason for hiding this comment

stevekuznetsov Jan 11, 2023

Choose a reason for hiding this comment

ncdc Jan 13, 2023

Choose a reason for hiding this comment

ncdc Jan 13, 2023

Choose a reason for hiding this comment

stevekuznetsov commented Dec 14, 2022

ncdc commented Jan 5, 2023

vincepri left a comment

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

ncdc commented Jan 13, 2023

openshift-ci bot commented Jan 13, 2023

vincepri commented Jan 13, 2023

vincepri commented Jan 13, 2023

ncdc commented Sep 29, 2022 •

edited

Loading