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 TCP sockets #276

Merged
merged 9 commits into from Apr 7, 2020
Merged

Conversation

moisesguimaraes
Copy link
Contributor

This PR is an attempt to add TLS support to pymemcache following the same approach I used to add TLS support to python-binary-memcached.

Signed-off-by: Moisés Guimarães de Medeiros <moguimar@redhat.com>
Copy link
Collaborator

@jparise jparise 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 this, @moisesguimaraes! Would is be possible to add test coverage for these changes?

@moisesguimaraes
Copy link
Contributor Author

@jparise, sure. I just wanted to raise the PR for discussion.

I can see that you don't automatically run memcached in your integration tests and I couldn't find it in the CI. I'd need to pass the key/cert to memcached and use the CA to build the context.

Or you do mean coverage with just mocks?

@jparise
Copy link
Collaborator

jparise commented Apr 2, 2020

I can see that you don't automatically run memcached in your integration tests and I couldn't find it in the CI. I'd need to pass the key/cert to memcached and use the CA to build the context.

We do start a memcached service instance for our tests:

https://github.com/jparise/pymemcache/blob/b5d586092fced9e5483fcd03f034c8478376ed72/.github/workflows/ci.yml#L40-L43

... but I'm not sure off the top of my head how to configure it with TLS secrets.

Or you do mean coverage with just mocks?

This could be the backup testing strategy.

@moisesguimaraes
Copy link
Contributor Author

ah thanks, I was looking for a check called integration in the CI and didn't see that file before.

@moisesguimaraes
Copy link
Contributor Author

@jparise the server params are something like this:

https://github.com/moisesguimaraes/poc-secure-memcached/blob/4081c173f01458f8fe74880ec7cbea5a1481deaa/docker-compose.yaml#L16-L25

but the memcached service must be compiled with --enable-tls. Where does that image comes from? Is it dockerhub latest? If so, I added TLS support to it a couple of months ago and it is fine, otherwise I need another plan.

@jparise
Copy link
Collaborator

jparise commented Apr 2, 2020

@jparise the server params are something like this:

moisesguimaraes/poc-secure-memcached:docker-compose.yaml@4081c17#L16-L25

I believe we can pass options to the containerized service like this:

memcached:
  image: memcached:latest
  ports:
  - 11211/tcp
  options: --foo --bar --baz

but the memcached service must be compiled with --enable-tls. Where does that image comes from? Is it dockerhub latest? If so, I added TLS support to it a couple of months ago and it is fine, otherwise I need another plan.

The image key specifies the Docker image to use, so I believe memcached:latest means DockerHub latest here.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Signed-off-by: Moisés Guimarães de Medeiros <moguimar@redhat.com>
Signed-off-by: Moisés Guimarães de Medeiros <moguimar@redhat.com>
@moisesguimaraes
Copy link
Contributor Author

ok, now the service is in place, I'll start working on the tests.

Signed-off-by: Moisés Guimarães de Medeiros <moguimar@redhat.com>
@moisesguimaraes
Copy link
Contributor Author

@jparise looks like the CI got stuck, how can I ask it to recheck?

Also I still need to investigate TLS using gevent on python2.

@jparise
Copy link
Collaborator

jparise commented Apr 3, 2020

@jparise looks like the CI got stuck, how can I ask it to recheck?

I just restarted it. (I have a Re-run option here: https://github.com/pinterest/pymemcache/pull/276/checks)

It's still failing, though. It looks like the git refs are busted somehow?

pymemcache/client/base.py Outdated Show resolved Hide resolved
Signed-off-by: Moisés Guimarães de Medeiros <moguimar@redhat.com>
Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for the work, @moisesguimaraes!

Would you be up for mentioning this new support in the documentation? After that, I'm happy to merge this.

Signed-off-by: Moisés Guimarães de Medeiros <moguimar@redhat.com>
@moisesguimaraes
Copy link
Contributor Author

moisesguimaraes commented Apr 3, 2020

Let's enable back the py2 TLS tests and see what happens there.

@moisesguimaraes
Copy link
Contributor Author

Nice, all tests passed now for all versions. I'll work on the docs on Monday o/

@moisesguimaraes
Copy link
Contributor Author

@jparise, if merged, do you have a timeline for the next release?

@jparise jparise merged commit f35f215 into pinterest:master Apr 7, 2020
@jparise jparise added this to the 3.1 milestone Apr 7, 2020
@jparise jparise mentioned this pull request Apr 7, 2020
@jparise
Copy link
Collaborator

jparise commented Apr 7, 2020

Thanks again, @moisesguimaraes!

I think we can release this pretty quickly. I put together a 3.1 milestone to track things that will be part of that release.

@jparise
Copy link
Collaborator

jparise commented Apr 9, 2020

@moisesguimaraes version 3.1 has been tagged and released.

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

Successfully merging this pull request may close these issues.

None yet

2 participants