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

Split config of auth and modules #859

Merged
merged 2 commits into from
Jun 23, 2023
Merged

Split config of auth and modules #859

merged 2 commits into from
Jun 23, 2023

Conversation

SuperQ
Copy link
Member

@SuperQ SuperQ commented Mar 26, 2023

Allow configuration of auth/version parameters separately from the walk and metrics in the generator and exporter configuration.

  • Simplify startup with ReloadConfig()
  • Make sure to init metrics on config reload.

Fixes: #619

@SuperQ SuperQ requested a review from RichiH March 26, 2023 12:53
@SuperQ SuperQ force-pushed the superq/auth_config branch from 3fb0b34 to bcd8ebf Compare March 26, 2023 13:00
@SuperQ SuperQ requested a review from roidelapluie March 26, 2023 13:16
@SuperQ SuperQ force-pushed the superq/auth_config branch 2 times, most recently from 503e1f4 to 9b3b0e9 Compare March 26, 2023 13:56
@SuperQ SuperQ marked this pull request as ready for review March 26, 2023 14:02
@SuperQ SuperQ force-pushed the superq/auth_config branch 2 times, most recently from f4ee7d2 to 4a7a6f0 Compare March 27, 2023 08:54
@SuperQ SuperQ force-pushed the superq/auth_config branch from 4a7a6f0 to ea8444c Compare April 4, 2023 17:31
@SuperQ
Copy link
Member Author

SuperQ commented Apr 4, 2023

CC @sebastien-coavoux

@sebastien-coavoux
Copy link
Contributor

Hey thanks for the tag, the only thing that might not be straighforward would be what to do with the auth.
This is supposed to be a get parameter received by l'exporter from prometheus from the code

            <label>Target:</label> <input type="text" name="target" placeholder="X.X.X.X" value="1.2.3.4"><br>
            <label>Auth:</label> <input type="text" name="auth" placeholder="auth" value="public"><br>
            <label>Module:</label> <input type="text" name="module" placeholder="module" value="if_mib"><br>

But that might not be clear when reading the parameter in the Readme

@SuperQ
Copy link
Member Author

SuperQ commented Apr 5, 2023

Also CC @dswarbrick for review.

@SuperQ SuperQ force-pushed the superq/auth_config branch from ea8444c to 97e2206 Compare April 5, 2023 08:15
@SuperQ
Copy link
Member Author

SuperQ commented Apr 5, 2023

@sebastien-coavoux Thanks, yea, I forgot to update the top-level README. I've updated that with some more information on how to use the the split auth/module.

I've also renamed the default auth to public_v2 to make it more clear that it's not directly a community string, but rather a configuration identifier. Kinda like how we changed default walk module to if_mib.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@SuperQ SuperQ force-pushed the superq/auth_config branch 4 times, most recently from 053ff00 to cf42453 Compare April 6, 2023 17:18
@SuperQ SuperQ requested a review from dswarbrick April 6, 2023 17:20
Copy link
Contributor

@dswarbrick dswarbrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dswarbrick
Copy link
Contributor

It seems that this would also address what #601 attempted to solve.

Copy link
Contributor

@marctc marctc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I've been working on adapting the changes to Grafana Agent's SNMP integration and overall I didn't find any problems. It will introduce breaking changes but for sake of simplicity. If you curious how is looking, here's WIP branch (cc @RichiH).

Copy link

@matthiasr matthiasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would a typical rollout of this version look like? It seems like it requires both changes to the configuration, and to the scrape parameters, at the same time?

How could we ease this transition? For example, could the exporter understand the legacy configuration for a while? Or could we still support modules referencing a certain auth, so that the scrape URL doesn't have to change atomically?

README.md Outdated Show resolved Hide resolved
@ZoldanYao
Copy link

This function is very very useful. I hope I can use this function asap.

@SuperQ
Copy link
Member Author

SuperQ commented Jun 22, 2023

Any last thoughts? I plan to merge this soon.

@@ -60,17 +63,21 @@ var (
DefaultRegexpExtract = RegexpExtract{
Value: "$1",
}

ErrUnsupportedVersion = errors.New("unsupported config version")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ErrUnsupportedVersion = errors.New("unsupported config version")
ErrUnsupportedVersion = errors.New("Unsupported config version, we only support config version 2. Please see https://github.com/prometheus/snmp_exporter/blob/main/README.md#debugging")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a Go error, it shouldn't be this verbose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the user see it on CLI?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but basically no. That's the whole reason for the "Possible old config file" error. The user won't directly see this error. It's for internal errors.Is() use.

@RichiH
Copy link
Member

RichiH commented Jun 22, 2023

There's no in-review way to add the

#Debugging

## Unsupported config version

There was a change from SNMP exporter 0.22 to 0.23. The `auth` block is now split
out from the scrape config. You will need to change both the configuration of the
SNMP exporter and Prometheus' scrape config. For details, please see our
[config upgrading doc](https://github.com/prometheus/snmp_exporter/blob/main/auth-split-migration.md).

to the end of the README.

@SuperQ
Copy link
Member Author

SuperQ commented Jun 22, 2023

@RichiH That change is not needed as we already give a URL on error.

@RichiH
Copy link
Member

RichiH commented Jun 23, 2023

Unless there's an active reason not to, I would prefer to leave more rather than fewer breadcrumbs for users hitting a wall.

@SuperQ
Copy link
Member Author

SuperQ commented Jun 23, 2023

There already is "Possible old config file". As I explained previously, due to the current strict yaml parsing, we can't do much about this. We would have to rewrite the yaml strict parser with our own validator. Frankly, I don't have the time or energy to do this.

@SuperQ SuperQ merged commit 9644498 into main Jun 23, 2023
@SuperQ SuperQ deleted the superq/auth_config branch June 23, 2023 12:27
@@ -0,0 +1,59 @@
# Module and Auth Split Migration

In [version 0.23.0](https://github.com/prometheus/snmp_exporter/releases/tag/v0.23.0) the configuration for the `snmp_epxorter` the configuration file format has changed. Configuration files for versions v0.22.0 and before will not work. The configuration was split from a flat list of modules to separate metric walking/mapping modules and authentication configuratoins.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/configuratoins/configurations/

SuperQ added a commit that referenced this pull request Jul 4, 2023
BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules #859
* [BUGFIX] Fix metrics path not using command-line argument value #904

Signed-off-by: SuperQ <[email protected]>
@SuperQ SuperQ mentioned this pull request Jul 4, 2023
SuperQ added a commit that referenced this pull request Jul 5, 2023
BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules #859
* [ENHANCEMENT] Improved Lookup process for label information #908
* [BUGFIX] Fix metrics path not using command-line argument value #904

Signed-off-by: SuperQ <[email protected]>
SuperQ added a commit that referenced this pull request Jul 6, 2023
BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules #859
* [ENHANCEMENT] Improved Lookup process for label information #908
* [BUGFIX] Fix metrics path not using command-line argument value #904

Signed-off-by: SuperQ <[email protected]>
RichiH pushed a commit that referenced this pull request Jul 6, 2023
BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules #859
* [ENHANCEMENT] Improved Lookup process for label information #908
* [BUGFIX] Fix metrics path not using command-line argument value #904

Signed-off-by: SuperQ <[email protected]>
SuperQ added a commit that referenced this pull request Jul 19, 2023
BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules #859
* [FEATURE] Improved Lookup process for label information #908
* [ENHANCEMENT] Add support for parsing SNMP transport from target #914
* [BUGFIX] Fix metrics path not using command-line argument value #904

Signed-off-by: SuperQ <[email protected]>
@SuperQ SuperQ mentioned this pull request Jul 19, 2023
SuperQ added a commit that referenced this pull request Jul 19, 2023
BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules #859
* [FEATURE] Improved Lookup process for label information #908
* [ENHANCEMENT] Add support for parsing SNMP transport from target #914
* [BUGFIX] Fix metrics path not using command-line argument value #904

Signed-off-by: SuperQ <[email protected]>
SuperQ added a commit that referenced this pull request Jul 19, 2023
BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules #859
* [FEATURE] Add support for parsing SNMP transport from target #914
* [ENHANCEMENT] Improved Lookup process for label information #908
* [BUGFIX] Fix metrics path not using command-line argument value #904

Signed-off-by: SuperQ <[email protected]>
SuperQ added a commit that referenced this pull request Jul 20, 2023
BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules #859
* [FEATURE] Add support for parsing SNMP transport from target #914
* [ENHANCEMENT] Improved Lookup process for label information #908
* [BUGFIX] Fix metrics path not using command-line argument value #904

Signed-off-by: SuperQ <[email protected]>
SuperQ added a commit that referenced this pull request Jul 20, 2023
BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules #859
* [FEATURE] Add support for parsing SNMP transport from target #914
* [ENHANCEMENT] Improved Lookup process for label information #908
* [BUGFIX] Fix metrics path not using command-line argument value #904

Signed-off-by: SuperQ <[email protected]>
SuperQ added a commit that referenced this pull request Aug 9, 2023
BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules #859
* [FEATURE] Add support for parsing SNMP transport from target #914
* [ENHANCEMENT] Improved Lookup process for label information #908
* [BUGFIX] Fix metrics path not using command-line argument value #904

Signed-off-by: SuperQ <[email protected]>
@SuperQ SuperQ mentioned this pull request Aug 9, 2023
SuperQ added a commit that referenced this pull request Aug 16, 2023
* Release v0.23.0

BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules #859
* [FEATURE] Add support for parsing SNMP transport from target #914
* [ENHANCEMENT] Improved Lookup process for label information #908
* [BUGFIX] Fix metrics path not using command-line argument value #904

Signed-off-by: SuperQ <[email protected]>

* Lint dashboards

Update dashboards to latest mixin linting.

Signed-off-by: SuperQ <[email protected]>

---------

Signed-off-by: SuperQ <[email protected]>
stephan-windischmann-sky pushed a commit to stephan-windischmann-sky/snmp_exporter that referenced this pull request Oct 27, 2023
BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules prometheus#859
* [ENHANCEMENT] Improved Lookup process for label information prometheus#908
* [BUGFIX] Fix metrics path not using command-line argument value prometheus#904

Signed-off-by: SuperQ <[email protected]>
Signed-off-by: Stephan Windischmann <[email protected]>
stephan-windischmann-sky pushed a commit to stephan-windischmann-sky/snmp_exporter that referenced this pull request Oct 27, 2023
BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules prometheus#859
* [FEATURE] Add support for parsing SNMP transport from target prometheus#914
* [ENHANCEMENT] Improved Lookup process for label information prometheus#908
* [BUGFIX] Fix metrics path not using command-line argument value prometheus#904

Signed-off-by: SuperQ <[email protected]>
Signed-off-by: Stephan Windischmann <[email protected]>
stephan-windischmann-sky pushed a commit to stephan-windischmann-sky/snmp_exporter that referenced this pull request Oct 27, 2023
* Release v0.23.0

BREAKING CHANGES:

This version of the exporter introduces a new configuration file format. This
new format separates the walk and metric mappings from the connection and
authentication settings. This allows for easier configuration of different
auth params without having to duplicate the full walk and metric mapping.

See auth-split-migration.md for more details.

* [CHANGE] Split config of auth and modules prometheus#859
* [FEATURE] Add support for parsing SNMP transport from target prometheus#914
* [ENHANCEMENT] Improved Lookup process for label information prometheus#908
* [BUGFIX] Fix metrics path not using command-line argument value prometheus#904

Signed-off-by: SuperQ <[email protected]>

* Lint dashboards

Update dashboards to latest mixin linting.

Signed-off-by: SuperQ <[email protected]>

---------

Signed-off-by: SuperQ <[email protected]>
Signed-off-by: Stephan Windischmann <[email protected]>
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 this pull request may close these issues.

Support auth alongside target & module as distinct property
9 participants