Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversion support for XRD versions? #2608

Closed
1 task
turkenh opened this issue Sep 30, 2021 · 11 comments · Fixed by #3940
Closed
1 task

Conversion support for XRD versions? #2608

turkenh opened this issue Sep 30, 2021 · 11 comments · Fixed by #3940
Assignees
Projects
Milestone

Comments

@turkenh
Copy link
Member

turkenh commented Sep 30, 2021

What problem are you facing?

Crossplane allows you to specify multiple versions in an XRD but does not support providing a conversion strategy.

Note that all versions must (by convention) have identical schemas; generated CRDs always use the 'None' conversion strategy to convert between the schemas of different CR versions. Crossplane may support other conversion strategies (e.g. webhooks or declarative conversion) in future.

See the related discussion: #2605

How could Crossplane help solve your problem?

Consider adding support for a conversion strategy for XRD versions.

Related Issues

@turkenh turkenh added the enhancement New feature or request label Sep 30, 2021
@negz
Copy link
Member

negz commented Oct 11, 2021

Thanks for raising this @turkenh - I didn't realise we didn't have an issue tracking it. As you pointed out, we deferred this for now because it seems like the only option would be to have folks write conversion webhooks (presumably in Go), which would somewhat defeat the "no code" nature of XRs. I think the first step here would be exploring what a declarative conversion strategy would look like. Some prior thought on this can be found upstream here.

@negz negz added this to Prioritised (Approx. High to Low) in Roadmap via automation Jan 4, 2022
@stale
Copy link

stale bot commented Aug 14, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 14, 2022
@muvaf muvaf removed the stale label Aug 17, 2022
@yhrn
Copy link

yhrn commented Oct 19, 2022

Hi,

We are evaluating Crossplane for our internal platform and this limitation is likely blocker for us. Has there been any further thinking or progress? Is webhook conversion definitely off the table?

I understand you want to stay true to the "no code" idea but if such a conversion strategy is so hard to implement that it just won't happen it really means that XRs misses out on one of the most fundamental and important features of k8s APIs.

@negz
Copy link
Member

negz commented Oct 25, 2022

There's tentative plans within Kubernetes API machinery to explore CEL based declarative conversion (after the recent CEL based validation work). I expect this could get us this functionality "for free". I'll try find a link.

In the meantime we're not opposed to allowing webhook based conversion to bridge the gap.

@github-actions
Copy link

Crossplane does not currently have enough maintainers to address every issue and pull request. This issue has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 7 days if no further activity occurs. Leaving a comment starting with /fresh will mark this issue as not stale.

@github-actions github-actions bot added the stale label Jan 24, 2023
@jbw976
Copy link
Member

jbw976 commented Jan 24, 2023

/fresh

This still sounds important for the long term story of migrating XRDs and the APIs they expose over time.

@negz
Copy link
Member

negz commented Mar 2, 2023

Supporting multiple versions in a declarative way - i.e. without writing conversion code - seems like a pretty hard problem. Tackling that problem is not on the horizon for us at the moment. I do recall that @ncdc prototyped CEL based conversion but I can't immediately find a reference to that.

I believe it would be straightforward to enable the webhook conversion strategy. The good news is that this would make supporting multiple heterogeneous API versions possible. That bad news is you'd need to write and run your own conversion webhook to do so. See https://book.kubebuilder.io/multiversion-tutorial/tutorial.html for details on how you'd do that.

https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#configure-customresourcedefinition-to-use-conversion-webhooks

If there is a demand for this, I believe we'd just need to:

  • Mirror the spec.conversion configuration of the CRD type in the XRD type.
  • Allow strategy: Webhook.
  • Propagate spec.conversion from XRDs to CRDs.

@negz negz added this to the v1.12 milestone Mar 2, 2023
@mithie
Copy link

mithie commented Mar 3, 2023

Thanks @negz for adding this to the v1.12 milestone. We would highly appreciate if that feature was available.

@ncdc
Copy link

ncdc commented Mar 3, 2023

We have an APIConversion type in kcp that looks like this:

apiVersion: apis.kcp.io/v1alpha1
kind: APIConversion
metadata:
  name: rev0002.widgets.example.io
  annotations:
    bootstrap.kcp.io/create-only: ""
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

The conversion aspects are not specifically CEL - they're just path notations. You can use CEL to perform transformations if needed, however.

This is not in Kubernetes at the moment, though. Just kcp.

@phisco
Copy link
Contributor

phisco commented Apr 3, 2023

Looks like we have a valid approach and given the number of "👍" also some agreement on your proposal, @negz. If no one raises any objection in the meantime, tomorrow I'll start working on it.

@phisco
Copy link
Contributor

phisco commented Apr 4, 2023

In #3940 I reused the CRD struct straight away, as the only values supported are Webhook and None, the default and current value. Let me know if you see any reason to define our own struct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Roadmap
Prioritised (Approx. High to Low)
Development

Successfully merging a pull request may close this issue.

8 participants