🐛 Metadata only objects should always preserve GroupVersionKind

[vincepri]

Currently we have a bug that GVK information is not passed into mapping
functions when using the OnlyMetadata option in the builder. This patch
adds a wrapper that preserves the GVK information when events come in.

This issue is most likely caused by kubernetes/kubernetes#80609
which we have been bumping into every once in a while here and there.

Signed-off-by: Vince Prignano vincepri@vmware.com

/hold

for further review

[vincepri]

/retest

[vincepri]

/retest

[DirectXMan12]

It occurs to me -- we manually construct a WithoutConversionCodecFactory, and as per kubernetes/kubernetes#80609, that's the thing, eventually, that's actually stripping the GVK. Could we just construct our own NegotiatedSerializer instead that doesn't do this and just uses the decoder directly?

Something like

cfg.NegotiatedSerializer = WithoutConversionGVKCodecFactory{CodecFactory: codecs}

// then just implement the required methods similarly to the existing codec factory, except without using WithoutVersionDecoder

[DirectXMan12]

nit: don't use the if

Suggested change

	if _, err := apiutil.GVKForObject(o, mgr.GetScheme()); err != nil {
	Expect(err).NotTo(HaveOccurred())
	}
	_, err := apiutil.GVKForObject(o, mgr.GetScheme())
	Expect(err).NotTo(HaveOccurred())

[DirectXMan12]

also, just check directly with GetObjectKind -- no need for GVKForObject, because we shouldn't need to use the scheme.

[DirectXMan12]

(and add a note as to why you're checking)

[DirectXMan12]

also, assertions in other goroutines need defer GinkgoRecover() at the top, but it's probably better to check check this when we pull it off the channel, or add a explict test case for it to better describe the desired behavior:

Context("when requesting For, Owns, and Watches as metadata", func() {
  It("should watch in metadata-only form", func() { /* existing test */ })

  It("should have GVK set on the metadata passed to the handlers", func() { /* new test */ })
})

[DirectXMan12]

I'm not sure I'm comfortable exposing this in our public API -- it seems a bit hacky, and I don't want to guarantee it'll stick around as the exact implementation.

[vincepri]

@DirectXMan12 @alvaroaleman A little update on this issue, while the NegotiatedSerializer would have been a good solution to explore, it's way too complicated and there are limitations coming from client-go and how the serializers are being used; moreover, the api-server when querying for a metadata object returns TypeMeta filled as:

TypeMeta: {
  Kind: "PartialObjectMetadata",
  APIVersion: "meta.k8s.io/v1",
}

Which seems to be the expected behavior described in the associated KEP.

For this reason, I updated this PR to follow a different approach: in the client and cache code we now make sure to always preserve the type information coming from the client. We were already doing something similar for Patch/Update on all calls in client.go file, I extended that behavior for Get/List calls when a PartialObjectMetadata[List] object is detected. In addition, the cache now makes sure to preserve the GVK when a new handler is passed in which should cover most if not all uses for the metadata objects.

The only one discrepancy I can think about between the metadata client and the unstructured one is for List calls: the unstructured client when querying a apps/v1 DeploymentList for example it returns each item in the list with TypeMeta information filled in (apps/v1 Deployment for the example). Should we do the same for Metadata List calls? If so, how can we reliably get the TypeMeta information of an object in a List GVK? Added logic in client.go to preserve the TypeMeta for each item as well using TrimSuffix as we're using everywhere else.

Note: The unstructured client already behaves properly and its default (forced) serializer makes sure to not clear the TypeMeta information.

[vincepri]

/hold cancel

[alvaroaleman]

Can we unexport this? IMHO it is an implementation detail and doesn't really have any use for others or does it?

[vincepri]

Done!

[alvaroaleman]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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,vincepri]

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

[vincepri]

/milestone v0.9.x