✨ crd: allow specifying spec.preserveUnknownFields #912

AlexanderYastrebov · 2024-04-10T13:21:36Z

Reinstate crd:preserveUnknownFields option removed by #607 to allow specifying the value of deprecated spec.preserveUnknownFields CRD field.

This is useful for updating CRDs that were automatically converted from v1beta1 version which had true as a default value and resulted in:

$ kubectl get crd foo.bar -o yaml | grep preserveUnknownFields
  preserveUnknownFields: true
    message: 'spec.preserveUnknownFields: Invalid value: true: must be false'

For #476

k8s-ci-robot · 2024-04-10T13:21:45Z

Hi @AlexanderYastrebov. 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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sbueringer

Makes sense to me as the alternative seems to be that users have to modify the CRD objects manually

pkg/crd/gen_integration_test.go

sbueringer · 2024-04-11T10:09:42Z

Thx!

/ok-to-test

/lgtm

Let's see if someone else has an opinion on it

(cc @vincepri @alvaroaleman )

k8s-ci-robot · 2024-04-11T10:09:48Z

LGTM label has been added.

Git tree hash: 431701314f6e96c0be966ba20abd755f0b20f418

vincepri · 2024-04-12T23:01:26Z

pkg/crd/gen_integration_test.go

+		yes := true
+		gen := &crd.Generator{
+			CRDVersions:           []string{"v1"},
+			PreserveUnknownFields: &yes,


Shouldn't we disallow adding true for v1?

As we don't support generating v1beta1 CRDs anymore, I think it's as easy as "always" disallowing true

Is it really necessary? API server does not allow preserveUnknownFields: true anyway:

kubectl apply -f pkg/crd/testdata/gen/bar.example.com_foos.yaml The CustomResourceDefinition "foos.bar.example.com" is invalid: * spec.preserveUnknownFields: Invalid value: true: must be false in order to use defaults in the schema * spec.preserveUnknownFields: Invalid value: true: cannot set to true, set x-kubernetes-preserve-unknown-fields to true in spec.versions[*].schema instead

I think we could let user specify true even though it is not supported but I added a check that prohibits that as suggested.

You can still deploy a v1 CRD with the field set to true if the CRD was before created with v1beta1?

Yes

Let me know if I should remove the check to allow true value.

Added a suggestion above to rename the exposed field and prevent a possible footgun for CRD authors

Let me know if I should remove the check to allow true value.

Yup, we should allow true. Sorry I thought that v1 CRDs cannot be deployed with true

Removed the check to allow true.

Reinstate crd:preserveUnknownFields option removed by kubernetes-sigs#607 to allow specifying the value of deprecated spec.preserveUnknownFields CRD field. This is useful for updating CRDs that were automatically converted from v1beta1 version which had true as a default value and resulted in: ``` $ kubectl get crd foo.bar -o yaml | grep preserveUnknownFields preserveUnknownFields: true message: 'spec.preserveUnknownFields: Invalid value: true: must be false' ``` For kubernetes-sigs#476

sbueringer · 2024-04-15T11:04:51Z

Thx!

/lgtm

/assign @vincepri

k8s-ci-robot · 2024-04-15T11:04:56Z

LGTM label has been added.

Git tree hash: bbc4ac1924619bfe6adfb8dca50436695ea8912d

pkg/crd/gen.go

…elds

sbueringer · 2024-04-16T09:58:38Z

/lgtm

k8s-ci-robot · 2024-04-16T09:58:43Z

LGTM label has been added.

Git tree hash: 44393af13c2f6c2c56d095f6eb888b8c5b1260da

sbueringer · 2024-04-19T09:46:28Z

/approve

k8s-ci-robot · 2024-04-19T09:46:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlexanderYastrebov, sbueringer

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~~ [sbueringer]

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

k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 10, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 10, 2024

k8s-ci-robot requested review from joelanford and vincepri April 10, 2024 13:21

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 10, 2024

AlexanderYastrebov force-pushed the add-preserveUnknownFields branch from 6397052 to e5997ba Compare April 10, 2024 13:23

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Apr 10, 2024

AlexanderYastrebov mentioned this pull request Apr 10, 2024

preserveUnknownFields=false doesn't work with crdVersions=v1 #476

Closed

sbueringer reviewed Apr 11, 2024

View reviewed changes

pkg/crd/gen_integration_test.go Show resolved Hide resolved

AlexanderYastrebov force-pushed the add-preserveUnknownFields branch from e5997ba to 818eebe Compare April 11, 2024 08:15

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 11, 2024

k8s-ci-robot assigned sbueringer Apr 11, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2024

vincepri reviewed Apr 12, 2024

View reviewed changes

AlexanderYastrebov force-pushed the add-preserveUnknownFields branch from 818eebe to dd877e4 Compare April 15, 2024 10:22

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2024

k8s-ci-robot requested a review from sbueringer April 15, 2024 10:22

k8s-ci-robot assigned vincepri Apr 15, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2024

sbueringer added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 15, 2024

vincepri reviewed Apr 15, 2024

View reviewed changes

pkg/crd/gen.go Outdated Show resolved Hide resolved

AlexanderYastrebov force-pushed the add-preserveUnknownFields branch from dd877e4 to 2c0f108 Compare April 15, 2024 16:24

crd: rename marker to DeprecatedV1beta1CompatibilityPreserveUnknownFi…

ef487eb

…elds

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2024

k8s-ci-robot merged commit e159968 into kubernetes-sigs:master Apr 19, 2024
6 checks passed

AlexanderYastrebov deleted the add-preserveUnknownFields branch April 19, 2024 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ crd: allow specifying spec.preserveUnknownFields #912

✨ crd: allow specifying spec.preserveUnknownFields #912

AlexanderYastrebov commented Apr 10, 2024 •

edited

Loading

k8s-ci-robot commented Apr 10, 2024

sbueringer left a comment

sbueringer commented Apr 11, 2024

k8s-ci-robot commented Apr 11, 2024

vincepri Apr 12, 2024

sbueringer Apr 15, 2024

sbueringer Apr 15, 2024

AlexanderYastrebov Apr 15, 2024

AlexanderYastrebov Apr 15, 2024

alvaroaleman Apr 15, 2024

AlexanderYastrebov Apr 15, 2024

vincepri Apr 15, 2024 •

edited

Loading

sbueringer Apr 15, 2024

AlexanderYastrebov Apr 15, 2024

sbueringer commented Apr 15, 2024

k8s-ci-robot commented Apr 15, 2024

sbueringer commented Apr 16, 2024

k8s-ci-robot commented Apr 16, 2024

sbueringer commented Apr 19, 2024

k8s-ci-robot commented Apr 19, 2024

✨ crd: allow specifying spec.preserveUnknownFields #912

✨ crd: allow specifying spec.preserveUnknownFields #912

Conversation

AlexanderYastrebov commented Apr 10, 2024 • edited Loading

k8s-ci-robot commented Apr 10, 2024

sbueringer left a comment

Choose a reason for hiding this comment

sbueringer commented Apr 11, 2024

k8s-ci-robot commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer commented Apr 15, 2024

k8s-ci-robot commented Apr 15, 2024

sbueringer commented Apr 16, 2024

k8s-ci-robot commented Apr 16, 2024

sbueringer commented Apr 19, 2024

k8s-ci-robot commented Apr 19, 2024

AlexanderYastrebov commented Apr 10, 2024 •

edited

Loading

vincepri Apr 15, 2024 •

edited

Loading