-
Notifications
You must be signed in to change notification settings - Fork 17
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
Addresses #27: Adds support for making SSL connections to redis via hiredis 1.0.0+ #36
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jeremy. Probably no need to tag Jeroen in this unless we need his help getting the underlying libraries working.
There are a bunch of things that will make this much easier:
- We should set up github actions and drop appveyor/travis. Travis is queuing for hours for me for some reason, and this will make it easier to configure with-and-without SSL as different versions. I can do that on a separate branch once I get a moment.
- From an interface point of view, if you can feature test from the headers that's fine. The important thing is that from the point of view of functions that are called from R - all must exist. At the moment we have different functions for different types of redis connections, so I guess we're getting a 3rd here? Make sure that a dummy one exists and throws an error in the case where SSL support is not present
- Some docs will be needed for use with SSL. You can put it in the README, the existing vignette or a new one. It would ideally cover the gist of installation of hiredis+ssl support on the 3 main platforms, and any limitations/special considerations of use with ssl
Is this PR alive? Any plans on adding TLS support? |
Hi @Enchufa2, thanks for highlighting this PR, yes, I'm really keen to get this merged in - I think there's probably an action on me to do some further tests and resolve remaining issues and this got rather lost due to other work! I'll take another look and hopefully this can be resolved and @richfitz can merge. Have you been able to test the implementation in the feature/ssl branch? Any feedback welcome. |
yes, comments to look at above. This will also need merging/rebasing against master so that it runs on gha |
Do you prefer rebasing vs. a merge commit of master into the branch? Just done the latter since a rebase at this stage will require a force push back to feature/ssl which might cause issues/nuisance for anyone actively using the code in this branch...if you're happy, I'll stick with the current approach - didn't want to go against any conventions used in the repo. |
I have no strong feelings - I tend to rebase mine, but don't mind what you do |
Not yet. The need has arisen to connect to a TLS-only Azure Redis, and I'm a bit in a rush here. I've seen no docs and I'm unsure about how to use this feature. Is it enough to set |
Ah, ok, apologies, just catching up with where things are with this. I'll make sure there's full documentation before this gets merged in but since you're in a rush, you should be able to get make a connection as an initial test as follows: [this has been tested in a clean Ubuntu 20.04 container with R 4.1.1 installed from packages based on the info at https://cran.r-project.org/bin/linux/ubuntu/, and also on macOS 10.15 with R 4.0.5 - if you're not running on macOS or Linux, it should also be buildable on Windows, I can try and provide some Windows instructions if you're using the Windows platform - I seem to recall that there were some issues that we had to work around] You'll need an install of hiredis with SSL support to build redux against, this should be straightforward to build from source:
Now you should be able to clone and build redux with SSL support (from the fork with the feature/ssl branch)...
You should now be ready to use
It should also be possible to pass the various other connection parameters using the following env vars: At an interactive R prompt the following should now be possible:
Hope this helps, let me know if you're having problems getting things going or anything isn't clear. As I say, detailed documentation will be added soon. |
Thanks for the details. The environment is a CentOS 7 docker image. I was using stunnel as a temporary fix. But will try and report any quirks. |
Just a quick update to the previous comments - thought I'd give this a quick test on CentOS 7, I was getting SSL connection issues, e.g.
Not clear quite what the cause of this is yet - it works fine on macOS and Ubuntu 20.04 but I found that as a workaround, turning on off optimisation on the build of Also note that the redux SSL support has now also been tested and works successfully with a build of redis 6 with SSL support - previously this was being used with stunnel/redis. |
Adds support to redux for making SSL connections to redis. Makes use of SSL support added in hiredis 1.0.0. Addresses #27.
Multi-platform support:
Known issues:
configure
script, if a version of hiredis is not found usingpkg-config
or viaINCLUDE_DIR
andLIB_DIR
being provided to point to a custom compiled version of hiredis, an error is shown with details of the package names to install on common distros. My understanding is that most (all?) distros don't yet have packages for hiredis 1.0.0 and there is no SSL support.Question: @richfitz, @jeroen: How would you like to handle the issue of missing SSL support on standard configurations on Linux/MacOS? For now, should we simply wrap the content of the
redux_redis_connect_ssl
function inconnection.c
in a pre-processor directive that online includes the content of this function for a Windows build and returns an error saying that the library was not built with SSL support for other platforms? This would at least ensure the library continues to operate as before on Linux/MacOS under a standard build.