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

query frontend, store: Add TLS support to redis_client #5674

Merged
merged 3 commits into from
Sep 28, 2022

Conversation

dbut023
Copy link
Contributor

@dbut023 dbut023 commented Sep 5, 2022

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Adds support for TLS connections to redis (Fixes #5310)
Modified original PR #5312 to use NewConfigClient as per pharau
Adds tls_client_cert, tls_client_key and tls_ca_cert options for Client Certificate and custom Root CA.
Adds tls_enabled and reuses tls_config struct from http to configure specifics.

Verification

Expanded unit tests to validate new configuration

@dbut023
Copy link
Contributor Author

dbut023 commented Sep 5, 2022

While looking to update the docs (& changelog), I noticed that http_config block puts the TLS config in a subobject tls_config

Would it be better to structure the redis TLS config using the same idiom?

@dbut023
Copy link
Contributor Author

dbut023 commented Sep 15, 2022

While looking to update the docs (& changelog), I noticed that http_config block puts the TLS config in a subobject tls_config

Would it be better to structure the redis TLS config using the same idiom?

I thought about it more and decided to go with the tls_config struct layout.

@dbut023 dbut023 changed the title query, query frontend: Add TLS support to redis_client query frontend, store: Add TLS support to redis_client Sep 16, 2022
@dbut023 dbut023 force-pushed the redis-ssl branch 2 times, most recently from ccaf096 to a5e856d Compare September 26, 2022 14:31
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Two suggestions but otherwise LGTM

pkg/cacheutil/redis_client.go Outdated Show resolved Hide resolved
pkg/cacheutil/redis_client_test.go Outdated Show resolved Hide resolved
…loses issue thanos-io#5310

Signed-off-by: Daniel Butler <16944132+dbut023@users.noreply.github.com>
Signed-off-by: Daniel Butler <16944132+dbut023@users.noreply.github.com>
Signed-off-by: Daniel Butler <16944132+dbut023@users.noreply.github.com>
Copy link
Collaborator

@matej-g matej-g 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 to me, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redis integration support TLSConfig
3 participants