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

Config parser loads invalid metric names. #315

Closed
SuperQ opened this issue Jul 11, 2018 · 7 comments
Closed

Config parser loads invalid metric names. #315

SuperQ opened this issue Jul 11, 2018 · 7 comments

Comments

@SuperQ
Copy link
Member

SuperQ commented Jul 11, 2018

The config parser needs to validate metric names against Prometheus valid metric names.

I don't remember if there's a function or not, but we could implement this with a basic regexp: [a-zA-Z_]([a-zA-Z0-9_])*

For example, this should fail:

if_mib:
  walk:
  - 1.3.6.1.2.1.2
  get:
  - 1.3.6.1.2.1.1.3.0
  metrics:
  - name: sys.UpTime
    oid: 1.3.6.1.2.1.1.3
    type: gauge
@brian-brazil
Copy link
Contributor

All the intelligence is in the generator, which already has appropriate munging to ensure that only valid metric names are produced.

The exporter is only intended to be used with the generator, and I'm against adding code just to deal with users who decide to hand-craft their config as there's many many ways such a config could be broken. If you hand-craft a config, it's your responsibility to ensure it's sane.

@SuperQ
Copy link
Member Author

SuperQ commented Jul 11, 2018

That attitude does not solve corruptions, is unfriendly to users, and does not help with defense in depth.

@SuperQ SuperQ reopened this Jul 11, 2018
@brian-brazil
Copy link
Contributor

Any corruption of the metric names will be caught by existing code, there are already three layers of checks for invalid metric names. This also seems like a rather niche form of corruption to worry about specifically.

I do not want to spend time supporting the use case of hand generating snmp.yml. Rather than spending lots of time to try and support such users and teach them the internals of both this exporter and SNMP, we should be pointing them towards the generator which is much easier and friendlier to use.

@jpVm5jYYRE1VIKL
Copy link

I also using handcrafted config. And as i understand it is quite wide used practice. Because to use generator need to fulfill too much conditions.

@RichiH
Copy link
Member

RichiH commented Aug 15, 2018

The generator has come a long way, but e.g. #180 is still blocking a lot of people from using it. I know of several reimplementations of snmp_exporter due to this and other factors.

Users crafting configs by hand will not stop just because the handcrafted part is kept brittle.

@SuperQ and me are willing & able to support people who handcraft configs so you would not need to care about that.

@brian-brazil
Copy link
Contributor

It is an explicit architecture decision that there is no sanity checking of the config in the exporter, and I do not intend to change that as that's a rabbit hole in terms of code complexity.

@brian-brazil
Copy link
Contributor

My stance has not changed, and given generator changes since the number of users hand generating configs should have gone down quite a bit too.

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.

4 participants