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

Support for JSON Patch #169

Closed
mxey opened this issue Jul 16, 2018 · 12 comments
Closed

Support for JSON Patch #169

mxey opened this issue Jul 16, 2018 · 12 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@mxey
Copy link
Contributor

mxey commented Jul 16, 2018

Sometimes, the strategic merge patch or JSON merge patch approach does not merge in a way one would like. For example, Ingress rules are replaced entirely.

kubectl patch supports the JSON Patch standard. It would be useful for Kustomize to support it as well.

@Liujingfang1
Copy link
Contributor

Kustomize uses stategic patch for types with schema and json patch for types without schema.

@mxey
Copy link
Contributor Author

mxey commented Jul 17, 2018

Kustomize uses JSON Merge Patch (RFC 7396) for types without schema. I am referring to JSON Patch (RFC 6902).

The command description for kubectl patch does not mention it, but it's supported, and shown in the cheatsheet.

@mengqiy
Copy link
Member

mengqiy commented Aug 6, 2018

IMO we should support other patch types.
cc: @monopole

@monopole
Copy link
Contributor

monopole commented Aug 7, 2018

yep, agreed

@Liujingfang1 Liujingfang1 added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 8, 2018
@Liujingfang1
Copy link
Contributor

Liujingfang1 commented Aug 8, 2018

one possible implementation: when applying a patch, check if it is a JSON patch. If it is, apply it as JSON patch. Otherwise, use current approach.
Need a way to find the correct object the JSON patch is applied to.

@ivan4th
Copy link
Contributor

ivan4th commented Aug 9, 2018

As far as I understand, the patch description just needs to include this info (ResId)?

// ResId conflates GroupVersionKind with a textual name to uniquely identify a kubernetes resource (object).
type ResId struct {
// GroupVersionKind of the resource.
gvk schema.GroupVersionKind
// original name of the resource before transformation.
name string
// namePrefix of the resource
// an untransformed resource has no prefix, fully transformed resource has an arbitrary number of prefixes
// concatenated together.
prefix string
// namespace the resource belongs to
// an untransformed resource has no namespace, fully transformed resource has the namespace from
// the top most overlay
namespace string
}

@Liujingfang1
Copy link
Contributor

We will need gvk and name, no need for prefix and namespace.
JSON patch doesn't contain this info, we could let the user to specify gvk and name for JSON patch in kustomization.yaml.

@Liujingfang1
Copy link
Contributor

We can use ObjectReference to specify which object the JSON patch is applied to.

There is one more concern. In a JSON patch, one can have operations for changing the object's metadata.name. If Kustomize supports this operation, the object becomes a different object after applying the patch. I don't think we expect this to happen in a kustomization. The reason is that, by looking at the resources, we can get the objects (names, prefix, namespace and so on). Having a JSON patch which potentially may change the object name breaks this rule and includes new uncertainty.

I suggest to skip the operations for changing the object metadata.name with a message. WDYT @mengqiy @monopole

@monopole
Copy link
Contributor

Yes, disallow name change in patch until we edfine a safe approach (and someone asks for it).

@Liujingfang1 Liujingfang1 self-assigned this Aug 24, 2018
@Liujingfang1
Copy link
Contributor

I created POCs for two different approaches. @mxey @ivan4th @sethpollack, can you take a look and let us know your feedback from an user aspect. Thank you.

#286 adds a new filed jsonPatches in kustomization.yaml, the format is

patches:     #This filed is not affected
- patch1.yaml 
- patch2.yaml

jsonPatches:
- path: patch1.json
  target:
     kind: Service
     name: some-service
- path: patch2.json
  target:
     kind: Deployment
     name: some-deployment

This approach is backward compatible, but doesn't have good extend-ability. If we need to support another kind of patch in the future, we have to add new fields in kustomization.yaml

#295 doesn't introduce new filed in kustomization.yaml, but allow current patches to accept a map. The format is

patches:
- patch1.yaml
- patch2.yaml
- path: patch1.json
  type: JSONPatch
  target:
     kind: Service
     name: some-service
- path: patch2.json
  type: JSONPatch
  target:
     kind: Service
     name: some-deployment

This approach is backward compatible and have good extendability. The drawback is this approach will make Kustomization API unstructured.

@mxey
Copy link
Contributor Author

mxey commented Aug 27, 2018

From a user perspective, either one would work for me.

Regarding supporting multiple patch types in patches, I think it might be confusing that some of them will need a target and others will not. Also it will make each entry more verbose with its type parameter. So I prefer the approach with a separate jsonPatches list.

@monopole
Copy link
Contributor

monopole commented Aug 27, 2018

@Liujingfang1 Thanks for showing both methods. I'm in favor of defining new fields.

Suggest

  • patchesStrategicMerge // Identical to the existing patches directive.
  • patchesJson6902 // Number avoids ambiguity with 7396.

We can keep patches as a perpetual alias for patchesStrategicMerge, and maybe someday mark the former as deprecated.

Later, as desired we could add

Prefixing all these directives with patches groups them alphabetically in the kustomization file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

5 participants