Store CallerReference value for import scenarios #1261

ezgidemirel · 2022-04-14T15:29:36Z

Signed-off-by: ezgidemirel ezgidemirel91@gmail.com

Description of your changes

This change lateInitialize the "CallerReference" value for import scenarios.

Fixes #1260

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

Created a CF distribution resource with example.yaml
Changed spec.deletionPolicy to Orphan and deleted the managed resource
Imported the same resource with following manifest

apiVersion: cloudfront.aws.crossplane.io/v1alpha1
kind: Distribution
metadata:
  annotations:
    crossplane.io/external-name: <external-name>
  name: ezgi
spec:
  forProvider:
    region: us-east-1
    distributionConfig: {}

Updated spec.forProvider.distributionConfig.comment
Applied the same steps with callerReference field set

ulucinar · 2022-04-15T08:57:45Z

apis/cloudfront/v1alpha1/custom_types.go

@@ -17,7 +17,9 @@ limitations under the License.
 package v1alpha1

 // CustomDistributionParameters includes the custom fields of Distribution.
-type CustomDistributionParameters struct{}
+type CustomDistributionParameters struct {
+	CallerReference *string `json:"callerReference,omitempty"`


Should we add some documentation stating that this field is for internal use and appropriately defaulted and the +kubebuilder:validation:Optional marker for this field?

M understanding is that both for provisioning & import scenarios, users of this API will never have to supply a value, we are adding this to be able to support import scenarios. This could also be only part of the status.

I added the kubebuilder marker. As discussed offline, if callerReference field is set, I use it during preCreate.

ulucinar · 2022-04-15T09:06:08Z

pkg/controller/cloudfront/distribution/setup.go

@@ -323,9 +323,13 @@ func postUpdate(_ context.Context, cr *svcapitypes.Distribution, resp *svcsdk.Up
 func preUpdate(_ context.Context, cr *svcapitypes.Distribution, udi *svcsdk.UpdateDistributionInput) error {
 	udi.Id = awsclients.String(meta.GetExternalName(cr))
 	udi.SetIfMatch(awsclients.StringValue(cr.Status.AtProvider.ETag))
-	udi.DistributionConfig.CallerReference = awsclients.String(string(cr.UID))
+	if *cr.Spec.ForProvider.CallerReference != "" { // import scenario


One point to note is that because we are adding this logic to only preUpdate, we are not supporting the use case of explicitly specifying a caller reference by the user of the API, i.e., for newly provisioned CloudFront Distributions, caller ref. will always be the uid, although we have exposed an (internal) API for it. I think this is not an important use case (as long as, for instance, AWS decides to impose a (backward-incompatible) format restriction on the caller ref. string). But I think, we should at least provide field documentation mentioning this. Is it possible to introduce this field only for the status kind?

I updated preCreate function to use specified callerReference if exists. Also, extended the documentation.

Thanks @ezgidemirel!

muvaf · 2022-04-19T20:41:15Z

pkg/controller/cloudfront/distribution/lateinit.go

@@ -36,6 +36,11 @@ import (
 // what the AWS SDK says that some of these fields are required.

 func lateInitialize(dp *svcapitypes.DistributionParameters, gdo *svcsdk.GetDistributionOutput) error {
+	// during import scenarios, we need the original caller reference info
+	if gdo.Distribution != nil && gdo.Distribution.DistributionConfig != nil {


Did you consider having an Initializer like the ones we use for tagging? It'd basically populate the caller reference with UID if it's not given and then we'd be able to treat that field just like other fields in all calls since managed reconciler makes sure the initializers are called before any external operation. In fact, we could even remove its ignore statement here.

Hi @muvaf, I tried to use an Initializer which sets spec.forProvider.distributionConfig.callerReference value with UID if it's not given. But, the import scenario failed again. Setting the value in preCreate guarantees that the resource does not exist on the cloud provider.

Thanks for pointing out the ignore statement. I missed that one.

muvaf

Thanks @ezgidemirel ! LGTM!

muvaf · 2022-04-20T11:49:07Z

pkg/controller/cloudfront/distribution/setup.go

@@ -85,151 +85,156 @@ func SetupDistribution(mgr ctrl.Manager, o controller.Options) error {
 }

 func preCreate(_ context.Context, cr *svcapitypes.Distribution, cdi *svcsdk.CreateDistributionInput) error {
-	cdi.DistributionConfig.CallerReference = awsclients.String(string(cr.UID))
+	if cr.Spec.ForProvider.DistributionConfig.CallerReference != nil && *cr.Spec.ForProvider.DistributionConfig.CallerReference != "" {


Suggested change

if cr.Spec.ForProvider.DistributionConfig.CallerReference != nil && *cr.Spec.ForProvider.DistributionConfig.CallerReference != "" {

if awsclients.StringValue(cr.Spec.ForProvider.DistributionConfig.CallerReference) != "" {

muvaf · 2022-04-20T11:51:46Z

pkg/controller/cloudfront/distribution/lateinit.go

@@ -48,6 +48,9 @@ func lateInitialize(dp *svcapitypes.DistributionParameters, gdo *svcsdk.GetDistr
 	in := dp.DistributionConfig
 	from := gdo.Distribution.DistributionConfig

+	// during import scenarios, we need the original caller reference info


Could you add a comment here about what we discussed offline? i.e. the caller reference is not exposed to users who would like to import the resource, hence we need to derive it from the first response from AWS if user didn't give it explicitly.

Signed-off-by: ezgidemirel <ezgidemirel91@gmail.com>

muvaf · 2022-04-20T14:30:04Z

/backport

github-actions · 2022-04-20T14:30:25Z

Successfully created backport PR #1268 for release-0.26.

ezgidemirel requested a review from ulucinar April 14, 2022 15:29

ezgidemirel force-pushed the fix-cloudfront-update branch from 564d3d8 to 43e9a4a Compare April 14, 2022 16:15

ulucinar reviewed Apr 15, 2022

View reviewed changes

ezgidemirel force-pushed the fix-cloudfront-update branch from 43e9a4a to fbd5ef8 Compare April 19, 2022 09:39

muvaf reviewed Apr 19, 2022

View reviewed changes

ezgidemirel force-pushed the fix-cloudfront-update branch from fbd5ef8 to fde54d0 Compare April 20, 2022 10:49

muvaf approved these changes Apr 20, 2022

View reviewed changes

Store CallerReference value for import scenarios

c47d6e0

Signed-off-by: ezgidemirel <ezgidemirel91@gmail.com>

ezgidemirel force-pushed the fix-cloudfront-update branch from fde54d0 to c47d6e0 Compare April 20, 2022 12:09

ulucinar approved these changes Apr 20, 2022

View reviewed changes

ulucinar merged commit 9ec5e72 into crossplane-contrib:master Apr 20, 2022

ezgidemirel deleted the fix-cloudfront-update branch April 20, 2022 12:44

muvaf added the backport release-0.26 label Apr 20, 2022

github-actions bot mentioned this pull request Apr 20, 2022

[Backport release-0.26] Store CallerReference value for import scenarios #1268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store CallerReference value for import scenarios #1261

Store CallerReference value for import scenarios #1261

ezgidemirel commented Apr 14, 2022 •

edited

ulucinar Apr 15, 2022

ezgidemirel Apr 19, 2022

ulucinar Apr 15, 2022

ezgidemirel Apr 19, 2022

ulucinar Apr 20, 2022

muvaf Apr 19, 2022

ezgidemirel Apr 20, 2022

muvaf left a comment

muvaf Apr 20, 2022

ezgidemirel Apr 20, 2022

muvaf Apr 20, 2022

ezgidemirel Apr 20, 2022

muvaf commented Apr 20, 2022

github-actions bot commented Apr 20, 2022

	if cr.Spec.ForProvider.DistributionConfig.CallerReference != nil && *cr.Spec.ForProvider.DistributionConfig.CallerReference != "" {
	if awsclients.StringValue(cr.Spec.ForProvider.DistributionConfig.CallerReference) != "" {

Store CallerReference value for import scenarios #1261

Store CallerReference value for import scenarios #1261

Conversation

ezgidemirel commented Apr 14, 2022 • edited

Description of your changes

How has this code been tested

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

muvaf 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

muvaf commented Apr 20, 2022

github-actions bot commented Apr 20, 2022

ezgidemirel commented Apr 14, 2022 •

edited