Skip to content

Enable TLS support for Redis#976

Merged
sylvestre merged 2 commits intomozilla:masterfrom
alnr:redis-tls
Nov 11, 2021
Merged

Enable TLS support for Redis#976
sylvestre merged 2 commits intomozilla:masterfrom
alnr:redis-tls

Conversation

@alnr
Copy link
Contributor

@alnr alnr commented Mar 19, 2021

This enables TLS for Redis backends. Tested against an AWS ElastiCache Redis with TLS. I'm a Rust newbie so I don't know if the dependency changes (in Cargo.lock) are OK.

@Xanewok
Copy link
Collaborator

Xanewok commented Apr 2, 2021

I don't think these lockfile changes are good since they pull Tokio 1.0 in addition to 0.1 used now. I think it's best to defer that until the futures migration is done (see #946); from there migrating to Tokio 0.3 or 1.0 should be somewhat easier. Also see 10b072d for more context

@alnr
Copy link
Contributor Author

alnr commented Aug 3, 2021

@Xanewok Could you take another look? This time, I haven't upgraded the redis-rs version at all, just fiddled with the features. Turns out several default features are not used and were bloating Cargo.lock.

I've also added a small improvement (masking the password in the Redis URL if there is one).

/// Create a new `RedisCache`.
pub fn new(url: &str) -> Result<RedisCache> {
let mut parsed = Url::parse(url)?;
if parsed.password().is_some() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add a comment to explain that it is for the TLS support? thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is unrelated to TLS. Still, I updated the diff to match paritytech/cachepot#98.

@alnr
Copy link
Contributor Author

alnr commented Nov 11, 2021

Hi @sylvestre
I rebased; pipeline is green :)

@sylvestre sylvestre merged commit 73f26e3 into mozilla:master Nov 11, 2021
@alnr alnr deleted the redis-tls branch November 15, 2021 11:19
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.

3 participants