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.

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

Invalid values.schema.json passes lint #12994

Open
michael-todorovic opened this issue Apr 30, 2024 · 5 comments
Open

Invalid values.schema.json passes lint #12994

michael-todorovic opened this issue Apr 30, 2024 · 5 comments
Labels
feature help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.

Comments

@michael-todorovic
Copy link

Hello,
We have helm 3.14.4

version.BuildInfo{Version:"v3.14.4", GitCommit:"81c902a123462fd4052bc5e9aa9c513c4c8fc142", GitTreeState:"clean", GoVersion:"go1.21.9"}

We have built a values.schema.json which works very well, this is super useful! However, recently, we introduced a bug (double curly brace) in it which makes the json invalid. Helm is happy with it:

╰ helm lint -f values.yaml -f tests/values/linter_global.yaml -f tests/values/container.yaml -f tests/values/network_mounts.yaml
==> Linting .
[INFO] Chart.yaml: icon is recommended

1 chart(s) linted, 0 chart(s) failed

However, the json is invalid:

╰ jsonlint values.schema.json 
Error: Parse error on line 1609:
...   }        }    },    "if": {      
---------------------^
Expecting 'EOF', got ','

This makes helm lint/helm template misleading as they report valid values, whereas it's not.

When I fix the jsonschema, I get failures that were hidden by the wrong status before:

╰ helm lint -f values.yaml -f tests/values/linter_global.yaml -f tests/values/container.yaml -f tests/values/network_mounts.yaml 
==> Linting .
[INFO] Chart.yaml: icon is recommended
[ERROR] values.yaml: - networkMounts.patrick: Additional property bob is not allowed

[ERROR] templates/: values don't meet the specifications of the schema(s) in the following chart(s):
workload-server-chart:
- networkMounts.patrick: Additional property bob is not allowed


Error: 1 chart(s) linted, 1 chart(s) failed

Could you check please?

@banjoh
Copy link

banjoh commented Apr 30, 2024

Are you able to provide a minimal example we can use to reproduce? I using a default chart generated using helm create and then used helm-schema to generate the json schema. After injecting some extra curly braces in several places, helm lint fails cause the json schema is invalid

@michael-todorovic
Copy link
Author

Hello, indeed I forgot the example 🤦
I ran helm create foobar and then injected this values.schema.json:

{
    "type": "object",
    "required": [
        "image"
    ],
    "properties": {
        "image": {
            "type": "object",
            "required": [
                "repository",
                "pullPolicy"
            ],
            "properties": {
                "repository": {
                    "type": "string",
                    "pattern": "^([-_/.a-z0-9]+)$"
                },
                "pullPolicy": {
                    "type": "string",
                    "pattern": "^(Always|Never|IfNotPresent)$"
                }
            }}
        }
    }
}

You can see the double curly brace after pullPolicy, which makes the json invalid. However, helm lint is happy:

╰ helm lint . -f values.yaml
==> Linting .
[INFO] Chart.yaml: icon is recommended

1 chart(s) linted, 0 chart(s) failed

helm template likely uses the same code path so it's happy as well.

We found out we broke our json thanks to helm package:

╰ helm package .                         
Error: failed to save: Invalid JSON in values.schema.json

I think we can expect lint+template to fail like package does 😄 For example, our CI lints on dev branches but packages only on release/pre-release

Thanks for your help 🙏

@banjoh
Copy link

banjoh commented May 1, 2024

I managed to reproduce the error as well. This is a valid issue

@gjenkins8 gjenkins8 added feature help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels May 9, 2024
@gjenkins8
Copy link
Contributor

gjenkins8 commented May 9, 2024

I've marked this as a feature, with the presumption that helm lint doesn't currently validate the chart's values.schema.json validity today. And as such, that would be be a useful feature.

If that presumption is wrong (and helm is validating values.schema.json), but somehow is failing to report something invalid. Then this would be a bug.

Either way, PR is welcome please.

@forestsword
Copy link

forestsword commented May 25, 2024

helm lint is stated explicitly in the docs as one of the commands where the schema is validated. My experience tells me it does. This is a bug.

I have an issue like this, but perhaps a more complicated schema. That led me to this issue. I also discovered that helm uses a jsonschema implementation that is more than 4 years old:

helm/go.mod

Line 35 in e90b456

github.com/xeipuuv/gojsonschema v1.2.0

The bug is perhaps not in helm but the fact that it uses an implementation of jsonschema that is dramatically out-of-date. This sort of change is beyond a helpful PR is welcome and deeply impacts the codebase. It needs to be addressed by the maintainers.

It looks like its a bug in the JSON decoder used by the current 4 year old module. I think you found a very rare issue. A misplaced character anywhere else in the json document causes a failure but at the end it seems like it becomes an empty schema, I don't know exactly. I was able to fix it by using the 3 year old module for qri-io instead: https://github.com/helm/helm/compare/main...forestsword:helm:change-jsonschema-module?expand=1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Projects
None yet
Development

No branches or pull requests

4 participants