Decoding should not clear apiVersion/kind

[liggitt]

What would you like to be added:
When using a typed client, decoding to a versioned struct (not an internal API type), the apiVersion/kind information returned from the server should not be dropped.

Why is this needed:
The GroupVersionKind() method included in the ObjectKind interface is largely useless when dealing with arbitrary runtime.Object instances, since typed instances drop this information here:

kubernetes/staging/src/k8s.io/apimachinery/pkg/runtime/helper.go

Lines 245 to 259 in 69a34f6

	// WithoutVersionDecoder clears the group version kind of a deserialized object.
	type WithoutVersionDecoder struct {
	Decoder
	}
	// Decode does not do conversion. It removes the gvk during deserialization.
	func (d WithoutVersionDecoder) Decode(data []byte, defaults *schema.GroupVersionKind, into Object) (Object, *schema.GroupVersionKind, error) {
	obj, gvk, err := d.Decoder.Decode(data, defaults, into)
	if obj != nil {
	kind := obj.GetObjectKind()
	// clearing the gvk is just a convention of a codec
	kind.SetGroupVersionKind(schema.GroupVersionKind{})
	}
	return obj, gvk, err
	}

This is the decoder used when a client requests a decoder that does not do conversion:

kubernetes/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/codec_factory.go

Lines 175 to 179 in 69a34f6

	// WithoutConversion returns a NegotiatedSerializer that performs no conversion, even if the
	// caller requests it.
	func (f CodecFactory) WithoutConversion() runtime.NegotiatedSerializer {
	return WithoutConversionCodecFactory{f}
	}

I could see clearing group/version/kind information when converting to an internal version, but I don't see the benefit of stripping it on decode if we're only dealing with a versioned struct.

/sig api-machinery
/cc @smarterclayton

Note that #3030 still needs to be resolved before apiVersion/kind could be depended on for individual objects for all API responses, but this would at least solve the issue with an update of an object clearing the apiVersion/kind in an update response (xref kubernetes-sigs/controller-runtime#526)

[YoubingLi]

If the apiVersion is not specified, GroupVersoinKind can be dropped when decoding the data.

Anyway, I will dig more about related issues before filing a PR.

[liggitt]

I think dropping WithoutVersionDecoder might be sufficient to accomplish this. I am looking into fixing #3030 in conversion, and adding tests to ensure conversion populates the apiVersion/kind appropriately

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[povsister]

Is there a real million stone for this issue? I see you moved this from v1.19 to v1.20

Although typed resource is already known to user, it's a bit weird to explicitly remove the GVK from de-serialized struct.
The explicit behavior sometimes makes things harder, eg: Printing struct Kind and version, without filled TypeMeta, I need reflect package to do so. Any comments or suggestion?

[matteoolivi]

@liggitt what's the status of this (or #80618)? Is there any concrete plan to move it forward?

[liggitt]

It's not on my list of items to push on for 1.22. If someone wanted to pick up #80618 (or an alternate approach) and resolve the items I identified, that would be excellent.

[alvaroaleman]

/reopen

I doubt a PR to cluster-api fixed this

[k8s-ci-robot]

@cici37: Closing this issue.

In response to this:

Closing due to age and inactivity. Please reopen if it's still a problem on the latest supported version.

/close

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.

[liggitt]

it is still an issue on the latest version :)

[cici37]

/triage accepted

[camilamacedo86]

HI folks, this issue makes it not possible properly test the controllers. Any idea when it would be fixed? Could we try to add some labels as a priority or add them in the next milestone?

[BenTheElder]

We can add priority/milestone labels but they only indicate intended priority/milestone, if nobody is signing up to fix this issue adding the label/milestone won't do anything and the milestone would be misleading.

With ~2k issues in this repo alone, it's hard to get to everything. This one is marked "help wanted" so I think the SIG Owners are looking for someone to pitch in #80609 (comment)

[caozhuozi]

/reopen

[YoniRomm]

Is there any progress on this issue?