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

Fix rethinkdb plugin to work with actual database and actual authorization protocol #2963

Merged
merged 4 commits into from
Jun 26, 2017

Conversation

vodolaz095
Copy link
Contributor

@vodolaz095 vodolaz095 commented Jun 26, 2017

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@vodolaz095
Copy link
Contributor Author

Previous PR - #2959

@vodolaz095
Copy link
Contributor Author

Issue related - #2958

@vodolaz095
Copy link
Contributor Author

code seems working - it passed your CI tests, and it works ok on my local machine

@danielnelson
Copy link
Contributor

What do you think about my suggestion to add username and password options to the plugin, and then use the old handshake method if authkey is set in the url, otherwise use the username and password?

I'd like to move away from the url encoded userinfo since it can be tricky to escape special characters, makes logging difficult, and its use is deprecated in rfc3986.

@vodolaz095
Copy link
Contributor Author

vodolaz095 commented Jun 26, 2017

i think DSL names is best approach, universal - it is possible to monitor servers with different versions and different credentials easily:

[[inputs.rethinkdb]]
  servers = [
# server without authorization, like old behaviour
    "rethinkdb.local:28015",
# servers WITH old authkey authorization, old behaviour, 
# using Handshake v0.4 protocol for authorization
    "username:auth_key@rethinkdb.local:28015",
    "rethinkdb://username:authkey@rethinkdb.local:28015",

# servers with actual Handshake v1 protocol - 
# username and password authorization
# using - new behaviour
    "rethinkdb2://username:authkey@rethinkdb2.local:28015",
]

so this approach allows to monitor both old and new versions on rethinkdb, and is backward compatible with approach being used before.

anyway, for me i'd like to use approach i made in PR or in my fork - because it works for me

maybe servers have to be an array of hashes consisting of username, password, hostname, port, authkey, protocol to use

@danielnelson
Copy link
Contributor

Oh yeah, my way is problematic with multiple servers and we don't want to make breaking changes to the config for this, so lets just go with this. Perhaps in the future we can have one server per plugin instance.

# ##
# ## If you use older versions of rethinkdb (<2.2) with auth_key, protocol
# ## have to be named "rethinkdb".
# servers = ["rethinkdb://username:auth_key@127.0.0.1:28015"]
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sampleConfig, don't have the header and remove the top level comment #. Test out how it looks with telegraf --input-filter rethinkdb --output-filter none config

@danielnelson danielnelson merged commit 1fdbfa4 into influxdata:master Jun 26, 2017
@danielnelson danielnelson added this to the 1.4.0 milestone Jun 26, 2017
@vodolaz095
Copy link
Contributor Author

thx!

@danielnelson
Copy link
Contributor

Thanks so much!

jeichorn pushed a commit to jeichorn/telegraf that referenced this pull request Jul 24, 2017
Allow rethinkdb input plugin to work with RethinkDB 2.3.5+ databases that requires username,password authorization and Handshake protocol v1.0

* remove top level header not required in sample config

* remove top level header not required in sample config
maxunt pushed a commit that referenced this pull request Jun 26, 2018
Allow rethinkdb input plugin to work with RethinkDB 2.3.5+ databases that requires username,password authorization and Handshake protocol v1.0

* remove top level header not required in sample config

* remove top level header not required in sample config
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.

2 participants