fakeClient.List tries to populate UnstructuredList with objects of underlying type

[wbrefvem]

If I have an UnstructuredList with a known GVK, passing it to fakeClient.List returns an error:

item[0]: can't assign or convert unstructured.Unstructured into <myGV>.<myKind>

Running in the debugger revealed that fakeClient.List passes the underlying type to client-go, which returns a list of the underlying type. So it makes sense that fake client would error when trying to populate the UnstructuredList with objects it gets from client-go.

There should be some way for fakeClient.List to handle Unstructured* objects. IIUC the real client delegates to an unstructured client. That seems heavy-handed here, but it may be the only way to avoid a bunch of spaghetti in fakeClient.List just to handle UnstructuredList as a special case.

This is related to #702 but is not the same issue.

[alvaroaleman]

@wbrefvem I am a bit surprised this fails, we have testcases for this:

controller-runtime/pkg/client/fake/client_test.go

Lines 124 to 142 in 745c7c9

	It("should be able to List using unstructured list", func() {
	By("Listing all deployments in a namespace")
	list := &unstructured.UnstructuredList{}
	list.SetAPIVersion("apps/v1")
	list.SetKind("DeploymentList")
	err := cl.List(context.Background(), list, client.InNamespace("ns1"))
	Expect(err).To(BeNil())
	Expect(list.Items).To(HaveLen(2))
	})
	It("should be able to List using unstructured list when setting a non-list kind", func() {
	By("Listing all deployments in a namespace")
	list := &unstructured.UnstructuredList{}
	list.SetAPIVersion("apps/v1")
	list.SetKind("Deployment")
	err := cl.List(context.Background(), list, client.InNamespace("ns1"))
	Expect(err).To(BeNil())
	Expect(list.Items).To(HaveLen(2))
	})

Running in the debugger revealed that fakeClient.List passes the underlying type to client-go, which returns a list of the underlying type. So it makes sense that fake client would error when trying to populate the UnstructuredList with objects it gets from client-go.

We don't do that, we serialize and deserialize:

controller-runtime/pkg/client/fake/client.go

Lines 333 to 338 in 745c7c9

	j, err := json.Marshal(o)
	if err != nil {
	return err
	}
	decoder := scheme.Codecs.UniversalDecoder()
	_, _, err = decoder.Decode(j, nil, obj)

• Is your type registered in the Scheme you are using?
• Would it be possible for you to provide a small repro?

[wbrefvem]

Is your type registered in the Scheme you are using?

Yes.

Would it be possible for you to provide a small repro?

controller-runtime version: v0.9.0-beta.0

I'll try to be as brief as possible:

func (v *ValidateHandler) Handle(ctx context.Context, request admission.Request) admission.Response {
        // logic that retrieves the GVK for this type omitted
        // "service" and "serviceGVK" refer to the unknown CR type, which encapsulate configs for microservices
        // the value of labelSelector is unique to the type, e.g. for type Foo it's something like `myService==foo`
        // the key point is that the labelSelectors for the existing CR and the incoming request are identical, because the types are the same.

	unstructuredServiceList := &unstructured.UnstructuredList{}
	unstructuredServiceList.SetGroupVersionKind(serviceGVK)
	err = v.Client.List(ctx, unstructuredServiceList, &client.ListOptions{
		LabelSelector: labelSelector,
		Namespace:     v.Namespace,
	})

	if err != nil {
		log.Error(err, "error listing unstructured objects", "object GVK", serviceGVK.String())
		return admission.Errored(http.StatusInternalServerError, err)
	}

	if len(unstructuredServiceList.Items) > 0 {
		log.Info("admission of object denied because one already exists in this namespace", "GVK", serviceGVK.String(), "name", unstructuredService.GetName(), "want: 0, have: ", fmt.Sprintf("%d", (len(unstructuredServiceList.Items))))
		return admission.Denied("there is already an instance of this service deployed in this namespace")
	}

        // more stuff omitted
}

The test case is configured with a resource of this type already created. The expected behavior is that the call to Client.List should return an UnstructuredList with exactly 1 item, and admission should be denied. Instead, it returns the error I provided above

func (vts *ValidateTestSuite) SetupTest() {
        // This returns a scheme with core/v1, apps/v1, and the group that contains all CRs that this webhook validates
	testScheme, err := utils.NewSchemeWithCommonTypes()
	assert.NoError(err)
        assert.NotNil(testScheme)
	vts.Client = fake.NewFakeClientWithScheme(testScheme)
	assert.NotEmpty(vts.Client)
} 

func (vts *ValidateTestSuite) TestCustomResourceAlreadyExists() {
	assert := vts.Assert()

	err := testutils.CreateCustomResourceFromFile(vts.VNCValidateHandler.Client, "<path to manifest for my CR>")
	assert.NoError(err)

        // vts.ValidRequest is valid in the sense that everything is correct, with the exception that 
        // a resource of this type already exists, which is not reflected in the Request object itself
	response := vts.VNCValidateHandler.Handle(context.Background(), *vts.ValidRequest)
	assert.NotEmpty(response)
	assert.False(response.Allowed)
        expectErrorMessage := "there is already an instance of this service deployed in this namespace"

        // This assertion fails
        assert.Equal(expectedErrorMesssage, response.Result.Message)
} 

Running the test suite fails:

expected: "there is already an instance of this service deployed in this namespace"
actual  : "item[0]: can't assign or convert unstructured.Unstructured into <my version>.<my type>"

[wbrefvem]

I should also add that the GVK is generated by decoding the request into unstructured.Unstructured and then calling GroupVersionKind() on the decoded Unstructured.

[xunleii]

Hello, I don't know if I do something wrong or if it's another issue, but I think we can easily reach this issue using the following example :

package main

import (
  "fmt"
  "context"

  "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
  "sigs.k8s.io/controller-runtime/pkg/client/fake"
)

func main() {
	c := fake.NewClientBuilder().Build()

	u := &unstructured.Unstructured{}
	u.SetAPIVersion("v1")
	u.SetKind("Namespace")
	u.SetName("default")
	_ = c.Create(context.Background(), u)

	l := &unstructured.UnstructuredList{}
	l.SetAPIVersion("v1")
	l.SetKind("NamespaceList")
	fmt.Println(c.List(context.Background(), l))
}

I also found that the tests seem not testing the case when tracked objects by the client are unstructured.Unstructured; sigs.k8s.io/controller-runtime/pkg/client/fake/client_test.go#L124 only test deployments defined/created by sigs.k8s.io/controller-runtime/pkg/client/fake/client_test.go#L835, which are appsv1.Deployment.

As far as I could go, the problem seems coming from this line : sigs.k8s.io/controller-runtime/pkg/client/fake/client.go#L332 and more precisely k8s.io/client-go/testing/fixture.go#L257.
Using the ObjectTracker from k8s.io/client-go/testing creates a typed list (like appsv1.DeploymentList), that will try to convert the unstructured.Unstructured to an appsv1.Deployment in meta.SetList.

• appsv1.DeploymentList is generated by k8s.io/client-go/testing/fixture.go#L236
• appsv1.Deployment is generated by k8s.io/apimachinery/pkg/apis/meta/help.go#L81
• finally, the error is throw at k8s.io/apimachinery/pkg/apis/meta/help.go#L254, when it tries to convert or assign the unstructured.Unstructured to a appsv1.Deployment

@wbrefvem during your tests, do you create resources using fakeclient.Create with unstructured.Unstructured objects ? (if yes, it could prove what I found)

I don't know if my thinking is correct or if I misunderstood something, but I think the issue could be really tricky to fix; because tracker.List try to list typed object (like apps.DeploymentList) and not an unstructured.UnstructuredList (because it didn't that), I don't know how we can fix that inside the sigs.k8s.io/controller-runtime (the problem comes from the ObjectTracker).

@alvaroaleman what do you think ?

EDIT: fix my investigation
EDIT 2: could be fixed with this patch on k8s.io/apimachinery: https://github.com/xunleii/apimachinery/commit/e2b8c6baf1259fd4c3e70bf4acea2611dbbbeb4f.patch. Not tested if there are regressions
EDIT 3: patch updated to remove useless code

[alvaroaleman]

I also found that the tests seem not testing the case when tracked objects by the client are unstructured.Unstructured

There are, for example

controller-runtime/pkg/client/fake/client_test.go

Line 124 in fca94d5

It("should be able to List using unstructured list", func() {

(But there are plenty more in that file)

If you can provide a fix together with a testcase that demonstonstrates the issue (i.E. doesn't pass right now) we'd be happy to take it. If you don't have a fix but a testcase, that might also help already.

k8s.io/apimachinery is part of kubernetes/kubernetes btw, so if that needs changing, you'd need to propose a patch there.

[xunleii]

Thanks for your response.

I also found that the tests seem not testing the case when tracked objects by the client are unstructured.Unstructured

There are, for example

controller-runtime/pkg/client/fake/client_test.go

Line 124 in fca94d5

It("should be able to List using unstructured list", func() {

(But there are plenty more in that file)

Sorry, I misspoke; I wanted to talk about listing unstructured objects and not listing using unstructured objects.

I can provide a testcase that could demonstrates the issue: master...xunleii:issues/1511-no-assign-or-convert-on-unstructured, but I don't know if this test is enough.

About the fix, I will propose a patch for k8s.io/apimachinery, referencing this issue. I just want to check with you if it's a real issue or if I misuse the package.

[alvaroaleman]

Sorry, I misspoke; I wanted to talk about listing unstructured objects and not listing using unstructured objects.

I can provide a testcase that could demonstrates the issue: master...xunleii:issues/1511-no-assign-or-convert-on-unstructured, but I don't know if this test is enough.

So what doesn't work is listing an object that is registered in the scheme after it was created through unstructured? If yes yeah, sounds like a bug (although it is slightly questionable why you are using unstructured.Unstructured for an object that is registered in the scheme/whose go types you have?)

[xunleii]

Yep, that is it.

I know, it is a bit weird to do that. I have this case for a testing library using human readable sentences (Gherkin in my case): in order to create/read/update/delete Kubernetes resources (like we do with API) in a generic way and with less code as possible, I use unstructured.Unstructured for all objects. For example When Kubernetes creates a new v1/Service 'default/test' will create a service named test in the namespace default.

[Somefive]

Similar issue encountered as @xunleii said.
When explicitly creating an object like Deployment and then getting it through UnstructuredList is ok for the fake client.
However, if the Deployment is created through Unstructured, the retrieval will failed.

[danielfbm]

Sorry, I misspoke; I wanted to talk about listing unstructured objects and not listing using unstructured objects.

I can provide a testcase that could demonstrates the issue: master...xunleii:issues/1511-no-assign-or-convert-on-unstructured, but I don't know if this test is enough.

So what doesn't work is listing an object that is registered in the scheme after it was created through unstructured? If yes yeah, sounds like a bug (although it is slightly questionable why you are using unstructured.Unstructured for an object that is registered in the scheme/whose go types you have?)

Its quite handy during unit tests to load a bunch of resources as unstructured.Unstructured using a yaml file, otherwise test case data preload becomes a lot of code 😫

[alvaroaleman]

#1662 should fix this