update fake dynamic client to return GVK #96020

deads2k · 2020-10-29T19:59:06Z

Thanks to @andrewsykim for #95951, the test he added to make this obvious, and the action wiring itself.

I created a proof commit to show that generated clients do not return GVK for from their serialized bytes. Whether this is right or wrong is a problem for another day. In comparison, the dynamic client does return GVK information so that it can be interpreted by other layers. The fake dynamic client did not do this.

This PR keeps the fake generated clients as they are, loads a GVR to listGVK mapping in the dynamic client by doing an unsafe guess, and allows for overrides of specific resources to specific list GVK mappings. It also adds several panics to catch test behavior that needs updating (coding errors).

/kind bug
@kubernetes/sig-api-machinery-bugs
/assign @andrewsykim
/priority important-soon

NONE

Today the dynamic fake client is not aware of *List kinds, so List calls return UnstructuredList objects without TypeMeta. This patch updates client-go's fake object tracker to store a map of GVR to list GVKs. In this way, the list GVK can be set for UnstructuredList objects. Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>

k8s-ci-robot · 2020-10-29T19:59:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [deads2k]
~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k]
~~staging/src/k8s.io/kubectl/OWNERS~~ [deads2k]

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

roycaihw · 2020-10-29T20:22:13Z

/assign @yliaog
/triage accepted

yliaog · 2020-10-29T23:58:25Z

staging/src/k8s.io/client-go/dynamic/fake/simple.go

+	return NewSimpleDynamicClientWithCustomResourceMapping(scheme, nil, objects...)
+}
+
+// NewSimpleDynamicClientWithCustomResourceMapping try not o use this.  In general you want to have the scheme have the List types registered


typo, try no o use -> try not to use

yliaog · 2020-10-30T00:16:42Z

staging/src/k8s.io/kubectl/pkg/cmd/wait/wait_test.go

@@ -799,7 +810,7 @@ func TestWaitForDeletionIgnoreNotFound(t *testing.T) {
 			Namespace: "ns-foo",
 		},
 	}
-	fakeClient := dynamicfakeclient.NewSimpleDynamicClient(scheme)
+	fakeClient := dynamicfakeclient.NewSimpleDynamicClientWithCustomResourceMapping(scheme, listMapping)



is this necessary? could you keep using NewSimpleDynamicClient(scheme) here and in other call sites above?

is this necessary? could you keep using NewSimpleDynamicClient(scheme) here and in other call sites above?

I can't because the existing tests separate resources and versions with unguessable names. I have to provide the mapping somehow and schemes don't allow it. I'm not sure how many other locations will face the same issue. Plurals in english are weird (and that's just english), so I think this is needed to provide people a path to upgrade.

ok. the other way is to make it always guessable, which means changing the guess function if/when necessary.

ok. the other way is to make it always guessable, which means changing the guess function if/when necessary.

external components (imagine you have a custom operator) cannot modify that code.

andrewsykim · 2020-10-29T23:31:23Z

staging/src/k8s.io/client-go/dynamic/fake/simple.go

+	return NewSimpleDynamicClientWithCustomResourceMapping(scheme, nil, objects...)
+}
+
+// NewSimpleDynamicClientWithCustomResourceMapping try not o use this.  In general you want to have the scheme have the List types registered


nit: s/o/to

andrewsykim · 2020-10-29T23:32:02Z

staging/src/k8s.io/client-go/dynamic/fake/simple.go

+
+// NewSimpleDynamicClientWithCustomResourceMapping try not o use this.  In general you want to have the scheme have the List types registered
+// and allow the default guessing for resources match.  Sometimes that doesn't work, so you can specify a custom mapping here.
+func NewSimpleDynamicClientWithCustomResourceMapping(scheme *runtime.Scheme, gvrToListKind map[schema.GroupVersionResource]string, objects ...runtime.Object) *FakeDynamicClient {


Would it be helpful to be explicit about List kinds and name this NewSimpleDynamicClientWithCustomListKinds

andrewsykim · 2020-10-30T00:53:36Z

staging/src/k8s.io/client-go/kubernetes_test/fake_client_test.go

+	if cm.GetObjectKind().GroupVersionKind() != (schema.GroupVersionKind{}) {
+		t.Fatal(cm.GetObjectKind().GroupVersionKind())
+	}
+	cmList, err := fakeKubeClient.CoreV1().ConfigMaps("foo-ns").Get(context.TODO(), "foo-name", metav1.GetOptions{})


I'm guessing this is supposed to call List and not Get

yliaog · 2020-10-30T19:00:41Z

staging/src/k8s.io/client-go/kubernetes_test/fake_client_test.go

+		t.Fatal(err)
+	}
+	if cmList.GetObjectKind().GroupVersionKind() != (schema.GroupVersionKind{}) {
+		t.Fatal(cm.GetObjectKind().GroupVersionKind())


s/cm/cmList/

yliaog · 2020-10-30T19:05:26Z

staging/src/k8s.io/kubectl/pkg/cmd/wait/wait_test.go

@@ -799,7 +810,7 @@ func TestWaitForDeletionIgnoreNotFound(t *testing.T) {
 			Namespace: "ns-foo",
 		},
 	}
-	fakeClient := dynamicfakeclient.NewSimpleDynamicClient(scheme)
+	fakeClient := dynamicfakeclient.NewSimpleDynamicClientWithCustomResourceMapping(scheme, listMapping)



ok. the other way is to make it always guessable, which means changing the guess function if/when necessary.

deads2k · 2020-11-02T13:00:00Z

ok. the other way is to make it always guessable, which means changing the guess function if/when necessary.

In usage, if your resource has a plural that isn't guessable, then you couldn't use this client to test your actual resource. Imagine writing controllers that use dynamic clients to access oxen or some such.

yliaog

i was thinking about making the guess function extendable for such non-guessable cases. but no matter your current way or the other way, there must be a way to provide a mapping of non-guessable Kind -> list Kind. i'm fine with the current way. thanks.

yliaog · 2020-11-02T17:35:12Z

staging/src/k8s.io/client-go/kubernetes_test/fake_client_test.go

+	"k8s.io/client-go/kubernetes/scheme"
+)
+
+// this test proves that the kube fake client does not return GVKs


add a TODO comment, so readers know this is not desired, and should be fixed?

add a TODO comment, so readers know this is not desired, and should be fixed?

sure. I will clarify.

yliaog · 2020-11-02T17:36:19Z

staging/src/k8s.io/client-go/kubernetes_test/fake_client_test.go

+		t.Fatal(cm.GetObjectKind().GroupVersionKind())
+	}
+}
+


the following two tests are not about fake client, should the file name be renamed to something other than fake_client_test.go?

yliaog · 2020-11-02T17:41:06Z

staging/src/k8s.io/client-go/dynamic/dynamicinformer/informer_test.go

+			gvrToListKind := map[schema.GroupVersionResource]string{
+				{Group: "apps", Version: "v1", Resource: "deployments"}: "DeploymentList",
+			}
+			fakeClient := fake.NewSimpleDynamicClientWithCustomResourceMapping(scheme, gvrToListKind, objs...)


sorry, i'm a bit confused. 'deployments' -> 'DeploymentList' should be guessable, no? is the switch from NewSimpleDynamicClient to NewSimpleDynamicClientWithCustomResourceMapping here really necessary?

sorry, i'm a bit confused. 'deployments' -> 'DeploymentList' should be guessable, no? is the switch from NewSimpleDynamicClient to NewSimpleDynamicClientWithCustomResourceMapping here really necessary?

The scheme they choose to test with does not have the deployment listed. This test is specifically about whether dynamic resources (those without scheme entries) can be properly handled. This means that it is correct for the usage.

clarifying in comments.

deads2k · 2020-11-04T16:15:36Z

updated for comments and verify. Clarified several points in the code.

andrewsykim · 2020-11-04T16:30:19Z

I think it would be useful to include release notes for this bug fix. Not familiar enough with apimachinery to lgtm this so I'll defer that to someone else who is.

…sistent

yliaog · 2020-11-04T20:13:36Z

/lgtm

fejta-bot · 2020-11-04T23:32:16Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

andrewsykim · 2020-11-05T04:38:44Z

@deads2k can we cherry-pick this?

deads2k · 2020-11-05T15:07:06Z

@deads2k can we cherry-pick this?

I'll go back to 1.19 if you like #96268 . The issue and original just missed that. I don't think it's really worth it to go further.

andrewsykim · 2020-11-05T15:12:30Z

Thanks @deads2k!

andrewsykim and others added 2 commits October 29, 2020 15:55

remove dead negotiation methods

6bebc88

k8s-ci-robot assigned andrewsykim Oct 29, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2020

k8s-ci-robot requested review from caesarxuchao and mengqiy October 29, 2020 20:00

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 29, 2020

k8s-ci-robot assigned yliaog Oct 29, 2020

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 29, 2020

andrewsykim mentioned this pull request Oct 29, 2020

[WIP] fake dynamic client: support *List kinds #95951

Closed

yliaog reviewed Oct 30, 2020

View reviewed changes

andrewsykim reviewed Oct 30, 2020

View reviewed changes

yliaog reviewed Oct 30, 2020

View reviewed changes

yliaog reviewed Nov 2, 2020

View reviewed changes

deads2k force-pushed the dynamiclist branch from a5d2285 to f13cfaa Compare November 4, 2020 16:13

deads2k added 2 commits November 4, 2020 14:57

demonstrate existing generated client and fake client behavior is con…

eaac8b8

…sistent

add GVK to fake dynamic client to match actual behavior

f438345

deads2k force-pushed the dynamiclist branch from f13cfaa to f438345 Compare November 4, 2020 19:57

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2020

k8s-ci-robot merged commit dbd2be0 into kubernetes:master Nov 5, 2020

k8s-ci-robot added this to the v1.20 milestone Nov 5, 2020

deads2k mentioned this pull request Nov 5, 2020

Automated cherry pick of #96020: fake dynamic client: support *List kinds #96268

Closed

ash2k mentioned this pull request Dec 21, 2020

refactor: Kubernetes v1.20.1 argoproj/gitops-engine#195

Merged

andrewsykim mentioned this pull request Aug 23, 2021

Migrate to envtest vmware-tanzu/tanzu-framework#85

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update fake dynamic client to return GVK #96020

update fake dynamic client to return GVK #96020

deads2k commented Oct 29, 2020 •

edited

k8s-ci-robot commented Oct 29, 2020

roycaihw commented Oct 29, 2020

yliaog Oct 29, 2020

yliaog Oct 30, 2020

deads2k Oct 30, 2020

yliaog Oct 30, 2020

deads2k Nov 4, 2020

andrewsykim Oct 29, 2020

andrewsykim Oct 29, 2020

andrewsykim Oct 30, 2020

yliaog Oct 30, 2020

yliaog Oct 30, 2020

deads2k commented Nov 2, 2020

yliaog left a comment

yliaog Nov 2, 2020

deads2k Nov 4, 2020

yliaog Nov 2, 2020

yliaog Nov 2, 2020

deads2k Nov 4, 2020

deads2k Nov 4, 2020

deads2k commented Nov 4, 2020

andrewsykim commented Nov 4, 2020

yliaog commented Nov 4, 2020

fejta-bot commented Nov 4, 2020

andrewsykim commented Nov 5, 2020

deads2k commented Nov 5, 2020

andrewsykim commented Nov 5, 2020

update fake dynamic client to return GVK #96020

update fake dynamic client to return GVK #96020

Conversation

deads2k commented Oct 29, 2020 • edited

k8s-ci-robot commented Oct 29, 2020

roycaihw commented Oct 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Nov 2, 2020

yliaog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Nov 4, 2020

andrewsykim commented Nov 4, 2020

yliaog commented Nov 4, 2020

fejta-bot commented Nov 4, 2020

andrewsykim commented Nov 5, 2020

deads2k commented Nov 5, 2020

andrewsykim commented Nov 5, 2020

deads2k commented Oct 29, 2020 •

edited