patches should throw error on missing targets

[Arabus]

Describe the bug

If kustomize build is run with a patchesJson6902 section specifying a nonexistant target it still reports success and outputs its resources as if it worked

Files that can reproduce the issue

kustomization.yaml (note the 'v2' in the version field)

kind: ""
apiversion: ""
resources:
- manifests/servicemonitor.yaml
patchesJson6902:
- target:
    group: monitoring.coreos.com
    kind: ServiceMonitor
    name: starboard-exporter
    namespace: starboard
    version: v2

  path: jsonpatches/patch.0.yaml

manifests/servicemonitor.yaml

apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
  labels:
    app: starboard-exporter
  name: starboard-exporter
  namespace: starboard
spec:
  endpoints:
  - path: /metrics
    port: metrics
  selector:
    matchLabels:
      app.kubernetes.io/instance: starboard-exporter
      app.kubernetes.io/name: starboard-exporter

jsonpatches/patch.0.yaml

- op: add
  path: /metadata/labels/release
  value: kube-prometheus-stack

Expected output

Some kind of error e.g, "WARNING: patchesJson6902[0] target not found for $target - please check kustomized resources for correctness" or "ERROR: patchesJson6902[0] target not found for $target! Bailing out."
An exit code > 0 to signal that something went wrong
Some suggestions of partial matches e.g., "Found $similartarget, did you mean $targetdiff?"

Actual output

apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
  labels:
    app: starboard-exporter
  name: starboard-exporter
  namespace: starboard
spec:
  endpoints:
  - path: /metrics
    port: metrics
  selector:
    matchLabels:
      app.kubernetes.io/instance: starboard-exporter
      app.kubernetes.io/name: starboard-exporter

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

[Arabus]

Partially responsible for roboll/helmfile#2031

[lwille]

I think this issue deserves more attention; patchesJson6902 was already difficult to debug before 😉

[eponymz]

The ability to detect/fail on missing targets would greatly benefit CI/CD Kubeval pipeline jobs, was shocked to see that this doesn't happen currently.

[natasha41575]

/triage accepted
/retitle patches should throw error on missing targets

We discussed this during the bug scrub, and have arrived at the decision that patches, patchesStrategicMerge, and patchesJson6902 should all throw an error in the case of a missing target rather than silently doing nothing.

[laxmikantbpandhare]

/assign

[laxmikantbpandhare]

@natasha41575 @KnVerey - I started working on this issue, and I was able to re-create the issue. In the current scenario, it is not applying the patch and throwing the file as it is if there is a mismatch in the target. In the above example, we can see /metadata/labels/release did not get added value kube-prometheus-stack as there are missing targets.

I tried replacements example as well and it threw an error if fieldPath is missing. I will try to replicate the same for the patches as well.

[laxmikantbpandhare]

fix for patches and patchJson6902 is completed and raised PR #4715 . patchStrategicMerge already throws an error if the target is missing.

[k8s-triage-robot]

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:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

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

/lifecycle stale

[lwille]

/remove-lifecycle stale

#4715 is still in review, so let's keep this issue open.

[iNoahNothing]

While I agree there is definitely a reason to want Kustomize to fail if it does not make an intended patch, I believe this should be behavior that is configurable rather than a hard requirement.

The behavior of not failing if a patch is not applied is a feature of the patchesJson6902 that allows you to create components that apply patches to resources if those resources exist but not fail if they do not. If, for example, there were multiple resources that a URL could be added to, with the current behavior of the patchesJson6902, you can create a general purpose component to add this URL to every type of resource instead of needing to specify the resource being used in the overlay.

Remove this functionality entirely would force my Kustomize code to be much less DRY

[Arabus]

Considering that section 4 of https://www.rfc-editor.org/rfc/rfc6902 states that nonexitent target object MUST be an error but section 5 states:

If a normative requirement is violated by a JSON Patch document, or
   if an operation is not successful, evaluation of the JSON Patch
   document SHOULD terminate and application of the entire patch
   document SHALL NOT be deemed successful.

It might be a good tradeoff to make this configurable and set the default to fail.

SN: I concede it makes your code less Dry to state every affected object OTOH it also makes it less explicit and prone to overmatching.

[annasong20]

@Arabus When I read RFC 6902, I interpreted the spec as conditional on the assumption that there is a "target JSON document" to apply it to. For example, I interpreted the multiple of occurrences of "The target location MUST exist for the operation to be successful." to mean - only throw an error if there exists a "target JSON document" and within the document, the "target location" does not exist.

Based on this interpretation, I don't think the RFC dictates whether we should throw an error for this issue, given that this issue debates how we should handle not finding a target document that satisfies our specified GVKNN. Please feel free to correct me.

[annasong20]

After further discussion with @KnVerey @natasha41575, in light of use cases like #4379 (comment) and @Arabus' suggestion #4379 (comment), we've decided to make the error-throwing upon finding no matching target configurable.

For patches, we will add a field (named along the lines of) allowNoTargetMatch under options that, when "true", prints a warning to stderr instead of throwing an error. The default value of this field will be "false".

For the deprecated patchesJson6902, we will throw an error, as it does not have an options field to configure and we aren't looking to grow a deprecated field.

[hjannasch]

Any news on this topic? Would be nice have this option in the near future.

[laxmikantbpandhare]

@hjannasch - I started looking into it and will update my PR #4715 soon.

[laxmikantbpandhare]

Update - Modified PR and it is in discussion with @annasong20