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

Prometheus regex substitutions have to be escaped #126083

Closed
sylvainf911 opened this issue Jun 7, 2021 · 5 comments · Fixed by #144984
Closed

Prometheus regex substitutions have to be escaped #126083

sylvainf911 opened this issue Jun 7, 2021 · 5 comments · Fixed by #144984
Labels
0.kind: bug Something is broken 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS

Comments

@sylvainf911
Copy link

Describe the bug
After upgrading from 20.09 to 21.05, instances scraped with ec2_sd were having all their "instance" field to ":9100"
instead of the public host name like before.

To Reproduce
In your prometheus.yml:

    relabel_configs:
      - target_label: "__address__"
        source_labels: ["__meta_ec2_public_dns_name"]
        replacement: "$1:9100"
        action: replace

When you do a nixos-rebuild switch, you can see in /run/prometheus/prometheus-substituted.yaml:

    relabel_configs:
      - target_label: "__address__"
        source_labels: ["__meta_ec2_public_dns_name"]
        replacement: ":9100"
        action: replace

The $1 is missing

If you double the $ sign ($$1 instead of $1), the /prometheus-substituted.yaml is containing the
correct value ( replacement: "$1:9100"').

This seems to be related to the envsubst program called when the service is started.

I did not find any reference to this is the 21.05 change log. Maybe it should be added?

Expected behavior
The $1 should be present to make the public host name substitution correctly.

Notify maintainers
benley
fpletz
globin
willibutz
Frostman

Metadata

  • system: "x86_64-linux"
  • host os: Linux 5.10.37, NixOS, 21.05.650.eaba7870ffc (Okapi)
  • multi-user?: yes
  • sandbox: yes
  • version: nix-env (Nix) 2.3.11
  • channels(root): "nixos-21.05.650.eaba7870ffc"
  • nixpkgs: /nix/var/nix/profiles/per-user/root/channels/nixos
@sylvainf911 sylvainf911 added the 0.kind: bug Something is broken label Jun 7, 2021
@veprbl veprbl added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Jun 18, 2021
@srhb
Copy link
Contributor

srhb commented Jun 22, 2021

This is caused by an oversight in #97933

Passing the entire config through envsubst "steals" all of prometheus' match group references, rendering configurations using them broken.

I believe a permanent solution is to only substitute specific, named strings.

Until that can be done, I believe this warrants a revert and a backport of said revert to 21.05, since this is a very fundamental feature in Prometheus, and I don't think we should be stealing syntax, as that would require teaching users special, ad-hoc syntax-requirements beyond what's mandated by nix syntax requirements. 🙂

cc @WilliButz @pkern

@pkern
Copy link
Contributor

pkern commented Jun 22, 2021

Oh, that's unfortunate and clearly unintended. It looks like there's no way to limit substitution with envsubst either, e.g. limit it to ${} or specific variables. :(

@srhb
Copy link
Contributor

srhb commented Jun 22, 2021

Hmm, according to the envsubst man page, maybe SHELL_FORMAT can be used? https://www.gnu.org/software/gettext/manual/html_node/envsubst-Invocation.html

It sounds like it limits the substitution to exactly the named variables.

(I don't think it's relevant for the fix, but note that we can't use the braced version either, since that's also permissible as prometheus match groups)

@srhb
Copy link
Contributor

srhb commented Jun 22, 2021

The user would of course have to avoid overlaps still, but opting into a list of these with a clear description sounds acceptable, and the default (empty list -> no variables substituted) would fix all current configurations.

basvandijk added a commit to basvandijk/nixpkgs that referenced this issue Nov 7, 2021
The option `services.prometheus.environmentFile` has been removed since it was causing [issues](NixOS#126083) and Prometheus now has native support for secret files.
@basvandijk
Copy link
Member

@srhb I created an updated version of this in: #144984.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug Something is broken 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS
Projects
None yet
5 participants