Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fakeclient requires types read through unstructured to be registered in the scheme #702

Closed
alvaroaleman opened this issue Nov 29, 2019 · 23 comments · Fixed by #1521
Closed
Assignees
Labels
area/fake-client This affects the fake implementation of the client (which is a bit undermaintained ATM) help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Milestone

Comments

@alvaroaleman
Copy link
Member

Writing a test that uses the fakeClient for code that uses *unstructured.UnstructuredList to list something, the test fails with a

failed to list ImageRegistryConfigs: non-list type *unstructured.UnstructuredList (kind "my-api/v1, Kind=Config") passed as output

The same code works fine when using a non-fake client.
/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 29, 2019
@alvaroaleman
Copy link
Member Author

Okay so actually it seems I should have just read the error message, because I didn't pass in the ListType which happens to work with the non-fake client.

However after fixing that, the next error is this:

deletion_test.go:155: Deletion failed: failed to cleanup TYPE: failed to list TYPE: no kind "TYPEList" is registered for version "APIVERSION" in scheme "k8s.io/client-go/kubernetes/scheme/register.go:65"

This issue seems to be rooted in the underlying object tracker used by the fake client. Not sure if and how to fix it. Maybe register *unstructured.Unstructured for the given GVK?

Not being able to use *unstructured.Unstructered with an unregistered GVK defeats the reason of using it in the first place.

@DirectXMan12
Copy link
Contributor

I think the objectracker needs to use the passed-in scheme, not the kubernetes scheme -- use NewFakeClientWithScheme instead of NewFakeClient (which totally violates our controller-runtime API conventions, but that's a whole different story...)

@alvaroaleman
Copy link
Member Author

alvaroaleman commented Dec 3, 2019

I think the objectracker needs to use the passed-in scheme, not the kubernetes scheme -- use NewFakeClientWithScheme instead of NewFakeClient (which totally violates our controller-runtime API conventions, but that's a whole different story...)

The problem is that it requires the type I am trying to get via an unstructured.Unstructured to be registered. But the reason I am using unstructured.Unstructured in the first place is that I do not have the go types.

@DirectXMan12
Copy link
Contributor

oh, yeah, sorry, triaging too fast :-P

@DirectXMan12
Copy link
Contributor

need to dig into objecttracker here probably

/help

@k8s-ci-robot
Copy link
Contributor

@DirectXMan12:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

need to dig into objecttracker here probably

/help

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.

@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Dec 6, 2019
DirectXMan12 pushed a commit that referenced this issue Jan 31, 2020
✨ fix webhook related scaffolding
@vincepri vincepri added this to the Next milestone Feb 21, 2020
@fejta-bot
Copy link

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 21, 2020
@DirectXMan12
Copy link
Contributor

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 1, 2020
@DirectXMan12 DirectXMan12 added the area/fake-client This affects the fake implementation of the client (which is a bit undermaintained ATM) label Jun 1, 2020
@fejta-bot
Copy link

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 31, 2020
@DirectXMan12
Copy link
Contributor

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 11, 2020
@mittachaitu
Copy link

Is there any specific reason to add this check?

@alvaroaleman
Copy link
Member Author

Is there any specific reason to add this check?

I think now that we changed the interface of List to take an client.ObjectList as second argument instead of a runtime.Object we can remove that check. Alternatively, we can skip the check if the incoming type is an *unstructured.UnstructuredList

xenolog pushed a commit to xenolog/controller-runtime that referenced this issue Dec 30, 2020
@kellengreen
Copy link

kellengreen commented Jan 27, 2021

Hello all, is there a workaround for this at the moment?

@alvaroaleman
Copy link
Member Author

Not to my knowledge. We are happy to take a PR for this that changes the client to work with both UnstructuredList and SomeObjectList if someone is interested.

@mittachaitu
Copy link

I would like to fix this issue

@mittachaitu
Copy link

mittachaitu commented Mar 21, 2021

Hello all, is there a workaround for this at the moment?

Yes, I think we have to make in the following way

list := &unstructured.UnstructuredList{}
list.SetAPIVersion("apps/v1")
list.SetKind("DeploymentList")
err = cl.List(context.TODO(), list)

It make sense to set APIVersion and Kind while making a query with unstructuredlist.

Note: We have to setAPIVersion and Kind before making call to List func

@xenolog
Copy link

xenolog commented Apr 2, 2021

I confirm, this workaround is good.

@alvaroaleman alvaroaleman changed the title Fakeclient considers UnstructuredList to not be a list type Fakeclient requires types read through unstructured to be registered in the scheme Apr 3, 2021
@alvaroaleman
Copy link
Member Author

@mittachaitu @xenolog this bug report isn't about the issue you are describing, see the second message. I have retitled it to make that clearer. What you are describing is a different issue, I've opened a fix for that: #1467

@slintes
Copy link

slintes commented May 3, 2021

Hi, is anyone working on this? My team is hitting this issue and we can spend some time trying to fix it.

@alvaroaleman
Copy link
Member Author

alvaroaleman commented May 3, 2021

@slintes no, feel free to pick it up!

@wbrefvem
Copy link

wbrefvem commented May 4, 2021

@alvaroaleman I'm not sure if this is the same issue, but I noticed that if I set the GVK for the UnstructuredList, fake client does fetch the correct list, but it still errors because client-go returns a list of the underlying type instead of Unstructured. Fake client tries to populate the UnstructuredList with objects of the underlying type, which produces an error.

Again, not sure if it's related, and not sure if I know an elegant fix. Explicitly handling UnstructuredList as a special case in fakeClient.List would work (i.e., just convert the items in the list that client-go returns to Unstructured), but that seems clunky and brittle.

@alvaroaleman
Copy link
Member Author

Sounds like a different but potentially related issue. This is about fetching objects through unstructured that are not registered in the Scheme, which implies that client-go is never going to return something other than Unstructured{,List}

@alvaroaleman
Copy link
Member Author

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/fake-client This affects the fake implementation of the client (which is a bit undermaintained ATM) help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants