Allow use of GSM for communal credential secret #458

sivaalamp · 2023-07-17T13:28:36Z

This adds a feature flag to get the secret for the communal credential from google secret manager (GSM) instead of from a k8s secret.

It is off by default. You must set the following annotation in the VerticaDB CR:

kind: VerticaDB
metadata:
  name: v
  annotations:
    vertica.com/use-gcp-secret-manager: "true" 
spec:
  communal:
    credentialSecret: <full name of the secret in GSM>
...

The name of the GSM secret is taken from the existing spec.communal.credentialSecret field.

…into main

CLAassistant · 2023-07-17T13:28:43Z

All committers have signed the CLA.

spilchen · 2023-07-17T18:29:53Z

pkg/meta/annotations.go

+	// This is a feature flag for accessing the secrets configured in Google Secret Manager.
+	// This is set to true by default to let the verticadb running cluster to access the secrets.
+	// The value of this annotation is treated as a boolean.
+	GcpGsmAnnotation     = "vertica.com/gcpgsmaccess"


Suggest using '-' to make the annotation more readable:

Suggested change

GcpGsmAnnotation = "vertica.com/gcpgsmaccess"

GcpGsmAnnotation = "vertica.com/use-gcp-secret-manager"

Changed the annotation

spilchen · 2023-07-17T18:31:27Z

pkg/meta/annotations.go

+// UseGCPSecretManager returns true nk all admin commands should use the vclusterOps
+// library rather than admintools.


Looks like a copy/paste error

Suggested change

// UseGCPSecretManager returns true nk all admin commands should use the vclusterOps

// library rather than admintools.

// UseGCPSecretManager returns true if access to the communal secret should go

// through Google's secret manager rather the fetching the secret from k8s meta-data.

Added the appropriate comments

spilchen · 2023-07-17T18:32:37Z

pkg/meta/annotations.go

@@ -37,6 +37,12 @@ const (
 	// treated as a boolean.
 	VClusterOpsAnnotation     = "vertica.com/vcluster-ops"
 	VClusterOpsAnnotationTrue = "true"
+
+	// This is a feature flag for accessing the secrets configured in Google Secret Manager.
+	// This is set to true by default to let the verticadb running cluster to access the secrets.


The default is false, but it's picked when calling the helper function. I would suggest removing this comment line all together then.

Suggested change

// This is set to true by default to let the verticadb running cluster to access the secrets.

The default value is changed to false now and removed the comment about the same

pkg/controllers/vdb/init_db.go

spilchen · 2023-07-17T18:54:08Z

pkg/controllers/vdb/init_db.go

@@ -269,12 +279,55 @@ func (g *GenericDatabaseInitializer) setKerberosAuthParms() {
 	g.ConfigurationParams.Set("KerberosEnableKeytabPermissionCheck", "0")
 }

+func (g *GenericDatabaseInitializer) setAuthFromGCSSecret(ctx context.Context) (string, ctrl.Result, error) {
+	name := "projects/otl-ot2-int-sb/secrets/new_vertica_secret/versions/1"


This name should come from the vdb.Spec.Communal.CredentialSecret

Introduced the annotations to read the project id and secret name

spilchen · 2023-07-17T19:04:11Z

pkg/controllers/vdb/init_db.go

+
+	result, err := client.AccessSecretVersion(ctx, req)
+	if err != nil {
+		g.Log.Info("google: could not find default credentials")


What does the error message mean? What are default credentials

Updated with appropriate error message

spilchen · 2023-07-17T19:06:18Z

pkg/controllers/vdb/init_db.go

@@ -65,6 +70,11 @@ type GenericDatabaseInitializer struct {
 	ConfigurationParams *vtypes.CiMap
 }

+type GcpSecret struct {


Lets not export this struct. Also, secret is too generic. Lets call out the fact that it's for credentials.

Suggested change

type GcpSecret struct {

type gcpCredentialSecret struct {

This comment is obsolete now as I changed it to map[string]string{} and this struct is no more present

spilchen · 2023-07-17T19:08:22Z

pkg/controllers/vdb/init_db.go

+	Accesskey string
+	Secretkey string


Does this need to have specific field names, or could we have used something more generic like map[string]string instead?

Removed the struct and introduced map

spilchen · 2023-07-17T19:10:04Z

pkg/controllers/vdb/init_db.go

@@ -65,6 +70,11 @@ type GenericDatabaseInitializer struct {
 	ConfigurationParams *vtypes.CiMap
 }

+type GcpSecret struct {
+	Accesskey string


Depending on other comments, this may not apply anymore. But I'm including it here so I don't forget... since we use struct to unmarshal JSON, we should have JSON style comments attached with it.

Suggested change

Accesskey string

Accesskey string ``json:"accesskey"`

This comment is obsolete now as I changed it to map[string]string{} and this struct is no more present

spilchen · 2023-07-17T19:10:49Z

pkg/controllers/vdb/init_db.go

+	}
+
+	var gs GcpSecret
+	err = json.Unmarshal([]byte(string(result.Payload.Data)), &gs)


What data type is result.Payload.Data?

it is an uint8[]

I think uint8 is the same as byte. Can we just pass in the payload without a cast?

No casting is done

roypaulin · 2023-07-17T20:12:48Z

pkg/controllers/vdb/init_db.go

+
+	result, err := client.AccessSecretVersion(ctx, req)
+	if err != nil {
+		g.Log.Info("google: could not find default credentials")


Where is it supposed to find the credentials? By credentials do you mean credentials for the GCP account?

It is my understanding that the host where the operator runs has a profile setup to allow it access to the GSM.

Added the proper logs now

spilchen · 2023-07-20T11:47:50Z

pkg/controllers/vdb/init_db.go

+	name := "projects/" + g.Vdb.Annotations[meta.GcpProjectIDAnnotation] +
+		"/secrets/" + g.Vdb.Annotations[meta.GcpSecNameAnnontation] +
+		"/versions/" + g.Vdb.Annotations[meta.GcpSecVersionAnnotation]


We shouldn't use annotations to come up with then secret name. We already have a field in the VerticaDB CR for the name and should use that: spec.communal.credentialSecret

Removed the new annotations and used the existing credentialSecret for GSM as well

spilchen · 2023-07-20T11:48:09Z

pkg/controllers/vdb/init_db.go

+	crc32c := crc32.MakeTable(crc32.Castagnoli)
+	checksum := int64(crc32.Checksum(result.Payload.Data, crc32c))
+	if checksum != *result.Payload.DataCrc32C {
+		return "", ctrl.Result{}, err


err is still nil here I believe.

pkg/controllers/vdb/init_db.go

spilchen · 2023-07-20T11:55:06Z

config/samples/v1beta1_verticadb.yaml

@@ -15,10 +15,16 @@ apiVersion: vertica.com/v1beta1
 kind: VerticaDB
 metadata:
  name: verticadb-sample
+  annotations:


Lets not update this file. This is used to build example text in the CSV -- you see it on opeatorhub.io for instance.

no change in this file now

…etes into gsm_gcp

spilchen · 2023-07-21T16:57:10Z

pkg/meta/annotations.go

+	// This is a feature flag for accessing the secrets configured in Google Secret Manager.
+	// The value of this annotation is treated as a boolean.
+	GcpGsmAnnotation     = "vertica.com/use-gcp-secret-manager"
+	GcpGsmAnnotationTrue = "true"


Do we need these? I don't see it being used anywhere.

These should be supplied in the yaml file to return true from UseGCPSecretManager so that it fetches gsm secrets.

But do we need this const defined in the code here? Where do we use it?

Removed the GcpGsmAnnotationTrue constant, as we can directly supply value true from the yaml file

spilchen · 2023-07-21T16:58:18Z

pkg/controllers/vdb/init_db.go

-		return res, err
+	if meta.UseGCPSecretManager(g.Vdb.Annotations) {
+		auth, res, err := g.setAuthFromGCSSecret(ctx)
+		if err != nil {


We need to check res. Lets use the helper we have:

Suggested change

if err != nil {

if verrors.IsReconcileAborted {

Added as suggested

pkg/controllers/vdb/init_db.go

spilchen · 2023-07-21T17:00:28Z

pkg/controllers/vdb/init_db.go

+	crc32c := crc32.MakeTable(crc32.Castagnoli)
+	checksum := int64(crc32.Checksum(result.Payload.Data, crc32c))
+	if checksum != *result.Payload.DataCrc32C {
+		return "", ctrl.Result{}, err


I know we talked about this on teams, but the code here looks wrong. I think my suggestion will solve it.

spilchen · 2023-07-21T17:01:23Z

pkg/controllers/vdb/init_db.go

+	defer client.Close()
+
+	req := &secretmanagerpb.AccessSecretVersionRequest{
+		Name: name,


Suggest removal of the name parameter:

Suggested change

Name: name,

Name: g.Vdb.Spec.Communal.CredentialSecret,

removed the name variable

spilchen · 2023-07-21T17:02:03Z

config/samples/v1beta1_verticadb.yaml

@@ -15,12 +15,13 @@ apiVersion: vertica.com/v1beta1
 kind: VerticaDB
 metadata:
  name: verticadb-sample
+


minor nit: can we just undo the changes in here entirely. I know semantically nothing changed, I would prefer if this file didn't change at all

No changes now in the file

This upgrades go to version 1.20. This is needed to be compatible with a future change to the vclusterOps library.

spilchen · 2023-07-24T16:41:30Z

pkg/controllers/vdb/init_db.go

+
+	err = json.Unmarshal([]byte(string(result.Payload.Data)), &GcpCred)
+	if err != nil {
+		return "", ctrl.Result{}, fmt.Errorf("checksum failure, expected %d but got %d", *result.Payload.DataCrc32C, checksum)


The error text does make sense here. Is this a copy+paste error?

Moved it already

pkg/controllers/vdb/init_db.go

spilchen · 2023-07-24T16:43:57Z

pkg/controllers/vdb/init_db.go

+		return "", ctrl.Result{Requeue: true}, nil
+	}
+
+	auth := fmt.Sprintf("%s:%s", strings.TrimSuffix(string(accessKey), "\n"), strings.TrimSuffix(string(secretKey), "\n"))


Why do you need to strip newlines from the secrets?

the configuration parameter takes the auth value in terms of accesskey:secretkey, which forming the auth parameter to format in the required format if there are any new lines those needs to be trimmed. I already have checked, if there are new lines in the keys, nothing will happen to the string.

But did you see a case where a newline was at the end of one of the keys?

The similar trimming is done in getCommunalAuth for the accesskey and secretkey. Followed the similar way of coding.

pkg/controllers/vdb/init_db.go

spilchen · 2023-07-25T14:20:33Z

@sivaalamp we are almost finished with the review. There are just two remaining comments to answer and/or address.

This is similar to #458 except we read from GSM for the superuser password secret. We use the same feature flag as before. Here is a sample CR to enable it. ``` kind: VerticaDB metadata: name: v annotations: vertica.com/use-gcp-secret-manager: "true" spec: superuserPasswordSecret: <full name of the secret in GSM> ... ``` The name of the GSM secret is taken from the existing `spec.superuserPasswordSecret` field. Note, if GSM is being used we can't use the canary query for health probes as that depends on the superuser password being a k8s secret. Instead, we check if vertica is running by seeing if anyone is listening on the vertica client port. --------- Co-authored-by: Matt Spilchen <[email protected]>

sivaalamp added 2 commits July 17, 2023 12:11

Made changes to read google secrets from GSM

133f3ad

Merge branch 'main' of https://github.com/sivaalamp/vertica-kubernetes …

31a324d

…into main

spilchen reviewed Jul 17, 2023

View reviewed changes

roypaulin reviewed Jul 17, 2023

View reviewed changes

sivaalamp and others added 3 commits July 18, 2023 14:05

Merge branch 'vertica:main' into main

4fd2e40

Merge branch 'vertica:main' into gsm_gcp

131e02b

Addressed the review comments

c0634ba

spilchen reviewed Jul 20, 2023

View reviewed changes

spilchen changed the title ~~Implement GSM for GCP secrets~~ Allow use of GSM for communal credential secret Jul 20, 2023

Matt Spilchen and others added 3 commits July 21, 2023 11:06

Add changie for CHANGELOG update

bb8b934

Fix for review comments

99738a2

Merge branch 'gsm_gcp' of https://github.com/sivaalamp/vertica-kubern…

48d2d3c

…etes into gsm_gcp

spilchen reviewed Jul 21, 2023

View reviewed changes

spilchen and others added 6 commits July 24, 2023 09:02

Upgrade to go 1.20 (vertica#459)

343ece0

This upgrades go to version 1.20. This is needed to be compatible with a future change to the vclusterOps library.

Addressed the review comments

27f03dd

Fix for review comments

8c83ef3

Add changie for CHANGELOG update

af1ecf1

Merge branch 'vertica:main' into gsm_gcp

e8d1b2b

Fix for review comments and unit test failures

aeed165

spilchen reviewed Jul 24, 2023

View reviewed changes

sivaalamp and others added 2 commits July 25, 2023 04:49

Test commit

ef13181

Merge branch 'vertica:main' into gsm_gcp

5c2cac3

spilchen reviewed Jul 25, 2023

View reviewed changes

pkg/controllers/vdb/init_db.go Show resolved Hide resolved

Added comments for duplicate code

c3d2d57

spilchen approved these changes Jul 26, 2023

View reviewed changes

spilchen merged commit bf12feb into vertica:main Jul 26, 2023
19 checks passed

spilchen mentioned this pull request Jul 31, 2023

Allow use of GSM for superuser password secret #474

Merged

	GcpGsmAnnotation = "vertica.com/gcpgsmaccess"
	GcpGsmAnnotation = "vertica.com/use-gcp-secret-manager"

		// UseGCPSecretManager returns true nk all admin commands should use the vclusterOps
		// library rather than admintools.

Allow use of GSM for communal credential secret #458

Allow use of GSM for communal credential secret #458

Conversation

sivaalamp commented Jul 17, 2023 • edited by spilchen Loading

CLAassistant commented Jul 17, 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spilchen commented Jul 25, 2023

sivaalamp commented Jul 17, 2023 •

edited by spilchen

Loading

CLAassistant commented Jul 17, 2023 •

edited

Loading