Adding secretsmanager support #305

RealHarshThakur · 2021-02-21T19:09:13Z

Signed-off-by: Harsh Thakur [email protected]

Description of your changes

Fixes ##294

I have:

Read and followed Crossplane's contribution process.
[] Run make reviewable test to ensure this PR is ready for review.

TODO:

Implement type SecretVersion . This is where data is going to reside
Add Kubernetes SecretRef

How has this code been tested

Add unit tests

Manually tried creating, updating, syncing and deleting the secret(GCP) resource.
Manually tried creating a secret version , changed it's states and deleting the secret version

Signed-off-by: Harsh Thakur <[email protected]>

hasheddan

@RealHarshThakur thanks for working on this and apologies for the review delay! I think we should be offering two different CRDs here to match the GCP API (Secret and SecretVersion).

Some helpful references could be the similar implementation in provider-aws as well as the terraform docs for this API.

apis/secretsmanager/v1alpha1/secret_types.go

Signed-off-by: Harsh Thakur <[email protected]>

hasheddan

Thanks so much for working on this @RealHarshThakur! Let me know if you have any questions :)

hasheddan · 2021-04-01T00:21:33Z

apis/secretsmanager/v1alpha1/secret_types.go

+
+	// Required. The resource name of the project to associate with the
+	// [Secret][google.cloud.secretmanager.v1.Secret], in the format `projects/*`.
+	Parent string `json:"parent,omitempty"`


If required we don't need omitempty.

Suggested change

Parent string `json:"parent,omitempty"`

Parent string `json:"parent"`

This should also be a cross resource reference to the Secret type.

Sorry, I didn't understand the cross resource reference part . Is it possible to reference just the field Secret.ObjectMeta.Name ?

hasheddan · 2021-04-01T00:22:02Z

apis/secretsmanager/v1alpha1/secret_types.go

+}
+
+// ReplicationAutomatic has fields for automatic replication of secrets
+type ReplicationAutomatic struct {


Should there be fields here?

The current version of google SDK doesn't have any but there are new fields in the latest versions. Shall I add a comment to mention it's intentionally empty?

hasheddan · 2021-04-01T00:22:59Z

apis/secretsmanager/v1alpha1/secret_types.go

+type ReplicationUserManagedReplica struct {
+	// Location of the secret.
+	// +immutable
+	Location string `json:"location,omitempty"`


Should be pointer type if optional, though if it is the only field in this embedded struct we could consider making it required and just the parent struct optional.

Suggested change

Location string `json:"location,omitempty"`

Location *string `json:"location,omitempty"`

Yes, making the struct optional reflects what GCP expects. If user has started filling the struct, Location should be a required field. Also, newer GCP SDK does have additional fields . I have already marked this as optional in ReplicationType struct at L67

hasheddan · 2021-04-01T00:23:28Z

apis/secretsmanager/v1alpha1/secret_types.go

+
+	// Output only. This must be unique within the project. External name of the object is set to this field, hence making it optional
+	// +optional
+	SecretID *string `json:"secretid,omitempty"`


Suggested change

SecretID *string `json:"secretid,omitempty"`

SecretID string `json:"secretid,omitempty"`

We typically do not use pointer types in status.

I am wondering if I should remove this field as it doesn't provide much value anyway. It's the same as external name of the CR

hasheddan · 2021-04-01T00:24:20Z

apis/secretversion/v1alpha1/secretversion_types.go

+
+const (
+	// SecretVersionSTATEUNSPECIFIED represents that SecretVersion state is not specified. This value is unused and invalid.
+	SecretVersionSTATEUNSPECIFIED SecretVersionState = 0


Can we make this camel case?

Suggested change

SecretVersionSTATEUNSPECIFIED SecretVersionState = 0

SecretVersionStateUnspecified SecretVersionState = 0

hasheddan · 2021-04-01T00:24:55Z

apis/secretversion/v1alpha1/secretversion_types.go

+type SecretVersionParameters struct {
+	// SecretRef refers to the secret object(GCP) created in Kubernetes
+	// Required
+	SecretRef string `json:"secretRef,omitempty"`


Should this be optional if payload is provided?

Sorry, the comment is misleading, SecretRef is the name of the secret created within GCP . Basically, the secrets manager expects us to create a Secret and then it can have multiple SecretVersion . It won't be possible to create a SecretVersion without the Secret . End users can choose to create the Secret manually and just manage SecretVersion through crossplane

For referencing K8s secret, I am continuing the work here : #317 .

Also, I wonder if there's a way to enforce the following validation: it is mandatory to pass in either Payload or KubeSecretRef . Right now, I have kept them both optional

hasheddan · 2021-04-01T00:26:13Z

apis/secretversion/v1alpha1/secretversion_types.go

+	// SecretVersionDESTROYED represents that SecretVersion state is destroyed and the secret data is no longer
+	// stored. A version may not leave this state once entered.
+	// SecretVersionDESTROYED SecretVersionState = 3
+	// +kubebuilder:validation:Enum=1;2;3


I wonder if we should instead make this ENABLED,DISABLED,... as that would likely map to the experience folks would expect here.

hasheddan · 2021-04-01T00:28:43Z

pkg/clients/secretversion/secretversion.go

+	"github.com/crossplane/provider-gcp/apis/secretversion/v1alpha1"
+	"github.com/google/go-cmp/cmp"
+	"github.com/googleapis/gax-go"
+	"google.golang.org/genproto/googleapis/cloud/secretmanager/v1"


I think both Secret and SecretVersion should be in secretmanager group, just be different types within it.

hasheddan · 2021-04-01T00:33:31Z

apis/secretsmanager/v1alpha1/register.go

+// Package type metadata.
+const (
+	Group   = "secretsmanager.gcp.crossplane.io"
+	Version = "v1alpha1"
+)


I left the same comment below in the client packages, but I think both Secret and SecretVersion should be in the secretsmanager.gcp.crossplane.io group and just be different types, rather than in different groups.

Yep, I should probably change the project structure too in clients and controllers too.

RealHarshThakur

@hasheddan Thanks for reviewing . Will work on the suggestions and start with writing tests

RealHarshThakur · 2021-04-04T23:56:26Z

apis/secretsmanager/v1alpha1/register.go

+// Package type metadata.
+const (
+	Group   = "secretsmanager.gcp.crossplane.io"
+	Version = "v1alpha1"
+)


Yep, I should probably change the project structure too in clients and controllers too.

RealHarshThakur · 2021-04-04T23:58:17Z

apis/secretsmanager/v1alpha1/secret_types.go

+
+	// Required. The resource name of the project to associate with the
+	// [Secret][google.cloud.secretmanager.v1.Secret], in the format `projects/*`.
+	Parent string `json:"parent,omitempty"`


Sorry, I didn't understand the cross resource reference part . Is it possible to reference just the field Secret.ObjectMeta.Name ?

RealHarshThakur · 2021-04-04T23:59:42Z

apis/secretsmanager/v1alpha1/secret_types.go

+}
+
+// ReplicationAutomatic has fields for automatic replication of secrets
+type ReplicationAutomatic struct {


The current version of google SDK doesn't have any but there are new fields in the latest versions. Shall I add a comment to mention it's intentionally empty?

RealHarshThakur · 2021-04-05T00:02:18Z

apis/secretsmanager/v1alpha1/secret_types.go

+type ReplicationUserManagedReplica struct {
+	// Location of the secret.
+	// +immutable
+	Location string `json:"location,omitempty"`


Yes, making the struct optional reflects what GCP expects. If user has started filling the struct, Location should be a required field. Also, newer GCP SDK does have additional fields . I have already marked this as optional in ReplicationType struct at L67

RealHarshThakur · 2021-04-05T00:06:05Z

apis/secretsmanager/v1alpha1/secret_types.go

+
+	// Output only. This must be unique within the project. External name of the object is set to this field, hence making it optional
+	// +optional
+	SecretID *string `json:"secretid,omitempty"`


I am wondering if I should remove this field as it doesn't provide much value anyway. It's the same as external name of the CR

RealHarshThakur · 2021-04-05T00:10:34Z

apis/secretversion/v1alpha1/secretversion_types.go

+type SecretVersionParameters struct {
+	// SecretRef refers to the secret object(GCP) created in Kubernetes
+	// Required
+	SecretRef string `json:"secretRef,omitempty"`


Sorry, the comment is misleading, SecretRef is the name of the secret created within GCP . Basically, the secrets manager expects us to create a Secret and then it can have multiple SecretVersion . It won't be possible to create a SecretVersion without the Secret . End users can choose to create the Secret manually and just manage SecretVersion through crossplane

RealHarshThakur · 2021-04-05T00:12:53Z

apis/secretversion/v1alpha1/secretversion_types.go

+type SecretVersionParameters struct {
+	// SecretRef refers to the secret object(GCP) created in Kubernetes
+	// Required
+	SecretRef string `json:"secretRef,omitempty"`


For referencing K8s secret, I am continuing the work here : #317 .

RealHarshThakur · 2021-04-05T00:15:14Z

apis/secretversion/v1alpha1/secretversion_types.go

+type SecretVersionParameters struct {
+	// SecretRef refers to the secret object(GCP) created in Kubernetes
+	// Required
+	SecretRef string `json:"secretRef,omitempty"`


Also, I wonder if there's a way to enforce the following validation: it is mandatory to pass in either Payload or KubeSecretRef . Right now, I have kept them both optional

Signed-off-by: Harsh Thakur <[email protected]>

hasheddan

@RealHarshThakur thanks for your patience here and my apologies on the long review cycle 😞 We have made a number of changes in this repo in #308 which will require a rebase here. Please let me know if you have any questions and I will be more than happy to help :)

I also noticed you are using the genproto GCP library here. I recently moved a few of our types off of it because it was somewhat difficult to work with (you may have noticed this in the steps required in the controller implementation). It may be easier to work with https://github.com/googleapis/google-api-go-client/blob/d82b0a56bb37a94bfd6d885bc7c3e1761de7b509/secretmanager/v1/secretmanager-gen.go#L1187 instead. I believe we are now on the latest version of this package 👍

turkenh · 2021-09-06T09:53:59Z

@RealHarshThakur what is your plan on this one? Do you have some time to revisit/rebase this PR?

RealHarshThakur · 2021-09-06T10:12:34Z

Hey @turkenh , the PR is almost ready. I need to add bits to reference K8s secrets, but I was doing in a separate PR. Sure, I can rebase it but I can't test it against GCP API anymore due to weird billing issue.

knelasevero · 2021-10-26T00:35:46Z

@RealHarshThakur I can help testing agains gcp API in my account if it would help. Would love to see this merged :)

RealHarshThakur · 2021-10-27T16:59:00Z

@knelasevero Awesome. Do you need any help from me?
I could give you access to this branch.(I don't know if there's a better way to collaborate)

UPDATE: invited you to my fork. Feel free to push commits directly.

knelasevero · 2021-10-31T14:44:25Z

@RealHarshThakur Thanks! I just accepted. I will be working on this next week.

RealHarshThakur · 2021-10-31T16:05:24Z

@knelasevero Great! JFYI: I did try to have a K8s secret reference in a separate PR. #317 .
I think there are some bits in it which still need thinking as there's no direct way to enforce immutability of fields but would love to hear your experience/opinion on it.

knelasevero · 2021-11-18T12:48:02Z

Hey, sorry to have vanished. I've been on sick leave for a while.

@RealHarshThakur do you think we can pair on this when I am back? I think we can close this one quicker that way

RealHarshThakur · 2021-11-18T14:32:54Z

Sure, shoot me an email ( [email protected] ) or at crossplane slack so we can figure out a time

knelasevero · 2021-11-22T16:30:10Z

Ok, I thought I would have more time, but client work will get most of my slots. Would you have time to rebase yourself and tweak whatever is needed? Then you can let me know and I test this in my account.

RealHarshThakur marked this pull request as draft February 21, 2021 19:09

RealHarshThakur force-pushed the secrets-manager branch from df61ffc to 9f2f4f6 Compare February 21, 2021 21:39

RealHarshThakur changed the title ~~Adding resource : secret~~ Adding secretsmanager support Feb 21, 2021

RealHarshThakur force-pushed the secrets-manager branch 2 times, most recently from 3704dd3 to d369831 Compare February 25, 2021 21:51

Adding support for secrets manager

b6bda43

Signed-off-by: Harsh Thakur <[email protected]>

RealHarshThakur force-pushed the secrets-manager branch from d369831 to 322384a Compare February 27, 2021 00:09

hasheddan self-requested a review March 2, 2021 13:12

hasheddan reviewed Mar 3, 2021

View reviewed changes

apis/secretsmanager/v1alpha1/secret_types.go Outdated Show resolved Hide resolved

apis/secretsmanager/v1alpha1/secret_types.go Outdated Show resolved Hide resolved

apis/secretsmanager/v1alpha1/secret_types.go Outdated Show resolved Hide resolved

Add secret version as a resource

7da053a

Signed-off-by: Harsh Thakur <[email protected]>

RealHarshThakur force-pushed the secrets-manager branch from 322384a to 7da053a Compare March 9, 2021 09:46

Fix conversion of secret version between states

f3e69e9

Signed-off-by: Harsh Thakur <[email protected]>

RealHarshThakur marked this pull request as ready for review March 22, 2021 18:16

RealHarshThakur added 2 commits March 23, 2021 02:04

Update year to 2021

98e54f6

Signed-off-by: Harsh Thakur <[email protected]>

Formatting fixes

13aba8c

Signed-off-by: Harsh Thakur <[email protected]>

RealHarshThakur mentioned this pull request Mar 24, 2021

Add support for Secrets Manager #317

Draft

2 tasks

hasheddan reviewed Apr 1, 2021

View reviewed changes

RealHarshThakur commented Apr 5, 2021

View reviewed changes

RealHarshThakur added 7 commits April 5, 2021 06:33

Move secret version into secrets manager group

d46faf7

Signed-off-by: Harsh Thakur <[email protected]>

Improved user input for secret version state

ce7478e

Signed-off-by: Harsh Thakur <[email protected]>

Mark data field as immutable

d3bc096

Signed-off-by: Harsh Thakur <[email protected]>

Move version example to secrets manager folder

faa48a1

Signed-off-by: Harsh Thakur <[email protected]>

Unit tests for secrets manager secret client

c81e138

Signed-off-by: Harsh Thakur <[email protected]>

Add test for secrets manager version client

2fc9218

Signed-off-by: Harsh Thakur <[email protected]>

Add unit tests for controllers

8fc8e3c

Signed-off-by: Harsh Thakur <[email protected]>

RealHarshThakur force-pushed the secrets-manager branch from c80d020 to 8fc8e3c Compare April 13, 2021 19:27

Add remaining tests for secretversion controller

e171b5f

Signed-off-by: Harsh Thakur <[email protected]>

hasheddan reviewed Jun 9, 2021

View reviewed changes

Feggah mentioned this pull request Oct 25, 2021

Support for GCP Secret Manager Secret resource #399

Closed

knelasevero mentioned this pull request Oct 27, 2021

Secret generation external-secrets/external-secrets#249

Closed

	Parent string `json:"parent,omitempty"`
	Parent string `json:"parent"`

	Location string `json:"location,omitempty"`
	Location *string `json:"location,omitempty"`

	SecretID *string `json:"secretid,omitempty"`
	SecretID string `json:"secretid,omitempty"`

	SecretVersionSTATEUNSPECIFIED SecretVersionState = 0
	SecretVersionStateUnspecified SecretVersionState = 0

Adding secretsmanager support #305

Are you sure you want to change the base?

Adding secretsmanager support #305

Conversation

RealHarshThakur commented Feb 21, 2021 • edited Loading

Description of your changes

How has this code been tested

hasheddan left a comment

Choose a reason for hiding this comment

hasheddan 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

Choose a reason for hiding this comment

RealHarshThakur Apr 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealHarshThakur Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealHarshThakur Apr 5, 2021 • 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

RealHarshThakur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealHarshThakur Apr 4, 2021 • edited Loading

Choose a reason for hiding this comment

RealHarshThakur Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

RealHarshThakur Apr 5, 2021 • 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

hasheddan left a comment

Choose a reason for hiding this comment

turkenh commented Sep 6, 2021

RealHarshThakur commented Sep 6, 2021

knelasevero commented Oct 26, 2021

RealHarshThakur commented Oct 27, 2021 • edited Loading

knelasevero commented Oct 31, 2021

RealHarshThakur commented Oct 31, 2021

knelasevero commented Nov 18, 2021

RealHarshThakur commented Nov 18, 2021

knelasevero commented Nov 22, 2021

RealHarshThakur commented Feb 21, 2021 •

edited

Loading

RealHarshThakur Apr 4, 2021 •

edited

Loading

RealHarshThakur Apr 5, 2021 •

edited

Loading

RealHarshThakur Apr 5, 2021 •

edited

Loading

RealHarshThakur Apr 4, 2021 •

edited

Loading

RealHarshThakur Apr 5, 2021 •

edited

Loading

RealHarshThakur Apr 5, 2021 •

edited

Loading

RealHarshThakur commented Oct 27, 2021 •

edited

Loading