gomemcache icon indicating copy to clipboard operation
gomemcache copied to clipboard

TLS support

Open bruceesmith opened this issue 4 years ago • 4 comments

Support of memcached TLS

Add support for the (experimental) TLS feature of memcached TLS

Function NewWithTLS(config *tls.Config, server ...string) *Client is added. Simply pass in a *tls.Config whose settings match those of the memcached daemon, then all other methods of gomemcache operate identically to those of the normal (non-TLS) case.

bruceesmith avatar Jun 16 '20 07:06 bruceesmith

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

googlebot avatar Jun 16 '20 07:06 googlebot

@googlebot I signed it!

bruceesmith avatar Jun 16 '20 07:06 bruceesmith

@googlebot I signed it!

bruceesmith avatar Jun 16 '20 08:06 bruceesmith

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot avatar Jun 16 '20 08:06 googlebot

@bradfitz Ping. Can you please take a look at this and maybe merge it?

It will be great to have TLS support for this client.

QuChen88 avatar Aug 28 '23 03:08 QuChen88

I'd prefer a new exported field on Client, not a new constructor.

Also, each server shard might have its own hostname/tls.Config. The option on Client should be more like TLSConfigForerver func(target string) (_ *tls.Config, ok bool) probably.

Ideally there would be tests too. I tried to run it against memcached built with TLS enabled and I couldn't even get memcached on Debian stable to work. (I'm surprised it'd need anything newer, so I was likely holding it wrong but then ran out of time.)

bradfitz avatar Sep 03 '23 04:09 bradfitz

Ideally there would be tests too. I tried to run it against memcached built with TLS enabled and I couldn't even get memcached on Debian stable to work. (I'm surprised it'd need anything newer, so I was likely holding it wrong but then ran out of time.)

I've addressed this part. This package now has better tests & GitHub Actions CI hooked up.

bradfitz avatar Sep 04 '23 04:09 bradfitz

I'm very sorry @bradfitz and @QuChen88, I just don't have the hours in my day at present to adjust my PR based on Brad's comments .........

bruceesmith avatar Sep 04 '23 04:09 bruceesmith

Turns out we don't actually need this since #158, @bruceesmith. That hook already provides everything necessary to do TLS dials.

I sent #169 to add a test & mention in the docs.

bradfitz avatar Sep 05 '23 02:09 bradfitz