Add cassandra TLS support #2769

aoby · 2019-11-06T01:07:25Z

Consolidate duplicate Cassandra setup and config into a new common/cassandra package
Add TLS support for Cassandra
Add TLS flags to cadence-cassandra-tool
Combine TLS config structs into new common auth.TLS
Add more TLS options:
- CaFile
- EnableHostVerification
- Remove TLS bundleFile option in favor of caFile option

CLAassistant · 2019-11-06T01:07:48Z

All committers have signed the CLA.

coveralls · 2019-11-06T01:43:50Z

Coverage decreased (-0.002%) to 66.653% when pulling 1d3bde4 on aoby:cassandra-tls-support into 925d24d on uber:master.

wxing1292 · 2019-11-06T01:52:14Z

common/persistence/cassandra/cassandraHelpers.go

 func LoadCassandraSchema(
-	dir string, fileNames []string, hosts []string, port int, keyspace string, override bool,
+	dir string, fileNames []string, hosts []string, port int, keyspace string, override bool, useTLS bool, tlsCertPath, tlsKeyPath string,
 ) (err error) {


plz format like this:

func LoadCassandraSchema( dir string, fileNames []string, hosts []string, port int, keyspace string, override bool, useTLS bool, tlsCertPath string, tlsKeyPath string, ) error {

it will be easier to read

wxing1292 · 2019-11-06T01:53:17Z

common/persistence/cassandra/cassandraHistoryPersistence.go

+	"github.com/uber/cadence/common/cassandra"
+
 	"github.com/gocql/gocql"

 	workflow "github.com/uber/cadence/.gen/go/shared"


plz merge from master then do

make fmt

it will sort all imports correctly

wxing1292 · 2019-11-06T01:58:09Z

common/service/config/config.go

+	TLS struct {
+		// Optional path to a TLS client certificate
+		CertPath string `yaml:"certPath"`
+		// Optional path to the key file for a TLS client certificate
+		KeyPath string `yaml:"keyPath"`
 	}


can you put the TLS config in 'common/auth'
and reuse this definition: https://github.com/uber/cadence/blob/master/common/messaging/kafkaConfig.go#L38
?

wxing1292 · 2019-11-06T01:59:12Z

tools/cassandra/cqlclient.go

+		UseTLS      bool
+		TLSCertPath string
+		TLSKeyPath  string


pls use the above TLS config for simplicity, like

CQLClientConfig struct { Hosts string Port int User string Password string Keyspace string Timeout int numReplicas int TLS auth.TLS }

wxing1292 · 2019-11-06T02:02:00Z

tools/cli/admin.go

@@ -64,6 +64,18 @@ func newAdminWorkflowCommands() []cli.Command {
 					Name:  FlagKeyspace,
 					Usage: "cassandra keyspace",
 				},
+				cli.BoolFlag{
+					Name:  FlagUseTLS,


can we change this to FlagEnableTLS ?

wxing1292 · 2019-11-06T02:02:54Z

the rest LGTM

meiliang86

Looks good overall. Can you address all the comments before merging? Thanks!

meiliang86 · 2019-11-06T21:33:56Z

common/cassandra/cassandraCluster.go

+	"strings"
+
+	"github.com/gocql/gocql"
+


Remove extra empty line.

meiliang86 · 2019-11-06T21:39:35Z

common/persistence/cassandra/cassandraHelpers.go

@@ -92,7 +65,7 @@ func DropCassandraKeyspace(s *gocql.Session, keyspace string) (err error) {

 // LoadCassandraSchema loads the schema from the given .cql files on this keyspace
 func LoadCassandraSchema(
-	dir string, fileNames []string, hosts []string, port int, keyspace string, override bool,
+	dir string, fileNames []string, hosts []string, port int, keyspace string, override bool, useTLS bool, tlsCertPath, tlsKeyPath string,


Seems like this method is only used for testing. Can you make it private?

aoby · 2019-11-06T23:46:06Z

Thanks @wxing1292 and @meiliang86 for the quick feedback!
I made the requested changes.

I realized in local testing that I needed the settings in cadence-cassandra-tool for schema setup, so I added CLI options there as well. I also added settings for the remaining Cassandra tls/ssl options, CaPath and EnableHostVerification.

Please take another look

wxing1292 · 2019-11-06T23:46:17Z

common/auth/tls.go

+		BundleFile string `yaml:"bundleFile"`
+
+		CaFile string `yaml:caFile` //optional depending on server config


these 2 variables are the same. let us keep "CaFile string yaml:caFile" for readability.
but before landing this, let me check with one customer who is already using TLS with kafka

plz also do a make fmt locally

I ran make fmt earlier, and it made some formatting changes (included in the last commit).
I just ran it again with no change. Is there something you expect to be reformatted?

one more thing, the Contributor License Agreement does not really recognize your name. one thing you could try is to squash all commits and force push

I ran make fmt earlier, and it made some formatting changes (included in the last commit).
I just ran it again with no change. Is there something you expect to be reformatted?

if that is the case, then plz just ignore

one more thing, the Contributor License Agreement does not really recognize your name. one thing you could try is to squash all commits and force push

I think because I made the first commit from my work account, which is private. I tried to re-author it but that apparently didn't work. I'll try the squash & force push...

- Consolidate duplicate cassandra setup and config into a new common/cassandra package - Add tls support Address feedback, add more options, add tls flags to cadence-cassandra-tool - Combine TLS structs into new common auth.TLS - Fix formatting - Add more cassandra TLS options: - CaFile - EnableHostVerification - Add TLS CLI flags to cadence-cassandra-tool Remove empty line, make test method private.

wxing1292 · 2019-11-07T01:10:00Z

there are also 4 files to be changed (bundleFile -> caFile):
https://github.com/uber/cadence/blob/master/config/development_active.yaml#L101
https://github.com/uber/cadence/blob/master/config/development_other.yaml#L101
https://github.com/uber/cadence/blob/master/config/development_standby.yaml#L101
https://github.com/uber/cadence/blob/master/tools/cli/admin.go#L376

vancexu · 2019-11-07T17:48:22Z

common/messaging/kafkaConfig.go

-		Enabled    bool   `yaml:"enabled"`
-		CertFile   string `yaml:"certFile"`
-		KeyFile    string `yaml:"keyFile"`
-		BundleFile string `yaml:"bundleFile"`


This will be a breaking change for users who use TLS in Kafka.
@wxing1292 Do you know if any customer is using this? Can we notice them and get this change in?

Otherwise for safety, I would keep it for kafka, and use the new auth.TLS for Cassandra and everything else.
Or just use the old name "bundleFile"

talked to our customer, they are ok

* Consolidate duplicate Cassandra setup and config into a new common/cassandra package * Add TLS support for Cassandra * Add TLS flags to cadence-cassandra-tool * Combine TLS config structs into new common auth.TLS * Add more TLS options: - CaFile - EnableHostVerification - Remove TLS bundleFile option in favor of caFile option

aoby force-pushed the cassandra-tls-support branch from c20328c to d946afc Compare November 6, 2019 01:22

wxing1292 requested review from wxing1292, meiliang86, andrewjdawson2016, yux0, yycptt and vancexu November 6, 2019 01:36

wxing1292 reviewed Nov 6, 2019

View reviewed changes

meiliang86 approved these changes Nov 6, 2019

View reviewed changes

wxing1292 reviewed Nov 6, 2019

View reviewed changes

aoby force-pushed the cassandra-tls-support branch from bbde8c2 to 6673a3f Compare November 7, 2019 00:02

aoby and others added 2 commits November 6, 2019 19:45

Remove tls bundleFile option in favor of caFile

09edd0f

Merge branch 'master' into cassandra-tls-support

a4ec580

vancexu suggested changes Nov 7, 2019

View reviewed changes

Fix yaml tag format and pass-through tls in another spot

dd24d94

aoby force-pushed the cassandra-tls-support branch from 839fa97 to dd24d94 Compare November 7, 2019 18:21

vancexu approved these changes Nov 7, 2019

View reviewed changes

Merge branch 'master' into cassandra-tls-support

1d3bde4

wxing1292 merged commit e0ef19e into uber:master Nov 7, 2019

aoby deleted the cassandra-tls-support branch November 13, 2019 18:27

ghost mentioned this pull request Feb 4, 2020

Cassandra SSL in official 0.10.4 release? #3025

Closed

aoby mentioned this pull request Feb 21, 2020

Add ServerName to TLS config. #3066

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cassandra TLS support #2769

Add cassandra TLS support #2769

aoby commented Nov 6, 2019 •

edited by wxing1292

Loading

CLAassistant commented Nov 6, 2019 •

edited

Loading

coveralls commented Nov 6, 2019 •

edited

Loading

wxing1292 Nov 6, 2019

wxing1292 Nov 6, 2019

wxing1292 Nov 6, 2019

wxing1292 Nov 6, 2019

wxing1292 Nov 6, 2019 •

edited

Loading

wxing1292 Nov 6, 2019

wxing1292 commented Nov 6, 2019

meiliang86 left a comment

meiliang86 Nov 6, 2019

meiliang86 Nov 6, 2019

aoby commented Nov 6, 2019

wxing1292 Nov 6, 2019

wxing1292 Nov 6, 2019

aoby Nov 6, 2019

wxing1292 Nov 6, 2019

wxing1292 Nov 6, 2019

aoby Nov 7, 2019

wxing1292 commented Nov 7, 2019 •

edited

Loading

vancexu Nov 7, 2019

wxing1292 Nov 7, 2019

		BundleFile string `yaml:"bundleFile"`

		CaFile string `yaml:caFile` //optional depending on server config

Add cassandra TLS support #2769

Add cassandra TLS support #2769

Conversation

aoby commented Nov 6, 2019 • edited by wxing1292 Loading

CLAassistant commented Nov 6, 2019 • edited Loading

coveralls commented Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxing1292 Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxing1292 commented Nov 6, 2019

meiliang86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aoby commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxing1292 commented Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aoby commented Nov 6, 2019 •

edited by wxing1292

Loading

CLAassistant commented Nov 6, 2019 •

edited

Loading

coveralls commented Nov 6, 2019 •

edited

Loading

wxing1292 Nov 6, 2019 •

edited

Loading

wxing1292 commented Nov 7, 2019 •

edited

Loading