Skip to content
This repository has been archived by the owner on Jul 22, 2020. It is now read-only.

Add support for Cassandra clusters with SSL enabled #25

Merged
merged 1 commit into from
Dec 12, 2016

Conversation

jcam3ron
Copy link
Contributor

@jcam3ron jcam3ron commented Dec 7, 2016

Summary

This change allows use with Cassandra clusters that have SSL enabled. It also supports SSL client auth if a keystore is supplied. I have updated tests, but there is no test that uses SSL because embedded Cassandra (cassandra-unit) does not support SSL connections. I have tested this manually, however.


Pull Request (PR) Checklist

Documentation

  • Documentation in README.md or Wiki updated

Code Review

  • Self code review -- take another pass through the changes yourself
  • Completed all relevant TODOs, or call them out in the PR comments

Tests

  • All tests passes

@jcam3ron
Copy link
Contributor Author

jcam3ron commented Dec 7, 2016

That's an interesting issue causing one of the test environments to fail. Looks like Kotlin must be coercing the "localhost" string to an InetAddress somewhere along the way?

@hhandoko
Copy link
Owner

hhandoko commented Dec 8, 2016

Hi @jcam3ron , I have to check the code but you might be right

I've re-run the test twice and it is failing consistently on the point mentioned. I wonder why it is failing for Apache Cassandra but passing for DataStax and embedded Cassandra.

@hhandoko
Copy link
Owner

hhandoko commented Dec 8, 2016

Hi @jcam3ron , I just noticed that the tests for Apache Cassandra is failing as well on an unrelated branch.

Looking at your PR and other failing tests, my guess is that it could either be:

  • the CI setup script [1], or
  • Apache Cassandra patch release changes

The latter seems more likely as we haven't updated any of the library / driver dependency version, and it was passing fine before recent commits and PRs.

I will create a separate issue for the failing CI, however, how important is this PR to you? I would prefer to address the failing CI before merging this in. Thoughts?

Notes:
[1] - https://github.com/builtamont-oss/cassandra-migration/blob/master/scripts/configure-apache-cassandra.sh

@jcam3ron
Copy link
Contributor Author

jcam3ron commented Dec 8, 2016

Hi @hhandoko, thanks for the quick response!

I'm fine with waiting for the CI fix - I can use a build from my branch until then. It's an odd issue, because the test that is failing doesn't appear to be using Cassandra at all - it simply creates a new ClusterConfiguration object and is failing on an assertion that clusterConfig.getContactpoints()[0] is localhost.

Is it possible that the cassandra.migration.cluster.contactpoints Property could somehow be set to something different for the failing environment (test is showing it is 127.0.0.1 rather than localhost)? My other thought was that localhost was somehow being resolved to 127.0.0.1 on this environment.

@hhandoko
Copy link
Owner

hhandoko commented Dec 8, 2016

Thanks for the additional info @jcam3ron , I'll look into that possibility as well.

@hhandoko
Copy link
Owner

hhandoko commented Dec 12, 2016

Hi @jcam3ron , fix / workaround merged to master, refer to #29. Please rebase / merge your branch and I can merge this PR.

@jcam3ron
Copy link
Contributor Author

done - ready to merge. Thanks @hhandoko

@hhandoko
Copy link
Owner

Thanks @jcam3ron .

Just to elaborate, I found no evidence of coercion from localhost to 127.0.0.1 (it's Array<String> everywhere), but did find that the contactpoint system properties was set to 127.0.0.1 as part of the mvn verify script [1]. I didn't consider it at first as tests are passing in other configuration.

What I did was to create a workaround for assertion to accept either localhost or 127.0.0.1 values for now. I'm in the process of converting the unit tests to Kotlin Spek [2] which will include clearing / resetting system properties before each unit test runs. I did thought of using System Rules [3] as part of the test library, will have to spike a bit on this.

Notes:
[1] - https://github.com/builtamont-oss/cassandra-migration/blob/master/scripts/verify.sh
[2] - https://jetbrains.github.io/spek/
[3] - http://stefanbirkner.github.io/system-rules/

@hhandoko hhandoko merged commit f537e46 into hhandoko:master Dec 12, 2016
@jcam3ron
Copy link
Contributor Author

Cool - thanks for the followup, that's good to know

@hhandoko hhandoko added this to the cassandra-migration-0.10 milestone Dec 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants