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

Add TLS support for Memcached client #3585

Merged
merged 5 commits into from
Apr 19, 2024
Merged

Conversation

sonisr
Copy link
Contributor

@sonisr sonisr commented Apr 17, 2024

What this PR does:

This is just a copy of grafana/loki#12318 (comment) but for Tempo.

  • Adds TLS to the Memcached Client.
  • Expose new configs to configure Memcache TLS.

Which issue(s) this PR fixes:
Fixes #3564

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@sonisr sonisr changed the title Adding TLS support for Memcached Client Add TLS support for Memcached Client Apr 17, 2024
@sonisr sonisr marked this pull request as ready for review April 17, 2024 14:02
@sonisr sonisr marked this pull request as draft April 17, 2024 14:16
@sonisr sonisr marked this pull request as ready for review April 17, 2024 15:00
@@ -181,7 +210,7 @@ func (c *memcachedClient) dialViaCircuitBreaker(network, address string, timeout
c.Unlock()

conn, err := cb.Execute(func() (interface{}, error) {
return net.DialTimeout(network, address, timeout)
return c.DialTimeout(network, address, timeout)
Copy link
Member

Choose a reason for hiding this comment

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

it seems like this DialTimeout method is available here as c.Client.DialTimeout is there a reason to store it directly on the object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I imagine you could use c.Client.DialTimeout in lieu of storing it directly on the object.

Personally, not so sure if there's any benefits of using one over the other here.

There doesn't seem to be any adverse effects from storing it directly on the object in Loki. Though, it could possibly differ in Tempo.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Thanks for the work! One small question but generally this looks good!

@sonisr sonisr changed the title Add TLS support for Memcached Client Add TLS support for Memcached client Apr 19, 2024
@joe-elliott
Copy link
Member

Awesome feature addition! Thank you.

@joe-elliott joe-elliott merged commit 2935e9c into grafana:main Apr 19, 2024
15 checks passed
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.

Support encryption in transit on Memcached cache
2 participants