feat(sesv2): Implement Simple Email Service v2 resources #791

kelvinwijaya · 2021-08-09T11:28:08Z

Description of your changes

Fixes #414

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

Tried Create resource operation with examples of custom resource manifests
Make sure Readiness Status shown as True and Condition as Available when the resources are provisioned successfully
Monitor Readiness Status shown as False and Condition as Unavailable OR Creating if wrong parameters being supplied in the custom resource manifests
Tried various Update resource operations depending on the targetted functionality especially when there is more than 1 PUT API operations.
Tried Delete operation of the resource and get deleted successfully

Test plan based on examples template:

Test emailidentity CRD operations for email address (verified with email acknowledgement status)
Test emailidentity CRD operations for domain (verified with DKIM and MailFrom status)
Test emailidentity Update operations for ConfigurationSetAttributes
Test emailidentity Update operations for MailFromAttributes
Test configurationset CRD operations which is referenced in the emailidentity
Test configurationset Update operations for DeliveryOptions
Test configurationset Update operations for ReputationOptions
Test configurationset Update operations for SuppressionOptions
Test configurationset Update operations for SendingOptions
Test configurationset Update operations for TrackingOptions
Test emailtemplate CRUD operations with HTML,Subject,Text

kelvinwijaya · 2021-08-09T12:28:43Z

Hi @muvaf @negz @hasheddan

I have just submitted a PR, not sure what causing the CI/check-diff step failing, can advise on this?

Is it because i modified some of the zz_.go file, which kind is allowed to be changed and not allowed to (e.g. zz_conversion.go or zz_generated_.go)?

Thanks,
Kelvin

chlunde · 2021-08-09T18:37:12Z

@kelvinwijaya yeah, I think it's either

because you modified a generated file
or didn't run make generate after rebasing/with the same tools, or after a local change

try make check-diff locally (make generate; git diff)

apis/sesv2/v1alpha1/zz_email_i_dentity.go

apis/sesv2/v1alpha1/generator-config.yaml

kelvinwijaya · 2021-08-10T01:21:24Z

@kelvinwijaya yeah, I think it's either

because you modified a generated file

or didn't run make generate after rebasing/with the same tools, or after a local change

try make check-diff locally (make generate; git diff)

Thanks @chlunde

I have rebased my code and removed handwritten code from the auto-generated one. But i noticed errors during the go building process:
ERRO Running error: buildir: analysis skipped: errors in package: [/Users/kelvinwijaya/go/src/github.com/kelvinwijaya/provider-aws/pkg/controller/sesv2/configurationseteventdestination/zz_conversions.go:48:9: elem declared but not used] 09:06:52 [FAIL]

Problem is if I don't modify this file to make it work as below, it wont compile:
Original:

for _, elem := range resp.EventDestinations {
		found = true
		break
	}

After modification (no compile error):

for _, elem := range resp.EventDestinations {
		if elem.Name != nil {
			found = true
			break
		}
	}

But after making this modification and commit the code. I ran make generate again and issue is this zz_conversion.go file will be overwritten again by the code-generator as there is diff. How do we resolve this before if i push this code? Else, the CI/check-diff will definitely throw error again.

Slack Thread: https://crossplane.slack.com/archives/C01718T2476/p1628649932065800

muvaf · 2021-08-12T10:46:05Z

@kelvinwijaya I wonder why the content of the for loop is empty. From your message in Slack, it seems it has some fields that should be used in the for loop. I think we need to open an issue there and to get the PR going, you can add this specific resource to the ignore list. Once the issue is addressed there, we can get back to adding it.

kelvinwijaya · 2021-08-13T01:35:23Z

Thanks @muvaf for your input, i will open a new case in aws-controller-k8s/code-generator project. I have excluded the problematic resource configurationseteventdestination so that this PR can keep going. This resource is not essential for SES to work , so I think I am good as for now and will definitely add it back when the code-generator fixes has been made

Thanks @chlunde also for pointing out the issue in code-generator, I will follow-up by opening an issue ticket with them

I have make sure All check have passed and would require you and other maintainers for the reviews. Many thanks in advanced.

kelvinwijaya · 2021-08-13T02:42:35Z

I have open a case to aws-controllers-k8s/community on the code-generation issue: aws-controllers-k8s/community#898

kelvinwijaya · 2021-08-25T15:22:07Z

Hi @chlunde

I have added Update operations for TrackingOptions. Feel free to review my PR whenever you ready or available

AaronME · 2021-09-24T17:28:24Z

@chlunde Would you mind taking another look and confirming your comments are resolved?

examples/sesv2/emailidentity-domain.yaml

kelvinwijaya · 2021-10-06T02:45:11Z

Hi @chlunde @AaronME

Would like to check if this PR will be pushed to the next provider-aws release

kelvinwijaya · 2021-12-02T01:56:16Z

Adding comment for reminder as this PR is currently blocked by:
#801 has been closed and replaced by #918

kelvinwijaya · 2022-01-14T06:01:24Z

Hi @chlunde @AaronME

It has been a while that this PR is blocked due to #801 #918, Is there a way we can push this implementation first while waiting for that. It was quite troublesome for my organization as we have to maintain downstream repo to support this additional feature

kelvinwijaya · 2022-01-14T10:36:24Z

Hi @chlunde @AaronME

Please ignore my comment earlier.

I notice the code-generator is now referencing to newer version (v0.14.1) with commit: 285d87b66b62fbfb859986ddf74c9f9b6ae743fb

Let me update the changes accordingly and update your guys with the new commit, I believe this should fix the zz_email_i_dentity.go and CustomEmailIDentityObservation and hopefully we can close this issue.

Thanks

kelvinwijaya · 2022-01-17T09:44:48Z

Hi @chlunde

I have done with my changes using the latest code-generator
Please help me review this PR again.

Many thanks!

kelvinwijaya · 2022-02-03T02:27:29Z

Hi @haarchri

Saw the upcoming release for provider-aws for v0.24.0: #1061

Currently this PR is still pending for review, will there be anyone in community to help out on this? I really hope this SES resource can be integrated to the aws provider release soon.

muvaf

Thanks @kelvinwijaya !

Makefile

muvaf · 2022-02-18T13:19:35Z

apis/sesv2/v1alpha1/zz_email_identity.go

+	DkimSigningAttributes *DkimSigningAttributes `json:"dkimSigningAttributes,omitempty"`
+	// The email address or domain that you want to verify.
+	// +kubebuilder:validation:Required
+	EmailIdentity *string `json:"emailIdentity"`


I think this should be taken from the external name annotation.

Not changing to external name annotation in consideration with email identity contain characters (e.g. my@acme.com) that is not valid k8s metadata name

pkg/controller/sesv2/emailidentity/setup.go

pkg/controller/sesv2/configurationset/setup.go

muvaf · 2022-02-18T14:27:17Z

pkg/controller/sesv2/tags.go

+}
+
+// UpdateTagsForResource with resourceName
+func UpdateTagsForResource(client sesv2iface.SESV2API, spec []*svcapitypes.Tag, resourceArn *string) error {


Seems like this is never used.

Yes, keeping the code once able to figure out: // TODO(kelvinwijaya): Updating Tags will be useful if we can determine the resourceArn from the Status return by Observation state object

Whats blocking this TODO from being implemented?

As for ConfigurationSet resource, the creation of Tags is passed via CreateConfigurationSet API only: https://docs.aws.amazon.com/ses/latest/APIReference-V2/API_CreateConfigurationSet.html

As for today, the tagging API only support update via arn format resource name which i cannot retrieve from the GET api: https://docs.aws.amazon.com/ses/latest/APIReference-V2/API_TagResource.html

I am keeping the code for future usage, but i can remove if this cause confusion as not being used for now.

muvaf · 2022-02-18T14:27:43Z

pkg/controller/sesv2/tags.go

+
+	tags := spec
+	for _, t := range GetExternalTags(mg) {
+		if _, exists := tagMap[awsclient.StringValue(t.Key)]; !exists {


We should override its value if it exists.

I am not sure on this one. If the user specifies a different value in the spec the controller should keep it. We some issues with this in the past.

I would keep it the way it is and only set tag values if the key isn't present already.

muvaf · 2022-02-18T14:31:06Z

pkg/controller/sesv2/tags.go

+}
+
+// AddExternalTags to spec if they don't exist
+func AddExternalTags(mg resource.Managed, spec []*svcapitypes.Tag) []*svcapitypes.Tag {


Would it be possible to have an implementation similar to this one that covers all cases including composition and ordering? https://github.com/crossplane/provider-aws/blob/8be1568/pkg/controller/ecr/repository/controller.go#L246

haarchri · 2022-06-26T12:42:09Z

@kelvinwijaya any chance to check the Open Points that we can finalize this PR ?

kelvinwijaya · 2022-07-13T10:10:28Z

Hi @haarchri

Let me have a look at the open points and try to resolve the review comments

MisterMX · 2023-06-29T10:20:38Z

@kelvinwijaya can you fix the issues and rebase to the latest master? Then we can work on getting this merged.

kelvinwijaya · 2023-06-30T01:43:26Z

@MisterMX , absolutely.. let me plan on review issues and merge the changes

kelvinwijaya · 2023-07-12T19:12:22Z

@MisterMX , for your review 7ff090b

kelvinwijaya · 2023-07-16T04:50:49Z

@MisterMX, added changes based on some reviews that i missed as conversation is not auto-populated in File changed due to comment given for autogenerated files (zz_files): b9cd5da

pkg/controller/sesv2/configurationset/setup.go

MisterMX · 2023-07-24T13:52:17Z

pkg/controller/sesv2/tags.go

+
+	tags := spec
+	for _, t := range GetExternalTags(mg) {
+		if _, exists := tagMap[awsclient.StringValue(t.Key)]; !exists {


I am not sure on this one. If the user specifies a different value in the spec the controller should keep it. We some issues with this in the past.

I would keep it the way it is and only set tag values if the key isn't present already.

MisterMX · 2023-07-24T13:53:32Z

pkg/controller/sesv2/tags.go

+}
+
+// UpdateTagsForResource with resourceName
+func UpdateTagsForResource(client sesv2iface.SESV2API, spec []*svcapitypes.Tag, resourceArn *string) error {


Whats blocking this TODO from being implemented?

MisterMX

Looks almost good to me. Only some minor issues remaining. @guilhermef after solving these can you squash your changes into a single commit?

pkg/controller/sesv2/configurationset/setup.go

MisterMX · 2023-07-26T07:43:39Z

pkg/controller/sesv2/configurationset/setup.go

+			CustomRedirectDomain: cr.Spec.ForProvider.TrackingOptions.CustomRedirectDomain,
+		}
+		if _, err := e.client.PutConfigurationSetTrackingOptionsWithContext(ctx, trackingOptionInput); err != nil {
+			return managed.ExternalUpdate{}, errors.Wrap(err, "update failed")
