Bug fixes for status patching and late initialization patching (#168)

Description of changes:
Commit#1
- Adds the implementation of AWSResource.DeepCopy() method. aws-controllers-k8s/runtime#48

Commit#2
- Fixes the patching bug in lateInitialize code to unblock sagemaker team
- BUG: rm.LateInitialize() method was adding lateInitialized fields to the same object passed in the parameter and returning as output.
```go
        lateInitializedLatest, err := rm.LateInitialize(ctx, latest)
	rlog.Exit("rm.LateInitialize", err)
	// Always patch after late initialize because some fields may have been initialized while
	// others require a retry after some delay.
	// This patching does not hurt because if there is no diff then 'patchResourceMetadataAndSpec'
	// acts as a no-op.
	if ackcompare.IsNotNil(lateInitializedLatest) {
		patchErr := r.patchResourceMetadataAndSpec(ctx, latest, lateInitializedLatest)
		// Throw the patching error if reconciler is unable to patch the resource with late initializations
		if patchErr != nil {
			err = patchErr
		}
	}
```
- Since lateInitializedLatest and latest were same object, above `patchResourceMetadataAndSpec` call sees no diff and does not patch lateInitialized fields into etcd.
- This PR solves the bug by adding lateInitialized fields in a copy of latest and returning that copy (without modifying latest)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Author: vijtrip2

Makefile

@@ -6,7 +6,7 @@ GO111MODULE=on
 AWS_SERVICE=$(shell echo $(SERVICE) | tr '[:upper:]' '[:lower:]')
 
 # Build ldflags
-VERSION ?= "v0.12.0"
+VERSION ?= "v0.13.0"
 GITCOMMIT=$(shell git rev-parse HEAD)
 BUILDDATE=$(shell date -u +'%Y-%m-%dT%H:%M:%SZ')
 IMPORT_PATH=github.com/aws-controllers-k8s/code-generator

go.mod

@@ -3,7 +3,7 @@ module github.com/aws-controllers-k8s/code-generator
 go 1.14
 
 require (
-	github.com/aws-controllers-k8s/runtime v0.12.0
+	github.com/aws-controllers-k8s/runtime v0.13.0
 	github.com/aws/aws-sdk-go v1.37.10
 	github.com/dlclark/regexp2 v1.4.0
 	// pin to v0.1.1 due to release problem with v0.1.2

go.sum

@@ -67,8 +67,8 @@ github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPd
 github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5/go.mod h1:wHh0iHkYZB8zMSxRWpUBQtwG5a7fFgvEO+odwuTv2gs=
 github.com/asaskevich/govalidator v0.0.0-20180720115003-f9ffefc3facf/go.mod h1:lB+ZfQJz7igIIfQNfa7Ml4HSf2uFQQRzpGGRXenZAgY=
 github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a/go.mod h1:lB+ZfQJz7igIIfQNfa7Ml4HSf2uFQQRzpGGRXenZAgY=
-github.com/aws-controllers-k8s/runtime v0.12.0 h1:G/lCEozh4Brsv1Ojqyl9D/whpq/YvcFtDZBWXf6YIgI=
-github.com/aws-controllers-k8s/runtime v0.12.0/go.mod h1:kG2WM4JAmLgf67cgZV9IZUkY2DsrUzsaNbmhFMfb05c=
+github.com/aws-controllers-k8s/runtime v0.13.0 h1:PYiNnQejjS/1H93bolFXGIzgQZSn/gRoPSAEU6UG0ec=
+github.com/aws-controllers-k8s/runtime v0.13.0/go.mod h1:kG2WM4JAmLgf67cgZV9IZUkY2DsrUzsaNbmhFMfb05c=
 github.com/aws/aws-sdk-go v1.37.10 h1:LRwl+97B4D69Z7tz+eRUxJ1C7baBaIYhgrn5eLtua+Q=
 github.com/aws/aws-sdk-go v1.37.10/go.mod h1:hcU610XS61/+aQV88ixoOzUoG7v3b31pl2zKMmprdro=
 github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973/go.mod h1:Dwedo/Wpr24TaqPxmxbtue+5NUziq4I4S80YR8gNf3Q=

pkg/generate/ack/runtime_test.go

@@ -131,19 +131,19 @@ func TestRuntimeDependency(t *testing.T) {
 	require.Implements((*acktypes.AWSResourceIdentifiers)(nil), new(fakeIdentifiers))
 	require.Implements((*acktypes.AWSResourceDescriptor)(nil), new(fakeDescriptor))
 
-	// ACK runtime 0.2.3 introduced a new logger that is now passed into the
+	// ACK runtime v0.2.3 introduced a new logger that is now passed into the
 	// Context and retrievable using the `pkg/runtime/log.FromContext`
 	// function.  This function returns NoopLogger if no such logger is found
 	// in the context, but this check here is mostly to ensure that the new
-	// function used in ACK runtime 0.2.3 and templates in code-generator
-	// consuming 0.2.3 are properly pinned.
+	// function used in ACK runtime v0.2.3 and templates in code-generator
+	// consuming v0.2.3 are properly pinned.
 	require.Implements((*acktypes.Logger)(nil), ackrtlog.FromContext(context.TODO()))
 
-	// ACK runtime 0.3.0 introduced a new RequeueOnSuccessSeconds method to the
+	// ACK runtime v0.3.0 introduced a new RequeueOnSuccessSeconds method to the
 	// resource manager factory
 	require.Implements((*acktypes.AWSResourceManagerFactory)(nil), new(fakeRMF))
 
-	// ACK runtime 0.4.0 introduced a new AdditionalKeys field to the
+	// ACK runtime v0.4.0 introduced a new AdditionalKeys field to the
 	// AWSIdentifiers type. By simply referring to the new AdditionalKeys field
 	// here, we have a compile-time test of the pinning of code-generator to
 	// ACK runtime v0.4.0...
@@ -155,20 +155,24 @@ func TestRuntimeDependency(t *testing.T) {
 	}
 	_ = ids
 
-	// ACK runtime 0.6.0 modified pkg/types/AWSResourceManager.Delete signature.
+	// ACK runtime v0.6.0 modified pkg/types/AWSResourceManager.Delete signature.
 	require.Implements((*acktypes.AWSResourceManager)(nil), new(fakeRM))
 
-	// ACK runtime 0.7.0 introduced SecretNotFound error.
+	// ACK runtime v0.7.0 introduced SecretNotFound error.
 	require.NotNil(ackerr.SecretNotFound)
 
-	// ACK runtime 0.8.0 removed the unused UpdateCRStatus method from
+	// ACK runtime v0.8.0 removed the unused UpdateCRStatus method from
 	// AWSResourceDescriptor
 	rdType := reflect.TypeOf((*acktypes.AWSResourceDescriptor)(nil)).Elem()
 	_, found := rdType.MethodByName("UpdateCRStatus")
 	require.False(found)
 
-	// ACK runtime 0.9.2 introduced the SetStatus method into AWSResource
+	// ACK runtime v0.9.2 introduced the SetStatus method into AWSResource
 	resType := reflect.TypeOf((*acktypes.AWSResource)(nil)).Elem()
 	_, found = resType.MethodByName("SetStatus")
 	require.True(found)
+
+	// ACK runtime v0.13.0 introduced the DeepCopy method into AWSResource
+	_, found = resType.MethodByName("DeepCopy")
+	require.True(found)
 }

pkg/generate/code/late_initialize.go

@@ -136,7 +136,7 @@ func getSortedLateInitFieldsAndConfig(
 //			}
 //		}
 //	}
-//	return latest
+//	return &resource{latestKo}
 func LateInitializeFromReadOne(
 	cfg *ackgenconfig.Config,
 	r *model.CRD,
@@ -151,8 +151,8 @@ func LateInitializeFromReadOne(
 	if len(lateInitializedFieldNames) == 0 {
 		return fmt.Sprintf("%sreturn %s", indent, targetResVarName)
 	}
-	out += fmt.Sprintf("%sobservedKo := rm.concreteResource(%s).ko\n", indent, sourceResVarName)
-	out += fmt.Sprintf("%slatestKo := rm.concreteResource(%s).ko\n", indent, targetResVarName)
+	out += fmt.Sprintf("%sobservedKo := rm.concreteResource(%s).ko.DeepCopy()\n", indent, sourceResVarName)
+	out += fmt.Sprintf("%slatestKo := rm.concreteResource(%s).ko.DeepCopy()\n", indent, targetResVarName)
 	// TODO(vijat@): Add validation for correct field path in lateInitializedFieldNames
 	for _, fName := range lateInitializedFieldNames {
 		// split the field name by period
@@ -204,7 +204,7 @@ func LateInitializeFromReadOne(
 			fNameIndentLevel = fNameIndentLevel - 1
 		}
 	}
-	out += fmt.Sprintf("%sreturn %s", indent, targetResVarName)
+	out += fmt.Sprintf("%sreturn &resource{latestKo}", indent)
 	return out
 }
 
@@ -235,7 +235,7 @@ func LateInitializeFromReadOne(
 //
 //
 // Sample Output:
-//	ko := rm.concreteResource(latest).ko
+//	ko := rm.concreteResource(latest).ko.DeepCopy()
 //	if ko.Spec.ImageScanningConfiguration != nil {
 //		if ko.Spec.ImageScanningConfiguration.ScanOnPush == nil {
 //			return true
@@ -288,7 +288,7 @@ func IncompleteLateInitialization(
 		out += fmt.Sprintf("%sreturn false", indent)
 		return out
 	}
-	out += fmt.Sprintf("%sko := rm.concreteResource(%s).ko\n", indent, resVarName)
+	out += fmt.Sprintf("%sko := rm.concreteResource(%s).ko.DeepCopy()\n", indent, resVarName)
 	for _, fName := range sortedLateInitFieldNames {
 		// split the field name by period
 		// each substring represents a field.

pkg/generate/code/late_initialize_test.go

@@ -100,15 +100,15 @@ func Test_LateInitializeFromReadOne_NonNestedPath(t *testing.T) {
 	assert.NotNil(crd.Config().ResourceFields(crd.Names.Original)["Name"].LateInitialize)
 	assert.NotNil(crd.Config().ResourceFields(crd.Names.Original)["ImageTagMutability"].LateInitialize)
 	expected :=
-		`	observedKo := rm.concreteResource(observed).ko
-	latestKo := rm.concreteResource(latest).ko
+		`	observedKo := rm.concreteResource(observed).ko.DeepCopy()
+	latestKo := rm.concreteResource(latest).ko.DeepCopy()
 	if observedKo.Spec.ImageTagMutability != nil && latestKo.Spec.ImageTagMutability == nil {
 		latestKo.Spec.ImageTagMutability = observedKo.Spec.ImageTagMutability
 	}
 	if observedKo.Spec.Name != nil && latestKo.Spec.Name == nil {
 		latestKo.Spec.Name = observedKo.Spec.Name
 	}
-	return latest`
+	return &resource{latestKo}`
 	assert.Equal(expected, code.LateInitializeFromReadOne(crd.Config(), crd, "observed", "latest", 1))
 }
 
@@ -125,8 +125,8 @@ func Test_LateInitializeFromReadOne_NestedPath(t *testing.T) {
 	assert.NotNil(crd.Config().ResourceFields(crd.Names.Original)["Name"].LateInitialize)
 	assert.NotNil(crd.Config().ResourceFields(crd.Names.Original)["ImageScanningConfiguration.ScanOnPush"].LateInitialize)
 	expected :=
-		`	observedKo := rm.concreteResource(observed).ko
-	latestKo := rm.concreteResource(latest).ko
+		`	observedKo := rm.concreteResource(observed).ko.DeepCopy()
+	latestKo := rm.concreteResource(latest).ko.DeepCopy()
 	if observedKo.Spec.ImageScanningConfiguration != nil && latestKo.Spec.ImageScanningConfiguration != nil {
 		if observedKo.Spec.ImageScanningConfiguration.ScanOnPush != nil && latestKo.Spec.ImageScanningConfiguration.ScanOnPush == nil {
 			latestKo.Spec.ImageScanningConfiguration.ScanOnPush = observedKo.Spec.ImageScanningConfiguration.ScanOnPush
@@ -163,7 +163,7 @@ func Test_LateInitializeFromReadOne_NestedPath(t *testing.T) {
 			}
 		}
 	}
-	return latest`
+	return &resource{latestKo}`
 	assert.Equal(expected, code.LateInitializeFromReadOne(crd.Config(), crd, "observed", "latest", 1))
 }
 
@@ -193,7 +193,7 @@ func Test_IncompleteLateInitialization(t *testing.T) {
 	assert.NotNil(crd.Config().ResourceFields(crd.Names.Original)["Name"].LateInitialize)
 	assert.NotNil(crd.Config().ResourceFields(crd.Names.Original)["ImageScanningConfiguration.ScanOnPush"].LateInitialize)
 	expected :=
-		`	ko := rm.concreteResource(latest).ko
+		`	ko := rm.concreteResource(latest).ko.DeepCopy()
 	if ko.Spec.ImageScanningConfiguration != nil {
 		if ko.Spec.ImageScanningConfiguration.ScanOnPush == nil {
 			return true

templates/pkg/resource/manager.go.tpl

@@ -177,43 +177,44 @@ func (rm *resourceManager) LateInitialize(
 		rlog.Debug("no late initialization required.")
 		return latest, nil
 	}
+	latestCopy := latest.DeepCopy()
 	lateInitConditionReason := ""
 	lateInitConditionMessage := ""
 {{- if $hookCode := Hook .CRD "late_initialize_pre_read_one" }}
 {{ $hookCode }}
 {{- end }}
-	observed, err := rm.ReadOne(ctx, latest)
+	observed, err := rm.ReadOne(ctx, latestCopy)
 	if err != nil {
 		lateInitConditionMessage = "Unable to complete Read operation required for late initialization"
 		lateInitConditionReason = "Late Initialization Failure"
-		ackcondition.SetLateInitialized(latest, corev1.ConditionFalse, &lateInitConditionMessage, &lateInitConditionReason)
-		return latest, err
+		ackcondition.SetLateInitialized(latestCopy, corev1.ConditionFalse, &lateInitConditionMessage, &lateInitConditionReason)
+		return latestCopy, err
 	}
 {{- if $hookCode := Hook .CRD "late_initialize_post_read_one" }}
 {{ $hookCode }}
 {{- end }}
-	latest = rm.lateInitializeFromReadOneOutput(observed, latest)
-	incompleteInitialization := rm.incompleteLateInitialization(latest)
+	lateInitializedRes := rm.lateInitializeFromReadOneOutput(observed, latestCopy)
+	incompleteInitialization := rm.incompleteLateInitialization(lateInitializedRes)
 	if incompleteInitialization {
 		// Add the condition with LateInitialized=False
 		lateInitConditionMessage = "Late initialization did not complete, requeuing with delay of 5 seconds"
 		lateInitConditionReason = "Delayed Late Initialization"
-		ackcondition.SetLateInitialized(latest, corev1.ConditionFalse, &lateInitConditionMessage, &lateInitConditionReason)
-		return latest, ackrequeue.NeededAfter(nil, time.Duration(5)*time.Second)
+		ackcondition.SetLateInitialized(lateInitializedRes, corev1.ConditionFalse, &lateInitConditionMessage, &lateInitConditionReason)
+		return lateInitializedRes, ackrequeue.NeededAfter(nil, time.Duration(5)*time.Second)
 	}
-	// Set LateIntialized condition to True
+	// Set LateInitialized condition to True
 	lateInitConditionMessage = "Late initialization successful"
 	lateInitConditionReason = "Late initialization successful"
-	ackcondition.SetLateInitialized(latest, corev1.ConditionTrue, &lateInitConditionMessage, &lateInitConditionReason)
-	return latest, nil
+	ackcondition.SetLateInitialized(lateInitializedRes, corev1.ConditionTrue, &lateInitConditionMessage, &lateInitConditionReason)
+	return lateInitializedRes, nil
 }
 
 // incompleteLateInitialization return true if there are fields which were supposed to be
 // late initialized but are not. If all the fields are late initialized, false is returned
 func (rm *resourceManager) incompleteLateInitialization(
-	latest acktypes.AWSResource,
+	res acktypes.AWSResource,
 ) bool {
-{{ GoCodeIncompleteLateInitialization .CRD "latest" 1 }}
+{{ GoCodeIncompleteLateInitialization .CRD "res" 1 }}
 }
 
 // lateInitializeFromReadOneOutput late initializes the 'latest' resource from the 'observed'

templates/pkg/resource/resource.go.tpl

@@ -81,4 +81,10 @@ func (r *resource) SetStatus(desired acktypes.AWSResource) {
 func (r *resource) SetIdentifiers(identifier *ackv1alpha1.AWSIdentifiers) error {
 {{- GoCodeSetResourceIdentifiers .CRD "identifier" "r.ko" 1}}
 	return nil
-}
+}
+
+// DeepCopy will return a copy of the resource
+func (r *resource) DeepCopy() acktypes.AWSResource {
+	koCopy := r.ko.DeepCopy()
+	return &resource{koCopy}
+}