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

Add support for JSON patch in fake client #69330

Merged
merged 1 commit into from Oct 15, 2018

Conversation

@vaikas-google
Contributor

vaikas-google commented Oct 2, 2018

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-google

This comment has been minimized.

Contributor

vaikas-google commented Oct 4, 2018

@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-google

This comment has been minimized.

Contributor

vaikas-google commented Oct 4, 2018

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

This comment has been minimized.

Contributor

jennybuckley commented Oct 4, 2018

/assign @caesarxuchao

@caesarxuchao

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

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

This comment has been minimized.

@caesarxuchao

caesarxuchao Oct 5, 2018

Member

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

This comment has been minimized.

@vaikas-google

vaikas-google Oct 9, 2018

Contributor

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?

This comment has been minimized.

@p0lyn0mial

p0lyn0mial Oct 9, 2018

Contributor

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 :)

This comment has been minimized.

@caesarxuchao

caesarxuchao Oct 9, 2018

Member

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

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.

// 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.

This comment has been minimized.

@caesarxuchao

caesarxuchao Oct 5, 2018

Member

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

This comment has been minimized.

@vaikas-google

vaikas-google Oct 9, 2018

Contributor

done

// 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

This comment has been minimized.

@caesarxuchao

caesarxuchao Oct 5, 2018

Member

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

This comment has been minimized.

@vaikas-google

vaikas-google Oct 9, 2018

Contributor

done

// 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

This comment has been minimized.

@caesarxuchao

caesarxuchao Oct 5, 2018

Member

contains

This comment has been minimized.

@caesarxuchao

caesarxuchao Oct 5, 2018

Member

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

This comment has been minimized.

@vaikas-google

vaikas-google Oct 9, 2018

Contributor

done

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

This comment has been minimized.

@caesarxuchao

caesarxuchao Oct 5, 2018

Member

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.

This comment has been minimized.

@vaikas-google

vaikas-google Oct 9, 2018

Contributor

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

This comment has been minimized.

@caesarxuchao

caesarxuchao Oct 9, 2018

Member

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.

This comment has been minimized.

@vaikas-google

vaikas-google Oct 10, 2018

Contributor

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

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

This comment has been minimized.

@p0lyn0mial

p0lyn0mial Oct 7, 2018

Contributor

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

This comment has been minimized.

@vaikas-google

vaikas-google Oct 9, 2018

Contributor

done

@@ -40,6 +52,20 @@ func newUnstructured(apiVersion, kind, namespace, name string) *unstructured.Uns
}
}
func newUnstructuredWithSpec(spec map[string]interface{}) *unstructured.Unstructured {
return &unstructured.Unstructured{

This comment has been minimized.

@p0lyn0mial

p0lyn0mial Oct 7, 2018

Contributor

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

This comment has been minimized.

@vaikas-google

vaikas-google Oct 9, 2018

Contributor

done

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

This comment has been minimized.

@p0lyn0mial

p0lyn0mial Oct 7, 2018

Contributor

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

This comment has been minimized.

@vaikas-google

vaikas-google Oct 9, 2018

Contributor

done

@vaikas-google

This comment has been minimized.

Contributor

vaikas-google commented Oct 9, 2018

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

@vaikas-google

This comment has been minimized.

Contributor

vaikas-google commented Oct 9, 2018

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

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

This comment has been minimized.

@caesarxuchao

caesarxuchao Oct 9, 2018

Member

Can you make the methods private as well?

@vaikas-google

This comment has been minimized.

Contributor

vaikas-google commented Oct 10, 2018

Thanks, PTAL.

@caesarxuchao

This comment has been minimized.

Member

caesarxuchao commented Oct 10, 2018

/retest

@caesarxuchao

A few small comments. Please squash. LGTM otherwise.

func (c *FakeEvents) PatchWithEventNamespace(event *api.Event, data []byte) (*api.Event, error) {
action := core.NewRootPatchAction(eventsResource, event.Name, data)
// TODO: Should be configurable to support additional patch strategies.

This comment has been minimized.

@caesarxuchao

caesarxuchao Oct 10, 2018

Member

Can you open an issue to track this?

This comment has been minimized.

@vaikas-google
patchType: types.StrategicMergePatchType,
patchBytes: []byte(`{"kind": "newkind"}`),
expectedPatchedObject: newUnstructured(testAPIVersion, "newkind", testNamespace, testName),
name: "strategic merge fails",

This comment has been minimized.

@caesarxuchao

caesarxuchao Oct 10, 2018

Member

Wrong name?

object: newUnstructured(testAPIVersion, testKind, testNamespace, testName),
patchType: types.MergePatchType,
patchBytes: []byte(`{}`),
wantErrMsg: "PatchType is not supported",

This comment has been minimized.

@caesarxuchao

caesarxuchao Oct 10, 2018

Member

Can you add a TODO and open a follow-up issue to add test for strategic merge patch?

This comment has been minimized.

@vaikas-google
@vaikas-google

This comment has been minimized.

Contributor

vaikas-google commented Oct 10, 2018

Thanks! PTAL.

@caesarxuchao

This comment has been minimized.

Member

caesarxuchao commented Oct 10, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 10, 2018

@vaikas-google

This comment has been minimized.

Contributor

vaikas-google commented Oct 10, 2018

@vaikas-google

This comment has been minimized.

Contributor

vaikas-google commented Oct 11, 2018

/assign @lavalamp @sttts

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 11, 2018

@vaikas-google

This comment has been minimized.

Contributor

vaikas-google commented Oct 11, 2018

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

@sttts

This comment has been minimized.

Contributor

sttts commented Oct 11, 2018

/approve

@sttts

This comment has been minimized.

Contributor

sttts commented Oct 11, 2018

Updated the release notes. Leaving lgtm to @caesarxuchao.

@caesarxuchao

This comment has been minimized.

Member

caesarxuchao commented Oct 11, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 11, 2018

@caesarxuchao

This comment has been minimized.

Member

caesarxuchao commented Oct 12, 2018

@lavalamp can you help approve? Thanks.

@vaikas-google

This comment has been minimized.

Contributor

vaikas-google commented Oct 15, 2018

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

This comment has been minimized.

Member

lavalamp commented Oct 15, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 15, 2018

[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:

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

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

18 checks passed

cla/linuxfoundation vaikas-google authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@vaikas-google vaikas-google deleted the vaikas-google:json-patch branch Oct 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment