Allow connecting to Redis with TLS #11
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wjt
force-pushed
the
T35672-support-redis-tls
branch
from
0c56658 to
e3f8c55
Compare
dbnicholson
approved these changes
Sep 27, 2024
| /* ioredis uses tls.connect() if the tls option is set, and passes it as | ||
| * additional options to tls.connect(). | ||
| */ | ||
| let tls = config.redis_tls ? {} : undefined; |
There was a problem hiding this comment.
I think we also need to support setting the CA certificate since ElastiCache uses a custom certificate and the node tls module only trusts Mozilla's list. That or just always override it to read /etc/ssl/certs/ca-certificates.crt or something like that.
Hmm, I guess not, actually. It does not appear that ElastiCache uses a custom certificate and the Mozilla list will probably work fine. I imagine you already tested this, anyways.
There was a problem hiding this comment.
I did test this. It's running in dev. I was dreading having to do all that tedious plumbing (or forcing node to use the system ca store in our derived image) but it just worked.
wjt
force-pushed
the
T35672-support-redis-tls
branch
2 times, most recently
from
6381b0c to
68f07b7
Compare
Passing a 4-argument function to `app.use` indicates that it is an error handler, called when an exception escapes a request handler. Previously, the function checked whether its first parameter, `err`, is truthy. According to the documentation this is not necessary. Previously, the function would always send a response. According to the documentation, you must first check whether the response headers have already been sent, and if so chain to the next handler (which will in practice be the default handler that abruptly closes the connection in that case). Previously, wheen running under NODE_ENV=test, the function would stringify the error and then join it to the error's stack trace. This is unnecessary because err.stack starts with err.toString(). Simplify this. https://expressjs.com/en/guide/error-handling.html
In the logs for the production instance I see: error: undefined This indicates that, somewhere, an undefined value is being passed to `logger.error()`. Previously there were four calls to `logger.error()` that did not include any additional log message. Add additional context to each. https://phabricator.endlessm.com/T35672
When an ElastiCache Redis cluster is upgraded, the old nodes stick around as long as someone is connected to them, but as readonly replicas. Attempting any write operation fails with: > READONLY You can't write against a read only replica (As described in the ioredis documentation, this can also happen when a replica is promoted to primary due to some error if the Auto-failure ElastiCache option is disabled; but the ElastiCache clusters in Endless's instance of this service have this option set to true, which leads to different behaviour.) Handle the READONLY case as suggested in the ioredis documentation: implement a callback for Redis protocol-level errors that matches against the prefix READONLY and triggers a reconnect in this case. (I would like to add an integration test for this but it's quite non-trivial to do so...) https://github.com/redis/ioredis/blob/main/README.md#reconnect-on-error https://phabricator.endlessm.com/T35146
Previously we've been using a client-side proxy to connect to our TLS-enabled ElastiCache cluster. But ioredis supports connecting with TLS natively. There are two ways to opt in: 1. If a connection string is passed to the Redis constructor, using rediss:// rather than redis:// as the scheme enables TLS. 2. Pass a 'tls' option whose value is a (possibly-empty) object of options to pass through to Node's tls.connect() method. Add a new REDIS_TLS environment variable, which can be set to '1' or 'true' to connect with TLS. If set, set the 'tls' option to an empty object to enable TLS. https://phabricator.endlessm.com/T35672
wjt
force-pushed
the
T35672-support-redis-tls
branch
from
68f07b7 to
8373c45
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we've been using a client-side proxy to connect to
our TLS-enabled ElastiCache cluster. But ioredis supports connecting with TLS
natively. There are two ways to opt in:
rather than redis:// as the scheme enables TLS.
pass through to Node's tls.connect() method.
Add a new REDIS_TLS environment variable, which can be set to '1' or 'true' to
connect with TLS. If set, set the 'tls' option to an empty object to enable TLS.
Stacked on top of:
https://phabricator.endlessm.com/T35672