Make spread encryption the default for v1 API create_db #590
Conversation
cchen-vertica
changed the title
cchen-vertica
force-pushed
the
cchen/make-spread-encryption-default
branch
from
b36e4e0
to
2e71868
Compare
| @@ -175,7 +175,7 @@ func (c *CreateDBReconciler) generatePostDBCreateSQL(ctx context.Context, initia | |||
| // config parm in the create db call. | |||
| if c.Vdb.Spec.EncryptSpreadComm != vapi.EncryptSpreadCommDisabled && ok && vinf.IsOlder(vapi.SetEncryptSpreadCommAsConfigVersion) { | |||
| sb.WriteString(fmt.Sprintf(`alter database default set parameter EncryptSpreadComm = '%s'; | |||
| `, c.Vdb.Spec.EncryptSpreadComm)) | |||
| `, vapi.EncryptSpreadCommWithVertica)) | |||
There was a problem hiding this comment.
Why did you do this? What if a user wants to disable spread encryption(if that even makes sense)?
There was a problem hiding this comment.
The code here is for enabling spread encryption. The if statement above makes sure the user does not want to disable spread encryption. When encryptSpreadComm is an empty string, 'c.Vdb.Spec.EncryptSpreadComm' won't give the correct value to the ddl, but 'vapi.EncryptSpreadCommWithVertica' will.
| v, ok := g.ConfigurationParams.Get("InitialDefaultSubclusterName") | ||
| Expect(ok).Should(BeTrue()) | ||
| Expect(v).Should(Equal(vdb.Spec.Subclusters[0].Name)) | ||
| v, ok = g.ConfigurationParams.Get("EncryptSpreadComm") |
There was a problem hiding this comment.
I think you can create a new test function for spread encryption where you can directly call setEncryptSpreadCommConfigIfNecessary() and have a few scenarios by changing EncryptSpreadComm and vertica version.
| @@ -36,3 +36,4 @@ spec: | |||
| imagePullSecrets: [] | |||
| volumes: [] | |||
| volumeMounts: [] | |||
| encryptSpreadComm: disabled | |||
There was a problem hiding this comment.
Why didn't you do the same for other e2e tests?
There was a problem hiding this comment.
I think encrypting spread channel will not affect other e2e tests. And since "enable" is the default behavior now, we can test the default behavior in other e2e tests.
It turns out the admintool e2e tests are using old Vertica version(12.0.3) that does not support spread channel encryption without a db restart. Do you think should we update the Vertica version for the admintool e2e tests or should I disable spread channel encryption for the admintool e2e tests?
pkg/controllers/vdb/config.go
Outdated
|
|
||
| // getEncryptSpreadComm will return "vertica" if encryptSpreadComm is set to | ||
| // an empty string, otherwise return the value of encryptSpreadComm | ||
| func (g *ConfigParamsGenerator) getEncryptSpreadComm() string { |
There was a problem hiding this comment.
Can we move this to VeritcaDB? And rename it so that it's exported (i.e. GetEncryptSpreadComm). There is a file (helpers.go) that has these kind of functions.
spilchen
changed the title
For v1 api, set 'vertica' as the default value of encryptSpreadComm: when encryptSpreadComm is set to an empty string or not set, the operator will also enable spread encryption when creating a database. Add another value "disabled" to encryptSpreadComm for the user to explicitly disable spread encryption.