search

kubernetes-sigs / kustomize

Customization of kubernetes YAML configurations
Apache License 2.0
11k stars 2.25k forks source link

patchesJson6902 op replace adds missing location #4380

Open Arabus opened 2 years ago

Arabus commented 2 years ago

Describe the bug

When running kustomize build with a patchesJson6902 patch to replace a value on a location not present at the given target, kustomize adds it. This is unexpected behaviour, violates https://datatracker.ietf.org/doc/html/rfc6902/#section-4.3 and should result in an Error instead.

Files that can reproduce the issue

kustomization.yaml

kind: ""
apiversion: ""
resources:
- templates/resources.yaml
patchesJson6902:
- target:
    kind: ConfigMap
    name: test
    namespace: default
    version: v1

  path: jsonpatches/patch.0.yaml

jsonpatches/patch.0.yaml

- op: replace
  path: /data/baz
  value: BAZ

templates/resources.yaml

apiVersion: v1
data:
  foo: FOO
kind: ConfigMap
metadata:
  labels:
    app: test
  name: test
  namespace: default

Expected output

Some kind of error stating that replacing a missing value is not supported

Actual output

apiVersion: v1
data:
  baz: BAZ
  foo: FOO
kind: ConfigMap
metadata:
  labels:
    app: test
  name: test
  namespace: default

Kustomize version

❯ kustomize version
{Version:kustomize/v4.4.1 GitCommit:b2d65ddc98e09187a8e38adc27c30bab078c1dbf BuildDate:2021-11-11T23:27:14Z GoOs:darwin GoArch:arm64}

Platform

❯ sw_vers
ProductName:    macOS
ProductVersion: 11.6.2
BuildVersion:   20G314

Additional context

The problem seems to haven been fixed in the upstream library kustomize uses, see https://github.com/evanphx/json-patch/issues/119 at the end. It might be suitable to bump the used version to v5 to solve this issue

k8s-ci-robot commented 2 years ago

@Arabus: This issue is currently awaiting triage.

SIG CLI takes a lead on issue triage for this repo, but any Kubernetes member can accept issues by applying the triage/accepted label.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
Arabus commented 2 years ago

Partially responsible for roboll/helmfile#2031

Arabus commented 2 years ago

Would be nice to receive some feedback on this e.g., why kustomize still uses the v4 version of that lib

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

Arabus commented 2 years ago

/remove-lifecycle rotten

KnVerey commented 2 years ago

Would be nice to receive some feedback on this e.g., why kustomize still uses the v4 version of that lib

While I'm not sure a bump has been explicitly considered, the barrier to upgrading would be behaviour parity with kubectl, which kustomize is embedded in (see kubectl patch --type json behaviour). As much as the maintainers would like to have the standards-compliant behaviour in theory, this would be a breaking change for our users. Kubectl so far has not upgraded following the discussion in https://github.com/kubernetes/kubernetes/pull/91622. On the Kustomize side, I think we have more freedom to do this in theory, but we would need to get buy-in for brining in the newer dependency to kubectl, and we would to make the change part of a major version release. As such, I'll add this issue to the Kustomize v5 project for consideration.

/triage under-consideration

KnVerey commented 1 year ago

Upon further investigation, we are going to need to wait for kubectl to pick up the version bump before we can do so, as having two versions in use in kubernetes/kubernetes has been deemed unacceptable in existing conversations I found. I will keep this issue open and tracked in the "major version bumps" project to ensure we periodically revisit whether this is possible yet. See also https://github.com/kubernetes/kubernetes/pull/113092 for example.