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

metricbeat/module/redis: Add TLS and username support for Redis module #35240

Merged
merged 15 commits into from
Jun 15, 2023

Conversation

shmsr
Copy link
Member

@shmsr shmsr commented Apr 27, 2023

What does this PR do?

PR adds support for TLS to Redis module in metricbeat. It also adds supports for accepting username (supported by by Redis 6.0+) from URI's userinfo or query parameter.

It has also has some nitpick changes pointed out by linters, etc.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • TLS support
  • Username support
  • Testing with Redis 5.x or lower
  • Testing with Redis 6.x or higher

How to test this PR locally

  • Use a redis-server with TLS enabled
  • Use a redis-server with username set (Redis 6.0+ should be used)
  • Use a redis-server with password set and without username
  • Use a redis-server with both TLS and AUTH (username and password set) enabled.

But how will get the certificate, CA, keys, etc.?

  • Invoke the script: https://github.com/redis/redis/blob/unstable/utils/gen-test-certs.sh (after cloning redis/redis)
  • It'll generate the following:
    • tests/tls/ca.{crt,key} Self signed CA certificate.
    • tests/tls/redis.{crt,key} A certificate with no key usage/policy restrictions.
    • tests/tls/client.{crt,key} A certificate restricted for SSL client usage.
    • tests/tls/server.{crt,key} A certificate restricted for SSL server usage.
    • tests/tls/redis.dh DH Params file.
  • Now run the redis-server in TLS mode (example):
$ redis-server --tls-port 6378 --port 0 --tls-cert-file ./tests/tls/redis.crt --tls-key-file ./tests/tls/redis.key --tls-ca-cert-file ./tests/tls/ca.crt
  • If you want to use redis-cli or any other client (metricbeat's redis module) then use the same ca.crt, redis.crt and redis.key:
redis-cli -p 6378 --tls --cert ./tests/tls/redis.crt --key ./tests/tls/redis.key --cacert ./tests/tls/ca.crt

Related issues

@shmsr shmsr requested a review from a team as a code owner April 27, 2023 10:37
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 27, 2023
@botelastic
Copy link

botelastic bot commented Apr 27, 2023

This pull request doesn't have a Team:<team> label.

@shmsr shmsr marked this pull request as draft April 27, 2023 10:38
@mergify mergify bot assigned shmsr Apr 27, 2023
@mergify
Copy link
Contributor

mergify bot commented Apr 27, 2023

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @shmsr? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 27, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-06-15T18:59:37.258+0000

  • Duration: 54 min 14 sec

Test stats 🧪

Test Results
Failed 0
Passed 4449
Skipped 888
Total 5337

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@shmsr shmsr marked this pull request as ready for review May 2, 2023 18:35
@shmsr shmsr requested a review from a team as a code owner May 2, 2023 18:35
@shmsr shmsr requested review from ycombinator and rdner and removed request for a team, ycombinator and rdner May 2, 2023 18:35
@shmsr
Copy link
Member Author

shmsr commented May 2, 2023

Not sure how it added the reviewers automatically as I didn't and I just updated the project. So I've manually removed the assigned reviewers for now. But please free to review the changes as getting more eyeballs for the same is good.

@shmsr shmsr requested a review from ishleenk17 May 15, 2023 05:18
@lalit-satapathy
Copy link
Contributor

Ping: @ishleenk17

@ishleenk17
Copy link
Contributor

@shmsr

General recommendation:

It will be good to have only the enhancement related changes in this PR.
The other generic syntax change etc changes can be part of a separate PR.
It makes reviewing of the actual changes easier.

@lalit-satapathy
Copy link
Contributor

The other generic syntax change etc changes can be part of a separate PR.

We can create separate PRs for other cosmetic changes, which is also important.

@shmsr
Copy link
Member Author

shmsr commented Jun 6, 2023

@shmsr

General recommendation:

It will be good to have only the enhancement related changes in this PR. The other generic syntax change etc changes can be part of a separate PR. It makes reviewing of the actual changes easier.

Yes, I agree and that's why I left out many such cosmetic changes. The changes that are non-related are mostly raised by golangci-lint ran as part of this repository's CI itself. If I don't fix them, the CI will fail because the action that runs the linter will fail.

For example:

errors.Wrap -> fmt.Errorf("... %w", ...) // raised by golangci-lint

But yes, I'll check if there are other changes (not raised by linters) and would raise them as part of another PR.

Update: I have only kept fixes as suggested by golangci-lint to the files that are changed as part of the related changes required by the issue for which PR is issued for. I'll fix the code smells for other files of the same module in some other PR.

@shmsr shmsr requested a review from a team as a code owner June 6, 2023 08:48
@shmsr shmsr requested a review from gpop63 June 6, 2023 09:08
@gpop63
Copy link
Contributor

gpop63 commented Jun 6, 2023

@shmsr please add a changelog entry in CHANGELOG.next.asciidoc

@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2023

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b redis-auth-tls upstream/redis-auth-tls
git merge upstream/main
git push upstream redis-auth-tls

@shmsr
Copy link
Member Author

shmsr commented Jun 8, 2023

@shmsr please add a changelog entry in CHANGELOG.next.asciidoc

@gpop63 Done.

@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2023

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b redis-auth-tls upstream/redis-auth-tls
git merge upstream/main
git push upstream redis-auth-tls

@ishleenk17
Copy link
Contributor

Changes look good. Has this change been tested with TLS-enabled redis TLS mode?

@shmsr
Copy link
Member Author

shmsr commented Jun 12, 2023

Changes look good. Has this change been tested with TLS-enabled redis TLS mode?

Yes. I have even given the steps on how to test this PR locally: #35240 (comment).

Copy link
Contributor

@ishleenk17 ishleenk17 left a comment

Choose a reason for hiding this comment

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

Looks good!
Please resolve conflicts and merge

@shmsr shmsr enabled auto-merge (squash) June 14, 2023 09:24
@shmsr
Copy link
Member Author

shmsr commented Jun 14, 2023

/test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs_team Indicates that the issue/PR needs a Team:* label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants