Allow URL format in Metricbeat Redis module

[matiasinsaurralde]

Potential fix for #10917

This change will keep the usual functionality when using this approach:

- module: redis
  period: 10s
  # Redis hosts
  hosts: ["127.0.0.1:6379", "127.0.0.2:6379"]

Additionally allowing the use of Redis URL syntax:

- module: redis
  period: 10s
  # Redis hosts
  hosts: ["127.0.0.1:6379", "redis://:[email protected]:6379"]

The password field will be used for all host:port values. On URL values, the password set in the scheme will have priority.

[elasticmachine]

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?

[andrewkroh]

The way we handle this in most of the other MetricSets is to register a "host parser" when registering the metricset. It would be done here:

https://github.com/elastic/beats/blob/18b42c8804adb1f2aa60d2edb9873c0fccbad8fa/metricbeat/module/redis/info/info.go#L37

The main benefit is that the password redacted so that we don't show it in the logs. An example you can look at is in mysql:

https://github.com/elastic/beats/blob/master/metricbeat/module/mysql/status/status.go#L43

Please consider refactoring this to use a HostParser.

[matiasinsaurralde]

@andrewkroh Great, I've updated it to use the URLHostParserBuilder, all formats seem to work correctly (host:port, redis://host:port, redis://:password@host:port).
Do you think that introducing a DefaultPort in URLHostParserBuilder makes sense too? At the moment using redis://host without specifying a port will fail.

[andrewkroh]

If you look at URLHostParserBuilder you'll see that it takes into account the value of the password config option when the URL doesn't contain a password. That should make this code block unnecessary. Likewise Password can be removed from the struct declaration above (like 42).

Then down below you can pass base.HostData().Password into CreatePool. Try that and make sure it's working like I think it supposed to 😉 .

[matiasinsaurralde]

I don't see the password being populated as you describe:
a) With this config:

- module: redis
  period: 10s
  hosts: ["redis://127.0.0.1:6379"]
  password: testpassword

These are the values:

• base.HostData().Password is empty.
• config.Password is testpassword

b) With this config:

- module: redis
  period: 10s
  hosts: ["redis://:[email protected]:6379"]
  password: testpassword

These are the values:

• base.HostData().Password is newpassword (in my changes we prioritize this one, the other hosts would use config.Password).
• config.Password is testpassword

c) With this config:

- module: redis
  period: 10s
  hosts: ["127.0.0.1:6379"]
  password: testpassword

These are the values:

• base.HostData().Password is empty.
• config.Password is testpassword.

[andrewkroh]

I found the problem have provided a patch that you can incorporate. The issue was that the URL parser was not setting the password when there was no username. I also added a test case to verify the behavior or the redis metricset.

url-parse-password.patch.txt
(you can use git apply <filename> to add these changes to your local branch and test them)

[matiasinsaurralde]

Great, thanks!

[andrewkroh]

Do you think that introducing a DefaultPort in URLHostParserBuilder makes sense too? At the moment using redis://host without specifying a port will fail.

Yeah, that makes sense to me.

[kaiyan-sheng]

@matiasinsaurralde Seems like there are some merge conflicts need to be solved here 😬

[kaiyan-sheng]

Hi @matiasinsaurralde Thank you for contributing! It will be great to have this fix in. I have opened #12408 to resolve merge conflicts so we can continue the work and merge it.
I am closing this for now and @matiasinsaurralde please feel free to reopen it and continue the work! Thanks again!