🐛 Handle deprecated preserveUnknownField in CRD spec

[kaushik229]

What this PR does / why we need it: In Kubernetes 1.20 the spec.preserveUnknownFields is not allowed to be set to true given Kubernetes has already deprecated this any cluster >=1.20 will not be able sync these resources, instead if a CRD has the field still set this disables that.

[k8s-ci-robot]

Welcome @kaushik229!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-nested 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-nested has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

[k8s-ci-robot]

Hi @kaushik229. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[christopherhein]

/ok-to-test

[christopherhein]

/retitle 🐛 Handle deprecated preserveUnknownField in CRD spec

[Fei-Guo]

I'd like to understand the case better. The problem will happen when super cluster runs k8s < 1.20 and vc runs k8s > 1.20. Should we support this? I believe we will have more compatibility issues if vc runs higher version than super.

[christopherhein]

The use case is a super cluster running 1.20 with a CRD that was created under the v1beta1 API where preserveUnknownField: true, and having it labeled to sync into another tenant control plane running v1.20, the object comes across with the deprecated field sent to true and will fail creation because of this.

The "easy" answer is set the super cluster version to false, which I 100% agree with, but where this causes issues is if folks use unknown fields their resources fail creation.

[Fei-Guo]

The use case is a super cluster running 1.20 with a CRD that was created under the v1beta1 API where preserveUnknownField: true, and having it labeled to sync into another tenant control plane running v1.20, the object comes across with the deprecated field sent to true and will fail creation because of this.

The "easy" answer is set the super cluster version to false, which I 100% agree with, but where this causes issues is if folks use unknown fields their resources fail creation.

Sure, but on the other hand, this change may silently create discrepancy between vc cr and super cr - the persisted data can be different. Shouldn't we suggest user to resolve the super crd first? If the unknown fields are necessary, just adding the validation schemas should be sufficient, right?

[kaushik229]

@Fei-Guo I'm new to this and was reading up a little on this. It seems this is little tricky but few recommendation I found asks to set it to false. https://github.com/kubernetes/kubernetes/issues/95702#issuecomment-713548542

[christopherhein]

Sure, but on the other hand, this change may silently create discrepancy between vc cr and super cr - the persisted data can be different. Shouldn't we suggest user to resolve the super crd first? If the unknown fields are necessary, just adding the validation schemas should be sufficient, right?

Definitely true, but also in that case the CRDs won't ever be synced at all cause they will fail the CRD creation validation in the tenant cluster.

[Fei-Guo]

Definitely true, but also in that case the CRDs won't ever be synced at all cause they will fail the CRD creation validation in the tenant cluster.

My suggestion is to set the field to false and correct all validation schema if unknown fields are necessary in super CRD before syncing it to tenant. We force users to do this which I believe is a good experience from management perspective. Do you think this is impractical? Maybe there are some other blocking factors that I am not aware of.

[christopherhein]

For us its a management issue, tbh, we can't update our CRD without breaking existing clients and we can't get these CRDs to sync so folks can use "valid" CRDs. Our alternative is to disable this syncer and manually sync the CRD with the spec.preserveUnknownFields: false in the tenant clusters.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christopherhein, kaushik229

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: - ~~[virtualcluster/OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-nested/blob/main/virtualcluster/OWNERS)~~ [christopherhein] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment