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

Support strict unmarshalling of the yaml files #68

Closed
jmichalek132 opened this issue Jun 24, 2024 · 5 comments · Fixed by #71
Closed

Support strict unmarshalling of the yaml files #68

jmichalek132 opened this issue Jun 24, 2024 · 5 comments · Fixed by #71

Comments

@jmichalek132
Copy link
Contributor

When mimir ruler faces unknown field in the rules yaml files if fails to unmarshall the file and the alerts / rules are not loaded.
Right now promruval doesn't catch this case.

Sample log line from mimir ruler with the error.

ts=2024-06-24T07:24:02.015162335Z caller=ruler.go:577 level=error msg="unable to list rules to sync" err="failed to fetch rule groups for user metamonitoring: failed to load rule group for user metamonitoring and namespace mimir-testing.yaml: error parsing /rules-repo/current/rules/metrics/metamonitoring/mimir-testing.yaml: /rules-repo/current/rules/metrics/metamonitoring/mimir-testing.yaml: yaml: unmarshal errors:\n  line 3: field wrong not found in type rulefmt.RuleGroup"

Testing yaml file

  - name: testGroup
    wrong: true
    rules:
      - alert: test2
        expr: up == 0
        for: 1m
        labels:
          team: "{{ .Labels.team }}"
          xxx: fooo
          severity: critical
        annotations:
          title: test alert
          playbook: http://foo.bar/nonexisting/playbook

It would be great if there was a validation option / flag to validate the rules file will pass strict unmarshalling, since there are small differences of what the rules fille looks like between prometheus / mimir / thanos it should be possible specifying which one to validate for.

@FUSAKLA
Copy link
Owner

FUSAKLA commented Jul 11, 2024

Hi! This is not even a feature request, more of a bug I'd say :)

If should have worked this way and even probably worked back then, until disabling of validations using comments was introduced.

It required quite a bit of a mangling using the yaml/v3 library and unfortunatelly caused to drop this feature.

It is enabled basicly if you see here the

decoder.KnownFields(true)
but does not happen in special cases here
err := value.Decode(dstNode)
the yaml.Node Decode function is used, which does not provide the KnownFields feature as decoder.

It is reported here go-yaml/yaml#460 2y ago with no response 😖

So I'd not even introduce any new flag, nor for specifying if it is for thanos/cortex/prometheus/mimir I'd say it's acceptable that it allows union of all the fields.

I just need to figure out how to fix it :/

@jmichalek132
Copy link
Contributor Author

Hi! This is not even a feature request, more of a bug I'd say :)

If should have worked this way and even probably worked back then, until disabling of validations using comments was introduced.

It required quite a bit of a mangling using the yaml/v3 library and unfortunatelly caused to drop this feature.

It is enabled basicly if you see here the

decoder.KnownFields(true)

but does not happen in special cases here

err := value.Decode(dstNode)

the yaml.Node Decode function is used, which does not provide the KnownFields feature as decoder.
It is reported here go-yaml/yaml#460 2y ago with no response 😖

So I'd not even introduce any new flag, nor for specifying if it is for thanos/cortex/prometheus/mimir I'd say it's acceptable that it allows union of all the fields.

I just need to figure out how to fix it :/

I do wonder if it would make sense down the line introduce a flag which specifies which backend you are targeting (prometheus. thanos, mimir).
Let's say you use the same git repo to deploy alerts to Mimir and Prometheus. If you give the Prometheus instance yaml with mimir specific fields it will still complain, so maybe having a flag signaling which of the backends you are targeting. and than error out if there are any unknown fields for that specific backend would make sense.

@jmichalek132
Copy link
Contributor Author

Anyway thank you for fixing this :) .

@FUSAKLA
Copy link
Owner

FUSAKLA commented Jul 15, 2024

Well, as I mentioned, it is more knobs to tweak for the user... But off course it would be more correct

TBH, the unmarshalling is a bit clunky as it is already, and this would make it even more complicated.
But now I'm thinking that it could be easier as it is right now 🤔

I will take a look

@FUSAKLA
Copy link
Owner

FUSAKLA commented Jul 16, 2024

So… Not perfect but it works :D

PTAL @jmichalek132 #77

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants