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.

Sign up for GitHub

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

Jump to bottom

Kubernete: Support for tls/x509 redis session connections #349

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Kubernete: Support for tls/x509 redis session connections #349

wants to merge 1 commit into from

Conversation

bradymiller
Copy link
Member

@bradymiller bradymiller commented Jan 30, 2023 •
edited
Loading

fixes #347

Several more steps to get this project ready for commit:

  1. TODO - tls redis connection with x509 is not working, still analyzing this.
  2. TODO - when tls redis x509 is working, will then need couple changes in haproxy to support this (header cert etc.)
  3. TODO - when redis goes down (delete pod or drain cluster) and then comes back, it becomes much slower to connect (from 15 ms to several seconds), which need to analyze
  4. TODO - when a new openemr pod is starting it needs to not be used until it has completed the startup - DONE in this commit (18f5774) which will bring into this PR soon (and is supported in 7.0.2 and 7.0.3)
  5. TODO - when working, bring over the changes into the applicable dockers (already in 7.0.1 and 7.0.2 and 7.0.3 (commits 80b872d, f2ed987, 8a11a89, a9ef6bf, 81cdb04, ebf6c98, 3c7625b, c41d5f6, 9c9ccc0, 8fc1454, 5bd533d, d7d935e) and when working bring into flex etc.)
    6. FUTURE - Currently building a development version of phpredis to support TLS. In future when the support for TLS is in the production version, can then remove need/support to do the build.
  6. FUTURE - If phpredis session ever supports sentinel mode (there is an issue to support this, but doesn't appear to be any work on this), then would no longer need the redisproxy.

When bring this in, change the 7.0.2 to nothing in the deployment script (so will then use the latest)

Some random commands (so I don't forget them) when I come back to this project in future:

httpd -k restart
php --ri redis
redis-cli --tls -h redis -a defaultpassword --cacert /var/www/localhost/htdocs/openemr/sites/default/documents/certificates/redis-ca
redis-cli --tls -h redis --user admin -a adminpassword --cacert /var/www/localhost/htdocs/openemr/sites/default/documents/certificates/redis-ca

bradymiller
bradymiller commented Jan 30, 2023
View reviewed changes
kubernetes/redis/statefulset-redis.yaml Outdated
@@ -22,7 +22,7 @@ spec:
- |
echo "Copying configuration file"
cp /tmp/redis/redis.conf /etc/redis/redis.conf
if [ "$(redis-cli -h sentinel -p 5000 ping)" != "PONG" ]; then
if [ "$(redis-cli --tls --cacert /certs/ca.crt -h sentinel -p 5000 ping)" != "PONG" ]; then
Copy link
Member Author

Choose a reason for hiding this comment

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

issue will probably crop up here if forcing x509, which can deal with when can test in future

bradymiller
bradymiller commented Jan 30, 2023
View reviewed changes
kubernetes/redis/statefulset-redis.yaml Outdated
@@ -33,11 +33,13 @@ spec:
fi
else
echo "Sentinel found, finding master"
MASTER="$(redis-cli -h sentinel -p 5000 sentinel get-master-addr-by-name mymaster | grep -E '(^redis-\d{1,})|([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})')"
MASTER="$(redis-cli --tls --cacert /certs/ca.crt -h sentinel -p 5000 sentinel get-master-addr-by-name mymaster | grep -E '(^redis-\d{1,})|([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})')"
Copy link
Member Author

Choose a reason for hiding this comment

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

issue will probably crop up here if forcing x509, which can deal with when can test in future

bradymiller
bradymiller commented Jan 30, 2023
View reviewed changes
kubernetes/redis/statefulset-sentinel.yaml Outdated
@@ -27,7 +27,7 @@ spec:
do
for i in ${nodes//,/ }
do
MASTER=$(redis-cli --no-auth-warning --raw -h $i --user admin -a $REDIS_PASSWORD info replication | awk '{print $1}' | grep master_host: | cut -d ":" -f2)
MASTER=$(redis-cli --tls --cacert /certs/ca.crt --no-auth-warning --raw -h $i --user admin -a $REDIS_PASSWORD info replication | awk '{print $1}' | grep master_host: | cut -d ":" -f2)
Copy link
Member Author

Choose a reason for hiding this comment

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

issue will probably crop up here if forcing x509, which can deal with when can test in future

@bradymiller bradymiller self-assigned this Jan 30, 2023
@bradymiller bradymiller changed the title Kubernete: Support for tls/x509 redis session connections (need to wait for next phpredis version release) Kubernete: Support for tls/x509 redis session connections Apr 9, 2023
@bradymiller bradymiller mentioned this pull request May 30, 2023
Open
@Jmevorach
Copy link
Contributor

Good news @bradymiller; the latest release of PHPRedis v6.0.2 (released on 12/16/23) has support for TLS connections and sentinel mode! This should satisfy 6 and 7 on the list above.

Additionally there's some good guidance on how to deploy a web application with Redis without the use of haproxy to Kubernetes that can be found here.

It's also possible to use nginx + cert-manager to set up encryption between services (i.e. OpenEMR and Redis) within a cluster as well which may eliminate the need entirely to implement x.509 connections for Redis by going down the current path.

Finally you should be able to configure a readiness probe within the cluster to detect when the OpenEMR containers are ready to serve traffic. You may be able to specify something like cat /root/instance-swarm-ready as the command for the readiness probe.

Hope this is helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bradymiller bradymiller

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kubernetes: support redis over the wire encryption
2 participants
@bradymiller @Jmevorach