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

support multiple passwords for redis output #5891

Closed
wants to merge 1 commit into from

Conversation

aboullaite
Copy link

This PR add ability to specify multiple passwords for redis. Users now should add a list of passwords instead of one.

  • if no password is specified then we suppose a no auth
  • if one password is specified we consider it as a default password
  • we map each password with host in case of multiple passwords. if the number of hosts and password doesn't match, we fail

@elasticmachine
Copy link
Collaborator

Can one of the admins verify this patch?

@ruflin
Copy link
Contributor

ruflin commented Dec 19, 2017

@aboullaite Thanks for your contribution, a few thoughts.

  • If we allow to set multiple passwords, we should make this change for all outputs
  • I would prefer if we find an other way to do this like for example supporting to set the password as part of the host part as we do with elasticsearch.
  • If we make such a change, we need tests.

The part I worry with the above that there will be too many variants on how users want to use it. Could we provide the password as part of the hosts hosts: ["localhost:6379"] array?

@ruflin ruflin added the discuss Issue needs further discussion. label Dec 19, 2017
@aboullaite
Copy link
Author

Hi @ruflin,

Thanks for your answer. I agree that this change should affect for all outputs, but I don't think adding it to the host is the best option, since will have another separator on the hosts list. That would be too much inputs for one var.

@andrewkroh
Copy link
Member

Could we provide the password as part of the hosts hosts: ["localhost:6379"] array?

This is how I would expect it to work. This is how the ES output works. And I believe this is how most Metricbeat modules handle this (but not all).

So for a config like

hosts:
- user:pass@remote:6379
- localhost:6379
username: default
password: secret

it would use user:pass for connecting to remote:6379 and would fallback to default:secret for localhost:6379 because it did not contain a credentials.

@aboullaite
Copy link
Author

After a deep thinking, I understand your point of you and I agreed on! and the format you @andrewkroh suggested seems to be the best one!

@ruflin
Copy link
Contributor

ruflin commented Dec 22, 2017

+1 on @andrewkroh proposal. @aboullaite Want to modify your PR?

@aboullaite
Copy link
Author

@ruflin Yes, I'm actually working!

@ruflin ruflin added needs update and removed discuss Issue needs further discussion. labels Jan 4, 2018
@einali
Copy link

einali commented Feb 20, 2019

hi everybody
is there any update about this improvement?
i need it !!

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@urso
Copy link

urso commented Feb 4, 2020

Hi, I'm closing this PR, because the PR has been stalled for quite some time and the proposed changes haven't been implemented yet. I created the follow up issue #16058

@urso urso closed this Feb 4, 2020
@andresrc andresrc added the Team:Integrations Label for the Integrations team label Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants