Add support to enable transport encryption TLS #31

shubhanilBag · 2020-01-13T16:55:40Z

This PR:

Adds configurable params to enable transport encryption
Supports adding custom TLS CA cert using a configurable k8s secret
Adds a clqshrc ConfigMap file that allows cqlsh to be used without any additional configuration when TLS is enabled
Adds integration tests to check
1.1 Node-to-Node transport encryption
1.2 Client-to-Node encryption
1.3 Node-to-Node along with Client-to-Node encryption
1.4 Data read and write using cqlsh
Adds and updates documentation for enabling TLS
~~6. Bumps operator version to 0.2.0~~

zmalik

This is looking really good!
leaving some comments to improve the parameter docs

zmalik · 2020-01-22T10:37:47Z

operator/params.yaml

+    default: "false"
+
+  - name: TRANSPORT_ENCRYPTION_REQUIRE_CLIENT_AUTH
+    description: "TODO"


lets add some description here

+1, and the following ones as well.

mpereira · 2020-01-29T12:09:32Z

@shubhanilBag could you please merge in the latest master?

mpereira

Hi @shubhanilBag, thanks for the great progress so far!

A few comments from my side.

mpereira · 2020-01-29T14:07:03Z

operator/operator.yaml

@@ -1,6 +1,6 @@
 apiVersion: kudo.dev/v1beta1
 name: "cassandra"
-operatorVersion: "0.1.2"
+operatorVersion: "0.2.0"


Let's revert the operator version bump. Version bumps should happen in the stable branches.

mpereira · 2020-01-29T14:07:14Z

operator/params.yaml

+    default: "false"
+
+  - name: TRANSPORT_ENCRYPTION_REQUIRE_CLIENT_AUTH
+    description: "TODO"


+1, and the following ones as well.

mpereira · 2020-01-29T14:08:19Z

operator/templates/generate-cqlshrc-sh.yaml

+  name: {{ .Name }}-generate-cqlshrc-sh
+  namespace: {{ .Namespace }}
+data:
+  generate-cqlshrc.sh: |


Could this be a configmap of cqlshrc itself instead of a bash script that generates it?

That is what I was going to do initially, but I made a script so that in future if additional configuration is needed for cqlsh, it can be in one script; like adding custom config containing additional config for cqlsh ref: https://docs.datastax.com/en/cql-oss/3.3/cql/cql_reference/cqlshUsingCqlshrc.html

What's the difference regarding supporting future additional configuration between:
A) a templated bash script that outputs a file
B) a templated file?

like here what we do for kafka custom properties taken from a user provided CM https://github.com/kudobuilder/operators/blob/ebaf9e6dba463aa4ff8ec7b4e537c7fdc8c3f629/repository/kafka/operator/templates/bootstrap.yaml#L175

If this is not required for cqlshrc file then I can revert back to the usual way for mounting a CM and cp it

@mpereira as discussed we are keeping the script based approach as POD_NAME env variable is used in the script to create the hostname

mpereira · 2020-01-29T14:11:59Z

templates/operator/templates/stateful-set.yaml.template

@@ -148,6 +154,17 @@ spec:
            - name: node-readiness-probe-sh
              mountPath: /etc/cassandra/node-readiness-probe.sh
              subPath: node-readiness-probe.sh
+            - name: cassandra-home
+              mountPath: /home/cassandra/.cassandra/


Should cassandra-home be just /home/cassandra/?

Both cqlsh and nodetool creates files in .cassandra, /home/cassandra is not used directly
so directly mounted it in .cassandra, otherwise it would need another mkdir.

cassandra@cassandra-node-0:~ $ ls -lah
total 0
drwxr-xr-x. 3 root root 24 Jan 29 14:46 .
drwxr-xr-x. 1 root root 23 Jan 29 14:46 ..
drwxrwsrwx. 2 root cassandra 66 Jan 29 14:49 .cassandra
cassandra@cassandra-node-0:~$ ls -lah .cassandra/
total 12K
drwxrwsrwx. 2 root cassandra 66 Jan 29 14:49 .
drwxr-xr-x. 3 root root 24 Jan 29 14:46 ..
-rw-------. 1 cassandra cassandra 5 Jan 29 14:49 cqlsh_history
-rw-r--r--. 1 cassandra cassandra 264 Jan 29 14:46 cqlshrc
-rw-r--r--. 1 cassandra cassandra 1.7K Jan 29 14:50 nodetool.history

Let's name it something other than "cassandra-home" then, since it implies /home/cassandra. Maybe something like cassandra-configuration-directory or dot-cassandra.

dot-cassandra SGTM 👍

mpereira · 2020-01-29T14:13:49Z

tests/utils/k8s/k8s.go

@@ -128,3 +128,25 @@ func ExecInPodContainer(

 	return stdout, nil
 }
+
+func FetchLogsOfPod(namespaceName, podName, containerName string) (*bytes.Buffer, error) {


Would PodContainerLogs be a more representative name for this function?

Let's go with FetchContainerLogs

How is that function different than the already existing k8s.GetPodContainerLogs by the way?

It's not, I made a duplicate by mistake 🤔! Thanks for pointing it out, will fix that

mpereira · 2020-01-29T14:14:25Z

tests/utils/cassandra/cassandra.go

@@ -120,3 +122,19 @@ func getConfigurationFromNodeLogs(

 	return configuration, nil
 }
+
+// CQLSH TODO function comment.
+func CQLSH(


Suggested change

func CQLSH(

func cqlsh(

mpereira · 2020-01-29T14:16:47Z

tests/suites/cassandra_tls/tls_test.go

+			Expect(err).To(BeNil())
+			assertNumberOfCassandraNodes(NodeCount)
+		})
+		It("Check container logs", func() {


Try to follow the active voice for the test names, so that it reads like:

It "checks for the container logs"

like the above

It "installs the operator from a directory"

On the other tests as well.

mpereira · 2020-01-29T14:18:02Z

tests/suites/cassandra_tls/tls_test.go

@@ -0,0 +1,214 @@
+package cassandra_tls


Let's put this test file in tests/suites/tls_test.go.

I followed kudo-kafka-operator's way of having tests in different packages with it's own folder. If we have the tls_test.go and sanity_test.go under same package sanity they will share the global vars and functions. We won't get full isolation. like https://github.com/mesosphere/kudo-cassandra-operator/blob/d6f6784bb48ae6b0473a4ed70fc97af783fad2e2/tests/suites/sanity_test.go#L23
and
https://github.com/mesosphere/kudo-cassandra-operator/blob/8932a7e5f58b52495466ca16265aace4b8b3ca97/tests/suites/cassandra_tls/tls_test.go#L19 will conflict if tls_test was in same folder

I see. In that case, let's move

tests/suites/sanity_test.go to tests/suites/sanity/sanity_test.go

tests/suites/cassandra_tls/tls_test.go to tests/suites/tls/tls_test.go.

ANeumann82 · 2020-01-29T16:10:04Z

tests/utils/kudo/kudo.go

@@ -411,7 +411,7 @@ func UpgradeOperator(
 func UninstallOperator(
 	operatorName string, namespaceName string, instanceName string,
 ) error {
-	uninstallScript := "../../scripts/uninstall_operator.sh"
+	uninstallScript := "../../../scripts/uninstall_operator.sh"


This is required because the suites moved one directory deeper?

Maybe we should pass in the scripts directory as an absolute path at some point and not rely on relative paths. Seems to be ok for now though.

Would be ok for now for me too. Is there a way to make this relative to the actual kudo.go file instead, though?

@ANeumann82 right, later we can do something like this: https://github.com/mesosphere/kudo-kafka-operator/pull/6/files#diff-dc3495b5db55989eec09ceab2776f370R29
Set a REPO_ROOT env var and create abs paths using that

It's already available to be passed as an environment variable fwiw: https://github.com/mesosphere/kudo-cassandra-operator/blob/7a143804c906b3f8c5fda40f359ab297506c35ff/run-tests.sh#L7

ANeumann82 · 2020-01-29T16:15:20Z

tests/suites/cassandra_sanity/sanity_test.go

+				"NODE_CPU_MC=200",
+				"NODE_MEM_MIB=800",
+				"PROMETHEUS_EXPORTER_CPU_MC=100",
+				"PROMETHEUS_EXPORTER_MEM_MIB=200",


These are probably development settings for local testing?

right! I will remove these ☑️ from here and tls test

but it's a good practice to have tests that consume as less resource as possible, I added these as the default ones where too large to run in konovy cluster with default config.
It can help in starting up running tests in small clusters faster, plus if we have extra pods in tests like KDC or like a client pod for testing out application features then those can fail to run due to resource starvation on the k8s nodes WDYT @ANeumann82 ?
fyi same practice is followed by kudo-kafka ref:https://github.com/mesosphere/kudo-kafka-operator/blob/b692a7f003afe31a3b35002820542afe62178cbd/tests/utils/client.go#L241

Yeah, I'm fine with that. I didn't know Cassandra enough yet to tune the settings down, but as long as we don't make anything flaky because Cassandra doesn't have enough CPU or Mem, i'm fine with having lower values for the tests.

These settings are too low, and can likely cause resource throttling issues. I've seen it happen during MWTs. Unless we're specifically testing CPU/MEM parameters, let's use the default parameter values.

Signed-off-by: Shubhanil Bag <[email protected]>

* wip * add todo Co-authored-by: Murilo Pereira <[email protected]>

Signed-off-by: Shubhanil Bag <[email protected]>

shubhanilBag · 2020-01-31T13:50:57Z

@zmalik @mpereira @ANeumann82 thanks for the thorough review 👍 added changes as requested in fd0ff10

mpereira

Great work @shubhanilBag. Just a few comments from my side regarding documentation, but nothing blocking. Thanks!

mpereira · 2020-01-31T15:12:55Z

README.md

@@ -48,6 +48,7 @@ kubectl kudo install cassandra
 - [Managing](/docs/managing.md)
 - [Upgrading](/docs/upgrading.md)
 - [Monitoring](/docs/monitoring.md)
+- [Security](./docs/secuity.md)


Suggested change

- [Security](./docs/secuity.md)

- [Security](./docs/security.md)

mpereira · 2020-01-31T15:13:38Z

docs/security.md

@@ -0,0 +1,68 @@
+# Securing KUDO Cassandra Operator instances
+
+The KUDO Cassandra service supports Cassandra’s native transport **encryption**


Let's rephrase all instances of "service" with "operator" to maintain consistency.

Suggested change

The KUDO Cassandra service supports Cassandra’s native transport **encryption**

The KUDO Cassandra operator supports Cassandra’s native transport **encryption**

mpereira · 2020-01-31T15:14:29Z

docs/security.md

+
+The KUDO Cassandra service supports Cassandra’s native transport **encryption**
+mechanism. The service provides automation and orchestration to simplify the use
+of these important features. For more information on Cassandra’s security, read


Let's also rephrase "Cassadra" instances to "Apache Cassandra" for the same reason.

Suggested change

of these important features. For more information on Cassandra’s security, read

of these important features. For more information on Apache Cassandra’s security, read

mpereira · 2020-01-31T15:15:59Z

docs/security.md

+
+```
+kubectl kudo install cassandra \
+    --instance=cassandra --namespace=kudo-cassandra \


Let's maintain consistency of either all parameters in one line, or one parameter per-line when there's lots of them.

Suggested change

--instance=cassandra --namespace=kudo-cassandra \

--instance=cassandra \

--namespace=kudo-cassandra \

mpereira · 2020-01-31T15:28:55Z

I pushed two small commits fixing a documentation thing.

@shubhanilBag would you be able to add an "unreleased" changelog entry as well?

Signed-off-by: Shubhanil Bag <[email protected]>

shubhanilBag · 2020-01-31T16:02:18Z

I pushed two small commits fixing a documentation thing.

@shubhanilBag would you be able to add an "unreleased" changelog entry as well?

@mpereira Added the doc fixes and added changelog entry in b3bf8f8

mpereira · 2020-02-01T01:05:18Z

🚢

mpereira · 2020-02-02T17:24:58Z

Looks like there are two test failures:

[18:14:38][Step 5/7] Summarizing 2 Failures:
[18:14:38][Step 5/7] 
[18:14:38][Step 5/7] [Fail] sanity-test [It] Configures Cassandra properties through custom properties 
[18:14:38][Step 5/7] /kudo-cassandra-operator/tests/suites/sanity/sanity_test.go:151
[18:14:38][Step 5/7] 
[18:14:38][Step 5/7] [Fail] sanity-test [It] Configures Cassandra JVM options through custom options 
[18:14:38][Step 5/7] /kudo-cassandra-operator/tests/suites/sanity/sanity_test.go:175
[18:14:38][Step 5/7] 
[18:14:38][Step 5/7] Ran 7 of 7 Specs in 1109.357 seconds
[18:14:38][Step 5/7] FAIL! -- 5 Passed | 2 Failed | 0 Pending | 0 Skipped

@shubhanilBag could you take a look when you get a chance?

Signed-off-by: Shubhanil Bag <[email protected]>

shubhanilBag · 2020-02-03T12:26:30Z

The sanity test suite was running the tests in this order:

install from community repo
upgrade to local version
Node Scaling tests (3nodes -> 4nodes)
Three parameter update tests

As the result the 3 param update tests were running with 4 nodes in the c* instance, so the SS took more time to update itself than the estimated 5mins of timeout, and failing the tests
In c0fb3bd I moved the node scaling tests to the end. The tests are passing and the update tests are a bit faster now.
cc: @mpereira

zmalik · 2020-02-03T12:30:40Z

tests/utils/cassandra/cassandra.go

@@ -120,3 +122,19 @@ func getConfigurationFromNodeLogs(

 	return configuration, nil
 }
+
+// Cqlsh TODO function comment.


add the comment?

added in a2162f4

zmalik

LGTM! 🎉

Signed-off-by: Shubhanil Bag <[email protected]>

ANeumann82

Looks good!

mpereira

🚢

shubhanilBag added the wip label Jan 13, 2020

shubhanilBag requested a review from mpereira January 13, 2020 16:55

shubhanilBag self-assigned this Jan 13, 2020

shubhanilBag requested a review from zmalik January 13, 2020 17:02

shubhanilBag force-pushed the nil/enable-tls branch from b8553e6 to 69b25e1 Compare January 17, 2020 15:48

shubhanilBag removed the wip label Jan 20, 2020

shubhanilBag marked this pull request as ready for review January 20, 2020 08:15

zmalik reviewed Jan 22, 2020

View reviewed changes

shubhanilBag force-pushed the nil/enable-tls branch from 646e3dd to 8932a7e Compare January 29, 2020 12:24

mpereira suggested changes Jan 29, 2020

View reviewed changes

ANeumann82 reviewed Jan 29, 2020

View reviewed changes

shubhanilBag and others added 10 commits January 30, 2020 15:39

Added support to enable TLS from secret

061a15a

Signed-off-by: Shubhanil Bag <[email protected]>

TLS support fixes (#30)

41260fb

* wip * add todo Co-authored-by: Murilo Pereira <[email protected]>

Updated templates and operator

0dde744

Signed-off-by: Shubhanil Bag <[email protected]>

Add integration tests for TLS

42b1684

Signed-off-by: Shubhanil Bag <[email protected]>

Bump operator version

8ebf016

Signed-off-by: Shubhanil Bag <[email protected]>

Add script to generate cqlshrc

a3be263

Signed-off-by: Shubhanil Bag <[email protected]>

Updated tests to perform a r+w check and added more testing contexts

9db0142

Signed-off-by: Shubhanil Bag <[email protected]>

Added documentation

7a3ec29

Signed-off-by: Shubhanil Bag <[email protected]>

Fix Documentation

6207e10

Signed-off-by: Shubhanil Bag <[email protected]>

Addressed PR reviews

fd0ff10

Signed-off-by: Shubhanil Bag <[email protected]>

shubhanilBag force-pushed the nil/enable-tls branch from 8932a7e to fd0ff10 Compare January 30, 2020 14:32

shubhanilBag requested a review from mpereira January 31, 2020 13:49

mpereira approved these changes Jan 31, 2020

View reviewed changes

mpereira added 2 commits January 31, 2020 16:23

Run format_files.sh after generate_parameters_markdown.py.

21eea07

Fix default markdown output file extension.

3dd71b0

mpereira and others added 2 commits January 31, 2020 16:30

Update development documentation table of contents.

fe0bd9c

Updated docs and changelog

b3bf8f8

Signed-off-by: Shubhanil Bag <[email protected]>

mpereira added 3 commits February 1, 2020 02:02

Remove bad table of contents entry.

a4a67eb

Small rephrase.

1915c39

Run tools/format_files.sh.

ea8152a

Change order of sanity tests

c0fb3bd

Signed-off-by: Shubhanil Bag <[email protected]>

zmalik reviewed Feb 3, 2020

View reviewed changes

zmalik approved these changes Feb 3, 2020

View reviewed changes

Updated function comment

a2162f4

Signed-off-by: Shubhanil Bag <[email protected]>

ANeumann82 approved these changes Feb 3, 2020

View reviewed changes

mpereira approved these changes Feb 3, 2020

View reviewed changes

ANeumann82 merged commit ff33829 into master Feb 7, 2020

ANeumann82 deleted the nil/enable-tls branch February 7, 2020 10:39

ANeumann82 mentioned this pull request Feb 14, 2020

enable custom TLS for Cassandra kudobuilder/operators#176

Closed

	- [Security](./docs/secuity.md)
	- [Security](./docs/security.md)

		@@ -0,0 +1,68 @@
		# Securing KUDO Cassandra Operator instances

		The KUDO Cassandra service supports Cassandra’s native transport encryption

	The KUDO Cassandra service supports Cassandra’s native transport encryption
	The KUDO Cassandra operator supports Cassandra’s native transport encryption

	of these important features. For more information on Cassandra’s security, read
	of these important features. For more information on Apache Cassandra’s security, read

	--instance=cassandra --namespace=kudo-cassandra \
	--instance=cassandra \
	--namespace=kudo-cassandra \

Add support to enable transport encryption TLS #31

Add support to enable transport encryption TLS #31

Conversation

shubhanilBag commented Jan 13, 2020 • edited Loading

zmalik left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpereira commented Jan 29, 2020

mpereira left a 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

shubhanilBag Jan 29, 2020 • edited Loading

Choose a reason for hiding this comment

mpereira Jan 29, 2020 • 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

Choose a reason for hiding this comment

shubhanilBag Jan 29, 2020 • 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

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

shubhanilBag Jan 29, 2020 • 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

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

shubhanilBag Jan 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubhanilBag commented Jan 31, 2020

mpereira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpereira Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpereira commented Jan 31, 2020

shubhanilBag commented Jan 31, 2020 • edited Loading

mpereira commented Feb 1, 2020

mpereira commented Feb 2, 2020

shubhanilBag commented Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmalik left a comment

Choose a reason for hiding this comment

ANeumann82 left a comment

Choose a reason for hiding this comment

mpereira left a comment

Choose a reason for hiding this comment

shubhanilBag commented Jan 13, 2020 •

edited

Loading

zmalik left a comment •

edited

Loading

shubhanilBag Jan 29, 2020 •

edited

Loading

mpereira Jan 29, 2020 •

edited

Loading

shubhanilBag Jan 29, 2020 •

edited

Loading

shubhanilBag Jan 29, 2020 •

edited

Loading

shubhanilBag Jan 29, 2020 •

edited

Loading

mpereira Jan 31, 2020 •

edited

Loading

shubhanilBag commented Jan 31, 2020 •

edited

Loading

shubhanilBag commented Feb 3, 2020 •

edited

Loading