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

<labelvalue>: a string of unicode characters accepts booleans and promtool validation succeeds #4439

Closed
vsliouniaev opened this Issue Jul 31, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@vsliouniaev
Copy link
Contributor

vsliouniaev commented Jul 31, 2018

Proposal

promtool should validate labels in a more restrictive fashion. Currently it's possible to use non-string values, but the spec says otherwise.

Use case. Why is this important?
For better interoperability that follow the Prometheus spec. For example, when submitting rule files that contain booleans to the prometheus-operator CRDs the tool fails to serialize them.

Bug Report

What did you do?

$ cat <<EOT >> badrule.conf
groups:
  - name: sample alert group
    rules:
      - alert: "something is wrong"
        expr: vector(1)
        labels:
          per_cluster: true
          per_instance: false
# EOT

$ promtool check rules badrule.yml

What did you expect to see?

Checking badrule.yml
  FAILED: ....

What did you see instead? Under which circumstances?

Checking badrule.yml
  SUCCESS: 1 rules found

Environment

  • System information:
Linux 4.15.0-1013-azure x86_64
  • Prometheus version:
promtool, version 2.3.2 (branch: HEAD, revision: 71af5e29e815795e9dd14742ee7725682fa14b7b)
  build user:       root@5258e0bd9cc1
  build date:       20180712-14:02:52
  go version:       go1.10.3

@vsliouniaev vsliouniaev changed the title <labelvalue>: a string of unicode characters accepts booleans <labelvalue>: a string of unicode characters accepts booleans and promtool validation succeeds Jul 31, 2018

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jul 31, 2018

Does Prometheus correctly parse this as a string?

@vsliouniaev

This comment has been minimized.

Copy link
Contributor Author

vsliouniaev commented Jul 31, 2018

Yes, Prometheus accepts this also

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jul 31, 2018

That doesn't sound like a bug on our end then. Promtool is meant to verify that the config will be parsed by Prometheus.

@vsliouniaev

This comment has been minimized.

Copy link
Contributor Author

vsliouniaev commented Jul 31, 2018

Is this a spec bug then? (-:

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Aug 1, 2018

When I load this rule file into Prometheus, it produces string output labels on the alert: per_cluster="true" and per_instance="false". So it seems like the YAML library just interprets true and false like any other string on string fields (like foo without quotes would also end up being "foo"). Is it not supposed to?

@vsliouniaev

This comment has been minimized.

Copy link
Contributor Author

vsliouniaev commented Aug 1, 2018

Per the JSON spec they are separate things.

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Aug 1, 2018

@vsliouniaev I stared at http://yaml.org/spec/1.2/spec.html for a while today, but it's too cryptic for me to understand whether it's illegal for a YAML parser to treat the unquoted true as a string "true" in a field that is explicitly string-typed in the language's receiver datastructure (a label value being a string). Do you have a pointer to where this is clarified? If it is indeed incorrect, that would be a bug in https://github.com/go-yaml/yaml/tree/v2.2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.