Implemented support for TLS and ServerCA handling for cloudmemorystore #513

andrewj-a42 · 2023-09-06T21:31:00Z

Description of your changes

The change is adding support for enabling 'SERVER_AUTHENTICATION' for the TransitEncryptionMode.
The code is setting the value of TransitEncryptionMode field for gcp client and is observing serverCACerts field.
As soon as the instance is provisioned and CA is generated it is added to the managed resource to the atProvider section.

Fixes #

I have:

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

How has this code been tested

This code was tested by building a fresh image and deploying it to a dev cluster with crossplane pod up and running.
Successful cloudMemoryStore provisioning was performed and with TransitEncryptionMode field set to 'SERVER_AUTHENTICATION' value, the redis provisioning process generated server CA certificate which became part of 'atProvider' section of the managed resource.

dee0sap

Code change looks good to me.

Hey @Feggah and @hasheddan
Is there anything more than approval required here?

dee0sap · 2023-09-08T17:15:16Z

Hey @Feggah
Would you please take a look? I approved the PR but still it doesn't seem to be progressing.
Maybe I am not having some required access on the repo ?

Feggah · 2023-09-08T22:28:24Z

Maybe I am not having some required access on the repo ?

You need to follow the new member process to be added to the Crossplane organization. Here is an example of when I was added as maintainer.

I'm unsure if the process is exactly the same because this repository was under the crossplane organization at that time. As it's in crossplane-contrib now, maybe @jbw976 could help to understand if the process is different for community providers today 🙂

dee0sap · 2023-09-08T22:35:22Z

@Feggah

Thanks, I thought something was missing. I'll go ahead and create the issue as described in that doc, operating under the assumption that the process hasn't changed.

jbw976 · 2023-09-12T00:51:17Z

I just took a look into this and found a couple issues:

the repository access settings were not yet updated to reflect the recent changes in OWNERS.md, so I have made those changes now.
- @dee0sap if you see any invites to join this crossplane-contrib org and this repo, then please do accept them 😉
The CI and PR validation workflow has not yet run on this PR because it is from a new contributor and must first be approved. I will click this now to get things going. In the future, I think anyone with maintainer perms can do this.
I see that @dee0sap opened MEMBERSHIP: dee0sap crossplane/org#67 to join the main crossplane org as a member - that's not strictly necessary here for this crossplane-contrib repo, but a good thing to do either way, so I'll follow up there too!

dee0sap · 2023-09-12T01:46:40Z

Thanks @jbw976 !

Hey @andrewj-a42 Please review the lint and check-diff failures.

andrewj-a42 · 2023-09-13T22:54:47Z

Thanks @dee0sap for the note on check-diff. I have just addressed those lint problems and locally it is clean now.
I hope these problems were only caused by the linting.

dee0sap · 2023-09-13T23:00:39Z

Hey @hasheddan and @Feggah

This all looks good to me but as this would be my first merge I want to give you guys the chance to double check things before I push the button. So I'll let this set for a day.

Feggah

Thanks for the PR! I added some comments mainly to emphasize our "code guidelines" for providers.

Other than that, not specifically to this PR, but a general tip that is important IMHO: I would recommend always striving for test cases where the want and args structs have the resource.Managed field.

This is important to test that the main functions (Observe, Create, Update) are modifying the Managed Resource structure as expected.

For example, check the test cases for the CloudSQL:

provider-gcp/pkg/controller/database/cloudsql_test.go

Lines 151 to 158 in 8760d8c

    
           type args struct { 
        
           	mg resource.Managed 
        
           } 
        
           type want struct { 
        
           	mg  resource.Managed 
        
           	obs managed.ExternalObservation 
        
           	err error 
        
           }

With this structure, we can validate that the expected modifications to the Managed Resource are happening. Test case example:

Note that the returned spec from the GCP API has BackupConfigurationStartTime set. On the Managed Resource side, it didn't have this field set. So we could validate that if the API returned the field BackupConfigurationStartTime and it wasn't set on the MR, it would be late-initialized.

Feggah · 2023-09-14T12:19:42Z

apis/cache/v1beta1/cloudmemorystore_instance_types.go

+	// +optional
+	TransitEncryptionMode string `json:"transitEncryptionMode,omitempty"`


We have a guideline to use pointers for optional fields in the Parameters struct. So this field should be:

Suggested change

// +optional

TransitEncryptionMode string `json:"transitEncryptionMode,omitempty"`

// +optional

TransitEncryptionMode *string `json:"transitEncryptionMode,omitempty"`

Feggah · 2023-09-14T12:21:07Z

apis/cache/v1beta1/cloudmemorystore_instance_types.go

+	// Cert is the certificate in the PEM format.
+	Cert string `json:"cert,omitempty"`
+
+	// CreateTime: Output only. The time when the certificate was created in
+	// RFC 3339 (https://tools.ietf.org/html/rfc3339) format, for example
+	// `2020-05-18T00:00:00.094Z`.
+	CreateTime string `json:"createTime,omitempty"`
+
+	// ExpireTime: Output only. The time when the certificate expires in RFC
+	// 3339 (https://tools.ietf.org/html/rfc3339) format, for example
+	// `2020-05-18T00:00:00.094Z`.
+	ExpireTime string `json:"expireTime,omitempty"`
+
+	// SerialNumber: Serial number, as extracted from the certificate.
+	SerialNumber string `json:"serialNumber,omitempty"`
+
+	// Sha1Fingerprint: Sha1 Fingerprint of the certificate.
+	Sha1Fingerprint string `json:"sha1Fingerprint,omitempty"`


Just a comment here to clarify the difference between the Parameters struct:

Setting omitempty and non-pointer fields in the Observation struct is correct 👍🏼

Feggah · 2023-09-14T12:24:10Z

apis/cache/v1beta1/cloudmemorystore_instance_types.go

+	// ServerCaCerts: Output only. List of server CA certificates for the
+	// instance.
+	ServerCaCerts []*ServerCACertsObservation `json:"serverCaCerts,omitempty"`


Generally, we don't use pointers in the Observation struct. If there isn't a specific reason for it, I would recommend changing to:

Suggested change

// ServerCaCerts: Output only. List of server CA certificates for the

// instance.

ServerCaCerts []*ServerCACertsObservation `json:"serverCaCerts,omitempty"`

// ServerCaCerts: Output only. List of server CA certificates for the

// instance.

ServerCaCerts []ServerCACertsObservation `json:"serverCaCerts,omitempty"`

Signed-off-by: Andrew Jakubowski <[email protected]>

andrewj-a42 · 2023-09-16T00:39:55Z

I just applied the above suggestions related to usage of pointers.

Feggah

LGTM 🙂

Will leave it for @dee0sap to merge it.

dee0sap · 2023-09-16T15:28:47Z

Thanks much @Feggah !

dee0sap · 2023-09-16T15:31:28Z

Oh, a question @Feggah

How and when are releases done? I just searched the markdown in this repo and didn't find any statement about that.

FYI over in provider-aws there is a statement that they do a release every month. That's not the exact wording, but that is the gist.

Feggah · 2023-09-16T15:39:03Z

How and when are releases done?

We don't have an official definition of that for this provider. We kind of did releases as the necessity arose.

Feel free to suggest a frequency or release version as needed 🙂

dee0sap · 2023-09-16T16:13:26Z

Thanks @Feggah

My teammates and I will discuss next week some time and then reach out to you.

dee0sap approved these changes Sep 6, 2023

View reviewed changes

andrewj-a42 force-pushed the master branch from 4901d36 to 046fcfb Compare September 6, 2023 22:04

andrewj-a42 force-pushed the master branch from 046fcfb to b4ba6df Compare September 13, 2023 22:49

Feggah reviewed Sep 14, 2023

View reviewed changes

Implemented support for TLS and ServerCA handling for cloudmemorystore

feabae5

Signed-off-by: Andrew Jakubowski <[email protected]>

andrewj-a42 force-pushed the master branch from b4ba6df to feabae5 Compare September 16, 2023 00:34

Feggah approved these changes Sep 16, 2023

View reviewed changes

dee0sap merged commit 01437a1 into crossplane-contrib:master Sep 16, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented support for TLS and ServerCA handling for cloudmemorystore #513

Implemented support for TLS and ServerCA handling for cloudmemorystore #513

andrewj-a42 commented Sep 6, 2023 •

edited

Loading

dee0sap left a comment

dee0sap commented Sep 8, 2023

Feggah commented Sep 8, 2023

dee0sap commented Sep 8, 2023

jbw976 commented Sep 12, 2023 •

edited

Loading

dee0sap commented Sep 12, 2023

andrewj-a42 commented Sep 13, 2023

dee0sap commented Sep 13, 2023

Feggah left a comment •

edited

Loading

Feggah Sep 14, 2023

Feggah Sep 14, 2023

Feggah Sep 14, 2023

andrewj-a42 commented Sep 16, 2023

Feggah left a comment

dee0sap commented Sep 16, 2023

dee0sap commented Sep 16, 2023

Feggah commented Sep 16, 2023

dee0sap commented Sep 16, 2023

	type args struct {
	mg resource.Managed
	}
	type want struct {
	mg resource.Managed
	obs managed.ExternalObservation
	err error
	}

		// +optional
		TransitEncryptionMode string `json:"transitEncryptionMode,omitempty"`

Implemented support for TLS and ServerCA handling for cloudmemorystore #513

Implemented support for TLS and ServerCA handling for cloudmemorystore #513

Conversation

andrewj-a42 commented Sep 6, 2023 • edited Loading

Description of your changes

How has this code been tested

dee0sap left a comment

Choose a reason for hiding this comment

dee0sap commented Sep 8, 2023

Feggah commented Sep 8, 2023

dee0sap commented Sep 8, 2023

jbw976 commented Sep 12, 2023 • edited Loading

dee0sap commented Sep 12, 2023

andrewj-a42 commented Sep 13, 2023

dee0sap commented Sep 13, 2023

Feggah left a comment • edited Loading

Choose a reason for hiding this comment

Feggah Sep 14, 2023

Choose a reason for hiding this comment

Feggah Sep 14, 2023

Choose a reason for hiding this comment

Feggah Sep 14, 2023

Choose a reason for hiding this comment

andrewj-a42 commented Sep 16, 2023

Feggah left a comment

Choose a reason for hiding this comment

dee0sap commented Sep 16, 2023

dee0sap commented Sep 16, 2023

Feggah commented Sep 16, 2023

dee0sap commented Sep 16, 2023

andrewj-a42 commented Sep 6, 2023 •

edited

Loading

jbw976 commented Sep 12, 2023 •

edited

Loading

Feggah left a comment •

edited

Loading