wrong line number in yaml syntax error message

[lovasoa]

What happened:

I tried to apply a Kubefile that contained a yaml syntax error.

The following error was returned

failed to decode Kubernetes YAML from /tmp/myfile.yml: yaml: line 13: mapping values are not allowed in this context

The error was not on line 13 of the actual file. The file was a multi resource file, and the error was on line 13 of the 3rd resource.

What you expected to happen:

I expect kubectl to return an actionable error message that points to the place where the syntax error is. Ideally, it would give the actual line where the error is in the file, the resource, and would print the incriminated lines.

How to reproduce it (as minimally and precisely as possible):

Apply a multi resource yaml file with a syntax error that is not on the first resource.

Environment:

• Kubernetes version (use kubectl version):
  • Client Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.5", GitCommit:"6b1d87acf3c8253c123756b9e61dac642678305f", GitTreeState:"archive", BuildDate:"2021-03-30T00:00:00Z", GoVersion:"go1.16", Compiler:"gc", Platform:"linux/amd64"}
  • Server Version: version.Info{Major:"1", Minor:"21+", GitVersion:"v1.21.2-eks-0389ca3", GitCommit:"8a4e27b9d88142bbdd21b997b532eb6d493df6d2", GitTreeState:"clean", BuildDate:"2021-07-31T01:34:46Z", GoVersion:"go1.16.5", Compiler:"gc", Platform:"linux/amd64"}
• OS (e.g: cat /etc/os-release): Fedora 34

[lovasoa]

/sig usability

/sig cli

[neolit123]

this is technically an issue that can be logged in the kubernetes-sigs/yaml repository.
the cause for the line mismatch is that the library in question converts YAML to JSON for internal usage.
on parsing errors (post conversion) the line would map to JSON.

i recall discussions about doing some post processing on the errors to provide a better explanation to the user, but that seemed brittle.

/sig api-machinery

[fedebongio]

@neolit123 do you happen to remember who was involved in the discussions? I feel this issue will become a bit stale if we don't assign it to someone in concrete.

[neolit123]

there was an issue about this either in k-sigs/yaml or in k/k but i cannot find it.
i think @dims and i were on it and we discussed briefly what can be done, but it was messy. i self-assigned myself but didn't have the time to work on it and the ticket became stale / got closed.

the SIG can ask for /help if they wish or assign someone from the active roster, but overall it seems like a backlog priority / UX improvement.

[RinkiyaKeDad]

/assign

Taking a look a it! :)

[fedebongio]

Thank you @RinkiyaKeDad !
/triage accepted

[fedebongio]

Thank you @neolit123 for the answer!

[RinkiyaKeDad]

Questions from slack thread:

The changes will be made to yaml.go, right? Also can someone please point me to how I can test the changes I make to that file?

[neolit123]

yaml.go seems the right place. to test the change you should feed erroneous input and see if the error output can be modified.
e.g. https://stackoverflow.com/questions/10971621/error-parsing-yaml-file-mapping-values-are-not-allowed-here

but not sure if API machinery would like to have this changed.
have you tried asking in k8s slack #sig-api-machinery ?

[neolit123]

actually, reading the OP again,

The error was not on line 13 of the actual file. The file was a multi resource file, and the error was on line 13 of the 3rd resource.

this seems like a problem with the code in apimachinery to split documents:
https://github.com/kubernetes/apimachinery/blob/a644435e2c133d990b624858d9c01985d7f59adf/pkg/util/yaml/decoder.go#L128

the code works, but you have a source YAML with N yaml documents separated by ---.
if the error is in one of them it reports line number X in that particular document. the user source YAML however, has multiple documents and X may not map.

so somehow, the error must include document number N to point the user at the right place.

[lovasoa]

Well, even with the document number, the user would have to manually count the documents and then the lines inside the documents. Ideally kubectl would return the actual line number in the file. All text editors count line numbers in files starting from the top of the file.

[neolit123]

true, returning the doc number is one option. calculating the line number in the original multi-doc is doable but likely trickier.

[luxas]

Yeah, I think it's better to

a) do a sweeping overlook about the errors we return, such that we always return struct errors usable with errors.Is and errors.As from json and yaml parsing libraries (this might require a KEP)
b) respect multi-frame files such that it's one of the fields of that struct error returned (and part of the error message, too

If we redesign sigs.k8s.io/yaml (which I think we should, anyways, ref: kubernetes-sigs/yaml#61 and kubernetes-sigs/yaml#59), we should have this at the forefront of what we do, but I think these changes are large and influential enough to require significant documentation and specification.

[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

[lovasoa]

/remove-lifecycle stale

[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

[lovasoa]

/remove-lifecycle stale

[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

[lovasoa]

/remove-lifecycle stale

[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

[lovasoa]

/remove-lifecycle stale

[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

[lovasoa]

/remove-lifecycle stale

[Bhargav-InfraCloud]

I recently faced this issue myself. When debugged a bit, I found that the error with the line number is not in sigs/yaml but a level deeper, in gopkg/yaml.
k8s.io/apimachinery/pkg/util/yaml -> sigs.k8s.io/yaml -> gopkg.in/yaml.v2

and the splitting of the YAML file happens in the apimachinery itself. So, IMO, the sigs or gopkg's YAML package needn't know about the multi-resource YAML file at all. The apimachinery/cli-runtime should add the error's actual line number.

These packages can derive the number of lines in each resource definition, but extracting the erroneous line's number from the wrapped error is not that straightforward. One approach that I can think of is using the regex match on the error message (if that is conventionally acceptable here). Once extracted, the error should be wrapped with aggregated line number (no. of lines in previous resources in the YAML file + separators + erroneous line's number). The final error should look something like this:

error: error parsing manifests.yaml at line 30: error converting YAML to JSON: yaml: line 4: did not find expected '-' indicator

where line 30 is the actual line number in the YAML file, but line 4 still be there. WDYT?