Skip to content

influxdb_user can't create cluster admins in current InlluxDB versions #33495

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

Closed
wwentland opened this issue May 25, 2016 · 13 comments · Fixed by #34704
Closed

influxdb_user can't create cluster admins in current InlluxDB versions #33495

wwentland opened this issue May 25, 2016 · 13 comments · Fixed by #34704
Labels
Bug broken, incorrect, or confusing behavior P4 Priority 4 RIoT Relates to integration with cloud providers, hypervisors, API-based services, etc. severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around State-Module
Milestone

Comments

@wwentland
Copy link
Contributor

wwentland commented May 25, 2016

Description of Issue/Question

The influxdb_user state can create two types of users, namely normal ones and cluster admins. The decision to create one or the other is based on the value of the "database" argument in that a cluster admin is created if no database is specified.

Unfortunately changes introduced in #31787 which allowed this state to be used in influxdb 0.9 and later versions, only allow for the creation of "normal users" and not cluster admins.

It would be relatively straightforward to fix this and I simply wanted to ask before providing a PR if we want to provide support for InfluxDB 0.8 or if support for that could be dropped.

Steps to Reproduce Issue

Try to create a cluster admin user using influxdb_user.present by not setting the database argument.

@Ch3LL
Copy link
Contributor

Ch3LL commented May 25, 2016

Looks like that definitely needs to get fixed. @bbinet do you have any opinions as well on wether InfluxDB 0.8 support should be kept into the module?

@Ch3LL Ch3LL added Bug broken, incorrect, or confusing behavior P4 Priority 4 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around RIoT Relates to integration with cloud providers, hypervisors, API-based services, etc. labels May 25, 2016
@Ch3LL Ch3LL added this to the Approved milestone May 25, 2016
@bbinet
Copy link
Contributor

bbinet commented May 25, 2016

I'd like to keep 0.8 support if possible because I've not migrated yet (and this could also be the case for other users because influxdb has dramatically changed in the following versions), but it could be renamed to something like "influxdb08".

@wwentland
Copy link
Contributor Author

wwentland commented May 26, 2016

Thanks for your comments, @bbinet. I am curious as to how you got the module working with InfluxDB 0.8 at all as current versions of the influxdb Python module require you to use from influxdb.influxdb08 import InfluxDBClient as detailed on http://influxdb-python.readthedocs.io/en/latest/include-readme.html#influxdb-v0-8-x-users.

This does not seem to be the case in the salt execution module, so I am a bit surprised that you can still use it.

Do you propose to maintain two influxdb modules, one for 0.8 and one for later versions, named influxdb08 and influxdb respectively? Which one would the state(s) use, or rather, how would they know which one to use?

@bbinet
Copy link
Contributor

bbinet commented May 26, 2016

I'm still using an old version of influxdb-python too.

Yes my proposal was to maintain two separate influxdb modules because the API has completely changed since v0.8. It could be as simple as renaming the current influxdb module with the "08" suffix, and start from scratch new modules for InfluxDB v0.13+.

Finally I'm also fine with dropping InfluxDB v0.8 support altogether: it will force me to migrate. But it might be problematic for some other saltstack users who are stuck with influxdb v0.8.

@wwentland
Copy link
Contributor Author

wwentland commented May 26, 2016

Thank you for elaborating! I see no inherent value in dropping support for an InfluxDB release that is still used by many people, apart from the fact that it would significantly simplify the codebase.

My feeling right now is that dropping 0.8 support at the moment would disgruntle some people who are still on 0.8 and can't move to newer versions for one reason or another.

I'll see how much work it'll be to support both 0.8 and later versions in salt.

@wwentland
Copy link
Contributor Author

wwentland commented May 27, 2016

The influxdb state and execution modules are unfortunately quite specific to InfluxDB 0.8. In particular the user administration has changed dramatically between 0.8 and 0.9.

InfluxDB 0.8 differentiated between cluster admins (i.e. users not tied to a database) and database users that are created in the context of a specific database. InfluxDB 0.9 changed this model to one in which users and admin users are created globally and then granted certain privileges on individual databases.

Compare https://docs.influxdata.com/influxdb/v0.8/api/administration/ to https://docs.influxdata.com/influxdb/v0.9/administration/authentication_and_authorization/#authorization to see what I mean.

The influxdb_user states reflect this difference and I am not sure how to express both models (database specific users vs. global ones with privileges) in the same state module.

Any opinions on how to solve this? I am interested in getting Saltstack to work with current InfluxDB releases and don't necessarily want to break the current situation for 0,8 users, but the more I look into this the more work I see :)

@wwentland
Copy link
Contributor Author

wwentland commented May 30, 2016

I see the following options at this point:

  1. Adapt current state and execution modules to InfluxDB 0.9+. This would entail significant changes
    and would mean that existing setups would break and are forced to upgrade InfluxDB or stop
    using salt for managing the few things salt is able to manage.

    To be honest, given that managing InfluxDB 0.8 is not even possible at this point with current versions of the Python library, I'm not sure how many people would be affected by this. There will, however, be some and I still don't like to disgruntle people for no good reason.

  2. Rename the current influx execution module and states that use it to influx08, influx08_user and influx08_database respectively and use the old names for InfluxDB 0.9+. This means that 0.8 users would have to adapt their states immediately and that their setup would break when they upgrade salt. It would, however, allow users of 0.9 to use "nicer" and "more intuitive" names.

  3. Introduce influxdbng/influxdb09 states and influxng/influx09 execution module that target InfluxDB 0.9+.

    This would mean that the majority of users would have to use suboptimal state names and it is a bit tricky to communicate that influxdb09_* states are appropriate for InfluxDB 0.13 also.

  4. Support both InfluxDB 0.8 and 0.9 in same state and execution module.

    This appears to be the nice variant, but becomes incredibly tricky as some concepts changed quite dramatically between 0.8 and 0.9. It would result in many documentation entries such as "The 'database' argument to influxdb_user is ignored on InfluxDB 0.9, but will .... on InfluxDB 0.8" and "This state only works on InfluxDB 0.8 because InfluxDB 0.8 has no concept of user privileges ..." and even more exceptions and special cases in the codebase.

Which route do you prefer? I am sort of torn between 2 and 3 and the "clean slate" of 1, with a slight preference for 2 (which is due to me not using 0.8).

@bbinet
Copy link
Contributor

bbinet commented May 30, 2016

Personally I'm all for the 2nd option.

@wwentland
Copy link
Contributor Author

@bbinet - I pushed some commits I'm currently working on to https://github.com/babilen/salt/tree/influxdb-current which should allow you to manage InfluxDB 0.8 databases and users with current versions of the influxdb-python module.

The states have been renamed to influxdb08_user, influxdb08_database and the connection/host configuration is passed in as influxdb08.*. Could you try if those changes work for you?

@bbinet
Copy link
Contributor

bbinet commented May 31, 2016

Ok, thanks. I don't have much time these days, so don't wait for me to move forward (I will test your changes when I have some spare time, but don't know when).

@wwentland
Copy link
Contributor Author

Sure, no rush. I just figured that some feedback is better than no feedback :)

@aabouzaid
Copy link
Contributor

Ok, I was going to implement the missing features that in v0.9. Because current InfluxDB module and state are so basic!

But I see @BABILEN did a good job! So what do we need to do now? Test that against InfluxDB 0.8 and 0.9?

@wwentland
Copy link
Contributor Author

wwentland commented Jul 8, 2016

I waited for a new https://github.com/influxdata/influxdb-python/ release that included influxdata/influxdb-python#331 and influxdata/influxdb-python#330.

Some testing of the modules and states at https://github.com/babilen/salt/tree/influxdb-current would be great. I'll open a PR soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior P4 Priority 4 RIoT Relates to integration with cloud providers, hypervisors, API-based services, etc. severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around State-Module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants