Enable client-side CQL encryption in Stargate if it is configured on the cluster (fixes #722) #733

olim7t · 2022-10-20T01:44:09Z

What this PR does:
Generate a config file to enable client encryption for Stargate's CQL module.
Adapt e2e tests (thankfully NGINX ingress forwards TCP port 9042 the same whether it uses SSL or not, so the only thing to change is client code).

Which issue(s) this PR fixes:
Fixes #722

Checklist

Changes manually tested
Automated Tests added/updated
Documentation added/updated
CHANGELOG.md updated (not required for documentation PRs)
CLA Signed: DataStax CLA

olim7t · 2022-10-20T01:44:55Z

apis/stargate/v1alpha1/stargate_types.go

@@ -43,7 +43,7 @@ type StargateTemplate struct {
 	// ContainerImage is the image characteristics to use for Stargate containers. Leave nil
 	// to use a default image.
 	// +optional
-	// +kubebuilder:default={repository:"stargateio", tag:"v1.0.45"}
+	// +kubebuilder:default={repository:"stargateio", tag:"v1.0.66"}


-Dstargate.cql.config_path was introduced in 1.0.64. I went with the latest.

olim7t · 2022-10-20T01:46:01Z

controllers/stargate/stargate_controller.go

-	desiredConfig map[string]interface{},
-	userConfigMapContent string,
+	dcConfig map[string]interface{},
+	userCassandraYaml, userCqlYaml string,


A bit of naming/refactoring in this function, because it was getting really confusing with two files.

olim7t · 2022-10-20T01:46:32Z

go.mod

@@ -7,7 +7,7 @@ require (
 	github.com/Masterminds/semver/v3 v3.1.1
 	github.com/apache/tinkerpop/gremlin-go v0.0.0-20220530191148-29272fa563ec
 	github.com/bombsimon/logrusr/v2 v2.0.1
-	github.com/datastax/go-cassandra-native-protocol v0.0.0-20210829124742-a80a54434112
+	github.com/datastax/go-cassandra-native-protocol v0.0.0-20220706104457-5e8aad05cf90


Pull in TLS support.

olim7t · 2022-10-20T01:52:33Z

test/framework/cql.go

+		if err != nil {
+			return nil, err
+		}
+		rootCas, err := extractCaCertificates(secret)


Alternatively we could extract the certificate manually once, commit it and just load that file.
Pros:

this avoids the extra dependency to jceks (keystore management).

Cons:

we'd have to keep the cert file and config map in sync, but the certs are valid until 2049...

this is test code and it works so why change it

Happy with what we have here, no need to change.

adejanovski

Awesome work @olim7t 👏
Just one tiny question around the use case of externally provided cql yaml options to Stargate, which is non blocking for me. If there are specific settings other than client_encryption_options which should go there instead of cassandra.yaml, we'll need to document them.

adejanovski · 2022-10-20T15:07:07Z

controllers/stargate/stargate_controller.go

 	if stargate.Spec.CassandraConfigMapRef != nil {
 		userConfigMap := &corev1.ConfigMap{}
 		configMapKey := types.NamespacedName{Namespace: req.Namespace, Name: stargate.Spec.CassandraConfigMapRef.Name}
 		err := r.Get(ctx, configMapKey, userConfigMap)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
-		userConfigMapContent = userConfigMap.Data["cassandra.yaml"]
+		userStargateCassandraYaml = userConfigMap.Data["cassandra.yaml"]
+		userStargateCqlYaml = userConfigMap.Data[stargateutil.CqlConfigName]


I'm trying to think of a case where we'd get a user input that should go into that Stargate cql config file.
Which settings other than client_encryption_options have to be passed through this file?

adejanovski · 2022-10-20T15:14:56Z

pkg/stargate/config.go

 		},
 	}
 }

-func MergeConfigMaps(userConfigMap string, generatedConfigMap string) string {
+func MergeYamlString(userConfigMap string, generatedConfigMap string) string {


yep, definitely more accurate 👍

adejanovski · 2022-10-20T15:17:43Z

test/framework/cql.go

+		if err != nil {
+			return nil, err
+		}
+		rootCas, err := extractCaCertificates(secret)


Happy with what we have here, no need to change.

…the cluster

olim7t · 2022-10-20T17:47:54Z

If there are specific settings other than client_encryption_options which should go there instead of cassandra.yaml, we'll need to document them.

Right, there might be more than the encryption options.
The full list supported by Stargate CQL is:

rpc_address
rpc_interface
rpc_interface_prefer_ipv6
rpc_keepalive *
native_transport_port
native_transport_port_ssl
native_transport_max_frame_size_in_mb *
native_transport_max_concurrent_connections *
native_transport_max_concurrent_connections_per_ip *
native_transport_flush_in_batches_legacy *
native_transport_allow_older_protocols *
native_transport_max_concurrent_requests_in_bytes_per_ip *
native_transport_max_concurrent_requests_in_bytes *
native_transport_idle_timeout_in_ms *

For the ones I marked with *: I think it make sense to copy to stargate_cql.yaml, I will add them to the list.

RPC address / interface: unlikely to be set globally on the CassandraDatacenter, if they are set to non-default values it's unlikely that it would be something global for all nodes.

Ports: the service hard-codes port 9042, and it doesn't allow both SSL and non-SSL either. So I don't think customizing those is a use case either.

codecov-commenter · 2022-10-20T21:57:01Z

Codecov Report

Merging #733 (3702173) into main (237a335) will decrease coverage by 0.02%.
The diff coverage is 55.81%.

@@            Coverage Diff             @@
##             main     #733      +/-   ##
==========================================
- Coverage   54.14%   54.12%   -0.03%     
==========================================
  Files          82       82              
  Lines        8342     8361      +19     
==========================================
+ Hits         4517     4525       +8     
- Misses       3376     3384       +8     
- Partials      449      452       +3

Impacted Files	Coverage Δ
apis/stargate/v1alpha1/stargate_types.go	`96.15% <ø> (ø)`
pkg/stargate/config.go	`0.00% <0.00%> (ø)`
controllers/stargate/stargate_controller.go	`46.36% <60.60%> (+0.30%)`	⬆️
pkg/stargate/deployments.go	`92.81% <100.00%> (+0.08%)`	⬆️
controllers/replication/secret_controller.go	`64.42% <0.00%> (-1.35%)`	⬇️

olim7t requested a review from a team as a code owner October 20, 2022 01:44

olim7t commented Oct 20, 2022

View reviewed changes

adejanovski self-requested a review October 20, 2022 13:07

adejanovski approved these changes Oct 20, 2022

View reviewed changes

Enable client-side CQL encryption in Stargate if it is configured on …

1a6e592

…the cluster

olim7t force-pushed the stargate-client-tls branch from 221817f to 1a6e592 Compare October 20, 2022 16:12

Add more retained options for stargate-cql.yaml

e4ac26b

olim7t force-pushed the stargate-client-tls branch from 06c82b7 to e4ac26b Compare October 21, 2022 16:11

Fix parameter in tests

3702173

olim7t merged commit 1055073 into k8ssandra:main Oct 21, 2022

olim7t deleted the stargate-client-tls branch October 21, 2022 20:28

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable client-side CQL encryption in Stargate if it is configured on the cluster (fixes #722) #733

Enable client-side CQL encryption in Stargate if it is configured on the cluster (fixes #722) #733

olim7t commented Oct 20, 2022

olim7t Oct 20, 2022 •

edited

Loading

olim7t Oct 20, 2022

olim7t Oct 20, 2022

olim7t Oct 20, 2022

adejanovski Oct 20, 2022

adejanovski left a comment

adejanovski Oct 20, 2022

adejanovski Oct 20, 2022

adejanovski Oct 20, 2022

olim7t commented Oct 20, 2022

codecov-commenter commented Oct 20, 2022 •

edited

Loading

Enable client-side CQL encryption in Stargate if it is configured on the cluster (fixes #722) #733

Enable client-side CQL encryption in Stargate if it is configured on the cluster (fixes #722) #733

Conversation

olim7t commented Oct 20, 2022

olim7t Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

olim7t Oct 20, 2022

Choose a reason for hiding this comment

olim7t Oct 20, 2022

Choose a reason for hiding this comment

olim7t Oct 20, 2022

Choose a reason for hiding this comment

adejanovski Oct 20, 2022

Choose a reason for hiding this comment

adejanovski left a comment

Choose a reason for hiding this comment

adejanovski Oct 20, 2022

Choose a reason for hiding this comment

adejanovski Oct 20, 2022

Choose a reason for hiding this comment

adejanovski Oct 20, 2022

Choose a reason for hiding this comment

olim7t commented Oct 20, 2022

codecov-commenter commented Oct 20, 2022 • edited Loading

Codecov Report

olim7t Oct 20, 2022 •

edited

Loading

codecov-commenter commented Oct 20, 2022 •

edited

Loading