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.

Sign up for GitHub

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

Jump to bottom

kubectl --validate should report line numbers of JSON syntax errors #12231

Closed
asridharan opened this issue Aug 4, 2015 · 15 comments
Closed

kubectl --validate should report line numbers of JSON syntax errors #12231

asridharan opened this issue Aug 4, 2015 · 15 comments
Labels
area/kubectl area/usability lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI.
Milestone
next-candidate

Comments

@asridharan
Copy link
Contributor

I have the following JSON config:

{
    "kind": "ReplicationController",
    "apiVersion": "v1",
    "metadata": {
        "name": "owncloud-db"
    },
    "spec": {
        "template": {
            "metadata": {
                "labels": {
                    "app": "owncloud_db"
                }
            },
            "spec": {
                "containers": [
                    {
                        "resources": {
                            "limits": {
                                "cpu": 0.5
                            }
                        },
                        "image": "mysql:5.7",
                        "name": "mysql-5.7",
                        "env": [
                            {
                                "name": "MYSQL_ROOT_PASSWORD",
                                "value": "owncloud"
                            },
                            {
                                "name": "MYSQL_USER",
                                "value": "owncloud"
                            },
                            {
                                "name": "MYSQL_PASSWORD",
                                "value": "owncloud"
                            },
                            {
                                "name": "MYSQL_DATABASE",
                                "value": "owncloud"
                            }, <<<<<<<< error
                        ],
                        "ports": [
                            {
                                "containerPort": 3306,
                                "name": "mysql"
                            }
                        ],
                        "volumeMounts": [
                            {
                                "name": "mysql-persistent-storage",
                                "mountPath": "/var/lib/mysql"
                            }
                        ]
                    }
                ],
                "volumes": [
                    {
                        "name": "mysql-persistent-storage",
                        "hostPath": {
                            "path": "/var/lib/mysql"
                        }
                    }
                ]
            }
        }
    }
}

Turns out there was a syntax error in the JSON file (I have tagged the line with ">>>>>>>>error"). However, when I gave this replication controller config to kubectl for validation the error was cryptic:

kubectl --v=4 create -f ~/kubecluster/apps/owncloud/owncloud.json --validate
I0804 16:20:09.910919   24428 decoder.go:141] decoding stream as JSON
F0804 16:20:09.911035   24428 helpers.go:72] invalid character ']' looking for beginning of value

I had to use an online tool to validate my json file, before getting it through kubectl. I moved from YAML to JSON thinking that it would be easier to manage declarative configs due to the indented (curly braces) syntax. However, seems like the kubectl JSON parser is rudimentary, at best, and definitely needs to be improved for better usability.
@alex-mohr alex-mohr added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 4, 2015
@bgrant0607 bgrant0607 added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. area/usability area/kubectl labels Aug 5, 2015
@bgrant0607
Copy link
Member

Sorry. The standard Go json parser has a number of limitations. We've been exploring alternatives: #3338.

@bgrant0607 bgrant0607 added this to the v1.1 milestone Aug 5, 2015
@asridharan
Copy link
Contributor Author

Thanks Brian,
Yeah saw #3338 too. But I thought that was specifically for parsing JSON
objects through REST API. Hence, created a separate issue. If you feel
these issues are the same you can go ahead and close mine, referencing
#3338.

On Tue, Aug 4, 2015 at 9:55 PM, Brian Grant notifications@github.com
wrote:

Sorry. The standard Go json parser has a number of limitations. We've been
exploring alternatives: #3338
#3338.

—
Reply to this email directly or view it on GitHub
#12231 (comment)
.

@lavalamp
Copy link
Member

The error you got (invalid character ']' looking for beginning of value) is actually the correct error to give when you give JSON a trailing , in a list. What error did you expect? The only thing I think it could have done better is to give a line number, but that's not so easy to get out of the parser we're using.

@asridharan
Copy link
Contributor Author

line number is exactly what I was expecting. The whole point of opening this issue is to highlight that the current JSON parser makes JSON configs pretty much unusable. The cryptic error messages makes triaging syntax errors in relatively large JSON files intractable. I assumed that's the reason this issue was kept open and tagged with priority 1. @lavalamp don't see the point of closing this issue without having a clear roadmap as to when and how its going to be resolved. At the very least we need to highlight this limitation in documentation and ask people to use an external parser to triage their JSON config issues.

@lavalamp lavalamp changed the title kubectl does not throw relevant json error when parsing JSON config kubectl --validate should report line numbers of JSON syntax errors Aug 27, 2015
@lavalamp lavalamp added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Aug 27, 2015
@lavalamp
Copy link
Member

@asridharan OK, thanks for clarifying. I've changed the name of this issue to be more accurate. We will probably be changing the json parser we use eventually.

@asridharan
Copy link
Contributor Author

Thanks @lavalamp

@lavalamp
Copy link
Member

Sorry, I meant to reopen, too. :)

@lavalamp lavalamp reopened this Aug 28, 2015
@bgrant0607 bgrant0607 added team/ux and removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Sep 9, 2015
@bgrant0607
Copy link
Member

Note that we want to be able to do this for YAML, too, and we already know that we lose ordering information when converting from yaml to json.

@bgrant0607 bgrant0607 added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Sep 9, 2015
@bgrant0607 bgrant0607 modified the milestones: v1.2-candidate, v1.1 Oct 5, 2015
@adohe-zz adohe-zz mentioned this issue Jan 5, 2016
Closed
@bgrant0607 bgrant0607 removed this from the next-candidate milestone Apr 28, 2016
@mfojtik mfojtik mentioned this issue May 2, 2016
Merged
@bgrant0607 bgrant0607 added this to the v1.3 milestone May 9, 2016
@bgrant0607
Copy link
Member

I believe the following issues will be remaining after #25038:

  • Line numbers for warnings/errors from swagger-based schema validation
  • Line numbers for apiserver parsing errors
  • Line numbers for apiserver admission control and value validation errors

cc @adohe @mfojtik

@bgrant0607
Copy link
Member

Concrete examples where line numbers would help:

  • bad yaml indent levels
  • integers where strings were expected (easy in yaml, where quotes are usually omitted)

cc @thockin

k8s-github-robot pushed a commit that referenced this issue May 10, 2016
Merge pull request #25038 from mfojtik/json-line
5ca769e 
Automatic merge from submit-queue

Display line number on JSON errors

Related issue: #12231

This PR will introduce line numbers for all JSON errors in the CLI:

(this is existing error reporting for YAML)
```console
$ kubectl create -f broken.yaml
yaml: line 8: mapping values are not allowed in this context
```

(this is error reporting proposed in this PR for JSON)
```console
$ kubectl create -f broken.json
json: line 35: invalid character '{' after object key
```

(and this is the current reporting:)
```console
$ kubectl create -f broken.json
invalid character '{' after object key
```
[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/.github/PULL_REQUEST_TEMPLATE.md?pixel)]()
@adohe-zz
Copy link

@bgrant0607 for:

Line numbers for apiserver parsing errors

I guess you mean here:
https://github.com/kubernetes/kubernetes/blob/master/pkg/apiserver/resthandler.go#L380

The original PR #25038 is good, but if we can move this error handling to specific decoder, I think we can solve the remaining issues.

@lavalamp lavalamp modified the milestones: next-candidate, v1.3 May 26, 2016
@lavalamp
Copy link
Member

Done as much as it's going to be for 1.3.

@fejta-bot
Copy link

Issues go stale after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 17, 2017
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle rotten
/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 16, 2018
@bgrant0607 bgrant0607 added sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed team/ux (deprecated - do not use) labels Jan 16, 2018
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/kubectl area/usability lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI.
Projects
None yet
Milestone
next-candidate
Development

No branches or pull requests
7 participants
@lavalamp @adohe-zz @asridharan @bgrant0607 @alex-mohr @k8s-ci-robot @fejta-bot