Support ConversionReview for webhooks

[clux]

Problem

I would like to be able to improve the state of CRD versioning and supply an easy extra endpoint to an existing admission controller capable of dealing with version conversion and help avoid problems like #774.

This feature is outlined in webhook conversion, and the structs are defined in apiextensions-apiserver/types.go (very similar to AdmissionReview)

See also versions in custom resource definitions.

Solution

A similar job to implementing the structs in kube::core::admission that we did for the initial admission controller feature (but without the hard setup parts since that's mostly done).

Note that these structs are also not in k8s-openapi and need special impls from us in kube-core. We also need to extend or write a new example with the new webhook behaviour.

Edit: We also need to support the conversion flag for CRDs in kube-derive. But can lift this to a separate issue once we have away to test it.

Documentation

This needs an example and needs documentation. Documentation should also ideally be added as part of the controller guide in kube-rs/website#5 but that can come later.

[clux]

Core types for this have been added in #999. Outstanding is integration with kube-derive to specify the how to do the conversion, which currently has to be done manually by tweaking the CRD.

[soenkeliebau]

I've read up on this issue a bit and just want to see if I understood the outstanding bit of work correct :)

Basically the plumbing is all there to create the actual webhook along the same lines as shown in https://github.com/kube-rs/kube/blob/main/examples/admission_controller.rs just with the ConversionReview types as parameters instead.

The missing piece is to extend kube-derive to allow setting the conversion field of the CRD to webhook and specify details to reach that service?
This would probably be mostly manual configuration as I presume the necessary service, pod, ... objects that influence this would be part of deploying the operator and not known to kube-rs?

which currently has to be done manually by tweaking the CRD

When you say "manually tweaking" do you mean manually after kube-rs has generated the crd, or is there a way in kube-rs to add the conversion settings programmatically? I haven't been able to find how I could do this atm without this issue being resolved, but thats probably just me :)

Edit:
As usual I had an epiphany after commenting..

I presume the way to do this would be to use merge_crds() and then set the conversion property on the resulting object before applying this or writing it to a file?

[clux]

Extremely late reply @soenkeliebau

do you mean manually after kube-rs has generated the crd, or is there a way in kube-rs to add the conversion settings programmatically?

I originally meant that we could set the conversion property on the crd in kube-derive in the crdspec here:

kube/kube-derive/src/custom_resource.rs

Lines 396 to 415 in 2e5e4de

	"spec": {
	"group": #group,
	"scope": #scope,
	"names": {
	"categories": categories,
	"plural": #plural,
	"singular": #name,
	"kind": #kind,
	"shortNames": shorts
	},
	"versions": [{
	"name": #version,
	"served": true,
	"storage": true,
	"schema": {
	"openAPIV3Schema": schema,
	},
	"additionalPrinterColumns": columns,
	"subresources": subres,
	}],

but you might be right that this is a property best set outside, since it only applies when users have multiple crd instances all deriving CustomResource, and in those cases it feels repetitive to set it everywhere.

Maybe the best way forward is to simply add a helper method to configure the property (if needed) via https://github.com/kube-rs/kube/blob/main/kube-core/src/crd.rs

[soenkeliebau]

No worries @clux :)

I played around with this a bit back in the day: https://github.com/stackabletech/hdfs-operator/blob/f0c4140d5b0487aa08813005feb2f8f853efbc6d/rust/operator-binary/src/stackable-hdfs-operator.rs#L83

And that seemed to work ok-ish. Not terribly convenient, but I guess a helper method would help (pun intended) here.

I guess a more integrated approach would be a "top level" struct that can be annotated with conversion settings and the supported versions which then hides away the call to merge_crds and adding the conversion settings.

Sort of like this

pub mod versioned {
    use super::*;

    #[derive(VersionedCustomResource)]
    #[kube(
    versions = ["v1", "v2"],
    conversion = "...",
    )]
    pub struct HdfsClusterSpec {
        //...
    }
}

But this was just written down pretty much as I thought about it, there will be about 30 issues with this that I have not thought about yet :)

Update: one obvious drawback with this approach is that you need to know the conversion config at compile time which would make it impossible to configure in helm charts and the like.
So would definitely need to be at least overrideable..

And then we are back at the helper fn ... my vote would be to add the helper fn and see how that works out.

[clux]

Ah, thank you, that's very helpful.
Given how large the Webhook struct is, it's probably not practical to do a #[kube(conversion = ...)] attr for this.

We could potentially export a fn here that lets the user pass in a WebhookConversion :

fn set_conversion_webhook(mut crd: Crd, webhook: WebhookConversion) -> Crd {
   crd.spec.conversion = Some(CustomResourceConversion {
        strategy: "Webhook".to_string(),
        webhook: Some(webhook),
    }
    crd
}

since that's at least a safe go-enum setter. however, this is a fairly small and awkward thing to import as a simple fn, so probably not the biggest deal to users.

maybe it makes sense to have a pub trait CustomResourceDefinitionExt to bundle these various helpers on as a long term thing if there is more behaviour that's commonly implemented on CustomResourceDefinition.

[clux]

helm configuration of webhooks

It feels pretty awkward to configure properties on a crd in helm charts to me. They are generally assumed to be static enough to be installable OOB.

If you're that deep into the operator world that you need conversion webhooks, wouldn't it make more sense to streamline the installation for users so that the operator (with the conversion hook) is installed in the place the crd is pointing at by default?

[soenkeliebau]

For 99% of use cases I agree, yes.

The scenario that I was thinking of is a user running two different versions of, say, the hdfs operator in the same Kubernetes. Both operators are configured to only reconcile resources in specific namespaces so they do not fight with each other.

Since the CRD is cluster scoped we can only point at one of the operators at any given time in the conversion webhook, and this has to be the higher version, because only this will know how to convert all versions.

By hard coding this in the Helm chart, that distinction becomes hard I think..

But, thinking about it a bit more, the better way would probably be to include a service in the helm chart that points at the operator and target the correct operator version via a labelselector..

So in the CRD we simply say "hdfs-conversion-hook" as the service name and in our helm charts we make sure that that service has a label selector pointing at the latest installed operator version, so that no traffic is routed to older operators.

Scenarion is a bit niche I guess though ..

[soenkeliebau]

Do we want to move forward with the helper fn you sketched above?

If so, should we move it straight to a CustomResourceDefinitionExt or wait until more candidates raise their head?

Happy to create a PR, even though you pretty much already wrote all the code above :)

[clux]

Let's wait for now to see if there are other useful things that pop up. I've referenced it from #428. Given that there's nothing we really should do in kube-derive, we can close this issue as done :-)

[soenkeliebau]