-
Notifications
You must be signed in to change notification settings - Fork 94
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
Redis authentication support #244
Conversation
import redis.embedded.RedisServer; | ||
import redis.embedded.RedisServerBuilder; | ||
|
||
public class RedisAuthenticationIntegrationTest { |
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.
It will be useful to also add another testcase showing failure to connect to redisServer (with Auth) and client (no auth).
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.
I had to drop from the DynoJedisClient
down to the JedisConnectionFactory
to get any kind of valuable unhappy path tests. I've added test methods to cover these cases from a lower layer, with the following cases added:
- Connect failed
- Password required
- Invalid password
@@ -87,6 +87,7 @@ project(':dyno-jedis') { | |||
compile project(':dyno-core') | |||
compile project(':dyno-contrib') | |||
compile 'redis.clients:jedis:2.9.0' | |||
testCompile 'com.netflix.spinnaker.embedded-redis:embedded-redis:0.8.0' |
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.
If embedded-redis doesn't work on Windows, use conditional ignore (https://junit.org/junit4/javadoc/4.12/org/junit/Assume.html) to ignore such tests
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.
Also I am curious to know why embedded-redis doesn't work on windows
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.
It looks like a change introduced on the Spinnaker fork of kstyrc/embedded-redis (which smelled abandoned to me). I saw it noted on the README of the Spinnaker fork.
The reason is the library actually embeds OS-specific executables that are executed as separate processes.
I will follow your recommendation to conditionally ignore this test and make sure things are green on Windows.
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.
This change is reflected here. Checked the tests are skipped on Windows
@Before
public void setUp() throws Exception {
// skip tests on windows due to https://github.com/spinnaker/embedded-redis#redis-version
Assume.assumeFalse(System.getProperty("os.name").toLowerCase().startsWith("win"));
}
b5adedb
to
ce1bce7
Compare
+ Adds log4j.properties to dyno-jedis test resources
ce1bce7
to
dbf8b69
Compare
* Supports Redis password authentication * Tests Redis auth against embedded Redis instance + Adds log4j.properties to dyno-jedis test resources
Inspired by #187
I'm interested in deploying Conductor to an environment that mandates Redis authentication. I've followed a similar pattern as #187 for adding a
password
field toHost.java
which then in turn is passed through theHostsUpdater.java
and set at the construction of theJedisConnectionFactory.java
.Interestings / questions:
Host.java
. Do you want any additional overloads that accept thepassword
?.testCompile
dependency onembedded-redis
Host#toString
masks the contents of thepassword
field. Just a little non-standardAlso, noticed constructing a
Host
to be getting a bit complicated with the overloads. Would you entertain another PR to introduce aHostBuilder
to simplify this?