Add support for JSON patch in fake client #69330

vaikas · 2018-10-02T13:34:59Z

What this PR does / why we need it:
Adds support for JSON Patch in client-go/dynamic/fake

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/client-go#478

Special notes for your reviewer:

Release note:

Fix https://github.com/kubernetes/client-go/issues/478 by adding support for JSON Patch in client-go/dynamic/fake

vaikas · 2018-10-04T07:24:45Z

@p0lyn0mial I'm having a hard time understanding how the modifications on the two files that keep failing the tests should be making their way into the pkg/client/... on the main kubernetes. There's two tests bazel-test and typecheck for example:

https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/69330/pull-kubernetes-typecheck/26534/

and I'm at a bit off a loss and feeling like I'm missing something obvious somewhere, got any pointers? Thanks in advance.

vaikas · 2018-10-04T09:49:11Z

ok, so looks like fake_node_expansion.go and fake_event_expansion.go are not recreated when you run ./hack/update-codegen.sh.
If I remove those two files, they are not recreated there (like others, if I remove fake_node.go
pkg/client/clientset_generated/internalclientset/typed/core/internalversion/fake/fake_node.go

it's recreated, but if I remove the fake_*expansion.go they are however not.

jennybuckley · 2018-10-04T20:52:08Z

/assign @caesarxuchao

caesarxuchao

I reviewed the first commit. Could you squash the other commits into meaningful ones?

caesarxuchao · 2018-10-05T16:44:26Z

staging/src/k8s.io/client-go/testing/fixture.go

+				if err = json.Unmarshal(modified, obj); err != nil {
+					return true, nil, err
+				}
+			default:


Can we return error by default? Does it break tests?

perhaps I misunderstand what you mean here, but by default is what the code did before that assumed that everything was a strategic patch (by ignoring the incoming patch type). So, I assume this code was added for a reason (with not explicit tests that I can see) so if I start returning error, that seems bad to me. Do you mean, that if the patch type is not strategicmergepatch that I return error and see if tests break?

In general the patch strategy is specified by the value of the patchStrategy key in a field tag.(in a struct) Additionally kubectl defaults to strategic, I'm not sure if there is server side defaulting ?

I think that the idea here is to return an error if the strategy was not specified because it would be better if the tests were more explicit. Hopefully returning an error by default will not fail other tests :)

The real server returns error is the patch type isn't specified:

kubernetes/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

Lines 72 to 74 in 48086eb

if !sets.NewString(patchTypes...).Has(contentType) {

scope.err(negotiation.NewUnsupportedMediaTypeError(patchTypes), w, req)

return

Do you mean, that if the patch type is not strategicmergepatch that I return error and see if tests break?

Yes. If tests break, and it requires significant changes to fix, please let us know. We can fix it in another PR.

caesarxuchao · 2018-10-05T16:46:34Z

staging/src/k8s.io/client-go/testing/fixture.go

+			// Before adding JSONPatch support, PatchType was ignored and everything
+			// was treated as strategic merge patch. JSONPatch flat out failed,
+			// so support for it has been added, not sure about the merge patch
+			// behaviour, so leaving it as is.


The history isn't useful. I would remove the comments.

caesarxuchao · 2018-10-05T16:47:42Z

staging/src/k8s.io/client-go/testing/fixture.go

+			// so support for it has been added, not sure about the merge patch
+			// behaviour, so leaving it as is.
+
+			// obj gets set to the new unmarshalled patched object


Isn't this as expected? The comment doesn't seem to add additional information.

caesarxuchao · 2018-10-05T16:49:57Z

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

+	// Name is a descriptive naem for this test suitable as a first argument to t.Run()
+	Name string
+
+	// Object contain the object to start the test with


Can we make this struct private and remove the comment? The fields are easy to understand.

caesarxuchao · 2018-10-05T17:24:39Z

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

+			PatchedObject: newUnstructuredWithSpec(map[string]interface{}{"foo": "bar"}),
+		}, {
+			Name:          "strategic merge change",
+			Object:        newUnstructured(testAPIVersion, testKind, testNamespace, testName),


Can you use a v1.Pod in the strategic merge patch test cases? Strategic merge patch is not supported for Unstructured. The test passed because the patch modified a field that's not a map or list.

Perhaps I'm confused here on what you are asking for, but since this is the fake for dynamic/simple, all it deals with is unstructured objects. I'm not modifying any of the existing code wrt. to strategic merge patch, just adding a JSON patch semantics, which were impossible to pass on before this change and everything failed at runtime. Bsically, I just want to add a test here that simply keeps the existing behavior

Yes, the dynamic client deals with unstructured objects. But the patch is applied to an object at the server side, the server doesn't care if it's sent via a dynamic client or not.

Concretely, if the dynamic client sends a strategic merge patch for a custom resource (which is implemented as Unstructured), it will receive an error. (see #58414 (comment) for background).

I don't want to have a test case that demonstrates wrong use case.

I've removed those test cases and just added a test that returns a failure for the patchtype when not specified. I feel like adding those tests is something that should have been done when the code was initially checked in, so adding those tests could be done in a follow on PR. Fair?

p0lyn0mial

ok, so looks like fake_node_expansion.go and fake_event_expansion.go are not recreated when you run ./hack/update-codegen.sh.

you are right, the fake_*expansion.go are not generated. They were created manually. The others files were generated by the client-gen https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/code-generator/cmd/client-gen

p0lyn0mial · 2018-10-07T18:13:37Z

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

+	PatchBytes []byte
+
+	// If test should fail or not
+	WantErr bool


how about removing this field and using only WantErrMsg ? Where a non empty vale implies that an error was wanted.

p0lyn0mial · 2018-10-07T18:15:31Z

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

@@ -40,6 +52,20 @@ func newUnstructured(apiVersion, kind, namespace, name string) *unstructured.Uns
 	}
 }

+func newUnstructuredWithSpec(spec map[string]interface{}) *unstructured.Unstructured {
+	return &unstructured.Unstructured{


we could call newUnstructured method and then just add spec key.

p0lyn0mial · 2018-10-07T18:21:11Z

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

@@ -64,3 +90,144 @@ func TestList(t *testing.T) {
 		t.Fatal(diff.ObjectGoPrintDiff(expected, listFirst.Items))
 	}
 }
+
+type PatchTestCase struct {


I would either make it private or inline (TestPatch), so that it doesn't leak when someone imports fake pkg.

vaikas · 2018-10-09T11:44:41Z

/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-kubemark-e2e-gce-big

vaikas · 2018-10-09T14:11:52Z

Thanks for the reviews, rebased and addressed the points I understood. PTAL.

caesarxuchao · 2018-10-09T19:27:08Z

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

+	expectedPatchedObject runtime.Object
+}
+
+func (tc *patchTestCase) Runner(t *testing.T) {


Can you make the methods private as well?

vaikas · 2018-10-10T17:54:00Z

Thanks! PTAL.

caesarxuchao · 2018-10-10T17:57:19Z

/lgtm

vaikas · 2018-10-10T18:38:08Z

/retest

…

vaikas · 2018-10-11T08:07:17Z

/assign @lavalamp @sttts

vaikas · 2018-10-11T08:15:42Z

yay! had to rebase, @caesarxuchao could you re-apply lgtm please.

sttts · 2018-10-11T09:26:50Z

/approve

sttts · 2018-10-11T09:27:32Z

Updated the release notes. Leaving lgtm to @caesarxuchao.

caesarxuchao · 2018-10-11T17:31:01Z

/lgtm

caesarxuchao · 2018-10-12T17:08:19Z

@lavalamp can you help approve? Thanks.

vaikas · 2018-10-15T15:30:53Z

is there somebody else besides @lavalamp that could take a look. My worry is that something else gets merged, need to rebase and need to start re-hunting for lgtm/approve from scratch again.

lavalamp · 2018-10-15T18:05:24Z

/approve

k8s-ci-robot · 2018-10-15T18:05:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, sttts, vaikas-google

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:

~~cmd/kubeadm/OWNERS~~ [lavalamp]
~~pkg/client/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [lavalamp,sttts]
~~staging/src/k8s.io/client-go/OWNERS~~ [lavalamp,sttts]
~~staging/src/k8s.io/code-generator/OWNERS~~ [lavalamp,sttts]
~~staging/src/k8s.io/csi-api/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/metrics/Godeps/OWNERS~~ [lavalamp,sttts]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [lavalamp,sttts]
~~staging/src/k8s.io/sample-controller/OWNERS~~ [lavalamp,sttts]

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

smarterclayton · 2019-06-05T18:15:00Z

staging/src/k8s.io/client-go/testing/fixture.go

+				if err != nil {
+					return true, nil, err
+				}
+				if err = json.Unmarshal(modified, obj); err != nil {


This unfortunately only works with Unstructured objects. Normal kube objects will fail on this because Unmarshal doesn't reset maps and a few other behaviors. I'll take fixing this.

Fixed in #78743

k8s-ci-robot requested review from soltysh and sttts October 2, 2018 13:35

vaikas force-pushed the json-patch branch from 65487f0 to d65f6ff Compare October 3, 2018 12:55

k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Oct 3, 2018

k8s-ci-robot added the area/kubeadm label Oct 4, 2018

k8s-ci-robot assigned caesarxuchao Oct 4, 2018

caesarxuchao reviewed Oct 5, 2018

View reviewed changes

p0lyn0mial reviewed Oct 7, 2018

View reviewed changes

vaikas force-pushed the json-patch branch from 90e11cd to 72ef920 Compare October 8, 2018 10:24

vaikas force-pushed the json-patch branch from 72ef920 to a79bd45 Compare October 9, 2018 12:58

caesarxuchao reviewed Oct 9, 2018

View reviewed changes

caesarxuchao mentioned this pull request Oct 10, 2018

Add tests for strategic merge patch for the fake dynamic client kubernetes/client-go#488

Closed

caesarxuchao mentioned this pull request Oct 10, 2018

fake_event_expansion / fake_node_expansion do not expose PatchType. #69632

Closed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2018

k8s-ci-robot assigned lavalamp and sttts Oct 11, 2018

Add support for JSON patch in fake client

a363b15

vaikas force-pushed the json-patch branch from ba5c338 to a363b15 Compare October 11, 2018 08:14

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2018

sttts added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 11, 2018

k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Oct 11, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2018

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

k8s-ci-robot merged commit 2f8b585 into kubernetes:master Oct 15, 2018

vaikas deleted the json-patch branch October 18, 2018 17:06

skriss mentioned this pull request Feb 21, 2019

fake client object tracker: support merge patch #74369

Merged

fejta mentioned this pull request Apr 9, 2019

Backwards incompatible client-go/testing across major versions makes it hard to use generated fakes for vendored CRD clients #76305

Closed

smarterclayton reviewed Jun 5, 2019

View reviewed changes

smarterclayton mentioned this pull request Jun 5, 2019

Calling PatchAction on typed objects should work correctly #78743

Merged

	if !sets.NewString(patchTypes...).Has(contentType) {
	scope.err(negotiation.NewUnsupportedMediaTypeError(patchTypes), w, req)
	return

Add support for JSON patch in fake client #69330

Add support for JSON patch in fake client #69330

Conversation

vaikas commented Oct 2, 2018 • edited by sttts Loading

vaikas commented Oct 4, 2018

vaikas commented Oct 4, 2018

jennybuckley commented Oct 4, 2018

caesarxuchao 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

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

p0lyn0mial 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

vaikas commented Oct 9, 2018

vaikas commented Oct 9, 2018

Choose a reason for hiding this comment

vaikas commented Oct 10, 2018

caesarxuchao commented Oct 10, 2018

vaikas commented Oct 10, 2018 via email

vaikas commented Oct 11, 2018

vaikas commented Oct 11, 2018

sttts commented Oct 11, 2018

sttts commented Oct 11, 2018

caesarxuchao commented Oct 11, 2018

caesarxuchao commented Oct 12, 2018

vaikas commented Oct 15, 2018

lavalamp commented Oct 15, 2018

k8s-ci-robot commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaikas commented Oct 2, 2018 •

edited by sttts

Loading