Arabus commented 2 years ago

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 commented 2 years ago

Partially responsible for roboll/helmfile#2031

lwille commented 2 years ago

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

eponymz commented 2 years ago

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 commented 2 years ago

/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 commented 2 years ago

/assign

laxmikantbpandhare commented 2 years ago

@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 commented 2 years ago

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

k8s-triage-robot commented 1 year 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:

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 commented 1 year ago

/remove-lifecycle stale

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

iNoahNothing commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

After further discussion with @KnVerey @natasha41575, in light of use cases like https://github.com/kubernetes-sigs/kustomize/issues/4379#issuecomment-1343471388 and @Arabus' suggestion https://github.com/kubernetes-sigs/kustomize/issues/4379#issuecomment-1344205011, 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 commented 1 year ago

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

laxmikantbpandhare commented 1 year ago

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

laxmikantbpandhare commented 1 year ago

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

k8s-triage-robot commented 1 month ago

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

Confirm that this issue is still relevant with /triage accepted (org members only)
Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

kubernetes-sigs / kustomize

patches should throw error on missing targets #4379

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