🐛 Add XPreserveUnknownFields to runtime.RawExtension

[eddycharly]

This PR adds XPreserveUnknownFields to runtime.RawExtension.

runtime.RawExtension has type: object but all fields will be pruned unless x-kubernetes-preserve-unknown-fields: true is specified.

It seems reasonable to add x-kubernetes-preserve-unknown-fields: true by default, as was done with apiextensions.JSON for example

controller-tools/pkg/crd/known_types.go

Lines 106 to 111 in a260f13

	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1": func(p *Parser, pkg *loader.Package) {
	p.Schemata[TypeIdent{Name: "JSON", Package: pkg}] = apiext.JSONSchemaProps{
	XPreserveUnknownFields: boolPtr(true),
	}
	p.AddPackage(pkg) // get the rest of the types
	},

[k8s-ci-robot]

Hi @eddycharly. 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.

[eddycharly]

/assign @joelanford

[joelanford]

I think my main concern is that anyone already using runtime.RawExtension with controller-gen is already generating a CRD schema without x-kubernetes-preserve-unknown-fields. This PR would cause those users to get a potentially undesired CRD schema change.

At a minimum, I think I'd label this as a breaking change rather than a bug fix. We might also need to consider making this behavior opt-in.

@benluddy was actually chatting with me the other day about a similar request to improve the auto-generated schema for time.Duration, which would make the schema more strict to align with the expectations of time.ParseDuration.

In this case, we would start allowing fields not mentioned in the schema to be preserved, which seems a little bit different than making a schema more strict.

Maybe @alvaroaleman and @vincepri have opinions?

[alvaroaleman]

I think my main concern is that anyone already using runtime.RawExtension with controller-gen is already generating a CRD schema without x-kubernetes-preserve-unknown-fields. This PR would cause those users to get a potentially undesired CRD schema change.

But if ppl do that without x-kubernetes-preserve-unknown-fields. then everything in that field will be pruned today or not, as its schema is unknown at the CRD level?

/ok-to-test

If we merge this, it will need a test btw.

[eddycharly]

I wonder how one can use runtime.RawExtension without x-kubernetes-preserve-unknown-fields.

It looks like it just doesn’t work to me, all fields get pruned and the object ends up empty 🤔

[eddycharly]

But if ppl do that without x-kubernetes-preserve-unknown-fields. then everything in that field will be pruned today or not, as its schema is unknown at the CRD level?

Exactly what we observed, we ended up with empty objects, all fields were pruned.

[eddycharly]

This looks strange, not sure where I should look at.

[eddycharly]

@joelanford @alvaroaleman do you have an idea where I should look in the code to find why this gets transformed into allOf ?

[eddycharly]

From what I understand it happens in the flattener. I’m a bit lost in the code though. Any hint would be appreciated.

[eddycharly]

I think it happens when kubebuilder:pruning:PreserveUnknownFields is present on a field and the target type. I can reproduce with any struct. Is this a bug ? Should both markers get merged together when generating the CRD ?
cc @alvaroaleman @joelanford

[FillZpp]

This PR could solve the question asked on stackoverflow and slack https://stackoverflow.com/questions/72577439/k8s-operator-read-raw-data .

Thanks @eddycharly , and this should be merged after #689 to avoid the allOf: x-kubernetes-preserve-unknown-fields. Am I right?

[eddycharly]

@FillZpp Yes #689 was a prerequisite for this one.

[alvaroaleman]

/label tide/merge-method-squash

[eddycharly]

Rebased and updated manifest.

[FillZpp]

/lgtm

[alvaroaleman]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, eddycharly

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 [alvaroaleman]

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

[alvaroaleman]

thanks!

[eddycharly]

@alvaroaleman do I need to create a cherry pick PR ?

[prateekpandey14]

/cherrypick release-0.9

[k8s-infra-cherrypick-robot]

@prateekpandey14: only kubernetes-sigs org members may request cherry picks. You can still do the cherry-pick manually.

In response to this:

/cherrypick release-0.9

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.

[FillZpp]

/cherrypick release-0.9

[k8s-infra-cherrypick-robot]

@FillZpp: new pull request created: #699

In response to this:

/cherrypick release-0.9

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.

[FillZpp]

/cherrypick release-0.9

[k8s-infra-cherrypick-robot]

@FillZpp: new pull request created: #703

In response to this:

/cherrypick release-0.9

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.