Add transformer to apply json patch6902 #300

Liujingfang1 · 2018-08-28T22:38:14Z

break up #298

To avoid import cycle, I add the transformer files to pkg/patch/transformer

k8s-ci-robot · 2018-08-28T22:38:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Liujingfang1

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:

~~OWNERS~~ [Liujingfang1]

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

monopole

@mengqiy ? WDYT?

monopole · 2018-08-28T23:43:06Z

pkg/patch/loadpatch.go

+)
+
+// NewPatchJson6902 load a slice of PatchJson6902
+func NewPatchJson6902(l loader.Loader, slice []PatchJson6902) (map[resource.ResId][]byte, error) {


LoadPatchJson6902Map

It doesn't make instances of PatchJson6902, and calling it Load a signals the need for a loader.

monopole · 2018-08-29T00:09:12Z

pkg/patch/loadpatch.go

+			"",
+			p.Target.Namespace,
+		)
+		content, err := l.Load(p.Path)


This approach is consistent with what we do for SMP, but seeing it now this way (and noticing this function has no test) makes me wonder if we'd be better off specifying the patch string directly in the kustomization file.

We didn't do this for SMP because those patches look like resources, and specifying a resource as a string field in another resource would be odd. Plus SMP's combine the path to the thing they are modifying with the thing they modify - they are different.

Seems like

patchesJson6902: - target: blah - op: replace path: /spec/template/spec/containers/0/name value: my-nginx - op: add path: /spec/replica value: 3

would be easier to maintain in a kustomization file than a distinct JSON patch file.

perhaps it boils down to a question of who writes the JSON patch - human or tool?

The separate file approach is better for the tool, this inlining seems better for the human.

Of course we'd convert these fields to standard JSON {"op": "replace", "path": "/spec/template/spec/containers/0/name", "value": "my-nginx"}, before feeding them into the library call.

If not, we need a regression test using a fake file loader for this Load function

@mxey WDYT?

This format is more straightforward to me.
From what I read from issues, human is more likely to write JSON patch, especially for changing some port, dns names.

sgtm, lets make it in line .

yay, no loader!

@monopole I like the inline approach in general, but I can see a need for a tool writing patches, like setting Ingress hostnames for feature branches. If it's inline, there should also be a command to add those.

I have some concerns:
Make json patch inline means that kustomize need to understand json patches e.g. how to decoding it. There are more fields than what @Liujingfang1 already wrote, e.g. from.
IMO making json patch inline pushes some complexity of json patch to the kustomize layer. Ideally, kustomize doesn't need to understand it. It should simply hand the byte array to the library.

monopole · 2018-08-29T00:11:01Z

pkg/patch/transformer/patchjson6902.go

+// patchJson6902Transformer applies patches.
+type patchJson6902Transformer struct {
+	patches map[resource.ResId][]byte
+}


How about just

type patchJson6902Map map[resource.ResId][]byte

and use this to hang transformer methods from. We return a transformer.Transformer anyway...

So this changes to

map[resource.ResId][]WhateverWeCallTheOpPathValueTuple

:)

Liujingfang1 · 2018-08-29T17:40:10Z

Changed the format of patchesJson6902. It doesn't rely on loader now. The patch operations can be specified directly in kustomization.yaml.

Liujingfang1 · 2018-08-29T19:04:37Z

If there are several or many operations, the kustomization.yaml will contain a large block of them. Here is an example of this format

resources:
- the-map.yaml
- another-map.yaml

patchesJson6902:
- target:
    kind: ConfigMap
    name: the-map
  jsonPatch:
  - op: add
    path: /data/username
    value: any

- target:
    kind: ConfigMap
    name: another-map
  jsonPatch:
  - op: add
    path: /data/username
    value: xiaoming
  - op: add
    path: /data/password
    value: mima
  - op: replace
    path: /data/altGreeting
    value: "Good Evening!"

Liujingfang1 · 2018-08-29T21:20:32Z

@monopole After coding on this format for a few hours, I realized it increases the complexity beyond my expectation. The JSON patch need to support 5 different operations. Some has path field, some has from field.

 [
     { "op": "test", "path": "/a/b/c", "value": "foo" },
     { "op": "remove", "path": "/a/b/c" },
     { "op": "add", "path": "/a/b/c", "value": [ "foo", "bar" ] },
     { "op": "replace", "path": "/a/b/c", "value": 42 },
     { "op": "move", "from": "/a/b/c", "path": "/a/b/d" },
     { "op": "copy", "from": "/a/b/d", "path": "/a/b/e" }
   ]

If we load a JSON patch from a file, we get []byte and feed it to json-patch library.

With this new format, we need to add logic validating the operations and path or from match.
We also need to construct a []byte for the JSON patch after loading a kustomization.yaml. Constructing this []byte by ourselves is error-prone. In my code, I use string for value, but the content in this filed can be int, string, slice depending on the type of the corresponding field. I think we'd better avoid handling that complexity in Kustomize.

monopole · 2018-08-29T23:43:40Z

A subset of 6902 - just add, delete and replace as described above might be enough (and would make the kustomization easier to read/maintain). From a coding point of view, we could silently attempt a conversion of anything that's not explicitly quoted to a number or slice, and if it worked take the appropriate action when creating the json to feed to evanphx/json-patch. Or perhaps skip that library and the json conversion entirely, and just perform the transformation directly, given op, path and value. Assuming the common use case here is swapping a hostname or tag, changing a replica count, etc., not something super complex.

But we have no data besides @mxey's comment as to how people want to do this.

So OK with doing the simpler coding problem of reading the file.

Can we keep it forward compatible with the less pedantic approach proposed above?

E.g. a structure with a target and a path, and possibly later a slice of operations (user would be required to omit the path field if they wanted to specify inline operations). That might be the best of both worlds.

Liujingfang1 · 2018-08-30T16:17:22Z

@monopole @mengqiy I found one library https://github.com/krishicks/yaml-patch which can handle YAML format for JSON patch. Seems we can use it and I will test it. If it works, we can keep the inline format without handling the complexity by ourselves.

monopole · 2018-08-30T16:52:10Z

sgtm! What we're going for here is a means to not have to write new kustomization directives for every small edit (e.g. changing the image name).

But I'm still in favor of doing both (reading in a complete JSON6902 patch file or accepting inline directives).

The copy aspect of 6902 could be a means to reduce pod template duplication.

mxey · 2018-08-30T19:22:58Z

I think Kustomize should support the entire JSON Patch spec, especially if it references the RFC in the property name. I personally have a potential use case for copy: turning one ingress rule into multiple ones for different hostnames.

Also, it would be consistent with what kubectl supports.

Liujingfang1 · 2018-08-30T22:44:12Z

Changed the struct of PatchJSON6902. It can handle both a relative path and inline operations.
The inline operations are in YAML format, the patching is handled by functions from https://github.com/krishicks/yaml-patch, which has a complete support for JSON patch in YAML format
The file from a relative path is currently assumed to be JSON format. The patching is handled by functions from https://github.com/evanphx/json-patch
The file can also be YAML format. We can add this support later.

@monopole @mengqiy @mxey PTAL

monopole

Looks great, a few nits.

monopole · 2018-08-30T23:16:57Z

pkg/patch/transformer/patchjson6902json.go

+		t.target.Name,
+		"",
+		t.target.Namespace,
+	)


If this is done in the factory, then both transformation types can have a ResId field instead of patch.Target, and we'd have only one copy of this code instead of two.

This sounds good. Will update it.

monopole · 2018-08-30T23:18:59Z

pkg/patch/transformer/patchjson6902json.go

+	decodedPatch, err := jsonpatch.DecodePatch(t.operations)
+	if err != nil {
+		return err
+	}


Also, this decode could move to the factory, so this error would happen earlier.

s/operations []byte/patch jsonpatch.Patch/ in patchJson6902JSONTransformer

then patchJson6902JSONTransformer would look more like patchJson6902YAMLTransformer

monopole · 2018-08-30T23:22:54Z

pkg/patch/transformer/patchjson6902yaml.go

+		"",
+		t.target.Namespace,
+	)
+


so this would go away

monopole · 2018-08-30T23:26:59Z

pkg/patch/transformer/patchjson6902yaml.go

+		return fmt.Errorf("found multiple objects that the patch can apply %v", matchedIds)
+	}
+
+	obj := baseResourceMap[matchedIds[0]]


should only have one copy of L60-78.

shared package private helper func

monopole · 2018-08-30T23:34:01Z

pkg/patch/transformer/patchjson6902.go

+	}
+
+	return transformers.NewNoOpTransformer(), nil
+}


could call this file factory.go

please add a test with a fake loader. Nothing too complex, but enough to capture the error conditions. i.e. don't actually look at the transformers returned

Liujingfang1 · 2018-08-31T17:35:19Z

address comments.
add factory.go and tests.

monopole

/lgtm

mengqiy · 2018-08-31T20:17:06Z

I'm glad that now we have a solution that can handle both inline and from-file json patch!

k8s-ci-robot requested review from grodrigues3 and monopole August 28, 2018 22:38

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 28, 2018

Liujingfang1 requested review from mengqiy and removed request for grodrigues3 August 28, 2018 22:39

monopole reviewed Aug 29, 2018

View reviewed changes

This was referenced Aug 30, 2018

Add extensible transformation #122

Closed

Easy way to update the container image name #109

Closed

Add github.com/krishicks/yaml-patch to vendor

95cf508

Liujingfang1 force-pushed the patchtransformer branch from dc10ff5 to 19acd33 Compare August 30, 2018 20:46

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 30, 2018

Add patchJson6902 transformer

7f0e9e3

Liujingfang1 force-pushed the patchtransformer branch from 19acd33 to 7f0e9e3 Compare August 30, 2018 22:35

monopole reviewed Aug 30, 2018

View reviewed changes

add PatchJson6902Factory to make transformer

cedf215

k8s-ci-robot assigned monopole Aug 31, 2018

monopole approved these changes Aug 31, 2018

View reviewed changes

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

k8s-ci-robot merged commit babfb3f into kubernetes-sigs:master Aug 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add transformer to apply json patch6902 #300

Add transformer to apply json patch6902 #300

Liujingfang1 commented Aug 28, 2018 •

edited

k8s-ci-robot commented Aug 28, 2018

monopole left a comment

monopole Aug 28, 2018

monopole Aug 29, 2018 •

edited

monopole Aug 29, 2018

Liujingfang1 Aug 29, 2018

monopole Aug 29, 2018

mxey Aug 29, 2018

mengqiy Aug 29, 2018

monopole Aug 29, 2018

monopole Aug 29, 2018

Liujingfang1 commented Aug 29, 2018

Liujingfang1 commented Aug 29, 2018

Liujingfang1 commented Aug 29, 2018

monopole commented Aug 29, 2018 •

edited

Liujingfang1 commented Aug 30, 2018

monopole commented Aug 30, 2018 •

edited

mxey commented Aug 30, 2018 •

edited

Liujingfang1 commented Aug 30, 2018 •

edited

monopole left a comment

monopole Aug 30, 2018

Liujingfang1 Aug 31, 2018

monopole Aug 30, 2018

monopole Aug 30, 2018

monopole Aug 30, 2018

monopole Aug 30, 2018

Liujingfang1 commented Aug 31, 2018

monopole left a comment

mengqiy commented Aug 31, 2018

Add transformer to apply json patch6902 #300

Add transformer to apply json patch6902 #300

Conversation

Liujingfang1 commented Aug 28, 2018 • edited

k8s-ci-robot commented Aug 28, 2018

monopole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monopole Aug 29, 2018 • edited

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

Liujingfang1 commented Aug 29, 2018

Liujingfang1 commented Aug 29, 2018

Liujingfang1 commented Aug 29, 2018

monopole commented Aug 29, 2018 • edited

Liujingfang1 commented Aug 30, 2018

monopole commented Aug 30, 2018 • edited

mxey commented Aug 30, 2018 • edited

Liujingfang1 commented Aug 30, 2018 • edited

monopole 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

Liujingfang1 commented Aug 31, 2018

monopole left a comment

Choose a reason for hiding this comment

mengqiy commented Aug 31, 2018

Liujingfang1 commented Aug 28, 2018 •

edited

monopole Aug 29, 2018 •

edited

monopole commented Aug 29, 2018 •

edited

monopole commented Aug 30, 2018 •

edited

mxey commented Aug 30, 2018 •

edited

Liujingfang1 commented Aug 30, 2018 •

edited