Support s3 Server Side Encryption #372

roypaulin · 2023-04-12T19:48:03Z

This adds the ability to use s3 server side encryption, provided the vertica server that you run supports it (>= 12.0.1).

There are 3 types of s3 server side encryption. Here are samples to set for each one:

SSE-S3

apiVersion: vertica.com/v1beta1
kind: VerticaDB
metadata:
  name: verticadb-sample
spec:
  communal:
    path: "s3://<name-of-encrypted-bucket>"
    s3ServerSideEncrytion: SSE-S3
...

SSE-KMS

apiVersion: vertica.com/v1beta1
kind: VerticaDB
metadata:
  name: verticadb-sample
spec:
  communal:
    path: "s3://<name-of-encrypted-bucket>"
    s3ServerSideEncrytion: SSE-KMS
    additionalConfig:
      S3SseKmsKeyId: "<kms key identifier>"
...

SSE-C

apiVersion: vertica.com/v1beta1
kind: VerticaDB
metadata:
  name: verticadb-sample
spec:
  communal:
    path: "s3://<name-of-bucket>"
    s3ServerSideEncrytion: SSE-C
    s3SseCustomerKeySecret: "sse-c-key"
...
---
apiVersion: v1
kind: Secret
metadata:
  name: sse-c-key
stringData:
  clientKey: <client-key-contents>

An additional change in here allows you to set any config parameter in the CR and pass it through to the server. This will allow you try out new server versions without waiting for the operator to expose them in the CRD. For example:

apiVersion: vertica.com/v1beta1
kind: VerticaDB
metadata:
  name: verticadb-sample
spec:
  communal:
    additionalConfig:
      <serverSideConfig>: "<value>"
      ...
...

roypaulin · 2023-04-12T19:54:45Z

Shouldn't we have internally a list of allowed parms forauth_conf that we could use to filter out wrong parms passed through additionalConfig?

spilchen · 2023-04-13T12:09:57Z

api/v1beta1/verticadb_webhook.go

+	// communal.s3ServerSideEncryption cannot change after creation
+	if v.Spec.Communal.S3ServerSideEncryption != oldObj.Spec.Communal.S3ServerSideEncryption {
+		err := field.Invalid(field.NewPath("spec").Child("communal").Child("s3ServerSideEncryption"),
+			v.Spec.Communal.S3ServerSideEncryption,
+			"communal.s3ServerSideEncryption cannot change after creation")
+		allErrs = append(allErrs, err)
+	}


Can we move this to a separate function (checkImmutableS3ServerSideEncryption)? I have been doing this for new additions to this function to avoid making it so big.

spilchen · 2023-04-13T12:12:20Z

api/v1beta1/verticadb_webhook.go

@@ -307,6 +338,38 @@ func (v *VerticaDB) validateCommunalPath(allErrs field.ErrorList) field.ErrorLis
 	return append(allErrs, err)
 }

+func (v *VerticaDB) validateS3ServerSideEncryption(allErrs field.ErrorList) field.ErrorList {
+	if !v.IsS3() || v.IsSseS3() || v.Spec.Communal.S3ServerSideEncryption == "" {


What is the v.IsSseS3() check needed for?

It is to return if SSE-S3, but we do not really need it here.

api/v1beta1/verticadb_types.go

spilchen · 2023-04-13T12:18:27Z

api/v1beta1/verticadb_types.go

+	// Available values are: SSE-S3, SSE-KMS and SSE-C.
+	// - SSE-S3: the S3 service manages encryption keys.
+	// - SSE-KMS: encryption keys are managed by the Key Management Service (KMS).
+	// 	 KMS key identifier must be supplied through communal.additionalConfig map.
+	// - SSE-C: the client manages encryption keys and provides them to S3 for each operation.
+	// 	 The client key must be supplied through communal.s3SseCustomerKeySecret.


I suggest we include mention of default value and how to disable encryption.

Suggested change

// Available values are: SSE-S3, SSE-KMS and SSE-C.

// - SSE-S3: the S3 service manages encryption keys.

// - SSE-KMS: encryption keys are managed by the Key Management Service (KMS).

// KMS key identifier must be supplied through communal.additionalConfig map.

// - SSE-C: the client manages encryption keys and provides them to S3 for each operation.

// The client key must be supplied through communal.s3SseCustomerKeySecret.

// Available values are: SSE-S3, SSE-KMS, SSE-C and empty string ("").

// - SSE-S3: the S3 service manages encryption keys.

// - SSE-KMS: encryption keys are managed by the Key Management Service (KMS).

// KMS key identifier must be supplied through communal.additionalConfig map.

// - SSE-C: the client manages encryption keys and provides them to S3 for each operation.

// The client key must be supplied through communal.s3SseCustomerKeySecret.

// - Empty string (""): No encryption. This is the default value.

spilchen · 2023-04-13T12:25:47Z

pkg/builder/builder.go

@@ -875,6 +875,21 @@ func BuildAzureSASCommunalCredSecret(vdb *vapi.VerticaDB, blobEndpoint, sas stri
 	return secret
 }

+// BuildS3SseCustomerKeySecret builds a secret that is setup for S3 SSE-C server-side encryption


I think this is for test purposes only. Can we add a comment to that affect?

Suggested change

// BuildS3SseCustomerKeySecret builds a secret that is setup for S3 SSE-C server-side encryption

// BuildS3SseCustomerKeySecret is a test helper that builds a secret that is setup for

// S3 SSE-C server-side encryption

spilchen · 2023-04-13T12:46:09Z

pkg/controllers/vdb/init_db.go

+	for _, row := range strings.Split(content, "\n") {
+		if row != "" {
+			s := strings.Split(row, " = ")
+			parmNames[strings.ToLower(s[0])] = s[1]


Why do we need to lowercase the parm name? Is this for easy comparison? I think the config parms are case sensitive. So I'm not sure if that is necessary.

For example awsauth and AWSauth are the same. The to lowercase is to avoid this case. I think we should also check additionalConfig keys for this kind of duplication:

additionalConfig: awsauth: "ffff:fff" AWSAuth: "aaaa:aaaa"

We should make sure only is passed into auth_conf. If both are passed, the 1st one in the file will be applied.

EDIT: the last one in the file will be applied.

Okay, please add a comment to explain why this is done.

spilchen · 2023-04-13T12:50:06Z

pkg/controllers/vdb/init_db.go

+// getAdditionalConfigParmsContent constructs a string containing additional server config parameters
+func (g *GenericDatabaseInitializer) getAdditionalConfigParmsContent(content string) string {
+	parmNames := genCommunalParmsMap(content)
+	acpContent := ""


Lets using a string builder rather than rebuilding a string for each parm that we add.

spilchen · 2023-04-13T12:51:26Z

pkg/controllers/vdb/init_db.go

@@ -358,6 +384,19 @@ func (g *GenericDatabaseInitializer) getAzureAuthParmsContent(ctx context.Contex
 	return dedent.Dedent(content), ctrl.Result{}, nil
 }

+// getAdditionalConfigParmsContent constructs a string containing additional server config parameters
+func (g *GenericDatabaseInitializer) getAdditionalConfigParmsContent(content string) string {


What is the behaviour in the server if we put in a config value that the server doesn't recognize. Is there an error in the server? A message in the vertica.log? Or is it silently ignored? We should document what happens here.

we get an error Invalid configuration parameter *****.

What happens to the server then?

The configuration is aborted and process exits.

How does someone debug this? I assume the create db is retried again and again. Each time most of the log files in the pod are removed before attempting create again.

Yes create db is retried again and again and you can see that error in the operator logs.

spilchen · 2023-04-13T12:56:45Z

pkg/controllers/vdb/init_db.go

+	clientKey, ok := secret.Data[cloud.S3SseCustomerKeyName]
+	if !ok {
+		g.VRec.Eventf(g.Vdb, corev1.EventTypeWarning, events.S3SseCustomerWrongKey,
+			"The sseCustomerKey secret '%s' does not have a key named '%s'", g.Vdb.Spec.Communal.S3SseCustomerKeySecret, cloud.S3SseCustomerKeyName)


Suggested change

"The sseCustomerKey secret '%s' does not have a key named '%s'", g.Vdb.Spec.Communal.S3SseCustomerKeySecret, cloud.S3SseCustomerKeyName)

"The s3SseCustomerKey secret '%s' does not have a key named '%s'", g.Vdb.Spec.Communal.S3SseCustomerKeySecret, cloud.S3SseCustomerKeyName)

spilchen · 2023-04-13T13:01:53Z

pkg/controllers/vdb/init_db.go

+	acpContent := ""
+	for k, v := range g.Vdb.Spec.Communal.AdditionalConfig {
+		_, ok := parmNames[strings.ToLower(k)]
+		if !ok {


We should log a warning if we don't include a parameter that was specified.

spilchen · 2023-04-13T13:05:50Z

Shouldn't we have internally a list of allowed parms forauth_conf that we could use to filter out wrong parms passed through additionalConfig?

I don't want to do any kind of filtering. One of the benefits of not filtering is if a new vertica server version comes out that exposes a new parm, you don't need a new operator version to try it out. So, really the operator can't ever know the config parms that are supported.

spilchen · 2023-04-13T13:12:32Z

api/v1beta1/verticadb_webhook.go

@@ -307,6 +338,38 @@ func (v *VerticaDB) validateCommunalPath(allErrs field.ErrorList) field.ErrorLis
 	return append(allErrs, err)
 }

+func (v *VerticaDB) validateS3ServerSideEncryption(allErrs field.ErrorList) field.ErrorList {


A few other things to validate:

should we validate if someone sets the communal.s3SseCustomerKeySecret without setting communal.s3ServerSideEncryption?

if communal.s3ServerSideEncryption is set but the communal path isn't s3?

My idea is that if communal.s3ServerSideEncryption is not set, server-side encryption is considered disabled and other parameters are just ignored. The is a check for that in init_db.go. Same if communal path is not s3, all sse are just ignored. I understand a need of validation for the later.

roypaulin · 2023-04-14T19:27:41Z

Is there anyway to add an e2e test for SSE-C? Does that encryption method have any dependency on the communal storage we use.

It would be difficult to add an e2e test. I was not able to get it to work on minio. I was getting an insecure connection issue. There is also a policy to setup.

spilchen · 2023-04-14T19:57:07Z

Is there anyway to add an e2e test for SSE-C? Does that encryption method have any dependency on the communal storage we use.

It would be difficult to add an e2e test. I was not able to get it to work on minio. I was getting an insecure connection issue. There is also a policy to setup.

Makes sense. Thanks for taking a look.

spilchen · 2023-04-17T00:13:59Z

api/v1beta1/verticadb_types.go

+	// To avoid duplicate values, if a parameter is already set through another CR field,
+	// (like S3ServerSideEncryption through communal.s3ServerSideEncryption), the corresponding
+	// key/value pair is skipped.
+	// These are set only during initial bootstrap. After the database has been initialized,
+	// changing the options in the CR will have no affect in the server.


Suggestion call out what happens if you use a parameter that isn't supported by your server version:

Suggested change

// To avoid duplicate values, if a parameter is already set through another CR field,

// (like S3ServerSideEncryption through communal.s3ServerSideEncryption), the corresponding

// key/value pair is skipped.

// These are set only during initial bootstrap. After the database has been initialized,

// changing the options in the CR will have no affect in the server.

// To avoid duplicate values, if a parameter is already set through another CR field,

// (like S3ServerSideEncryption through communal.s3ServerSideEncryption), the corresponding

// key/value pair is skipped. If a config value is set that isn't supported by the server version

// you are running, the server will fail to start. These are set only during initial bootstrap. After

// the database has been initialized, changing the options in the CR will have no affect in the server.

spilchen · 2023-04-17T00:24:06Z

pkg/controllers/vdb/init_db.go

+			g.VRec.Eventf(g.Vdb, corev1.EventTypeWarning, events.AdditionalConfigParmIgnored,
+				"The additional config parameter '%s' has been ignored because it has already been set in previous steps", k)


A log message is probably sufficient. An event might be a bit alarming.

spilchen · 2023-04-17T00:28:01Z

pkg/controllers/vdb/init_db.go

+	const DefaultSseSupportedVersion = "v12.0.1"
+	// The '%s' are placeholders for the version extracted from a vdb
+	// and the minimum version you must be on to setup server side encryption.
+	eventMsg := "The engine (%s) doesn't have the required change for server-side encryption. " +


This message is very similar to the one in hasCompatibleVersionForKerberos. Could we put that in the generic function and may just pass in the portion that's different (i.e. "server-side encryption")?

spilchen · 2023-04-17T13:10:58Z

pkg/controllers/vdb/init_db.go

@@ -395,8 +395,7 @@ func (g *GenericDatabaseInitializer) getAdditionalConfigParmsContent(content str
 		// we skip to the next parm.
 		_, ok := parmNames[strings.ToLower(k)]
 		if ok {
-			g.VRec.Eventf(g.Vdb, corev1.EventTypeWarning, events.AdditionalConfigParmIgnored,


I'm not sure if you were ready for a re-review, but we should remove this event you added.

spilchen

Looks good. Thanks for applying the revisions.

roypaulin · 2023-04-17T13:43:53Z

Looks good. Thanks for applying the revisions.

Thanks for the comments! They were very helpful and made me understand some aspects of the code.

roypaulin added 2 commits April 12, 2023 21:19

Add server-side encrytion to the operator

da624bc

Merge remote-tracking branch 'origin' into sse

c5ee33a

roypaulin requested a review from spilchen April 12, 2023 19:48

spilchen reviewed Apr 13, 2023

View reviewed changes

spilchen changed the title ~~Support Server Side Encryption~~ Support s3 Server Side Encryption Apr 13, 2023

Add changie

496ed81

spilchen reviewed Apr 13, 2023

View reviewed changes

roypaulin added 2 commits April 14, 2023 21:18

Address review comments

182ae9c

Remove Uppercase

2e0160e

spilchen reviewed Apr 17, 2023

View reviewed changes

Refactor some code

04754f3

spilchen reviewed Apr 17, 2023

View reviewed changes

Removed unused event

60b77e7

spilchen approved these changes Apr 17, 2023

View reviewed changes

roypaulin merged commit 3196931 into vertica:main Apr 17, 2023

roypaulin deleted the sse branch May 4, 2023 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support s3 Server Side Encryption #372

Support s3 Server Side Encryption #372

roypaulin commented Apr 12, 2023 •

edited by spilchen

Loading

roypaulin commented Apr 12, 2023

spilchen Apr 13, 2023

spilchen Apr 13, 2023

roypaulin Apr 13, 2023

spilchen Apr 13, 2023

spilchen Apr 13, 2023

spilchen Apr 13, 2023

roypaulin Apr 13, 2023 •

edited

Loading

spilchen Apr 13, 2023

spilchen Apr 13, 2023

spilchen Apr 13, 2023

roypaulin Apr 13, 2023

spilchen Apr 13, 2023

roypaulin Apr 13, 2023

spilchen Apr 13, 2023

roypaulin Apr 13, 2023

spilchen Apr 13, 2023

spilchen Apr 13, 2023

spilchen commented Apr 13, 2023

spilchen Apr 13, 2023

roypaulin Apr 13, 2023

roypaulin commented Apr 14, 2023

spilchen commented Apr 14, 2023

spilchen Apr 17, 2023

spilchen Apr 17, 2023

spilchen Apr 17, 2023

spilchen Apr 17, 2023

spilchen left a comment

roypaulin commented Apr 17, 2023

	// BuildS3SseCustomerKeySecret builds a secret that is setup for S3 SSE-C server-side encryption
	// BuildS3SseCustomerKeySecret is a test helper that builds a secret that is setup for
	// S3 SSE-C server-side encryption

	"The sseCustomerKey secret '%s' does not have a key named '%s'", g.Vdb.Spec.Communal.S3SseCustomerKeySecret, cloud.S3SseCustomerKeyName)
	"The s3SseCustomerKey secret '%s' does not have a key named '%s'", g.Vdb.Spec.Communal.S3SseCustomerKeySecret, cloud.S3SseCustomerKeyName)

		g.VRec.Eventf(g.Vdb, corev1.EventTypeWarning, events.AdditionalConfigParmIgnored,
		"The additional config parameter '%s' has been ignored because it has already been set in previous steps", k)

Support s3 Server Side Encryption #372

Support s3 Server Side Encryption #372

Conversation

roypaulin commented Apr 12, 2023 • edited by spilchen Loading

roypaulin commented Apr 12, 2023

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

roypaulin Apr 13, 2023 • 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

Choose a reason for hiding this comment

spilchen commented Apr 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roypaulin commented Apr 14, 2023

spilchen commented Apr 14, 2023

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

spilchen left a comment

Choose a reason for hiding this comment

roypaulin commented Apr 17, 2023

roypaulin commented Apr 12, 2023 •

edited by spilchen

Loading

roypaulin Apr 13, 2023 •

edited

Loading